Add expectations that pullrequest.listFiles() is called

[holly-cummins]

I’m a little bit confused by what’s going on here, so would welcome some insight on why this fix is (apparently?) needed.

When I run mvn quarkus:dev, I get 4 test failures, all in PullRequestOpenedTest#triage*. I don’t see the failures withmvn install test`.

No interactions wanted here:
-> at io.quarkus.bot.it.PullRequestOpenedTest.lambda$triageComment$23(PullRequestOpenedTest.java:313)
But found this interaction on mock 'GHPullRequest#527350930':
-> at io.quarkus.bot.util.Triage.matchRuleFromChangedFiles(Triage.java:89)
***
For your reference, here is the list of all invocations ([?] - means unverified).
1. -> at io.quarkus.bot.TriagePullRequest.triagePullRequest(TriagePullRequest.java:55)
2. [?]-> at io.quarkus.bot.util.Triage.matchRuleFromChangedFiles(Triage.java:89)
3. -> at io.quarkus.bot.TriagePullRequest.triagePullRequest(TriagePullRequest.java:78)
4. -> at io.quarkus.bot.TriagePullRequest.triagePullRequest(TriagePullRequest.java:94)

2022-08-12 16:28:44,122 ERROR [io.qua.test] (Test runner thread) >>>>>>>>>>>>>>>>>>>> 4 TESTS FAILED <<<<<<<<<<<<<<<<<<<<

--
4 tests failed (14 passing, 0 skipped), 18 tests were run in 3550ms. Tests completed at 16:28:44.
Press [r] to re-run, [o] Toggle test output, [:] for the terminal, [h] for more options>

Tracing through the code, I can see that the problem line is this one

verifyNoMoreInteractions(mocks.ghObjects());

mocks.ghObjects[1] is the pull request, and Triage calls listFiles on it, in both test modes. So I understand why the continuous testing is failing, but I don’t understand why the normal testing is passing.

If I do the tactical fix of verifying the extra interaction, both test modes pass. That also confuses me - if Mockito knows the interaction has happened well enough for verify to pass, why is verifyNoMoreInteractions passing?

I’ve attached a pragmatic change with the fix, but I’d love to understand better the root cause of the difference.

[holly-cummins]

Just as a sense check, I put in an check to make sure that the object I'd add my verify to was the same as the object verifyNoMoreInteractions was being called on:

                .then().github(mocks -> {
                    verify(mocks.pullRequest(527350930))
                            .addLabels("area/test1", "area/test2", "area/test3");
                    verify(mocks.pullRequest(527350930), times(2)).listFiles();
                   System.out.println("HOLLY equality check " + (mocks.pullRequest(527350930) == mocks.ghObjects()[1]));
                    verifyNoMoreInteractions(mocks.ghObjects());
                });

With both continuous testing and mvn test, the output is as expected:

HOLLY equality check true

[yrodiere]

The invocations you verified are actually stubbed (there's a call to when( ... listFiles() ) above).

Since we're using MockitoExtension in this test, and this extension defaults to setting the Mockito strictness to "strict", we should not have to verify stubbed invocations. See the javadoc of org.mockito.quality.Strictness#STRICT_STUBS:

Cleaner, more DRY tests ("Don't Repeat Yourself"): If you use org.mockito.Mockito.verifyNoMoreInteractions(Object...) you no longer need to explicitly verify stubbed invocations. They are automatically verified for you.

So, something is wrong with the integration with Mockito, and this PR is a workaround that should not be necessary.

As to where the problem lies exactly... I'd need to have a closer look. From the top of my head, I'd say the most likely cause is the weird dance with classloaders when doing continuous testing; it's possible that MockitoExtension is relying (perhaps indirectly) on static state that ends up being reinitialized by Quarkus' classloader operations.

I'd have to investigate, though... I'll give it a try when I have some time, unless you did first :)

[holly-cummins]

Thanks, @yrodiere! Do you think it's worth raising an issue against the mockito extension (as in "the Quarkus mockito integration"), if only to act as a reminder?

[yrodiere]

Thanks, @yrodiere! Do you think it's worth raising an issue against the mockito extension (as in "the Quarkus mockito integration"), if only to act as a reminder?

Probably better to raise the issue here, as I'm not entirely sure where the problem comes from yet. I wouldn't want to point fingers until we have proof :)

[yrodiere]

I reported the problem upstream: https://github.com/quarkusio/quarkus/issues/27383

[holly-cummins]

I think we can declare this stale, so closing. :)