Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression? Strictness set in @MockitoSettings ignored after upgrade from 4.5.1 to 4.6.0 #2656

Closed
5 tasks done
big-andy-coates opened this issue May 31, 2022 · 16 comments · Fixed by #2659
Closed
5 tasks done

Comments

@big-andy-coates
Copy link
Contributor

big-andy-coates commented May 31, 2022

Upgrading from Mockito 4.5.1 to 4.6.0 and it looks to me as though #2650 or something around there has introduced a regression.

I'm seeing PotentialStubbingProblem exceptions where previously there were none, because the test class is annotated with @MockitoSettings(strictness = Strictness.LENIENT).

The issue seems to be that StrictnessSelector prefers the strictness set in the mock to the test level strictness, and the mock defaults to STRICT_STUBS, so always overrides the strictness set in @MockitoSettings.

I've added a failing example test that demonstrates the issue here: big-andy-coates#1

The above test fails with:

expected: 
  Optional.empty
 but was: 
  Optional[org.mockito.exceptions.misusing.PotentialStubbingProblem: 
  Strict stubbing argument mismatch. Please check:
   - this invocation of 'test' method:
      rootMock.test("Bar");
      -> at org.mockitousage.ProductionCode.simpleMethod(ProductionCode.java:9)
   - has following stubbing(s) with different arguments:
      1. rootMock.test("Foo");
        -> at org.mockitousage.StrictnessTest$LenientMockitoSettings.should_not_throw_on_potential_stubbing_issue(StrictnessTest.java:176)
  Typically, stubbing argument mismatch indicates user mistake when writing tests.
  Mockito fails early so that you can debug potential problem easily.
  However, there are legit scenarios when this exception generates false negative signal:
    - stubbing the same method multiple times using 'given().will()' or 'when().then()' API
      Please use 'will().given()' or 'doReturn().when()' API for stubbing.
    - stubbed method is intentionally invoked with different arguments by code under test
      Please use default or 'silent' JUnit Rule (equivalent of Strictness.LENIENT).
  For more information see javadoc for PotentialStubbingProblem class.]
  • The mockito message in the stacktrace have useful information, but it didn't help
  • The problematic code (if that's possible) is copied here;
    Note that some configuration are impossible to mock via Mockito
  • Provide versions (mockito / jdk / os / any other relevant information)
  • Provide a Short, Self Contained, Correct (Compilable), Example of the issue
    (same as any question on stackoverflow.com)
  • Read the contributing guide
@chadlwilson
Copy link

chadlwilson commented May 31, 2022

I was coming to the same conclusion and basic re-prod steps. Thanks for raising this!

public static Strictness determineStrictness(
Stubbing stubbing, MockCreationSettings mockSettings, Strictness testLevelStrictness) {
if (stubbing != null && stubbing.getStrictness() != null) {
return stubbing.getStrictness();
}
if (mockSettings.getStrictness() != null) {
return mockSettings.getStrictness();
}
return testLevelStrictness;
}

mockSettings.getStrictness() seems to always be non-null due to the default at

protected Strictness strictness = Strictness.STRICT_STUBS;

Which I think means it's impossible for it to fall back to testLevelStrictness from @MockitoSettings.

The weird thing is you wont get failures due to UnnecesaryStubbingExceptions (since the test-level setting seems to be respected for the post-test reporting and UnnecessaryStubbingReporter is disabled), but you will get the ones for PotentialStubbingProblem as above.

The secondary problem we have which obscured this one is that tracking stubbings like this seems to cause issues for our Groovy-based tests since there are NPEs thrown in Location.getSourceFile() when stubbing is tracked. That's why we disable it at test level for the groovy tests (gocd/gocd@ff7b8ee). This is probably a separate issue/concern though:

&& !s.getInvocation()
.getLocation()
.getSourceFile()
.equals(invocation.getLocation().getSourceFile())) {

java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because the return value of "org.mockito.invocation.Location.getSourceFile()" is null
        at org.mockito.internal.junit.DefaultStubbingLookupListener.potentialArgMismatches(DefaultStubbingLookupListener.java:81)
        at org.mockito.internal.junit.DefaultStubbingLookupListener.onStubbingLookup(DefaultStubbingLookupListener.java:52)
        at org.mockito.internal.listeners.StubbingLookupNotifier.notifyStubbedAnswerLookup(StubbingLookupNotifier.java:31)
        at org.mockito.internal.handler.MockHandlerImpl.handle(MockHandlerImpl.java:93)
        at org.mockito.internal.handler.NullResultGuardian.handle(NullResultGuardian.java:29)
        at org.mockito.internal.handler.InvocationNotifierHandler.handle(InvocationNotifierHandler.java:34)

@TimvdLippe
Copy link
Contributor

TimvdLippe commented May 31, 2022

That indeed sounds like a regression, apologies for that. Since we already have a PR with a regression test, can one of you also supply the fix for it? If all other tests in our suite pass, we should be good to go.

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented May 31, 2022

No worries! ;)

I'm happy to submit a PR, but before I do I think its worth discussing what the fix should be.

It seems to me that the issue is the default values for strictness introduced by @sivaprasadreddy. By defaulting the new strictness to Strictness.STRICT_STUBS the test level strictness gets ignored.

