Execution order of extensions changed with 1.7.0

[JoergSiebahn]

Since 1.7.0 some extension annotations at class level are not applied before @RegisterExtension any more.

Specifically @SetSystemProperty is using beforeEach now but used beforeAll and has been executed before @RegisterExtension annotated extension fields in the test class.

We are using DropwizardAppExtension to bootstrap an application for all tests in the test class. In our case, the application needs the system property on startup to be configured for the test.

This change has been introduced with #496.

We see our test failing with the update to 1.7.0.

The PR and associated issues refer to alignment and consistent behavior. That sounds reasonable at a first glance. My personal assumption would be that something at class level is wrapped around something at method level and is applied as a class extension.

Do you have any suggestion how a test can be configured with 1.7.0 when it uses a static class extension that needs a system property to be configured for the test. In our specific case we could try to bootstrap the application in the test method and shut it down in a finally statement. But in general that is not suitable as it should be started only once for many test methods for performance reasons.

[beatngu13]

Hey @JoergSiebahn,

thank you for the detailed issue description! 🙏 We actually considered this a bug, but your use case is definitely valid.

We will look into this and try to come up with something.

[JoergSiebahn]

Hey @beatngu13,

thank you for the quick response and for looking into this. For the moment I prepared my idea of a quick fix in the update PR. We have to discuss internally if we consider it as a breaking change for the consumers of our library to decide for a merge or waiting for our next major.

I'm happy to hear about a better idea of a workaround or solution for such kind of tests.

[Michael1993]

The easiest and (in my opinion) obvious solution would be to make this behaviour configurable. We could introduce an enum, e.g.:

enum ExecutionPhase {
    BEFORE_EACH,
    BEFORE_ALL
}

The problem with this approach is that we basically re-introduce the "bug" we were trying to fix - extensions behaving kind of unpredictably...?

[nipafx]

Thanks for opening the issue @JoergSiebahn. This is a complicated topic because the semantics aren't clear. As you said, in the past annotating a class meant (A) "set system property before all tests", but we got feedback from some users who were surprised by this because they saw annotating the class as a shorthand for annotating each method and so thought it meant (B) "set system property before each test". To make matters worse, some of our extension did (A), others (B). After some back and forth, particularly on what should happen if users override a value (in this case, a system property) in one of their tests, we decided to go with (B).

If I remember correctly, we considered adding (A) back in (i.e. having (A+B) behavior) for all such extensions: I think this should be possible and we may explore that in the coming weeks (we're not the fastest moving project). My recommendation is to sit on 1.6.2 for a while longer and wait how this shakes out before making any drastic changes.

[JoergSiebahn]

Thanks for giving an outlook to possible future changes @nipafx. I appreciate how you all take feedback into account for a little feature that has much more perspectives than I initially saw.

[beatngu13]

Due to our new reset mechanism, I think we can no longer combine BeforeEach/BeforeAll. But as @Michael1993 said, we could provide a configuration option:

@TestMethodOrder(OrderAnnotation.class)
// Default execution phase BeforeEach/AfterEach.
@SetSystemProperty(key = "foo", value = "bar")
class MyTestA {

	@BeforeAll
	static void setUpOnce() {
		assertThat(System.getProperty("foo")).isNull();
	}

	@BeforeEach
	void setUp() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
	}

	@Order(1)
	@Test
	void test1() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
		// Not visible in test2() because of AfterEach reset.
		System.setProperty("foo", "baz");
	}

	@Order(2)
	@Test
	void test2() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
	}

}

@TestMethodOrder(OrderAnnotation.class)
// Alternative execution phase BeforeAll/AfterAll.
@SetSystemProperty(key = "foo", value = "bar", executionPhase = BEFORE_ALL)
class MyTestB {

	@BeforeAll
	static void setUpOnce() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
	}

	@BeforeEach
	void setUp() {
		// Before test1() "foo" = "bar", after test() "foo" = "baz".
		assertThat(System.getProperty("foo")).isNotNull();
	}

	@Order(1)
	@Test
	void test1() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
		// Visible in test2() because of AfterAll reset.
		System.setProperty("foo", "baz");
	}

	@Order(2)
	@Test
	void test2() {
		assertThat(System.getProperty("foo")).isEqualTo("baz");
	}

}

Thoughts?

[nipafx]

If at all possible, I'd like to avoid an option because it makes the user choose.

Due to our new reset mechanism, I think we can no longer combine BeforeEach/BeforeAll.

That would be too bad. Can you explain why that may be the case?

[beatngu13]

Let's recall your comment here:

Assume the extension sets a global state (say, a system property or default locale) before and then resets the former state after.

• If it does that before/after the class, changes to that global state in test methods carry from one test to another.
  E.g. you could set a specific default locale before the test class starts, and then the (probably ordered) test methods can successively change the state and rely on previous changes.
  Then again, all changes to that state will carry from test to test, which may be detrimental to test stability.
• If it does that before/after each method, changes to that global state in test methods are overridden, which probably makes tests more stable.
  But it means that carrying the state from test to test as described in the first case is impossible.

Technically, the first option is more powerful as it allows more varied behavior (carrying the state from test to test before resetting after all of them is otherwise not possible).

One basically can't have both, at least that is my impression. See also my example above: in MyTestA, each test gets a fresh state since we reset after each test execution. In contrast, MyTestB allows to – as you said – "carrying the state from test to test before resetting after all of them is otherwise not possible".

[beatngu13]

After today's maintainer meeting: we would first try to keep the current/new behavior, but make the extension additionally visible in @BeforeAll. That is:

@TestMethodOrder(OrderAnnotation.class)
@SetSystemProperty(key = "foo", value = "bar")
class MyTest {

	@BeforeAll
	static void setUpOnce() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
	}

	@BeforeEach
	void setUp() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
	}

	@Order(1)
	@Test
	void test1() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
		// Not visible in test2() because of AfterEach reset.
		System.setProperty("foo", "baz");
	}

	@Order(2)
	@Test
	void test2() {
		assertThat(System.getProperty("foo")).isEqualTo("bar");
	}

}

@JoergSiebahn would that work for you?

[JoergSiebahn]

Hey @beatngu13, sorry for the late response. It sounds reasonable to me, that an annotation at class level applies to everything in the class. That would work for our case.
A little remark to the missing dependency of test1 and test2 in your example: I would avoid such prerequisites and requirement of the order to another test case. Independent tests are much easier to read in my opinion. I assume, this specific example could be mitigated, by annotating test2 with @SetSystemProperty(key = "foo", value = "baz") so that test2 reads baz?

[beatngu13]

Hi @JoergSiebahn,

thanks for the feedback! 👍 So, we will first follow the aforementioned approach, where we "try to keep the current/new behavior, but make the extension additionally visible in @BeforeAll".

PS: I only used the @Order annotation to make it more clear how the extension would behave. From the JUnit 5 user guide:

By default, test classes and methods will be ordered using an algorithm that is deterministic but intentionally nonobvious.