Working on Issue #3156, Adding MockitoMockedStatic and MockitoMockedS… #3161
Conversation
…MockedStaticTest
| if (tree.getModifiers().getAnnotations().stream() | ||
| .anyMatch(annotation -> | ||
| state.getSourceForNode(annotation).equals("@org.mockito.Mock") | ||
| && state.getSourceForNode(tree.getType()).equals("org.mockito.MockedStatic"))) { |
There was a problem hiding this comment.
Should this check for MockedConstruction too? Both have the same issue I think in that mocking them results in a lifecycle-bound mock that has to be closed to prevent interfering with external components
There was a problem hiding this comment.
Would you reckon that should be a separate file or in the same one?
There was a problem hiding this comment.
Not entirely sure. Might be best to wait for confirmation from one of the maintainers! I'd assume it is almost identical logic minus the naming though so whether it is something that is reusable by just passing the class name and error message or not..?
They might not even want to cover the construction case though, so I'd definitely wait for confirmation :-)
There was a problem hiding this comment.
Let's check for both in this same check. In that case, let's name the checker MockitoIncompatibleMockTypes
...cts/errorprone/src/test/java/org/mockito/errorprone/bugpatterns/MockitoMockedStaticTest.java
Show resolved
Hide resolved
...cts/errorprone/src/test/java/org/mockito/errorprone/bugpatterns/MockitoMockedStaticTest.java
Show resolved
Hide resolved
|
Regarding the module errors you posted in those screenshots, I guess you will need to add the JVM flags at https://github.com/mockito/mockito/blob/main/subprojects/errorprone/errorprone.gradle#L20 to your test runner configuration in IntelliJ. Failing that, you should be able to just run When I've used errorprone before, I've used these JVM args: In Maven, IntelliJ seems to be able to automatically pull these from the |
|
I have added those variables to my test configuration under VM. I now get this. Does this mean it ran well and passed? I would assume it didn't as it skipped it. |
|
Think so. I believe you should get a reports directory in one of the |
Well it seems to fail when I remove the semi-colon again and when it is there runs it just concerns me when it says skipped. I am currently using JDK 17.0.2 which is just what I had installed at the time. Let me know if the above warning is something I should be concerned about "WARNING: package com.sun.tools.javac.type not in jdk.compiler" |
|
oh is that the retry test? Think that just reruns failed tests to deflake them |
| public Description matchVariable(VariableTree tree, VisitorState state) { | ||
| if (tree.getModifiers().getAnnotations().stream() | ||
| .anyMatch(annotation -> | ||
| state.getSourceForNode(annotation).equals("@org.mockito.Mock") |
There was a problem hiding this comment.
Do not use getSourceForNode to compare annotations, as that is equivalent to comparing toString(). Instead, use https://errorprone.info/api/latest/com/google/errorprone/util/ASTHelpers.html#isSameType(com.sun.tools.javac.code.Type,com.sun.tools.javac.code.Type,com.google.errorprone.VisitorState)
| if (tree.getModifiers().getAnnotations().stream() | ||
| .anyMatch(annotation -> | ||
| state.getSourceForNode(annotation).equals("@org.mockito.Mock") | ||
| && state.getSourceForNode(tree.getType()).equals("org.mockito.MockedStatic"))) { |
There was a problem hiding this comment.
Let's check for both in this same check. In that case, let's name the checker MockitoIncompatibleMockTypes
| import org.junit.runners.JUnit4; | ||
|
|
||
| @RunWith(JUnit4.class) | ||
| public class MockitoMockedStaticTest { |
There was a problem hiding this comment.
I think you missed the case where you have a @Mock private MockedStatic<Object> in the original issue description. You can use // BUG: diagnostic contains: to verify that the compilation would fail. Example: https://github.com/google/error-prone/blob/18d5cdf10ec1cc798a588d5c48a9e02a8888c6a1/core/src/test/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMapTest.java#L60
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3161 +/- ##
============================================
- Coverage 85.53% 85.49% -0.04%
- Complexity 2914 2917 +3
============================================
Files 334 335 +1
Lines 8861 8868 +7
Branches 1096 1098 +2
============================================
+ Hits 7579 7582 +3
- Misses 993 995 +2
- Partials 289 291 +2
☔ View full report in Codecov by Sentry. |
…taticTest
Checklist
including project members to get a better picture of the change
commit is meaningful and help the people that will explore a change in 2 years
Fixes #<issue number>in the description if relevantFixes #<issue number>if relevantThis pull request is for code commenting for TimvdLippe in regards to the Issue #3156 where there is an error with MockedStatic and Mock, so I am creating a catch for it with regards to previous methods. The main intention of this is to get some code feedback.
When running tests.