New panache-mock module for mocking Panache static methods

[FroMage]

Tied to Mockito, usage:

// start mocking this entity's static methods
        PanacheMock.mock(Person.class);

// mocked method returns the Mockito defaults: 0 in this case
        Assertions.assertEquals(0, Person.count());

// specify a new return value
        Mockito.when(Person.count()).thenReturn(23l);
        Assertions.assertEquals(23, Person.count());

// specify a new return value
        Mockito.when(Person.count()).thenReturn(42l);
        Assertions.assertEquals(42, Person.count());

// now delegate to the original method
        Mockito.when(Person.count()).thenCallRealMethod();
// Here H2 is running so we do get a result
        Assertions.assertEquals(0, Person.count());

// Verify that our mock method was called
        Mockito.verify(PanacheMock.getMock(Person.class), Mockito.times(4)).count();

// Same with params
        Mockito.when(Person.findById(12l)).thenReturn(p);
        Assertions.assertSame(p, Person.findById(12l));
        Assertions.assertNull(Person.findById(42l));

        Mockito.when(Person.findById(12l)).thenThrow(new WebApplicationException());
        try {
            Person.findById(12l);
            Assertions.fail();
        } catch (WebApplicationException x) {
        }
        
// We can even mock the user's public static methods
        Mockito.when(Person.findOrdered()).thenReturn(Collections.emptyList());
        Assertions.assertTrue(Person.findOrdered().isEmpty());

[FroMage]

So the injected implementation looks like this at the start of every static public method:

if(PanacheMock.IsMockEnabled && PanacheMock.isMocked(Person.class)) {
  try {
    return (int)PanacheMock.mockMethod(Person.class, "count", new Class<?>[] {}, new Object[] {});
  } catch (PanacheMock.InvokeRealMethodException e) {
    // fall-through
  }
}

I still have to make this conditional to the TEST mode.

Now the question is how do I tie this with the quarkus-test module?

Users probably don't want to write tests like this:

    @AfterEach
    public void afterEach() {
// make sure one test's mocking doesn't leak to other tests
        PanacheMock.reset();
    }

    @Test
    public void testPanacheMocking() {
// specify all mocked classes
        PanacheMock.mock(Person.class, Dog.class);
    }

So perhaps I should make PanacheMock.reset() called by default after each test? How? I don't want to make the quarkus-test module depend on panache-mock.

Is there value in this alternative form to declare mocked classes?

    @Test
    @PanacheMockClasses({Person.class, Dog.class})
    public void testPanacheMocking() {
    }

Doesn't look very justified to me.

[loicmathieu]

This looks promising !

So perhaps I should make PanacheMock.reset() called by default after each test? How? I don't want to make the quarkus-test module depend on panache-mock.

You need to turn PanacheMock in a Mockito extension or a JUnit extension.

For Mockito extension point, I read quickly the documentation and you can provide you own MockMaker https://javadoc.io/static/org.mockito/mockito-core/3.3.3/org/mockito/plugins/MockMaker.html this seems to be close to what you want to achieve or you can use one of the integration point of Mockito: https://javadoc.io/static/org.mockito/mockito-core/3.3.3/org/mockito/Mockito.html#41

For JUnit extension, the better is to implements AfterTestExecutionCallback https://junit.org/junit5/docs/current/user-guide/#extensions-lifecycle-callbacks-before-after-execution then create a JUnit Extension that extends the QuarkusTest extension and adds mocking capabilities. So the user can just use @PanacheTest instead of @QuarkusTest.

If you don't need to integrate too deep into Mockito the JUnit extension is the easiest.

Some early feedback on your PR:

• I don't see the PanacheMock class in it.
• Mockito.verify(PanacheMock.getMock(Person.class), Mockito.times(4)).count(); it didn't seems very convenient to need to call getMock to verify the mock behaviour. Maybe the PanacheMock.mock(Person.class) can return a mock instance that we can use with the verify method, or maybe you can provides a PanacheMock.verify(Person.class, Mockito.times(4)) method that allow verifycation.
• As I understand the PR, you add the hability to create method customizer to mock them. I'm not fund of using the standard build mechanism for mocking stuff, I would expect mocking happening at test execution time only, but I can understand that it's the easiest (and maybe the only) way to do it.

[gsmet]

Looks like there's some code missing.

Also, big question: should this really be an extension or should it be part of our test framework modules? I would favor the latter if there's no good reason to make it an extension.

[FroMage]

Looks like there's some code missing.

Yeah, I was missing the extension, sorry. Updated.

[FroMage]

Also, big question: should this really be an extension or should it be part of our test framework modules?

So ATM, the extension depends on PanacheMethodCustomizer and PanacheMethodCustomizerBuildItem which are in panache-common, and I didn't want to make quarkus-junit5 depend on panache-common.

This works because hibernate-orm-panache depends on panache-common. It didn't feel right to make it depend on quarkus-junit5 either.

We could place the customizer interface and build item somewhere else, because they don't really depend on anything Panache, but where?

[FroMage]

