WIP Mockito strict stubs enforcement

[Mordavolt]

Attempt at enforcing @MockitoSettings(strictness = STRICT_STUBS) if an import from org.mockito detected.
This is WIP as it detects violating classes, but suggests removing them :D
Also, auto-fix is not implemented, as I am not sure if I am on the right path at all.

[Stephan202]

Quick skim; very cool! 🎉. I'm afraid I won't have much time this week to really dive in, seeing as the "Corona project" is still in full swing. But if after these hints you're still stuck I'll find time :)

[Stephan202]

We'd need to find the relevant class and add it to that. We should be able to find that by recursing down the AST. That said, I wonder whether the following approach might have worked better:

• Instead of matching on CompilationUnitTree, match on class declarations.
• Ignore non-top-level classes.
• Then do the logic you have now.

[Mordavolt]

The only reason I need to access it on compilation level is access to imports. Is it possible to access imports on a classTree level?

[Mordavolt]

In my head this version "should work", but for some reason, it does not replace. Do you have any tips?

[Stephan202]

@Mordavolt my primary tip is to run failing tests in a debugger :). Rebaed and added a commit to fix the build.

About getting the imports some other way: IIRC that's possible yes; will come back to you.

[Stephan202]

Added some comments for further improvements, and a commit with a simpler implementation :)

[Stephan202]

Since STRICT_STUBS is the default, I think @MockitoSettings would do.

[Stephan202]

I think this requires some more work/thought:

1. If a class explicitly specifies another type of strictness we should probably respect that.
2. I suspect this won't match @MockitoSettings (no arguments), while that default enables strictness.
3. Not yet an issue, but once @MockitoSettings gets a second argument we should be prepared to merge the settings. (Or fall back to warning only.)

[Stephan202]

We should also add negative test cases: already-annotated classes, inner classes.

[Stephan202]

Rebased. @Mordavolt are you still interested in this PR?

(@hisener @nathankooij WDYT of this proposal?)

[Stephan202]

We should exclude non-test code, like the Mockito references under picnic-java-support-modules/test-support/src/main. For this looking at a file's path is a bad practice; instead we could look at the class name (fast, but "meh") or check for the presence of a JUnit test method, like in #6. Currently slightly tending to the latter.

[hisener]

This was on my list but never find time to review it. After a very quick look, isn't STRICT_STUBS already the default in MockitoExtension?

• https://github.com/mockito/mockito/blob/e88fe26110afd6641c464cb0e87bd898b41bf457/subprojects/junit-jupiter/src/main/java/org/mockito/junit/jupiter/MockitoExtension.java#L145-L147
• https://github.com/mockito/mockito/blob/e88fe26110afd6641c464cb0e87bd898b41bf457/subprojects/junit-jupiter/src/main/java/org/mockito/junit/jupiter/MockitoExtension.java#L129

[Mordavolt]

@hisener Yes, you are exactly correct. But we don't use the extension. So the whole purpose of this PR is to enforce using the extension, so also enforce using the strict mocks. We do it indirectly by adding @MockitoSettings.

@ExtendWith(value=MockitoExtension.class)
@Retention(value=RUNTIME)
public @interface MockitoSettings

[hisener]

Does MockitoExtension also work for Mockito#mock calls? 🤔 It looks like it's mainly for @Mock annotation.

Even though I am not a big fan of @Mock annotation, we might also register this extension globally rather than adding @MockitoSettings everywhere. 🤔 https://junit.org/junit5/docs/current/user-guide/#extensions-registration-automatic

[nathankooij]

Does MockitoExtension also work for Mockito#mock calls? 🤔 It looks like it's mainly for @Mock annotation.

Even though I am not a big fan of @Mock annotation, we might also register this extension globally rather than adding @MockitoSettings everywhere. 🤔 https://junit.org/junit5/docs/current/user-guide/#extensions-registration-automatic

Yes, it seems that the use of MockitoSettings would imply the use of @Mock which I'm not sure if that's something we want (I mean, I suspect there are too many cases where it will not be applicable due to involved mock setup). So in the case that you'd want to go that way, I'd suggest indeed to use MockitoExtension directly or set it up globally for your project. At least I don't see how @MockitoSettings affects Mockito#mock calls, but could be I'm overlooking something right now.

Alternatively, we could detect (in)correct usage of Mockito#mock with the EP rule, but I'm not sure what that would entail exactly. 🤔

(I do agree that strict stubbing is a fantastic idea, though. :))

[Mordavolt]

So two points regarding this:

1. I would like to avoid initializing Mockito for tests where mockito is not used;
2. IIUC adding @ExtendWith(value=MockitoExtension.class) on a test that uses mock(Abc.class) has no impact except for the strictness of the mocks.

