Junit5: Introduce extension API for creating the test instance

[smoyer64]

Overview

The javadocs for the JUnit 4 TestRule states:

The Statement that executes the method or suite is passed to each annotated Rule in turn,
and each may return a substitute or modified Statement, which is passed to the next Rule, if any.

I currently have a JUnit 4 rule that uses this ability to replace the framework's provided instance with one that has been initialized via JavaEE CDI (Weld) but this isn't currently possible with JUnit 5. The MockitoExtension post-processes the instance provided by the engine - doing that in a WeldExtension wouldn't be terribly hard if we're simply talking about processing the @Inject annotations but fully initializing the instance as a proper CDI managed object would be non-trivial and would require duplicating much of the functionality already provided by the CDI container.

Feature request

Provide an Callback that allows an Extension to produce or replace the test instance. In the case of CDI, you'd replace:

    Object testInstance = new ClassUnderTest();

with something like:

    Object testInstance = beanManager.select(ClassUnderTest.class).get();

Deliverables

• [x] Provide the ability to replace or alter the engine's provided test instance.

[smoyer64]

The weld team has responded:

(08:45:02 AM) mkouba: smoyer1: hi
(08:45:14 AM) mkouba: what do you mean by "initialize"?
(08:45:20 AM) mkouba: to perform injection?
(08:49:20 AM) smarlow_ [~smarlow@redhat/jboss/smarlow] entered the room.
(08:49:43 AM) smoyer1: well ... injection is part of it right?
(08:50:12 AM) smoyer1: if it's @Observing events those need to be registered too ... same with Interceptors, etc
(08:50:47 AM) smoyer1: basically ... instead of doing beanManager.select(ClassUnderTest.class).get
(08:51:44 AM) smoyer1: I need that beanManager to perform the same functions on a test instance that was originally created with new ClassUnderTest()
(08:54:57 AM) mkouba: that's not possible, you can only perform injection on a non-contextual instance
(08:55:09 AM) mkouba: observers are initialized during bootstrap
(08:55:39 AM) mkouba: interceptors and decorators are only applied to instances created by the container
(08:57:16 AM) smoyer1: I was afraid of that ... I'd created an Extension that @Observes the ProcessInjectionTarget and couldn't build the CreationalContext I needed to make it work
(08:58:29 AM) mkouba: smoyer1: btw, are you aware of https://github.com/weld/weld-junit?
(08:58:40 AM) mkouba: so far we only have JUnit4 extension

[marcphilipp]

This has been discussed in #203. As stated there, I think replacing would be potentially dangerous. So a way out would be a new extension point as proposed by @sbrannen:

With that in mind, I would propose a new extension -- call it InstanceFactory or similar -- that can only be registered once per test container. Any attempt for a second extension to register itself as such a factory would then result in an exception.

[marcphilipp]

@junit-team/junit-committers What do you think?

[smoyer64]

I've read issue #203 before and for some reason it didin't register to me that this issue is describing the same problem. Sorry for duplicating an old issue!

If you want to close this as a duplicate, I'll add a comment on #203 indicating that JavaEE CDI (assuming the implementation provides no custom API) also requires the ability to be a test instance producer/provider. And I agree whole-heartedly that it's better to simply let CDI produce the instance instead of replacing one provided by the engine.

[marcphilipp]

No worries! Let's keep this issue open as we've now identified a use-case where this extension would actually suffice.

[smoyer64]

