-
Notifications
You must be signed in to change notification settings - Fork 72
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
Jmockit Expectations with no times or result should be transformed to Mockito verify statement #530
Conversation
/cc @tinder-dthomson since he originally created the JMockit migration recipes; figured you'd enjoy seeing this addition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great to me already; indeed seems better not to complete drop Expectations
without adding a corresponding verify
that the method is at least invoked. Even makes me slightly worry for replacements done previously(!). I don't have prior exposure to JMockit though, so I appreciate the links to attached to the description.
We'll give @tinder-dthomson a little while to chime in, and if no response is received than I'd say we can merge this as is. Thanks a lot!
Hey folks! Thanks for introducing this change. This behavior was not an oversight but rather a conscious decision on my part. My thought was that if a test wants to verify an invocation for a method without results, it belongs in the |
src/main/java/org/openrewrite/java/testing/jmockit/ExpectationsBlockRewriter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/jmockit/ExpectationsBlockRewriter.java
Outdated
Show resolved
Hide resolved
…even if someone mistakenly puts times and minTimes together, it still migrates without issue
… Mockito verify statement (openrewrite#530) * Ensure Jmockit expectations with no times or result transform to a mockito verify * Minor polish to text blocks * Make times, minTimes, maxTimes more flexible as it was originally so even if someone mistakenly puts times and minTimes together, it still migrates without issue --------- Co-authored-by: Tim te Beek <tim@moderne.io>
) * Adding Kotlin support to the recipes, fixing the recipes to work use preVisit instead of visitCompilationUnit. * Do a single recipe run per unit test * Stop after pre visit, since we're only updating imports * Adding Kotlin support for the UpdateBeforeAfterAnnotations, along with new tests for kotlin * Added kotlin tests for AddParameterizedTestAnnotation * Add Picnic AssertJ rules to AssertJ best practices (#527) * Add Picnic AssertJ rules to AssertJ best practices * Include Picnic's JUnitToAssertJRulesRecipes in migration * Exclude `jakarta.xml.bind-api` from TimeFold * Move the exclude to rewrite-third-party * refactor: Only publish build scans if authenticated Use this link to re-run the recipe: https://app.moderne.io/recipes/builder/kLJjXlflM?organizationId=T3BlblJld3JpdGU%3D Co-authored-by: Moderne <team@moderne.io> * Drop Java 17 requirement through rewrite-third-party (#531) * Jmockit Expectations with no times or result should be transformed to Mockito verify statement (#530) * Ensure Jmockit expectations with no times or result transform to a mockito verify * Minor polish to text blocks * Make times, minTimes, maxTimes more flexible as it was originally so even if someone mistakenly puts times and minTimes together, it still migrates without issue --------- Co-authored-by: Tim te Beek <tim@moderne.io> * Rewrite both JMockit `@Mocked` and `@Injectable` annotated arguments (#532) * Ensure Jmockit expectations with no times or result transform to a mockito verify * Minor polish to text blocks * Make times, minTimes, maxTimes more flexible as it was originally so even if someone mistakenly puts times and minTimes together, it still migrates without issue * Add feature to enable migration of Jmockit Injectable annotation exactly as we are doing the Mocked annotation ie method parameter annotation as well as field annotation. Have moved all of the code from JMockitMockedVariableToMockito to JMockitAnnotationToMockito for code reuse. Also add the corresponding test cases and jmockit.yml file modification. * Use `@Option` instead of inheritance * Drop Option and just replace both variants * Update display name and description to match application --------- Co-authored-by: Tim te Beek <tim@moderne.io> * Adding Kotlin support for the UpdateBeforeAfterAnnotations, along with new tests for kotlin * Added kotlin tests for AddParameterizedTestAnnotation * Restoring changes to issue to retain the references * Remove trailing whitespace in text blocks * Update src/test/java/org/openrewrite/java/testing/junit5/UpdateBeforeAfterAnnotationsTest.java Co-authored-by: Tim te Beek <timtebeek@gmail.com> * Removing tests for kotlin and keep one in the Documentation example. We can tackle the issues and add tests separately if we see bug in workflows. * Removing extra tests for Kotlin, and keeping one, fixing issue with failing build. * Restored changes to the disabled test. * Minor touch up --------- Co-authored-by: Amitoj Duggal <a.duggal@mytaxi.com> Co-authored-by: Tim te Beek <tim@moderne.io> Co-authored-by: Tim te Beek <timtebeek@gmail.com> Co-authored-by: Moderne <team@moderne.io> Co-authored-by: Shivani Sharma <s.happyrose@gmail.com>
Currently Jmockit Expectation statements when they do not have a times or result, when migrated to mockito disappear. However, when migrated to mockito, it should have a corresponding verify statement as per following documentation:
From my experience, when people specify this in the Expectation, it almost always corresponds to a mockito verify with no explicit times, basically saying that this statement occurs exactly once.
In mockito usually times is used to specify the exact number of invocations and this gives more finegrained verification, whereas the JMockit Expectation the developer typically intends "exactly once" when specifying no times even though it really means at least once.
If we prefer to use at least once, I can modify it as such, but a simple verify with no times is usually what the original author of the JMockit Expectation intends.
Checklist below done