I'd therefore suggest removing the defaults, i.e. setting them to a 'not set' value (which would mean extending Strictness enum which doesn't feel right, or having a new enum for @Mock that includes this 'not set' value). However, this looks like it might undo part of the intent of @sivaprasadreddy's PR, which is to default to STRICT_STUBS.

I'll have a quick hack about to see how things turn out, but any thoughts on this appreciated.

@TimvdLippe
Copy link
Contributor

TimvdLippe commented May 31, 2022

The intent of the PR was to introduce a new API surface for configuring strictness, not to change the default. So I consider it a regression that the new default became STRICT_STUBS. Not sure how I missed that in the review.

Setting it to an unset value (e.g. null) sounds like the appropriate solution to me.

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented May 31, 2022

Cool.

Though the challenge here is that @Mock.strictness() returns an enum: the default value can not be null...

I can switch it to use a local Strictness enum, but that'll be a breaking change...

Code written against 4.6.0 release:

public class StrictnessMockAnnotationTest {

    public @Rule MockitoRule rule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);

    @Mock(strictness = Strictness.LENIENT)
    IMethods lenientMock;

Would need to change to:

public class StrictnessMockAnnotationTest {

    public @Rule MockitoRule rule = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);

    @Mock(strictness = Mock.Strictness.LENIENT)
    IMethods lenientMock;

(Note the additional prefix of Mock. in the @Mock annotation.

@TimvdLippe
Copy link
Contributor

TimvdLippe commented May 31, 2022

Looking at the original PR again, I indeed completely overlooked the default values. These should have been LENIENT rather than STRICT_STUBS. If we change them to LENIENT, I think we retain the correct behavior?

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented May 31, 2022

I think changing defaults won't fix the issue that the defaults stop the test level settings ever taking affect.

@TimvdLippe
Copy link
Contributor

TimvdLippe commented May 31, 2022

If I understand the new code of the StrictnessSelector correctly, we now always take the strictness set in the creation settings. That's because it is initialized to a non-null value. However, we have a field that is of type Strictness that we can assign null to. Then we change the default of the annotation value to LENIENT and then I think the logic will work as it did before, doesn't it?

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented May 31, 2022

I don't think so. The MockAnnotationProcessor will copy the default (LENIENT) from the annotation into the settings. For the settings to have null (and hence for the selector to select the test level strictness), the mock needs to have some concept of not set for its strictness.

Give me one sec and I'll dump an example into my PR,

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented May 31, 2022

Done. See big-andy-coates#1

@TimvdLippe
Copy link
Contributor

TimvdLippe commented May 31, 2022

What if we add a new value to org.mockito.quality.Strictness called TEST_DEFAULT_VALUE which is our "not set" value. Then we check if that value exists in the creation settings and don't set it if that's the default. We can then explicitly document the strictness to say "the test suite strictness value will take precedence".

It's not ideal to ship this to users, but I think it's the most explicit and also will fix this issue?

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented May 31, 2022

Yeah, I had thought of that. It would fix the issue and wouldn't be a breaking change for 4.6.0 -> 4.6.1, but...

It leaves this TEST_DEFAULT_VALUE in the enum, and the enum is used in other places and in all those other places does it actually make sense to use it? i.e. are we polluting the API going forward, making the API less clean, more easily misused, or having to add code to reject some enum values in some places, just to work around this bug in 4.6.0?

For example, it wouldn't make sense to set the test level strictness to TEST_DEFAULT_VALUE. Though I guess we could just ignore any user code that did this...

For example, what would be the behaviour if someone used TEST_DEFAULT_VALUE for the test level strictness?

@MockitoSettings(strictness = Strictness.TEST_DEFAULT_VALUE)
class MyWeirdTest {
   ...
}

I guess we could update all the code to check for this new value and default to STRICT_STUBS...?

As I see it the two options are:

  1. Add local enum to Mock class with a NOT_SET value
    • pros: cleaner API going forward
    • cons: breaking change for anyone using the new strictness value of the @Mock class, which was released in 4.6.0.
  2. Add a TEST_DEFAULT_VALUE to the existing Strictness enum.
    • pros: not a breaking change.
    • cons: more ambiguous API moving forward.

I'm happy to go with either. I can even knock up PRs for both if you want. I'm not sure what the policy is on breaking changes on newly released features that are broken.

What's the time frame for 4.6.1?

@TimvdLippe
Copy link
Contributor

TimvdLippe commented May 31, 2022

In that case, I think option 1 is the cleanest. Since we just shipped the new feature, I would be okay wit changing that. It is unfortunate, but something that we can patch over after a day.

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented May 31, 2022

OK, PR coming soon...

(FYI, I've just been trying to raise the PR for option 2 and its not pretty, so I think this is the right call)

big-andy-coates added a commit to big-andy-coates/mockito that referenced this issue May 31, 2022
@big-andy-coates big-andy-coates mentioned this issue May 31, 2022
8 tasks
big-andy-coates added a commit to big-andy-coates/mockito that referenced this issue May 31, 2022
@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented May 31, 2022

@TimvdLippe PR raised: #2659

@chadlwilson
Copy link

chadlwilson commented Jun 3, 2022

Thanks to both of you! Seems to work fine for us now on 4.6.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants