Recipe for closing unclosed static mocks #739
Conversation
src/main/java/org/openrewrite/java/testing/mockito/CloseUnclosedStaticMocks.java
Show resolved
Hide resolved
There was a problem hiding this comment.
I am hardly an SME on this topic. But the description you have provided and the test case contributed make sense to me. I have reviewed the code from a general perspective and have no comments. LGTM.
I am gonna give my colleagues a chance to submit some feedback they might have. Otherwise let's merge.
|
Thanks for the contribution! |
| void shouldWrapMockStaticInTryWithResources() { | ||
| //language=java | ||
| rewriteRun( | ||
| spec -> spec.afterTypeValidationOptions(TypeValidation.builder().identifiers(false).build()), // TODO Remove escape hatch |
There was a problem hiding this comment.
Tried for a bit if we could remove this ignore, but it seemed a bit tricky. May try again when fresh, although I'm not opposed to merging as is.
There was a problem hiding this comment.
We can achieve this by splitting the logic into two passes. The first pass will scan the file to determine what changes are needed, and the second pass will apply those changes. This approach is similar to scanningRecipe, but we don’t need to use scanningRecipe here since all the required information is available within the same file.
However, this might not be worth the effort, given that the functionality is already implemented.
…it4to5Migration`
|
I've pushed one last change to include this by default, given the challenge outlined above, such that folks do not need to seek this out explicitly for a recipe run. Thanks again! |
|
Saw some unfortunate effects here after including it, so I've had to take it out from being included d1e6bbb You'll likely see the same where we now add a teardown to any nested class, which would need to be adjusted: I'll keep the recipe in for this release as I didn't want to revert your work and make it troublesome to include, but any help with the fix would be appreciated. |
I have already fixed all those issues in our internal version. I will publish the new PR with updated changes. |
What's changed?
Recipe to close unclosed static mocks in tests.
What's your motivation?
In JUnit4, when a timeout is specified, the test executes in a separate thread. Mockito stores static mocks in thread-local storage, which works fine in this context. However, in JUnit5, tests typically run on the main thread, even with timeout, leading to issues when static mocks are not explicitly closed.
This discrepancy causes tests to fail during migration from JUnit4 to JUnit5. Additionally, unclosed static mocks can leak into subsequent tests, leading to unpredictable failures and making debugging significantly more difficult.
Anyone you would like to review specifically?
@timtebeek
Checklist