It's always amazing to me how real-world use-cases tend to clarify the problem you're trying to solve. I hope I'm not being too extreme these last few days ... I'm not here to offend anyone (and if fact I'm a huge cheerleader when it comes to JUnit 5).

Saying no, in general, won't offend me at all, and the junit-team has never ignored any of the use-cases I've offered. Not yet or no but are still appreciated and I know what a huge undertaking it is to create something that everyone agrees with.

[marcphilipp]

No worries! You're certainly not offending anyone by providing input and suggestions. 🙂

We have been thinking about introducing a quota for new issues per user per day, though. 😜

[smoyer64]

I have a lot of colleagues at Penn State with Github accounts ... at least it won't seem like I'm dominating things that way! My day job will involve work not related to testing for the next couple of weeks so you'll have a chance to catch your breath ... inhale, exhale.

[marcphilipp]

Team decision: Introduce a new Extension subinterface TestInstanceFactory with a single method createTestInstance(Class testClass, ClassLoader classLoader).

[kcooney]

@marcphilipp what happens if more that one TestInstanceFactory is installed​?

[sbrannen]

what happens if more that one TestInstanceFactory is installed​?

An exception will be thrown, thereby halting the execution of the affected test class.

[sbrannen]

... as stated in #203:

Any attempt for a second extension to register itself as such a factory would then result in an exception.

[kcooney]

@sbrannen cool. Thanks!

[nipafx]

Would it make sense to mirror the parameter resolver's behavior and have a supports and a create method? (All extensions get asked, only one is allowed to say yes.)

I have to admit that I don't have a use case for that and that due to extensions most likely being added to the class itself (although ServiceLoader is now also an option) I don't think there is a high chance of several extensions meeting each other but leave it to users to find crazy use cases. 😜

[sbrannen]

Would it make sense to mirror the parameter resolver's behavior and have a supports and a create method? (All extensions get asked, only one is allowed to say yes.)

That could also be a viable option. Thanks for suggesting it.

[bedert]

Perfect - i was just trying to replace my WeldJunit4Runner with a JUnit5 equivalent and came across this issue. With the proposed solution I could initialize Weld in the TestInstanceFactory and retrieve the instance of the test class from Weld. Just wondering what the best place for Weld shutdown would be, assuming that Weld is started/stopped once per test class. Probably in a AfterAllCallback right?

[sbrannen]

Probably in a AfterAllCallback right?

yes

[filiphr]

I am thinking of migrating a Runner that we use for testing an annotation processor to JUnit 5. I am not sure if this is doable with what is currently there, or I would need this issue to migrate my Runner.

I am sorry if this is the wrong place for my comment. I can remove it if you think that it is not relevant to the issue.

Our runner extends the JUnit 4 ParentRunner. Internally it creates 2 child runners. When one test is run it is actually run twice, once with each runner. Each runner creates it's own custom Classloader (because we need to actually compile some classes with javac and ejc before the test runs). The custom classloader then picks up the compiled classes from the custom locations.

My idea was to use Extension for each of child runners. However, after thinking more about it, I think that this is not correct. My other idea was to to do something like a test factory that would create the multiple test instances, from one test method. however, I am not sure if I would have a control over how those instances are executed (each one should be done with a different statement).

I also read #201, which might be related as well.

[marcphilipp]

If we introduced TestInstanceFactory extension point, you could combine that with a TestTemplateInvocationContextProvider that provides two invocations, one for each of the runs. Does that make sense?

[filiphr]

@marcphilipp Yes it makes sense thanks a lot. I'll have a better look and maybe I would be able to make it work with what we have, by just changing the current thread classloader (this is what we do with JUnit 4 at the moment). I have some more question for the TestTemplate, but I'll write in the gitter room

[kcooney]

@filiphr just thinking out loud, would an extension point that allowed you to specify the class loader that is used to load the test class do? Or do you really need to control creation of the instance?

The problems I see with an extension controlling the creation of the instances are 1. The extension can return an unrelated class, which would be confusing, 2. It could prevent JUnit from caching Method objects, 3. It wouldn't allow the test to use an extension that passed in constructor parameters

[filiphr]

I'll try to do something first and I will get back to you. That might actually work.

I had a better look. Currently we are changing the classloader before the execution of the method. Maybe we can use BeforeTestExecutionCallback and do our compilation there.

[filiphr]

@kcooney I had a better look. We are actually reloading the class between methods (not always as it can be cached). I think that if I have an extension point via which I can specify the class loader and store this classloader for using in my other entries, it might actually work. I will have to additionally check what will happen in between method calls when we need to modify something in it.

If you are interested I can show you the project and what we have in place right now. It is an open source project 😄

[bangarharshit]

This is also super useful with Robolectric.

[kevcodez]

What is the current status on this? This seems to block CDIUnit aswell as Powermock to properly integrate JUnit 5 - just wondering.

[sbrannen]

@kevcodez, we haven't forgotten about this issue. Rather, we're just busy with other issues.

There are in fact quite a few open issues that we _should_ address, but we unfortunately cannot tackle everything at once.

In any case, this issue is currently in the 5.x backlog which means we will get to it in an upcoming 5.x release. Heck, we might even move it to the 5.2 backlog (once that exists). 😉

[vmassol]

Hi guys, I currently have a JUnit4 custom Suite https://github.com/xwiki/xwiki-rendering/blob/2e7af2e60fd6661ec48142324cd3fe10411fe0bc/xwiki-rendering-test/src/main/java/org/xwiki/rendering/test/integration/RenderingTestSuite.java#L78 which automatically registers new tests to the junit runner and which allows to write very simple tests:

@RunWith(RenderingTestSuite.class)
public class IntegrationTests
{
}

The custom suite reads *.test files found in the CL's classpath and generate dynamic tests from them.

I'd like to move this to JUnit5 but I'm a bit stuck ATM since it seems the best approach would be to use some test factory or parametrized test but this would require each of my tests to declare a test method with something like:

class IntegrationTests
{
   @TestFactory
   Stream<DynamicTest> renderingTests()
   {
           return RenderingTestSuite.generateStreamOfDynamicTests();
   }
}

I'd really like to be able that my test keep being as simple as the following (and this without duplication):

@ExtendWith(RenderingExtension.class)
public class IntegrationTests
{
}

Are there plans to support this? Is this issue related to this? The title seems so but I'd like confirmation. Thanks a lot!

PS: I've also asked the question on Gitter and got some replies here: https://gitter.im/junit-team/junit5?at=5b142454c712f5612555bfcc

[sormuras]

@vmassol I don't think that a custom TestInstanceFactory can help you with your use-case.

You want to generate "tests" at runtime, here we talk about how to create the instance of a class that contains "testable" members.

[vmassol]

@sormuras Ok too bad, should I open another issue for this use case?

[sormuras]

Yes, please.

[vmassol]

Done, thx, see https://github.com/junit-team/junit5/issues/1444

[sbrannen]

This has been addressed in master in 5b4c642be9b1d09f9b9cebad7dd55fa40aae78bc and further refined in 87a3005b96c99f7a30fa89d6466058771683ac3b, 8f8e94752528127d90e18e6a06f3f31cb8de83f9, and aae5f5df3053dc45de1eb32fae864973dccc51d3.

[dpreussler]

@sbrannen i just looked at the implementation. It seem's is not possible to use a different class loader to instantiate the tests.
i think a few of us where hoping that this extension will bring sth back Runners easily supported: using a different class loader.

Use cases would be for sth like https://github.com/robolectric/robolectric

I tried to test convert more simple Runner of:
https://github.com/dpreussler/kotlin-testrunner
which also uses a different classloader

Bu the flaw in this version of TestInstanceFactory seems that it already receives a Class instead of a String. So loading the classs with a different classloader leads to errors like:

org.junit.jupiter.api.extension.TestInstantiationException: TestInstanceFactory [...Extension] failed to return an instance of [de.jodamob.kotlin.testrunner.tests.TestWithRunnerOnClass] and instead returned an instance of [de.jodamob.kotlin.testrunner.tests.TestWithRunnerOnClass].

do you need this check in ClassTestDescriptor:

if (!this.testClass.isInstance(instance)) {

haven't tried yet but if this would not be there or just comaring the names, it might be fine

[sbrannen]

Hi @dpreussler,

We do indeed need to ensure that the returned object is of the correct type; otherwise, method invocations will fail against an incompatible target object.

Comparing the names would not suffice, since a factory could return a subclass of the test class type (e.g., a _proxy_).

With regard to using a different ClassLoader to instantiate the test class, that is a topic that has been discussed in #201 but never finalized.

Perhaps we should extract the TestInstanceClassLoaderProvider proposal from #201 (or simply a ClassLoaderProvider or ClassLoaderFactory) and push that out with 5.3.

That would of course delay the 5.3 release, but it may be a good idea to publish that now alongside TestInstanceFactory and the parallel test execution support coming in 5.3.

@junit-team/junit-lambda, thoughts?

[sbrannen]

Reopening to discuss the previous comments.

[sbrannen]

We do indeed need to ensure that the returned object is of the correct type; otherwise, method invocations will fail against an incompatible target object.

For example, if the test class is instantiated by a different ClassLoader, attempts to invoke @BeforeAll methods (that were loaded reflectively via the original test Class) will result in an error like the following.

java.lang.IllegalArgumentException: object is not an instance of declaring class
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:564)
    at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:515)
    at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:115)
    at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.lambda$9(ClassTestDescriptor.java:363)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:72)
    at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.invokeBeforeAllMethods(ClassTestDescriptor.java:362)

[sbrannen]

Thus, if an extension wishes to use a custom ClassLoader for instantiating the test class, that ClassLoader would have to be provided to the JupiterTestEngine early in the test execution process (i.e., before Jupiter attempts to load lifecycle callback methods for the given test class).

[sbrannen]

I suppose the Jupiter engine could alternatively _lazily_ look up callback methods instead of performing an eager lookup with caching. Note, however, that doing so would result in a slight decrease in performance for all test classes that contain more than one testable method.

[sbrannen]

Bu the flaw in this version of TestInstanceFactory seems that it already receives a Class instead of a String. So loading the classs with a different classloader leads to errors like:

org.junit.jupiter.api.extension.TestInstantiationException: TestInstanceFactory [...Extension] failed to return an instance of [de.jodamob.kotlin.testrunner.tests.TestWithRunnerOnClass] and instead returned an instance of [de.jodamob.kotlin.testrunner.tests.TestWithRunnerOnClass].

Thanks for pointing that out, @dpreussler! 👍

I just added a test for this (currently expected) behavior in 2d1c81c23e3438eb5740a4e8d3352e122fd929b7.

[sbrannen]

And... I just improved the error message for such use cases in a4de34402a23335e0d3cfd42ab89c02baf5035e0.

[dpreussler]

yes so far I was only be able to use different classloader when building a custom engine but that's overkill for cases like this.

I guess TestInstanceClassLoaderProvider from https://github.com/junit-team/junit5/issues/201 might be a way.
My question would be if it so different the instantiate a test and to load that class behind?
TestInstanceFactory is nice if would not already have loaded the Class, it could have 2 methods and the default would do class.ForName()

[sbrannen]

I guess TestInstanceClassLoaderProvider from #201 might be a way.

My question would be if it so different the instantiate a test and to load that class behind? TestInstanceFactory is nice if would not already have loaded the Class, it could have 2 methods and the default would do class.ForName()

Custom test instantiation and custom class loading may at times be related, but they may well be orthogonal concerns.

More importantly, adding support for a custom ClassLoader is considerably more work than supporting custom test instantiation. For example, JUnit Jupiter would have to ensure that the user-supplied ClassLoader is used everywhere that Jupiter uses reflection, looks up annotations, etc. Thus, a custom ClassLoader would have to be _retrieved_ very early in the game, in contrast to the TestInstanceFactory which is used rather late in the overall test plan execution.

[sbrannen]

In light of the above discussions, I am re-closing this issue.

Any concerns regarding the need for a custom ClassLoader should be discussed in #201 or in a new issue.