Mockito.verify(PanacheMock.getMock(Person.class), Mockito.times(4)).count(); it didn't seems very convenient to need to call getMock to verify the mock behaviour. Maybe the PanacheMock.mock(Person.class) can return a mock instance that we can use with the verify method, or maybe you can provides a PanacheMock.verify(Person.class, Mockito.times(4)) method that allow verifycation.

I could add that, yes.

As I understand the PR, you add the hability to create method customizer to mock them. I'm not fund of using the standard build mechanism for mocking stuff, I would expect mocking happening at test execution time only, but I can understand that it's the easiest (and maybe the only) way to do it.

Well we have to inject some code in the methods at build time so they can be mocked. We should only do it in the TEST mode, though, so I think it's OK.

[loicmathieu]

Just read the PR again with the panache-mock extension and it is clearer now ;)

Well we have to inject some code in the methods at build time so they can be mocked. We should only do it in the TEST mode, though, so I think it's OK.

I agree, in fact I found now the approach OK, we can reuse it if needed (for example to fix some Kotlin issue ?, or to provides specialization for some databases ?) so it's great.

Also, big question: should this really be an extension or should it be part of our test framework modules?

I think that being a panache-mock extension is OK, Panache users will tipically add it with the test scope if they need it (not everybody uses panache entity).
If some other areas need it, you can then move some of it's facility inside a global Quarkus mock extension (for example to mock messaging system ?) ...

Of course, if it could be only a JUnit or a Mockito extension, this would be better as everything will be a test runtime and scoped to a single test instead of at build time and scopped at the entire test suite. But I'm OK with this anyway.

[FroMage]

Of course, if it could be only a JUnit or a Mockito extension, this would be better as everything will be a test runtime and scoped to a single test instead of at build time and scopped at the entire test suite. But I'm OK with this anyway.

As I said, there has to be something at build-time to inject a hook into the static methods so they can delegate to mocking. I don't know how to do otherwise.

[FroMage]

• Rebased on master
• Now depend on the new quarkus-junit5-mockito
• Uses that to reset all mocks between each test method
• Added mongodb-panache test
• Added PanacheMock.verify(Class) methods
• Only generate the mock calls in TEST mode

[FroMage]

So now the only thing left IMO is where this extension sits.

ATM it's an extension panache-mock because it must generate a BuildItem that *-panache will collect and call.

That build item is coupled with an interface PanacheMethodCustomiser. Those are currently defined in panache-common which is not an extension.

I could move their implementations to the panache-junit5-mockito module, except it would have to depend on panache-common (perhaps fine, WDYT @geoand?), and it would have to be able to inject a build item into the build. I've no idea how to do that, if it's even possible when you're not a Quarkus extension.

Perhaps @geoand or @stuartwdouglas have ideas?

[geoand]

Is having a specific panache testing module not a good option?
It is certainly easier (and perhaps cleaner?) than doing anything else.

[FroMage]

No idea. @gsmet asked the question, and it's a good question.

It does make sense that quarkus-junit5-mockito would give us mocks for everything, including Panache stuff: it makes it easier for users because they get all quarkus/mockito in one place.

OTOH it's not that annoying to have a dedicated extension IMO.

So I'm really open to opinions here.

[geoand]

I think that the panache testing stuff should be a dedicated module (that perhaps includes quarkus-junit5-mockito).

I don't think it will be a problem for users to add a test dependency. We can also make it clear in the release notes and the docs. Plus we can augment our project generation to add that dependency when the Panache extension is added.

[loicmathieu]

I'm OK with panache-mock being an extension, as long as you don't create a FeatureBuildItem it will be an hidden one and the user will not see it.

A few questions:

• Users will ask if they can use PanacheMock to mock a repository. I don't think it provides any value to do this compared to raw Mockito so this should be documented (I don't know what will happens if some do PanacheMock.mock(PersonRepository.class)).
• Can you add an example of mocking a PanacheQuery ? Maybe we will need support for this also as it will be very common use case: PanacheMock.mockPanacheQuery(results) could return a mock of PanacheQuery with the list of results inside, then a user can paginate it, get the first result, ... Maybe it's a different story ;)
• I usually end my tests with Mockito.verifyNoMoreInterration() to check that all calls to my mocks has been verify, will this methods works ? Can you have a test for it ?

[FroMage]

I'm OK with panache-mock being an extension, as long as you don't create a FeatureBuildItem it will be an hidden one and the user will not see it.

Yeah, I should definitely remove it.

Users will ask if they can use PanacheMock to mock a repository. I don't think it provides any value to do this compared to raw Mockito so this should be documented (I don't know what will happens if some do PanacheMock.mock(PersonRepository.class)).

Well Mockito can mock repos already so there's no need for that. It will not work if you pass a repo. I could add a type constraint for PanacheEntityBase but it would add a dependency to it, and we don't want that, unless we add a marker super interface to both orm/mongo entities. MockablePanacheEntity with no method. That should work.

