QuarkusComponentTest

[mkouba]

https://groups.google.com/g/quarkus-dev/c/gMgnPb_NzgU

TODO

• Documentation
• Improve test coverage

[quarkus-bot]

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

• title should count at least 2 words to describe the change properly

This message is automatically generated by a bot.

[github-actions]

🙈 The PR is closed and the preview is expired.

[Ladicek]

I like this very much! I only looked at the code very briefly, but didn't find any glaring issue. Will review properly next week.

[geoand]

I'll have a look next week.

[holly-cummins]

, looking next week!

[stuartwdouglas]

This is awesome, I will look at adding support for continuous testing. It's a real pity we can't call this QuarkusUnitTest and rename QuarkusUnitTest to QuarkusExtensionTest.

My only concern is that our testing story might be getting a but muddled now. We need to be sure that we have a good docs/guides that explain to beginners what the different options are and the use cases for them.

[geoand]

My only concern is that our testing story might be getting a but muddled now

Yeah, I'm afraid of the same... We totally need to do an excellent job explaining when to use what

[holly-cummins]

Meanwhile, this awesome work, and a big changeset, isn't merged, because we need to get the bigger picture straight. I think as soon as we document any names it's very hard to change them, but does it make sense to split this into something which is mergeable, but we don't tell people about it. Then it's a question of designing the optimal user experience, which might include changing the names of other things, and possibly also these things ... and bringing in the documentation once we're happy with the names and relationships of everything?

[holly-cummins]

@mkouba, in this example, can I check my understanding?

@QuarkusComponentTest(Foo.class) 
@TestConfigProperty(key = "bar", value = "true") 
public class FooTest {

    @Inject
    Foo foo; 

    @TestMock // or @Inject, or @InjectMock 
    Charlie charlie; 

    @Test
    public void testPing() {
        Mockito.when(charlieMock.ping()).thenReturn("OK"); 
        assertEquals("OK", foo.ping());
    }
}

The expectation is that the injected Foo is the real Foo, but anything injected into Foo would be a mock?
And here Charlie is a mock, but if we'd used @Inject, it would have still been a mock? And the only circumstances under which Charlie would be a real thing are

