Ensure expectations always result in verify. Modify test cases for this

[shivanisky]

What's changed?

JMockit Expectations need to have a corresponding Mockito when and verify. This is what happens with a Jmockit Expectations: https://www.javadoc.io/doc/org.jmockit/jmockit/latest/mockit/Expectations.html
"During replay, invocations matching a recorded expectation must occur at least once (unless specified otherwise); if, by the end of the test, no matching invocation occurred for a given recorded expectation, the test will fail with a MissingInvocation error"

However, for Mockito when statements, they are just for stubbing. So in Mockito, if you only have a when, then the test will not fail if that invocation didn't occur. It is the verify statement which ensures that the invocation occurred. Hence Jmockit Expectation statements directly translate to a Mockito when and a Mockito verify.
https://medium.com/@izharishaksa/understanding-when-and-verify-in-mockito-when-and-why-to-use-them-b4934c9aaa7b
"Use verify(): When you want to ensure a specific method was called on the mock object."

Anyone you would like to review specifically?

@timtebeek @tinder-dthomson

Have you considered any alternatives or workarounds?

Yes - no other way to fix this issue

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[tinder-dthomson]

"However, for Mockito when statements, they are just for stubbing"

This is actually not true. If you use when, the method must be invoked. It is not just for stubbing. The default expected number of invocations is 1, but you can change this using times.

[tinder-dthomson]

Here's the reference documentation for strictness of stubbing: https://www.javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/quality/Strictness.html

By default, Mockito is strict in version 4.x and above.

[shivanisky]

Yes - you may be right! I had a discussion with my manager 2 weeks ago and this came from the discussion with him as he did a lot of migration work prior. From the code I have seen, is not strict. So I am very confused. It blares out a warning, but doesn't fail the test. I will check locally with latest mockito. Perhaps is older version of mockito that's why I haven't seen this.

"the test fails when unused stubs are present (see UnnecessaryStubbingException)."

I have not seen this exception - maybe is because of older version of mockito. Will check locally.

Thanks!

[shivanisky]

Yes - you may be right! I had a discussion with my manager 2 weeks ago and this came from the discussion with him as he did a lot of migration work prior. From the code I have seen, is not strict. So I am very confused. It blares out a warning, but doesn't fail the test. I will check locally with latest mockito. Perhaps is older version of mockito that's why I haven't seen this.

"the test fails when unused stubs are present (see UnnecessaryStubbingException)."

I have not seen this exception - maybe is because of older version of mockito. Will check locally.

Thanks!

Using mockito v 4.11.0 and not seeing strict stubbing. Will check with latest mockito. They said planned as default for v4, but not sure if they put it in. I will double check.

[tinder-dthomson]

We've been using Mockito v4.5.1 for quite a while, and the UnnecessaryStubbingException does get thrown when the stub is not invoked.

I wonder if you have some configuration enabling LENIENT mode? Your stubs may also be using Mockito.lenient() to trigger this behavior.

[shivanisky]

We've been using Mockito v4.5.1 for quite a while, and the UnnecessaryStubbingException does get thrown when the stub is not invoked.

I wonder if you have some configuration enabling LENIENT mode? Your stubs may also be using Mockito.lenient() to trigger this behavior.

I really hope that you are right. That would be great! I need the test to fail if the stub is not invoked

[tinder-dthomson]

One more thing to check is whether or not your test classes are annotated properly.

For JUnit 4, it's @MockitoJUnitRunner

For JUnit 5, it's @MockitoExtension

You can also explicitly enable strict mode if all else fails. See this article (a bit old but concepts still hold): https://www.baeldung.com/mockito-unnecessary-stubbing-exception

Either way, I don't believe the recipe should create verifications from Expectations unless it's a void method or requirestimes or some other verify capability.

[shivanisky]

I think it may be because the mockito extension or mockito junit runner wasn’t enabled - will check - thanks so much. As jmockit yml recipe enables junit 5 extension all good there. So for legacy Jmockit NonStrictExpectations (some codebases use it widely), should migrate to lenient stubbing? Maybe by just adding .lenient()? I would like to do a PR for this next. What do you think? Also I was seeing JMockit Delegates being trashed with the when template being printed out in the output file with this recipe. I’m not sure if the last Verifications PR fixed it, I doubt it did. Also was seeing an IndexOutOfBoundsException being thrown when running this recipe on a very small amount of files, it’s very possible the previous Verifications PR fixed this, but maybe not. I will run it all and see. So close to being done. Some small cases will be difficult to migrate and Delegate migration will take some time, which I may not get to. Thanks Shivani

…

On Thu, 4 Jul 2024 at 1:36 PM, tinder-dthomson ***@***.***> wrote: One more thing to check is whether or not your test classes are annotated properly. For JUnit 4, it's @MockitoJUnitRunner For JUnit 5, it's @MockitoExtension You can also explicitly enable strict mode if all else fails. See this article (a bit old but concepts still hold): https://www.baeldung.com/mockito-unnecessary-stubbing-exception Either way, I don't believe the recipe should create verifications from Expectations unless times or something other verify capability is needed. — Reply to this email directly, view it on GitHub <#544 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BI27SFTTXOV3PAHDHIQJOKDZKS7K7AVCNFSM6AAAAABKKU47JGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBYGA3DENBRGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[shivanisky]