Combining those two statements I've come to this solution - if a test imports mockito, then I want to enforce MockitoExtension.class, otherwise I would like to do nothing.
Do you think just enabling MockitoExtension.class for all unit tests is a better solution? I'm willing to discuss this.

[Stephan202]

Do you think just enabling MockitoExtension.class for all unit tests is a better solution? I'm willing to discuss this.

I guess it depends on the overhead. If the overhead is significant enough to offset the time lost due to build failures triggered by this check, then we should likely be selective. TBH, I don't expect that to be the case. 🤔

(Whatever we do; I still had fun reviewing the PR 😉)

[nathankooij]

So two points regarding this:

1. I would like to avoid initializing Mockito for tests where mockito is not used;
2. IIUC adding @ExtendWith(value=MockitoExtension.class) on a test that uses mock(Abc.class) has no impact except for the strictness of the mocks.

Combining those two statements I've come to this solution - if a test imports mockito, then I want to enforce MockitoExtension.class, otherwise I would like to do nothing.
Do you think just enabling MockitoExtension.class for all unit tests is a better solution? I'm willing to discuss this.

Well, I think if you're willing to use @Mock, then I assume you'd also apply this consistently within the project so in that case I don't think applying the extension generally would be a big problem (I would think the overhead to be minimal).

I do understand your use case a bit better now, but my worry is that enforcing the extension when imports are detected doesn't enforce strict stubs, since you could still be using Mockito#mock instead of @Mock. I don't know if it would be feasible to "ban" Mockito#mock in favor of @Mock. If so, then I think that'd be a better route, since the use of @Mock must imply the use of MockitoExtension. So my overall feeling is that I just don't know how much there's to gain with this rule.

Regarding point 2) I'm not sure I follow. The extension is a no-op for mock(Abc.class), since it only works for @Mock.

So I was operating under the impression that MockitoExtension can't check Mockito#mock calls. Turns out this is not completely true (maybe you already knew :)) -- mocks which are created in a BeforeEach will be captured via the following mechanism:

• Mockito has a "mocking progress" thread local;
• When a mock is created, listeners associated with mocking progress are called with the created mocks;
• MockitoExtension registers a listener in a @BeforeEach with the current mocking progress; (that's why mocks assigned to fields directly are not picked up, since that runs before @BeforeEach)
• MockitoExtension calls finishMocking in a @AfterEach which triggers the registered listener.
• Since the listener observed the created mock, it can also validate the mock after test execution.

With that said, I can see how enforcing MockitoExtension in case of the presence of Mockito imports and JUnit (so doesn't matter if it's using Mockito#mock or @Mock) would work. However, we might need to also detect when mocks are "incorrectly" created, e.g. not as part of @BeforeEach. In other words, directly assigning mocks to fields should not be allowed.

[Stephan202]

In other words, directly assigning mocks to fields should not be allowed.

Ouch, that'd introduce quite some (otherwise unnecessary) boilerplate 🤔

[nathankooij]

In other words, directly assigning mocks to fields should not be allowed.

Ouch, that'd introduce quite some (otherwise unnecessary) boilerplate 🤔

Just for clarification, since my description was a bit ambiguous: I mean you cannot combine field declaration and assignment (e.g. private final Field field = mock(Field.class);), but if you assign a value to that field in a @BeforeEach or a @Test itself, that's fine. That's probably not that much more boilerplate since most mocks are created for each test separately. But probably also doesn't work for mocks created as beans. So I can see how a rule like this (either in this form, or a more advanced form where we also check for Mockito#mock usage) can make sense in a project like WMS, but don't think it's something we can apply universally right now (too many caveats IMO). So we can still go with what @Mordavolt suggested if we apply it to just WMS.

[Mordavolt]

To update you on this. I wanted to abandon this PR and go the "enable extension everywhere" way, but I don't think it's that simple.
IIUC correctly I need to -Djunit.jupiter.extensions.autodetection.enabled=true and that is simple pom change, but then I need to

Specifically, a custom extension can be registered by supplying its fully qualified class name in a file named org.junit.jupiter.api.extension.Extension within the /META-INF/services folder in its enclosing JAR file.

And I could not figure out where to put this file. I'd like it to be in the source code, but META-INF is about the jar (right?), so I couldn't figure out how to enable the extension. Any help would be appreciated. Has anyone made JUnit autoload an extension in all test classes?

[Stephan202]

@Mordavolt that location is the service loader path. It's the file that is auto-generated when you use the @AutoService annotation :)

But in this case you'd have to create it manually. That can be done by placing it under src/test/resources; Maven will take care of the rest. So in this case it'd besrc/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extension.

(This would have to be done for every module. There is a way around that, but ask me another time :) )

[Mordavolt]

Closing this PR in favour of https://github.com/PicnicSupermarket/picnic-wms/pull/1812.
TL;DR will enable mockito extension for all unit tests.
Thank you for the discussion and your very valuable insights. Sorry to waste your time :)