-
Notifications
You must be signed in to change notification settings - Fork 725
SONARJAVA-5708 Implement rule S8924: Mockito core methods should be imported statically #5689
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
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
2210a35
implement check
NoemieBenard cb6d955
add assertions
NoemieBenard 7ace68a
update rule metadata
NoemieBenard 82659bf
update rule metadata
NoemieBenard fb668c2
add autoscan diff
NoemieBenard f634a47
fix ruling test
NoemieBenard df8f8e7
restrain list of flagged methods
NoemieBenard 0b7d3cd
fix autoscan
NoemieBenard ae34266
move ruling test diff to the correct location
NoemieBenard b00bbbe
fix tests
NoemieBenard 6a8b7d5
apply review comments
NoemieBenard 2210c36
remove redundant check
NoemieBenard File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "ruleKey": "S3577", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 48, | ||
| "falseNegatives": 49, | ||
| "falsePositives": 0 | ||
| } |
4 changes: 2 additions & 2 deletions
4
its/autoscan/src/test/resources/autoscan/diffs/diff_S5969.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "ruleKey": "S5969", | ||
| "hasTruePositives": false, | ||
| "falseNegatives": 7, | ||
| "falseNegatives": 8, | ||
| "falsePositives": 0 | ||
| } | ||
| } |
4 changes: 2 additions & 2 deletions
4
its/autoscan/src/test/resources/autoscan/diffs/diff_S6068.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "ruleKey": "S6068", | ||
| "hasTruePositives": false, | ||
| "falseNegatives": 11, | ||
| "falseNegatives": 14, | ||
| "falsePositives": 0 | ||
| } | ||
| } |
6 changes: 6 additions & 0 deletions
6
its/autoscan/src/test/resources/autoscan/diffs/diff_S8924.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "ruleKey": "S8924", | ||
| "hasTruePositives": false, | ||
| "falseNegatives": 35, | ||
| "falsePositives": 0 | ||
| } |
14 changes: 14 additions & 0 deletions
14
its/ruling/src/test/resources/sonar-server/java-S8924.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| "org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/ce/log/CeLoggingTest.java": [ | ||
| 64 | ||
| ], | ||
| "org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/component/ws/TreeActionTest.java": [ | ||
| 88 | ||
| ], | ||
| "org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/language/ws/LanguageWsTest.java": [ | ||
| 51 | ||
| ], | ||
| "org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/rule/ws/ShowActionTest.java": [ | ||
| 150 | ||
| ] | ||
| } |
152 changes: 152 additions & 0 deletions
152
...hecks-test-sources/default/src/test/java/checks/tests/MockitoStaticImportCheckSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| package checks.tests; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.Mockito; | ||
|
|
||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.eq; | ||
| import static org.mockito.Mockito.doReturn; | ||
| import static org.mockito.Mockito.doThrow; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.never; | ||
| import static org.mockito.Mockito.spy; | ||
| import static org.mockito.Mockito.times; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| class MockitoStaticImportCheckSample { | ||
|
|
||
| interface MyService { | ||
| int getValue(); | ||
| String process(String input); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_mock() { | ||
| MyService service = Mockito.mock(MyService.class); // Noncompliant {{Use a static import for "mock".}} | ||
| verify(service).getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_when() { | ||
| MyService service = mock(MyService.class); | ||
| Mockito.when(service.getValue()).thenReturn(42); // Noncompliant {{Use a static import for "when".}} | ||
| verify(service).getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_spy_on_instance() { | ||
| MyService service = mock(MyService.class); | ||
| MyService spied = Mockito.spy(service); // Noncompliant {{Use a static import for "spy".}} | ||
| verify(spied).getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_spy_on_class() { | ||
| MyService spied = Mockito.spy(MyService.class); // Noncompliant {{Use a static import for "spy".}} | ||
| verify(spied).getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_doReturn() { | ||
| MyService spied = spy(MyService.class); | ||
| Mockito.doReturn(42).when(spied).getValue(); // Noncompliant {{Use a static import for "doReturn".}} | ||
| verify(spied).getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_doThrow() { | ||
| MyService spied = spy(MyService.class); | ||
| Mockito.doThrow(new RuntimeException()).when(spied).getValue(); // Noncompliant {{Use a static import for "doThrow".}} | ||
| verify(spied).getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_verify() { | ||
| MyService service = mock(MyService.class); | ||
| service.getValue(); | ||
| Mockito.verify(service).getValue(); // Noncompliant {{Use a static import for "verify".}} | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_times() { | ||
| MyService service = mock(MyService.class); | ||
| service.getValue(); | ||
| service.getValue(); | ||
| verify(service, Mockito.times(2)).getValue(); // Noncompliant {{Use a static import for "times".}} | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_never() { | ||
| MyService service = mock(MyService.class); | ||
| verify(service, Mockito.never()).getValue(); // Noncompliant {{Use a static import for "never".}} | ||
| } | ||
|
|
||
|
|
||
| @Test | ||
| void noncompliant_verify_and_times() { | ||
| MyService service = mock(MyService.class); | ||
| service.getValue(); | ||
| service.getValue(); | ||
| Mockito.verify(service, // Noncompliant {{Use a static import for "verify".}} | ||
| Mockito.times(2)) // Noncompliant {{Use a static import for "times".}} | ||
| .getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_verify_and_never() { | ||
| MyService service = mock(MyService.class); | ||
| Mockito.verify(service, // Noncompliant {{Use a static import for "verify".}} | ||
| Mockito.never()) // Noncompliant {{Use a static import for "never".}} | ||
| .getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void noncompliant_mock_inside_when() { | ||
| Mockito.when( // Noncompliant {{Use a static import for "when".}} | ||
| Mockito.mock(MyService.class).getValue()) // Noncompliant {{Use a static import for "mock".}} | ||
| .thenReturn(42); | ||
| verify(Mockito.mock(MyService.class)).getValue(); // Noncompliant {{Use a static import for "mock".}} | ||
| } | ||
|
|
||
| @Test | ||
| void compliant_all_methods() { | ||
| MyService service = mock(MyService.class); | ||
| MyService spied = spy(service); | ||
|
|
||
| when(service.getValue()).thenReturn(42); | ||
| when(service.process(any())).thenReturn("result"); | ||
| when(service.process(eq("input"))).thenReturn("result"); | ||
|
|
||
| doReturn(42).when(spied).getValue(); | ||
| doThrow(new RuntimeException()).when(spied).getValue(); | ||
|
|
||
| service.getValue(); | ||
| service.getValue(); | ||
| verify(service, times(2)).getValue(); | ||
| verify(service, never()).process(any()); | ||
| } | ||
|
|
||
| @Test | ||
| void compliant_out_of_scope_methods() { | ||
| MyService service = mock(MyService.class); | ||
| MyService spied = spy(MyService.class); | ||
|
|
||
| when(service.process(Mockito.any())).thenReturn("result"); | ||
| when(service.process(Mockito.eq("input"))).thenReturn("result"); | ||
| Mockito.inOrder(service); | ||
| Mockito.reset(service); | ||
| Mockito.doCallRealMethod().when(spied).getValue(); | ||
| Mockito.doNothing().when(spied).getValue(); | ||
| Mockito.doAnswer(invocation -> 42).when(spied).getValue(); | ||
| verify(service).getValue(); | ||
| } | ||
|
|
||
| @Test | ||
| void compliant_argument_matchers_prefix() { | ||
| MyService service = mock(MyService.class); | ||
| when(service.process(org.mockito.ArgumentMatchers.any())).thenReturn("result"); | ||
| when(service.process(org.mockito.ArgumentMatchers.eq("input"))).thenReturn("result"); | ||
| verify(service).getValue(); | ||
| } | ||
| } |
58 changes: 58 additions & 0 deletions
58
java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStaticImportCheck.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks.tests; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import org.sonar.check.Rule; | ||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||
| import org.sonar.plugins.java.api.semantic.MethodMatchers; | ||
| import org.sonar.plugins.java.api.tree.ExpressionTree; | ||
| import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; | ||
| import org.sonar.plugins.java.api.tree.MethodInvocationTree; | ||
| import org.sonar.plugins.java.api.tree.Tree; | ||
|
|
||
| @Rule(key = "S8924") | ||
| public class MockitoStaticImportCheck extends IssuableSubscriptionVisitor { | ||
|
|
||
| private static final String MOCKITO_CLASS = "org.mockito.Mockito"; | ||
|
|
||
| private static final MethodMatchers MOCKITO_METHODS = MethodMatchers.create() | ||
| .ofTypes(MOCKITO_CLASS) | ||
| .names("doReturn", "doThrow", "mock", "never", "spy", "times", "verify", "when") | ||
| .withAnyParameters() | ||
| .build(); | ||
|
gitar-bot[bot] marked this conversation as resolved.
|
||
|
|
||
| @Override | ||
| public List<Tree.Kind> nodesToVisit() { | ||
| return Collections.singletonList(Tree.Kind.METHOD_INVOCATION); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitNode(Tree tree) { | ||
| MethodInvocationTree mit = (MethodInvocationTree) tree; | ||
| ExpressionTree methodSelect = mit.methodSelect(); | ||
| if (!methodSelect.is(Tree.Kind.MEMBER_SELECT)) { | ||
| return; | ||
| } | ||
| MemberSelectExpressionTree mset = (MemberSelectExpressionTree) methodSelect; | ||
| String methodName = mset.identifier().name(); | ||
| if (MOCKITO_METHODS.matches(mit.methodSymbol())) { | ||
| reportIssue(methodSelect, "Use a static import for \"%s\".".formatted(methodName)); | ||
| } | ||
| } | ||
| } | ||
42 changes: 42 additions & 0 deletions
42
java-checks/src/test/java/org/sonar/java/checks/tests/MockitoStaticImportCheckTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks.tests; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.sonar.java.checks.verifier.CheckVerifier; | ||
|
|
||
| import static org.sonar.java.checks.verifier.TestUtils.testCodeSourcesPath; | ||
|
|
||
| class MockitoStaticImportCheckTest { | ||
|
|
||
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(testCodeSourcesPath("checks/tests/MockitoStaticImportCheckSample.java")) | ||
| .withCheck(new MockitoStaticImportCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void test_without_semantic() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(testCodeSourcesPath("checks/tests/MockitoStaticImportCheckSample.java")) | ||
| .withCheck(new MockitoStaticImportCheck()) | ||
| .withoutSemantic() | ||
| .verifyNoIssues(); | ||
| } | ||
| } |
76 changes: 76 additions & 0 deletions
76
sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8924.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| <p>This rule raises an issue when Mockito core methods are called with the <code>Mockito.</code> prefix instead of being statically imported.</p> | ||
| <p>This applies to commonly used Mockito methods such as:</p> | ||
| <ul> | ||
| <li><code>mock()</code></li> | ||
| <li><code>when()</code></li> | ||
| <li><code>verify()</code></li> | ||
| <li><code>spy()</code></li> | ||
| <li><code>doReturn()</code></li> | ||
| <li><code>doThrow()</code></li> | ||
| <li><code>times()</code></li> | ||
| <li><code>never()</code></li> | ||
| <li><code>any()</code></li> | ||
| <li><code>eq()</code></li> | ||
| </ul> | ||
| <h2>Why is this an issue?</h2> | ||
| <p>Mockito was designed with static imports in mind to create a more readable, fluent testing DSL (Domain-Specific Language). When you use the | ||
| <code>Mockito.</code> prefix for method calls, the test code becomes more verbose and harder to read.</p> | ||
| <p>Compare these two examples:</p> | ||
| <pre> | ||
| Mockito.when(Mockito.mock(MyService.class).getValue()).thenReturn(42); | ||
| </pre> | ||
| <p>versus:</p> | ||
| <pre> | ||
| when(mock(MyService.class).getValue()).thenReturn(42); | ||
| </pre> | ||
| <p>The second version reads more naturally, almost like plain English, which is the intent of Mockito’s API design. This readability improvement | ||
| becomes even more significant in larger test files with many mock interactions.</p> | ||
| <h3>What is the potential impact?</h3> | ||
| <p>Using the <code>Mockito.</code> prefix reduces code readability and makes test files unnecessarily verbose.</p> | ||
| <h2>How to fix it</h2> | ||
| <p>Add a static import for Mockito methods at the top of your test class and remove the <code>Mockito.</code> prefix from method calls. You can import | ||
| specific methods or use a wildcard import for commonly used Mockito methods.</p> | ||
| <h3>Code examples</h3> | ||
| <h4>Noncompliant code example</h4> | ||
| <pre data-diff-id="1" data-diff-type="noncompliant"> | ||
| import org.mockito.Mockito; | ||
|
|
||
| public class MyServiceTest { | ||
| private MyService myService = Mockito.mock(MyService.class); | ||
|
|
||
| @Test | ||
| public void testBehavior() { | ||
| Mockito.when(myService.getValue()).thenReturn(42); // Noncompliant | ||
|
|
||
| int result = myService.getValue(); | ||
|
|
||
| Mockito.verify(myService).getValue(); // Noncompliant | ||
| assertEquals(42, result); | ||
| } | ||
| } | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <pre data-diff-id="1" data-diff-type="compliant"> | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| public class MyServiceTest { | ||
| private MyService myService = mock(MyService.class); | ||
|
|
||
| @Test | ||
| public void testBehavior() { | ||
| when(myService.getValue()).thenReturn(42); | ||
|
|
||
| int result = myService.getValue(); | ||
|
|
||
| verify(myService).getValue(); | ||
| assertEquals(42, result); | ||
| } | ||
| } | ||
| </pre> | ||
| <h2>Resources</h2> | ||
| <h3>Documentation</h3> | ||
| <ul> | ||
| <li>Mockito Documentation - How do I import Mockito? - <a | ||
| href="https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html">Official Mockito documentation</a></li> | ||
| </ul> | ||
|
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.