Enclosing classes vs Before/AfterAll

[nipafx]

In #473/#476 we removed the search for annotations on enclosing classes because it's handled by the Before/AfterAll callbacks. That made me wonder whether there are other extensions that misuse PioneerAnnotationUtils::findClosestEnclosingAnnotation. I looked at two extensions:

• Issue extension uses it correctly because it needs to file a report entry for each test based on the annotation on some enclosing class.
• Default time zone (and I assume locale as well) use it correctly because they don't implement the Before/AfterAll extension points. That means the classes are not set up by Jupiter and so the method needs to go hunting for enclosing annotations. This can be refactored quite easily to the all callbacks without looking for enclosing (although one test fails, then).

Questions:

• do we prefer one approach over the other?
• do we want to refactor towards that?

[Bukama]

Maybe (obviously) I don't get it: When the two extensions use it correctly then why change them?

[nipafx]

I find it a bit weird that different extensions take two different approaches to achieve the same goal. As I see it, this is not an "it depends" situation - the extensions have the exact same requirements, so one solution should suffice. Maybe one of them is better and we should pick that one, but even if not - having just one eases learning and maintenance.

[Bukama]

Ah okay, thanks for clarification. I agree with you that we should have one approach for the same thing and not multiple

[Michael1993]

Does it really matter? Both approaches solve the same problem just in different ways. Personally, I think not implementing BeforeAll and AfterAll leads to better control of the extension lifecycle (basically meaning there is no weird bug from finding the same annotation in both BeforeEach and BeforeAll), so that's my preference.

[nipafx]

I found another dimension to this triggered by what @beatngu13 wrote on #496:

Not sure if we should remove this from the docs. Today, we did a JUnit 5 workshop with @marcphilipp and stumbled over this:

@ClearSystemProperty(key = "foo")
@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
class SystemPropertyTest {

	@Test
	@Order(1)
	void test1() {
		System.setProperty("foo", "bar");
		assertThat(System.getProperty("foo")).isEqualTo("bar");
	}

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

}

test2() fails since test1() leaves the foo system property in a dirty state. However, users had the impression that annotating the test class with @ClearSystemProperty would prevent this (which would be the case without BeforeAllCallback/AfterAllCallback 😉).

I would suggest to keep the extension scope in the docs, but maybe rephrase it a bit:

@DefaultLocale and @DefaultTimeZone implement BeforeAllCallback (for class-level annotations) in addition to BeforeEachCallback (for method-level annotations). Hence, you can also use them in setup/teardown methods if the enclosing container is annotated accordingly.

Thoughts?

Checking the Javadoc, it says:

ClearSystemProperty is repeatable and can be used on the method and* on the class level. If a class is notated, the configured property will be cleared for all tests inside that class.

The last sentences can indeed be easily misunderstood to mean that the property is cleared before each test. And at the moment (i.e. before #496 is merged), the default locale / time zone extensions do behave as the reader of the above text expected and set a value before each test. Then again, this doesn't really work well with manually setting the value (in this case the default locale), either:

The class level annotation will store the current default locale before each test and reset it after. If a test sets the default locale manually with Locale.setDefaule, the extension will read that value before running the next test and then reset to that value afterwards. That means, the manually configured locale will be carried over from test to test by the extension and remain the default even when the extension is out of scope.

As it is now, #496 will make the locale / time zone extensions implement the before/after all extension points, which at least means that the when the extension goes out of scope, it resets the value to the one it was before executing any of the tests and that means the manually set value will be overridden for good. That's a good thing, I think.

Now the question is whether we should try to also create that behavior where a class level annotation sets before each test execution - maybe without storing the old value, knowing that the value at the beginning of executing the test class was stored and will eventually be restored?

[nipafx]

Just so it doesn't get overlooked: #496 changes the behavior of default locale / time zone extensions inasmuch that a class-level annotation no longer sets a value before each test, which means if users manually set locales or time zones, their test behavior changes.

[Bukama]

If I understood @nipafx correctly than one way of implementation it will break the extension, while the other way does not?

[nipafx]

We just had a lengthy discussion about what this should means:

@CoolExtension
class MyTests {

	@Test
	void testMethodA() {
		// ...
	}

	@Test
	void testMethodB() {
		// ...
	}

}

Namely, is @CoolExtension gonna do its thing once before/after all tests (meaning it "brackets" the test class) or once before/after each test (meaning it "brackets" each test method).
This results in different behaviors.
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).
But it is also more error-prone and if the common use case is indeed to just apply the extension to each test method, then the only way to achieve that with the first option is to copy-paste the annotation on each method (and nobody wants that!).
I also think that the second option is the more common interpretation of what applying an annotation to a class means in this context.

Taken together, here's my proposal:

• document the second option as the default in the contribution guide
• refactor all extensions to behave like described in the second option except where that doesn't make sense
• document each extension's behavior in its docs

So we will really only have that as a default, individual extensions can always diverge, where it makes sense.
Either way, it needs to be described in their docs.

[nipafx]

@beatngu13 Can you update #496 according to my last comment? 😬

[nipafx]

PR is ready to merge. We need to discuss whether this requires a 2.0 release.

It's definitely a "breaking change" in the sense that tests may behave differently afterwards. Then again, this could easily be considered a bug fix ("prevent leaking state from one test to another"). I guess philosophically, since somebody always relies on bugs, fixing those could be considered breaking changes as well. So, really, is there any difference between rewriting an API and fixing a bug? 🤷🏾‍♂️ 😁

Anybody convinced by that equivocation? Can we release with 1.7?