[junit5] ServiceUseExtension is not stateless about osgi-test HOT 5 CLOSED

Then the documentation is contradictory: https://junit.org/junit5/docs/current/user-guide/#extensions-registration-programmatic Note the examples that reference the field. How can that work if there is no state? Also note that ServiceUseExtension must be instantiated programmatically.

from osgi-test.

Possibly the documentation is a little misleading. But I couldn't see the example that you referred to where it references a field. Perhaps you're referring to the Kotlin example? I'm far from a Kotlin guru, but I think that server.serverUrl in Kotlin is equivalent to server.getServerURL() in Java (so it's not actually a field reference, but a method reference). I could be wrong.

For a definitive word on this, there was this comment by @sbrannen on the AssertJ GitHub repo: assertj/assertj#1418 (comment):

As a general rule of thumb, extensions in JUnit Jupiter should never be stateful.

Instead, they should store state in the ExtensionContext.Store whenever possible in order to allow JUnit to properly scope the state for the current test class, current test method, etc.

This comment was made in the context of the (now deprecated) JUnitJupiterSoftAssertions, which was also registered programmatically via @RegisterExtension.

The whole thread serves as a useful example, as the original JUnitJupiterSoftAssertions extension had a similar issue (state variable) and this issue was where the fix was discussed and implemented. Basically, instead of storing it in an instance variable you put it in the ExtensionContext.Store object.

from osgi-test.

Here's what I'm talking about:

	@RegisterExtension
	BundleContextExtension		bcExt	= new BundleContextExtension();
	@RegisterExtension
	ServiceUseExtension<Foo>	fooUse	= new ServiceUseExtension.Builder<>(Foo.class, bcExt)	//
		.cardinality(0)
		.build();

	@Test
	public void test() throws Exception {
		assertThat(fooUse.getService()).isNull();
	}

The semantics of this use case according to the docs don't make any sense to need to store the state anywhere by in the instance object.

from osgi-test.

I think you're talking about extensions that can be declared via @ExtendWith(DatabaseExtension.class). In that case I'd agree, but we're not able to use that at all since we need to build out the definition of the service and, as far as I'm concerned, specifically want the semantics to be per method. So I fail to see the reasoning behind storing the state elsewhere.

from osgi-test.

I agree that we must use @RegisterExtension rather than @ExtendWith. I also agree that it should be per-method semantics.

However, both of these also applied to the AssertJ Jupiter soft assertions extension, and @sbrannen was still concerned about about the extension keeping its own state (as per the issue that I referenced). I don't fully understand the details of why, but I think it had to do with some of the other features of Jupiter around test ordering and test class lifecycles - the specific (failing) use cases are documented here and here. They involve the use of the @TestMethodOrder annotation and @TestInstance(PER_CLASS) lifecycle semantics.

The answer they came up with was to use parameter injection to inject the SoftAssertions object into the test method as a parameter. So the equivalent for your use case would look something like this:

@RegisterExtension
ServiceUseExtension<Foo>	fooUseExt = new ServiceUseExtension.Builder<>(Foo.class, bcExt)	//
	.cardinality(0)
	.build();

@Test
public void test(ServiceAware<Foo> fooUse) throws Exception {
	assertThat(fooUse.getService()).isNull();
}

...or perhaps as an alternative:

@RegisterExtension
ServiceUseExtension<Foo>	fooUseExt = new ServiceUseExtension.Builder<>(Foo.class, bcExt)	//
		.cardinality(0)
		.build();

// Instance injected by fooUseExt at the appropriate time
ServiceAware<Foo> fooUse;

@Test
public void test() throws Exception {
	assertThat(fooUse.getService()).isNull();
}

The first implementation would be better if only a couple of your test methods needed the ServiceAware functionality, and the second implementation would be better if most of your methods needed the ServiceAware functionality.

Implemented this way, ServiceUseExtension doesn't even need to extend/implement ServiceAware.

Also, using the ExtensionContext.Store, it should be possible for the ServiceUseExtension to automatically find the BundleContextExtension without having to explicitly pass it in as a parameter.

I can have a bit of a play with this and submit a PR for your consideration. I'm interested for my own reasons as I'm currently trying to implement an extension of my own and I'm trying to work through and understand these issues (which is how I actually got onto this issue of state in the first place).

I'd also be interested if @sbrannen or someone from the JUnit team could weigh in with a quick word of advice.

from osgi-test.