• if it had been listed in the `@QuarkusComponentTest annotation
• if it was something foundational in Quarkus, like a CDI engine

If I wanted two things to be real, could I add two @QuarkusComponentTest annotations, like

@QuarkusComponentTest(Foo.class)
@QuarkusComponentTest(AlsoReal.class)

?

[holly-cummins]

Sorry, me again. :)

Would it be correct to say that the behaviour of QuarkusComponentTest is to make everything except core CDI and the thing listed as the annotation value a mock?

As a straw man, here are some alternative programming models. It’s the same core code and user need as what @mkouba implemented, just a different window dressing.

Option A

@QuarkusTest(mockEverything=true) // terrible attribute name, but you get the idea
@TestConfigProperty(key = "bar", value = "true") 
public class FooTest {

    @UnderTest
    Foo foo; 

    @Inject 
    Charlie charlie; 

    @Test
    public void testPing() {
        Mockito.when(charlie.ping()).thenReturn("OK"); 
        assertEquals("OK", foo.ping());
    }
}

Here, we have the same confusion where ‘@Inject’ is actually injecting a mock, but at least there’s a clear distinction between the ‘inject a mock’ annotation and the ‘make this real’ annotation. The behaviour of QuarkusTest is now rather ambiguous/broad, depending on the presence of the mockEverything attribute. An alternative model would be an @MockEverything annotation, but I think the attribute would be simpler to implement and clearer.

One thing I like about this is the slight improvement in DRYness. I don’t have to specify Foo.class as a parameter to the QuarkusComponentTest annotation, because I’m already injecting it into my test. I just use a different form of injection. If I had a few things I wanted to be real, I could inject them all.

Option B, de-mocking

@QuarkusTest(goFast=true) // another terrible attribute name, but you get the idea
@TestConfigProperty(key = "bar", value = "true") 
public class FooTest {

    @Inject // this tells QuarkusComponentTest to make this real!
    Foo foo; 

    @InjectMock // this makes/keeps this fake! 
    Charlie charlie; 

    @Test
    public void testPing() {
        Mockito.when(charlie.ping()).thenReturn("OK"); 
        assertEquals("OK", foo.ping());
    }
}

I think this one is evil in terms of the semantics, and nightmarish to implement. It's also my favourite. :) The idea is that goFast=true conveys the intention (Quarkus, please be Sir-Mock-A-Lot, and I am willing to sacrifice accuracy). The semantics of @Inject are sort of the same as normal - if you use @Inject, it’s real - but with the added wrinkle here, that @Inject is actually a de-mocking mechanism. So it drags the thing back from the land of the mocks, to the land of the living. If you use @Inject, you must want it to be real.

Then, it’s clear that to get a mock, you’d have to use @InjectMock. If we don’t want to bring in the quarkus-junit5-mockito package, we’d have to make another annotation with the same name, and then just use build-time checking to sort out if users get confused. On the other hand, if the mock is always a mockito mock, maybe depending on quarkus-junit5-mockito is ok? We could even, for bonus points, implement org.quarkus.powermock.InjectMock in the future if we wanted to be more flexible with the dependency and let people choose a mocking framework, while keeping the basic semantics of “I would like this to be a mock, please.”

The nice thing about this is that, until you think about it too hard, the semantics and programming model seem to be exactly what people are used to. Inject gets you something real, InjectMock gets you something mocked. The fact that most things are mocked anyway is conveyed by the goFast=true or mockEverything=true or whatever qualifier.

[geoand]

On a side note, one thing that I believe this feature will result in, is people opting for Panache Repository instead of Panache Active Record - unless we also integrate with quarkus-panache-mock

[mkouba]

Is it ... auto-generated? But not using mockito? And already existing whether users refer to it or not? Or have I got muddled, and it is it that it is is using Mockito, but not quarkus-junit-mockito?

It's a mocked dependency. By default, a Mockito mock is used. But a user can provide any type of mock through the programmatic API, i.e. no mocking library needs to be used.

The expectation is that the injected Foo is the real Foo, but anything injected into Foo would be a mock?
And here Charlie is a mock, but if we'd used @Inject, it would have still been a mock? And the only circumstances under which Charlie would be a real thing are

More or less correct. The "real" is not a good name though. All the beans are real, but only the components (and some built-in beans) are discovered/created from the classes.

If I wanted two things to be real, could I add two @QuarkusComponentTest annotations, like

Nope, you can just do @QuarkusComponentTest({Foo.class, AlsoReal.class}).

Would it be correct to say that the behaviour of QuarkusComponentTest is to make everything except core CDI and the thing listed as the annotation value a mock?

I think so. The goal is to test the components listed in the annotation or otherwise passed to the QuarkusComponentTestExtension.

@holly-cummins

[mkouba]

On a side note, one thing that I believe this feature will result in, is people opting for Panache Repository instead of Panache Active Record - unless we also integrate with quarkus-panache-mock

@geoand Good point. But I have no idea how quarkus-panache-mock actually works. So I can't say more about this topic ;-).

[geoand]

@FroMage can enlighten us :)

[mkouba]

As a straw man, here are some alternative programming models. It’s the same core code and user need as what @mkouba implemented, just a different window dressing.

Interesting ideas. The only advantage of "explicit" components definition is that you don't need to inject your components at all but use programmatic lookup in the test instead (e.g. CDI.current().select(Foo.class) or even Arc.container().instance(Foo.claass)).

Option A
One thing I like about this is the slight improvement in DRYness. I don’t have to specify Foo.class as a parameter to the QuarkusComponentTest annotation, because I’m already injecting it into my test.

On the other hand, you need to specify the mockEverything=true, and if you accidentally change it to mockEverything=false you'll break the things anyway...

An alternative model would be an @MockEverything annotation, but I think the attribute would be simpler to implement and clearer.

What's the difference between @MockEverything and @QuarkusComponentTest? ;-)

Option B, de-mocking

That's basicaly what we have now, except that you want the components to be collected from the test injection points and reuse the @QuarkusTest annotation (I'm not completely sure it's a good idea).

We could even, for bonus points, implement org.quarkus.powermock.InjectMock in the future if we wanted to be more flexible with the dependency and let people choose a mocking framework, while keeping the basic semantics of “I would like this to be a mock, please.”

In that case, we should probably introduce a new annotation in the io.quarkus.test.junit package... In theory, it does not have to be @InjectMock but more like @ConfigureMock or something along these lines. So that it's clear that we aim to "configure" the mock under the hood.

[mkouba]

Meanwhile, this awesome work, and a big changeset, isn't merged, because we need to get the bigger picture straight. I think as soon as we document any names it's very hard to change them, but does it make sense to split this into something which is mergeable, but we don't tell people about it. Then it's a question of designing the optimal user experience, which might include changing the names of other things, and possibly also these things ... and bringing in the documentation once we're happy with the names and relationships of everything?

We could also mark this as an experimental/unstable feature, or?

[geoand]

We could also mark this as an experimental/unstable feature, or?

+1 for experimental. I would also update the Javadoc to reflect this

[mkouba]

Next round of changes:

• QuarkusComponentTest#value() is now used to define additional component classes, the initial set is derived from the test class, i.e. the types of all fields annotated with @Inject are considered the component types
• renamed @TestMock to @ConfigureMock (note that the underlying mock does not necessarily have to be a mockito mock... it could be anything)
• marked the feature as experimental: updated the docs, added @io.smallrye.common.annotation.Experimental to QuarkusComponentTest and QuarkusComponentTestExtension, updated the description in the pom.xml

A simple test now looks like:

@QuarkusComponentTest
@TestConfigProperty(key = "bar", value = "true") 
public class FooTest {

    @Inject
    Foo foo; 

    @ConfigureMock
    Charlie charlie; 

    @Test
    public void testPing() {
        Mockito.when(charlieMock.ping()).thenReturn("OK"); 
        assertEquals("OK", foo.ping());
    }
}

[quarkus-bot]

Failing Jobs - Building 887708b

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
⌛	JVM Tests - JDK 17	Build	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 19
✔️	Maven Tests - JDK 11
⌛	Maven Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
✖	Native Tests - Amazon	Build	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Amazon #

- Failing: integration-tests/amazon-lambda integration-tests/amazon-lambda-http 

📦 integration-tests/amazon-lambda

✖ io.quarkus.it.amazon.lambda.AmazonLambdaSimpleIT.testSimpleLambdaSuccess - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.amazon.lambda.deployment.DevServicesLambdaProcessor#startEventServer threw an exception: java.lang.RuntimeException: java.util.concurrent.ExecutionException: java.net.BindException: Address already in use

📦 integration-tests/amazon-lambda-http

✖ io.quarkus.it.amazon.lambda.AmazonLambdaSimpleIT.testJaxrsCognitoJWTSecurityContext - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.amazon.lambda.deployment.DevServicesLambdaProcessor#startEventServer threw an exception: java.lang.RuntimeException: java.util.concurrent.ExecutionException: java.net.BindException: Address already in use

[holly-cummins]

Interesting ideas. The only advantage of "explicit" components definition is that you don't need to inject your components at all but use programmatic lookup in the test instead (e.g. CDI.current().select(Foo.class) or even Arc.container().instance(Foo.claass)).

I agree this is a use case we should support. I think what you've ended up with, further down the thread, of supporting both active and passive models, is really nice. If users do nothing except exercise a thing (passive), it hopefully works, and if it doesn't work, they can configure it to work (active).

On the other hand, you need to specify the mockEverything=true, and if you accidentally change it to mockEverything=false you'll break the things anyway...

Yeees, although if you set mockEverything=false and are surprised that your things aren't mocked, you perhaps deserve it. :)

An alternative model would be an @MockEverything annotation, but I think the attribute would be simpler to implement and clearer.

What's the difference between @MockEverything and @QuarkusComponentTest? ;-)

That's a very good question. :) I think the advantages of @MockEverything are

• Doesn't add more to what's already a rather confusing hierarchy of @QuarkusXXTest
• Very explicit about what the behaviour is. You don't need to go look up the test pyramid diagram, try and map the concepts in your head to the terms we're using, remember that a @QuarkusUnitTest definitely isn't a unit test, remember that a @QuarkusTest is actually an integration test but not quite as much of an integration test as @QuarkusIntegrationTest is, and @QuarkusComponentTest is a unit test, unless you stick to a very old-school definition of unit tests, in which case it could be considered a component test, or it could even be considered an integration test. And in that old-school scheme@QuarkusIntegrationTest is not an integration test, but an end-to-end test.

I sometimes show this test pyramid diagram in my talks. IMO, the definitions of the tiers are a mess, because there's a lot of subjectivity and they've changed over time as frameworks/databases have become lighter.

The disadvantages of @MockEverything are:

• Adds another annotation people writing tests need to know about, and because it's not a @QuarkusXXTest, it's less discoverable.
• Although I have my personal rant/concerns with the names of levels in the test pyramid, that's the scheme we've used everywhere else (...except for @QuarkusTest, which is perhaps an important exception.)

So, yeah. I think a mockEverything=false attribute is preferable to YANA (yet another annotation :) ).

For me personally, the advantage of the mockEverything=false scheme are

• As above, avoids all arguments about layers of the test pyramid and whether our names are right/aligned to user's understandings of the layers
• Would get documented alongside @QuarkusTest so it would be nice and discoverable
• Isn't quite a default, since changing the default behaviour of @QuarkusTest would be sociopathic, so mockEverything would have to default to false, but it feels a bit more like we're keeping the annotation with the shortest name, QuarkusTest, as the one people will use most often, and just adding a handy knob for its behaviour.

In that case, we should probably introduce a new annotation in the io.quarkus.test.junit package... In theory, it does not have to be @InjectMock but more like @ConfigureMock or something along these lines. So that it's clear that we aim to "configure" the mock under the hood.

I like this ... we probably want to think about the table of when someone should use @InjectMock and when they should use @ConfigureMock. I think those names do capture the distinction of @InjectMock creates and @ConfigureMock just accesses an existing one.

Would it ever be valid to use @ConfigureMock outside a @QuarkusComponentTest test? ... and if your @QuarkusComponentTest test decided, for some reason, that it wanted a mock of something that normally wouldn't be mocked at all because it's not on code paths used by the implementation, should that be a @ConfigureMock or a @InjectMock? My understanding is that it would be @InjectMock, which is confusing, because a user has to know what @QuarkusComponentTest might have auto-mocked. On the other hand, that scenario is pretty darn unlikely, if it's even possible.

[holly-cummins]

I have another thought. Do you think it's a valid use case for people to use mockEverything=true/goFast when developing locally, and mockEverything=false in a CI? I guess it depends how much confidence they can get from the mock-everything path, and how much the things we decide not to mock influence the behaviour.

If it was a valid use case, the cleanest way to support it would be to use @QuarkusTest everywhere and have something like

quarkus.tests.gorealfast=true
%ci.quarkus.tests.gorealfast=false

which changed the behaviour of @QuarkusTest. And then maybe setting an attribute on an individual test would override the global config, for tests which will just never work in gorealfast mode.

Another way of getting that behaviour, but with the @QuarkusXXTest hierarchy, is that we introduce a new @QuarkusFatTest (strawman name, naming things is hard!). Users would usually use @QuarkusTest, and under the covers it would delegate either to @QuarkusFatTest or @QuarkusComponentTest, depending on the level of mockiness, controlled by application.properties. If they wanted to avoid the delegation, they just use QuarkusFatTest or QuarkusComponentTest explicitly.

The drawback of this idea is

• It's YANA-tastic, and introduces two new annotations into the test hierarchy :)
• Because all this is new, we don't have a sense, yet, about how many 'real world' tests would work in both modes. The whole proposal depends on most tests working in both ways.
• Documenting all this extra complexity this doesn't sound like fun
• Having tests that randomly change behaviour depending on config could drive users mad

[mkouba]

I think that it's getting harder and harder to follow this discussion. I suggest we merge this as is (it's an experimental feature anyway), then create a new topic for follow-up discussions in https://github.com/quarkusio/quarkus/discussions and tune up the API and docs in follow-up PRs.

@holly-cummins @geoand @Ladicek @stuartwdouglas WDYT?

[holly-cummins]

@mkouba I think merging makes sense. From my perspective, the changes to @Inject are really great and resolve almost all my concerns. The semantics are now (to me) clearer and more consistent with the current state.

• @Inject means "inject a real thing," rather than "maybe inject a real thing, and maybe inject a mock, and you need to understand the implications of the class level annotations to know which it will be"
• Naming things is hard, so I'm not sure what the optimum-optimum name for @ConfigureMock is, but Configure Mock clearly expresses the intent, so it's pretty close
• "Do people have a consistent understanding of what gets mocked at each level of the test pyramid?" is a pretty subjective concern
• "One test annotation to rule them all (with attributes to define them)" is a style question, so also subjective

I think we may have more work to do to explore the UX and technical aspects of the edge cases where these new annotations get combined with other annotations in ways we don't expect, but that can be done post-merge.

[mkouba]

I'm going to merge this PR; the Native Tests - Amazon failures are unrelated.

[FroMage]

@FroMage can enlighten us :)

Well, why do you think this makes it better to use Repositories?

[geoand]

Repositories are trivial to mock.

[FroMage]

Well, sure, but more trivial than active records?

        PanacheMock.mock(Person.class);

Because that's all it takes to get the same thing as here, or in the repo pattern:

  @InjectMock
    PersonRepository personRepository;

Sure, in this case you will replace @InjectMock with just @Inject. I could also make it so when a @QuarkusComponentTest is running, I auto-mock all Panache active records, but I wonder if it's worth it because the code I showed you seems trivial enough. Provided you don't have 10 entities you need to mock. Perhaps it's the numbers that make it problematic?

[geoand]

That's my point. Users will have to include quarkus-panache-mock if they want their stuff to work with this new type of test.

[FroMage]

I'm not sure it's going to be easier to use repositories than adding one Maven dependency to get active records mocked ;)

But OK, if users ask us about this, we know we can enhance the new @QuarkusComponentTest to automatically support this in the future.