Can you add an example of mocking a PanacheQuery ? Maybe we will need support for this also as it will be very common use case: PanacheMock.mockPanacheQuery(results) could return a mock of PanacheQuery with the list of results inside, then a user can paginate it, get the first result, ... Maybe it's a different story ;)

I can't because it would add a dependency to orm/mongo-panache. Unless we create a common interface, which we didn't want to do. Not sure how to proceed here.

I usually end my tests with Mockito.verifyNoMoreInterration() to check that all calls to my mocks has been verify, will this methods works ? Can you have a test for it ?

It should work, I can try it.

[loicmathieu]

Well Mockito can mock repos already so there's no need for that. It will not work if you pass a repo. I could add a type constraint for PanacheEntityBase but it would add a dependency to it, and we don't want that, unless we add a marker super interface to both orm/mongo entities. MockablePanacheEntity with no method. That should work.

Just add a sentence inside the documentation this should be OK for now.

Regarding mocking of PanacheQuery, we can add it later if needed. As we don't have a supertype for both orm/mongo it will need to be created in specific extension ... not sure we want that now.

[FroMage]

Just got this:

org.mockito.exceptions.verification.NoInteractionsWanted: 
No interactions wanted here:
-> at io.quarkus.it.mongodb.panache.MongodbPanacheResourceTest.testPanacheMocking(MongodbPanacheResourceTest.java:401)
But found this interaction on mock 'personEntity':
null
***
For your reference, here is the list of all invocations ([?] - means unverified).
1. null
2. null
3. null
4. null
5. [?]null
6. [?]null
7. [?]null
8. null

So I'm guessing my mocking is lacking something :)

[FroMage]

OK that's fixed.

[stuartwdouglas]

My concerns have been addressed

[geoand]

I believe this is something we should include in 1.4. @gsmet WDYT?

[loicmathieu]

LGTM

[FroMage]

So that leaves @gsmet

Dismissed.

[FroMage]

The tests fail for vertx-web, but I don't see how that can be related to this PR. @cescoffier are you aware of current/intermitent test failures in vertx-web?

[cescoffier]

Actually, vert.x web is quite stable.... Haivng a look.

[cescoffier]

Looks like a race condition or something clearly wrong unrelated to this PR:

2020-04-16T13:06:58.3356832Z 2020-04-16 13:06:58,109 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (vert.x-eventloop-thread-0) HTTP Request to /hello failed, error id: faca1667-b810-4e7e-88b6-8e522e4f8169-1: java.lang.NullPointerException
2020-04-16T13:06:58.3437894Z 	at io.quarkus.vertx.web.base.RouteBaseTest_RouteBaseTest$SimpleBean_RouteHandler_hello_254a2a71f98e407c82cb72702a943a981ffe2f58.invoke(RouteBaseTest_RouteBaseTest$SimpleBean_RouteHandler_hello_254a2a71f98e407c82cb72702a943a981ffe2f58.zig:84)
2020-04-16T13:06:58.3465221Z 	at io.quarkus.vertx.web.runtime.RouteHandler.handle(RouteHandler.java:44)
2020-04-16T13:06:58.3553001Z 	at io.quarkus.vertx.web.runtime.RouteHandler.handle(RouteHandler.java:15)
2020-04-16T13:06:58.3609712Z 	at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1034)

[cescoffier]

Ah ah, I believe it's the race condition addressed by @mkouba !

[FroMage]

Already fixed in master you mean?

[mkouba]

AFAIK it's not merged yet

[mkouba]

@FroMage @cescoffier yes, this should be addressed by #8579. Unfortunately, I can't "force merge" anymore.

[FroMage]

Rebased on master to see if the test failures are gone, they're definitely not related to this PR.

[FroMage]

Caused by: java.lang.ClassNotFoundException: org.eclipse.jetty.alpn.ALPN$Provider

I declare CI to be dead.

[geoand]

Please rebase PR onto the latest master in order to pick up a CI fix

[FroMage]

@gsmet just merged it. Not sure if it's too late to make 1.4 or wait for 1.5?

[gsmet]

If it changed only the test code, I would have said 1.4 but I can see it changes quite a lot of things in the regular code too so I'm leaning towards 1.5.

[FroMage]

Yes, and 1.5 is not far off and let's be honest, nobody actually cares about mocking, as we've uncovered in this PR that nobody is mocking panache repositories ;)

[loicmathieu]

we've uncovered in this PR that nobody is mocking panache repositories ;)

For the record, I mock panache repositories without any issue since the beginning !

[FroMage]

That can't be true: this PR uncovered so many bugs in the generated code for repositories that prevented them from being mocked!

Or you've never mocked any method with generics or primitives in their signature ;) findById didn't work, neither did count().

[FroMage]

When I tried mocking repos, Mockito just threw exceptions when it loaded the generated classes with bytebuddy.

[loicmathieu]

I mocked all my repos on raw JUnit tests with Mockito without any issue, even the findById methods. I think Daniel Petisme also (he report it during it's effort on JHipster).
But we both uses raw JUnit tests without QuarkusTest it should be why it worked.