Have created issue for NonStrictExpectations migration here
#545

[tinder-dthomson]

I think it may be because the mockito extension or mockito junit runner wasn’t enabled - will check - thanks so much. As jmockit yml recipe enables junit 5 extension all good there. So for legacy Jmockit NonStrictExpectations (some codebases use it widely), should migrate to lenient stubbing? Maybe by just adding .lenient()? I would like to do a PR for this next. What do you think? Also I was seeing JMockit Delegates being trashed with the when template being printed out in the output file with this recipe. I’m not sure if the last Verifications PR fixed it, I doubt it did. Also was seeing an IndexOutOfBoundsException being thrown when running this recipe on a very small amount of files, it’s very possible the previous Verifications PR fixed this, but maybe not. I will run it all and see. So close to being done. Some small cases will be difficult to migrate and Delegate migration will take some time, which I may not get to. Thanks Shivani
…
On Thu, 4 Jul 2024 at 1:36 PM, tinder-dthomson @.> wrote: One more thing to check is whether or not your test classes are annotated properly. For JUnit 4, it's @MockitoJUnitRunner For JUnit 5, it's @MockitoExtension You can also explicitly enable strict mode if all else fails. See this article (a bit old but concepts still hold): https://www.baeldung.com/mockito-unnecessary-stubbing-exception Either way, I don't believe the recipe should create verifications from Expectations unless times or something other verify capability is needed. — Reply to this email directly, view it on GitHub <#544 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/BI27SFTTXOV3PAHDHIQJOKDZKS7K7AVCNFSM6AAAAABKKU47JGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBYGA3DENBRGQ . You are receiving this because you authored the thread.Message ID: @.>

Indeed there are several features of JMockit that are not yet supported, including Delegate and MockUp. Certainly grateful for the contributions (saw PR #546)!

For the IndexOutOfBoundsException , I suspect there is some case that the recipe is missing. @timtebeek did mention that this was observed by some users at some point, I just don't know how to reproduce it.

If you could share an example test file where this is happening (feel free to replace the proprietary class/method names with placeholders like foo), that would be valuable.

[timtebeek]

Indeed great to see the continued work on this! As to debugging those troublesome cases: the Moderne CLI has a nifty --jvm-debug flag that allows you to attach a debugger easily against a prebuilt LST. When combined with setting the active recipe to your IDE classpath you have a very quick way to test against real cases, outside of what's already covered by unit tests.

[shivanisky]

One more thing to check is whether or not your test classes are annotated properly.

For JUnit 4, it's @MockitoJUnitRunner

For JUnit 5, it's @MockitoExtension

You can also explicitly enable strict mode if all else fails. See this article (a bit old but concepts still hold): https://www.baeldung.com/mockito-unnecessary-stubbing-exception

Either way, I don't believe the recipe should create verifications from Expectations unless it's a void method or requirestimes or some other verify capability.

Yes confirmed - when using just MockitoAnnotations.openMocks, then the when is not verified. When using MockitoExtension, is verified.

[shivanisky]

For the IndexOutOfBoundsException , I suspect there is some case that the recipe is missing. @timtebeek did mention that this was observed by some users at some point, I just don't know how to reproduce it.

If you could share an example test file where this is happening (feel free to replace the proprietary class/method names with placeholders like foo), that would be valuable.

I can confirm the IndexOutOfBoundsException is no longer happening, I believe the Verifications PR fixed it :)

[timtebeek]

I can confirm the IndexOutOfBoundsException is no longer happening, I believe the Verifications PR fixed it :)

Great! Let me know if there's still anything to be done for this PR, as it's listed as draft with conflicts.
I'll review & merge this PR first, just in case there's more potential for conflicts there still.

• JMockit NonStrictExpectations with times should be migrated to default when without lenient(), refactor #549

[shivanisky]

You can close this one if you like - the code changes are useless - it’s more like a discussion thread at this stage. Thanks for your ongoing support and help it’s made the process so much easier!

…

On Thu, 11 Jul 2024 at 7:35 PM, Tim te Beek ***@***.***> wrote: I can confirm the IndexOutOfBoundsException is no longer happening, I believe the Verifications PR fixed it :) Great! Let me know if there's still anything to be done for this PR, as it's listed as draft with conflicts. I'll review & merge this PR first, just in case there's more potential for conflicts there still. - #549 <#549> — Reply to this email directly, view it on GitHub <#544 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BI27SFVQAOU2LEER4JREWQDZLZGW3AVCNFSM6AAAAABKKU47JGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRSGQ3TIMRSGI> . You are receiving this because you were assigned.Message ID: ***@***.***>

[timtebeek]

Sure! Thanks again for all the effort you've put in. I'll indeed close this one then, but we can still continue the conversation if needed.