search

quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.49k stars 2.59k forks source link

@QuarkusComponentTest feedback #34926

Open manofthepeace opened 1 year ago

manofthepeace commented 1 year ago

Description

I started playing with the new @QuarkusComponentTest. At first glance I really do like it as it is really fast in comparison of QuarkusTest.

While using I found 2 things that made it a bit tedious when I first tried it.

1- In quarkus the no args constructor are generated, but QuarkusComponentTest requires it when there is already a non no-arg constructor present. (i.e. single constructor without the Inject annotation)

I am not sure this is actually possible to fix, but it would be nice if I did not have to change my sources to use that test strategy. Meaning it would be nice if it could auto-magically create the no arg ctor as quarkus does when running the an app.

This is the error I get while running the test is the following jakarta.enterprise.inject.spi.DeploymentException: Normal scoped beans must declare a non-private constructor with no parameters: CLASS bean [types=[org.acme.Foo, java.lang.Object], qualifiers=[@Default, @Any], target=org.acme.Foo]

code to reproduce is as follow; adding commented out ctor + the @Inject makes the test work, but the app works without it.

@ApplicationScoped
public class Foo {
    Charlie charlie;

    //Foo() {}

    //@Inject
    Foo(Charlie charlie) {
        this.charlie = charlie;
    }
    String ping() {
        return charlie.ping();
    }
}

2- When Using @InjectMock on a bean that is not needed for example changing the above Foo class like this;

@ApplicationScoped
public class Foo {

    String ping() {
        return "OK";
    }

}

If my test class still has the @InjectMock of the Charlie bean, a NPE will follow like this one; which make troubleshooting quite hard as it does not tell what the issue is nor where it is.

java.lang.NullPointerException: Cannot invoke "io.quarkus.arc.InjectableBean.getKind()" because the return value of "io.quarkus.arc.InstanceHandle.getBean()" is null
    at io.quarkus.test.component.QuarkusComponentTestExtension$FieldInjector.<init>(QuarkusComponentTestExtension.java:854)
    at io.quarkus.test.component.QuarkusComponentTestExtension.injectFields(QuarkusComponentTestExtension.java:807)
    at io.quarkus.test.component.QuarkusComponentTestExtension.postProcessTestInstance(QuarkusComponentTestExtension.java:234)
    at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeTestInstancePostProcessors$10(ClassBasedTestDescriptor.java:377)
    at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.executeAndMaskThrowable(ClassBasedTestDescriptor.java:382)
    at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeTestInstancePostProcessors$11(ClassBasedTestDescriptor.java:377)
    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
    at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
    at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
    at java.base/java.util.stream.StreamSpliterators$WrappingSpliterator.forEachRemaining(StreamSpliterators.java:310)
    at java.base/java.util.stream.Streams$ConcatSpliterator.forEachRemaining(Streams.java:735)
    at java.base/java.util.stream.Streams$ConcatSpliterator.forEachRemaining(Streams.java:734)
    at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
    at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeTestInstancePostProcessors(ClassBasedTestDescriptor.java:376)
    at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$instantiateAndPostProcessTestInstance$6(ClassBasedTestDescriptor.java:289)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.instantiateAndPostProcessTestInstance(ClassBasedTestDescriptor.java:288)
    at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$testInstancesProvider$4(ClassBasedTestDescriptor.java:278)
    at java.base/java.util.Optional.orElseGet(Optional.java:364)
    at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$testInstancesProvider$5(ClassBasedTestDescriptor.java:277)
    at org.junit.jupiter.engine.execution.TestInstancesProvider.getTestInstances(TestInstancesProvider.java:31)
    at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$prepare$0(TestMethodTestDescriptor.java:105)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.prepare(TestMethodTestDescriptor.java:104)
    at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.prepare(TestMethodTestDescriptor.java:68)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$prepare$2(NodeTestTask.java:123)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.prepare(NodeTestTask.java:123)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:90)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
    at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
    at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
    at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
    at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
    at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
    at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
    at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
    at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
    at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
    at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
    at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
    at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
    at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
    at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
    at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
    at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
    at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:95)
    at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:91)
    at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:60)
    at org.eclipse.jdt.internal.junit5.runner.JUnit5TestReference.run(JUnit5TestReference.java:98)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:756)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)
    Suppressed: java.lang.NullPointerException: Cannot invoke "java.util.List.iterator()" because the return value of "org.junit.jupiter.api.extension.ExtensionContext$Store.get(Object, java.lang.Class)" is null
        at io.quarkus.test.component.QuarkusComponentTestExtension.preDestroyTestInstance(QuarkusComponentTestExtension.java:244)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeAllAfterMethodsOrCallbacks$13(TestMethodTestDescriptor.java:276)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeAllAfterMethodsOrCallbacks$14(TestMethodTestDescriptor.java:276)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeAllAfterMethodsOrCallbacks(TestMethodTestDescriptor.java:275)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestInstancePreDestroyCallbacks(TestMethodTestDescriptor.java:264)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.cleanUp(TestMethodTestDescriptor.java:153)
        at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.cleanUp(TestMethodTestDescriptor.java:68)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$cleanUp$10(NodeTestTask.java:167)
        at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.cleanUp(NodeTestTask.java:167)
        at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:98)
        ... 39 more

Implementation ideas

No response

quarkus-bot[bot] commented 1 year ago

/cc @geoand (testing)

manovotn commented 1 year ago

Also CC @mkouba @Ladicek

1- In quarkus the no args constructor are generated, but QuarkusComponentTest requires it when there is already a non no-arg constructor present. (i.e. single constructor without the Inject annotation)

I'd say this is expected given that we skip lots of Quarkus bootstrapping. In fact, I'd expect more of these functionalities that are there for convinience won't work because they mainly reside in Quarkus integration code whereas here we try to boot very minimal CDI (Arc) and some MP config with that.

2- When Using @InjectMock on a bean that is not needed for example changing the above Foo class like this;

That does look like a bug, thanks for reporting it back.

geoand commented 1 year ago

@manovotn even though 1 might be expected, it's definitely not a good used experience. We should try and see if we can overcome this limitation

Ladicek commented 1 year ago

The issue 1 is not caused by "skipping Quarkus bootstrap" [1], it's because we don't do bytecode transformations in @QuarkusComponentTest. I once tried adding bytecode transformations to ArcTestContainer and it didn't end well -- I'm pretty sure it would be the same case here. Everything comes from the system classloader, and some classes the test class refers to may get transformed, so in the end, we'd have to do the same classloader dance that @QuarkusTest does.

[1] Skipping Quarkus bootstrap would cause other issues, though. For example, I think static methods won't be intercepted in @QuarkusComponentTest, because that is solely implemented in the Quarkus ArC extension and not in ArC itself. (And, of course, it also requires bytecode transformation, but that would be a secondary problem here :-) )

manovotn commented 1 year ago

You're right, I meant to say we skip the bytecode transformation that full blown Quarkus normally does.

I think static methods won't be intercepted in @QuarkusComponentTest, because that is solely implemented in the Quarkus ArC extension and not in ArC itself. (And, of course, it also requires bytecode transformation, but that would be a secondary problem here :-) )

Yes, that's the other case I could think of. I don't think we use transformation in other cases though; the rest should be via just annotation manipulation, right? (for instance adding @Inject or scopes to classes with producers but no bean defining annotation etc...)

Ladicek commented 1 year ago

I briefly looked at ArC and this is what we use bytecode transformations for:

Ladicek commented 1 year ago

I figured out how to support bytecode transformations in ArcTestContainer, so it should be as well possible with @QuarkusComponentTest. It requires the same classloader dance as @QuarkusTest, but that doesn't surprise anyone at this point I guess :-)

mkouba commented 1 year ago

1- In quarkus the no args constructor are generated, but QuarkusComponentTest requires it when there is already a non no-arg constructor present. (i.e. single constructor without the Inject annotation)

We should be able to fix this thanks to @Ladicek's work. TBH I'm not very happy we'll introduce this kind of hack. But if it helps :shrug:.

2- When Using @InjectMock on a bean that is not needed for example changing the above Foo class like this;

If my test class still has the @InjectMock of the Charlie bean, a NPE will follow like this one; which make troubleshooting quite hard as it does not tell what the issue is nor where it is.

You should get a different error in the main branch, something like The injected field X expects a mocked bean; but obtained null. But we should probably improve the error message a little bit ;-)

ennishol commented 5 months ago

@manovotn even though 1 might be expected, it's definitely not a good used experience. We should try and see if we can overcome this limitation

I also tried the new extension and it is sort of disappointing that quarkus does support beans without a no-args constructor but @QuarkusComponentTest does not. Then maybe, the name should be @CdiComponentTest to stress that the quarkus feature is not supported here.

I understand that CDI specs require it, but it would be a better user experience if the @QuarkusComponentTest does match what the framework offers. I'm not sure about the other CDI based framework offer but comparing with spring it is really a limitation from the user perspective

Otherwise, it is an awesome extension :)

Ladicek commented 5 months ago

I had a pull request that allowed this, see #35473. It requires a fairly ugly "classloader dance", so I ended up closing it -- but that doesn't mean we don't want to do this. It is very much supposed to work (at least from my perspective :-) )

@holly-cummins is pursuing a holistic cleanup of classloader manipulation in the testing extensions, which should make this much less invasive. It's a long run though.

ennishol commented 5 months ago

Thanks! Nice to know it was not forgotten or abandoned. Looking forward to the improvement!