Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot use @Inject and @TestInstance(TestInstance.Lifecycle.PER_CLASS) #1828

Closed
cristhiank opened this issue Apr 2, 2019 · 5 comments
Closed
Assignees
Milestone

Comments

@cristhiank
Copy link
Contributor

Quarkus testing brokes when you mark a @QuarkusTest with @TestInstance(TestInstance.Lifecycle.PER_CLASS) and the test has an @Inject field

https://junit.org/junit5/docs/current/user-guide/#writing-tests-test-instance-lifecycle

This happens because Quarkus is bootstrapped in a @BeforeAll hook

public void beforeAll(ExtensionContext context) throws Exception {
ExtensionContext root = context.getRoot();
ExtensionContext.Store store = root.getStore(ExtensionContext.Namespace.GLOBAL);
ExtensionState state = (ExtensionState) store.get(ExtensionState.class.getName());
PropertyTestUtil.setLogFileProperty();
boolean substrateTest = context.getRequiredTestClass().isAnnotationPresent(SubstrateTest.class);
if (state == null) {
TestResourceManager testResourceManager = new TestResourceManager(context.getRequiredTestClass());
testResourceManager.start();
if (substrateTest) {
NativeImageLauncher launcher = new NativeImageLauncher(context.getRequiredTestClass());
launcher.start();
state = new ExtensionState(testResourceManager, launcher, true);
} else {
state = doJavaStart(context, testResourceManager);
}
store.put(ExtensionState.class.getName(), state);
} else {
if (substrateTest != state.isSubstrate()) {
throw new RuntimeException(
"Attempted to mix @SubstrateTest and JVM mode tests in the same test run. This is not allowed.");
}
}
}

And JUnit's behavior is to instantiate the Test class before calling the @BeforeAll hook.
The class instantiation is done in:
https://github.com/quarkusio/quarkus/blob/master/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java#L297-L309
and it tries to inject the dependencies but the ArC Container has not been initialized yet (chicken or egg problem)

I have a reproducer in https://github.com/cristhiank/quarkus-bugs

cristhiank added a commit to cristhiank/quarkus-bugs that referenced this issue Apr 2, 2019
@cristhiank
Copy link
Contributor Author

cristhiank commented Apr 2, 2019

I wonder if it would be better to do the dependency injection implementing a JUnit TestInstancePostProcessor ?.

https://junit.org/junit5/docs/current/user-guide/#extensions-test-instance-post-processing

@manovotn
Copy link
Contributor

manovotn commented Apr 3, 2019

Hmm, TestInstancePostProcessor probably won't help here. Quarkus is booted in beforeAll() and this invocation will still come first if the lifecycle is set to PER_CLASS.

Maybe it would make sense to instead move the Quarkus boot to createInstance() and store some information about it into context store so we do it only once per test class. Then you could be always sure that you have quarkus and arc booted by the time you attempt injection?

I'll dig into this and see if I am right or wrong :)

@geoand
Copy link
Contributor

geoand commented Apr 3, 2019

Having Quarkus booted only once is a design decision as far as I understand (we only want to do it once since it contains the entire augmentation phase which is something we don't want running multiple times).
So perhaps we should also perform the same check that is done in beforeAll (meaning that we could maybe check the state and call doJavaStart iff the state does not contain the Quarkus info).

@manovotn
Copy link
Contributor

manovotn commented Apr 3, 2019

@geoand yeah I understood as much.
There is already most of the logic needed to check if we are booted (ExtensionState is being stored into root context), so I am now testing that moving it into createInstance() won't break anything.

@manovotn manovotn self-assigned this Apr 3, 2019
@geoand
Copy link
Contributor

geoand commented Apr 3, 2019

@manovotn awesome!

@gsmet gsmet closed this as completed in 1ad1730 Apr 4, 2019
@gsmet gsmet added this to the 0.13.0 milestone Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants