diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1481.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1481.json index 0a9e1c62b2b..2c9fcebef19 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1481.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1481.json @@ -1,6 +1,6 @@ { "ruleKey": "S1481", "hasTruePositives": true, - "falseNegatives": 8, + "falseNegatives": 9, "falsePositives": 0 } diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2699.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2699.json index b01aa5d1a61..039237eaa74 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S2699.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S2699.json @@ -1,6 +1,6 @@ { "ruleKey": "S2699", "hasTruePositives": true, - "falseNegatives": 164, + "falseNegatives": 171, "falsePositives": 1 } diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S3577.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S3577.json index 6b9a0072838..f6f9d38e209 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S3577.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S3577.json @@ -1,6 +1,6 @@ { "ruleKey": "S3577", "hasTruePositives": true, - "falseNegatives": 49, + "falseNegatives": 50, "falsePositives": 0 } diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S5778.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S5778.json index 9fc6644b170..b283342d7b4 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S5778.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S5778.json @@ -1,6 +1,6 @@ { "ruleKey": "S5778", "hasTruePositives": false, - "falseNegatives": 44, + "falseNegatives": 46, "falsePositives": 0 } diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S9017.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S9017.json new file mode 100644 index 00000000000..a1908412882 --- /dev/null +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S9017.json @@ -0,0 +1,6 @@ +{ + "ruleKey": "S9017", + "hasTruePositives": false, + "falseNegatives": 8, + "falsePositives": 0 +} diff --git a/java-checks-test-sources/default/src/test/java/checks/tests/MockitoStubbingChainCheckSample.java b/java-checks-test-sources/default/src/test/java/checks/tests/MockitoStubbingChainCheckSample.java new file mode 100644 index 00000000000..aa2cd691ff4 --- /dev/null +++ b/java-checks-test-sources/default/src/test/java/checks/tests/MockitoStubbingChainCheckSample.java @@ -0,0 +1,103 @@ +package checks.tests; + +import org.junit.jupiter.api.Test; +import org.mockito.stubbing.OngoingStubbing; + +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doCallRealMethod; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class MockitoStubbingChainCheckSample { + + interface UserRepository { + User findUser(long id); + void deleteUser(long id); + User saveUser(User user); + } + + record User(String name) {} + + @Test + void incompleteWhen() { + UserRepository repo = mock(UserRepository.class); + when(repo.findUser(42)); // Noncompliant {{Complete this stubbing by adding "thenReturn()", "thenThrow()", "thenAnswer()", or "thenCallRealMethod()".}} + //^^^^^^^^^^^^^^^^^^^^^^^ + } + + @Test + void completeWhen() { + UserRepository repo = mock(UserRepository.class); + when(repo.findUser(42)).thenReturn(new User("Alice")); // Compliant + when(repo.findUser(43)).thenThrow(new RuntimeException()); // Compliant + when(repo.findUser(44)).thenAnswer(inv -> new User("Bob")); // Compliant + when(repo.findUser(45)).thenCallRealMethod(); // Compliant + } + + @Test + void incompleteDoMethods() { + UserRepository repo = mock(UserRepository.class); + doReturn(new User("Alice")); // Noncompliant {{Complete this stubbing by adding ".when(mock).method()".}} + //^^^^^^^^^^^^^^^^^^^^^^^^^^^ + doThrow(new RuntimeException()); // Noncompliant {{Complete this stubbing by adding ".when(mock).method()".}} [[sc=5;ec=36]] + doAnswer(inv -> null); // Noncompliant {{Complete this stubbing by adding ".when(mock).method()".}} + doNothing(); // Noncompliant {{Complete this stubbing by adding ".when(mock).method()".}} + doCallRealMethod(); // Noncompliant {{Complete this stubbing by adding ".when(mock).method()".}} + } + + @Test + void incompleteDoChainMissingMethodCall() { + UserRepository repo = mock(UserRepository.class); + doThrow(new RuntimeException()).when(repo); // Noncompliant {{Complete this stubbing by adding the method to stub.}} + //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + @Test + void completeDoMethods() { + UserRepository repo = mock(UserRepository.class); + doReturn(new User("Alice")).when(repo).findUser(42); // Compliant + doThrow(new RuntimeException()).when(repo).deleteUser(99); // Compliant + doAnswer(inv -> new User("Bob")).when(repo).saveUser(new User("Bob")); // Compliant + doNothing().when(repo).deleteUser(1); // Compliant + doCallRealMethod().when(repo).findUser(1); // Compliant + } + + @Test + void whenStoredInVariable() { + UserRepository repo = mock(UserRepository.class); + OngoingStubbing stub = when(repo.findUser(42)); // Compliant - variable declaration skipped + stub.thenReturn(new User("Alice")); + } + + @Test + void whenStoredViaAssignment() { + UserRepository repo = mock(UserRepository.class); + OngoingStubbing stub; + stub = when(repo.findUser(42)); // Compliant - assignment expression skipped + stub.thenReturn(new User("Alice")); + } + + @Test + void incompleteWhenInsideLambda() { + UserRepository repo = mock(UserRepository.class); + org.junit.jupiter.api.Assertions.assertThrows(RuntimeException.class, () -> { + when(repo.findUser(1)); // Noncompliant {{Complete this stubbing by adding "thenReturn()", "thenThrow()", "thenAnswer()", or "thenCallRealMethod()".}} + }); + } + + @Test + void completeWhenInsideLambda() { + UserRepository repo = mock(UserRepository.class); + org.junit.jupiter.api.Assertions.assertThrows(RuntimeException.class, () -> { + when(repo.findUser(1)).thenReturn(new User("Alice")); // Compliant + }); + } + + OngoingStubbing helperMethod(UserRepository repo) { + return when(repo.findUser(42)); // Compliant - return statement skipped + } +} + diff --git a/java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java b/java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java new file mode 100644 index 00000000000..deccd175084 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java @@ -0,0 +1,122 @@ +/* + * 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.sonar.check.Rule; +import org.sonar.plugins.java.api.JavaFileScanner; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.BlockTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.LambdaExpressionTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.ReturnStatementTree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S9017") +public class MockitoStubbingChainCheck extends BaseTreeVisitor implements JavaFileScanner { + + private static final String WHEN_MESSAGE = "Complete this stubbing by adding \"thenReturn()\", \"thenThrow()\", \"thenAnswer()\", or \"thenCallRealMethod()\"."; + private static final String DO_MESSAGE = "Complete this stubbing by adding \".when(mock).method()\"."; + private static final String STUBBER_WHEN_MESSAGE = "Complete this stubbing by adding the method to stub."; + + private static final MethodMatchers MOCKITO_WHEN = MethodMatchers.create() + .ofTypes("org.mockito.Mockito") + .names("when") + .withAnyParameters() + .build(); + + private static final MethodMatchers MOCKITO_DO_METHODS = MethodMatchers.create() + .ofTypes("org.mockito.Mockito") + .names("doReturn", "doThrow", "doAnswer", "doNothing", "doCallRealMethod") + .withAnyParameters() + .build(); + + // Detects do*().when(mock) that is not followed by a method call on the mock + private static final MethodMatchers STUBBER_WHEN = MethodMatchers.create() + .ofSubTypes("org.mockito.stubbing.Stubber") + .names("when") + .withAnyParameters() + .build(); + + private JavaFileScannerContext context; + + @Override + public void scanFile(JavaFileScannerContext context) { + if (context.getSemanticModel() == null) { + return; + } + this.context = context; + scan(context.getTree()); + } + + @Override + public void visitVariable(VariableTree tree) { + // skip: the stub may be stored in a variable and completed later + } + + @Override + public void visitAssignmentExpression(AssignmentExpressionTree tree) { + // skip: the stub may be stored in a pre-declared variable and completed later + } + + @Override + public void visitReturnStatement(ReturnStatementTree tree) { + // skip: returning a partial stub is valid (e.g. helper methods) + } + + @Override + public void visitMethodInvocation(MethodInvocationTree mit) { + visitMethodInvocation(mit, false); + // also check stubs inside block-bodied lambda arguments (e.g. assertThrows(() -> { when(...); })) + for (ExpressionTree arg : mit.arguments()) { + if (arg instanceof LambdaExpressionTree lambda + && lambda.body() instanceof BlockTree block) { + visitBlock(block); + } + } + } + + private void visitMethodInvocation(MethodInvocationTree mit, boolean isChained) { + if (!isChained && isIncompleteStubbing(mit)) { + return; + } + if (mit.methodSelect() instanceof MemberSelectExpressionTree mset + && mset.expression() instanceof MethodInvocationTree innerMit) { + visitMethodInvocation(innerMit, true); + } + } + + private boolean isIncompleteStubbing(MethodInvocationTree mit) { + if (MOCKITO_WHEN.matches(mit)) { + context.reportIssue(this, mit, WHEN_MESSAGE); + return true; + } + if (MOCKITO_DO_METHODS.matches(mit)) { + context.reportIssue(this, mit, DO_MESSAGE); + return true; + } + if (STUBBER_WHEN.matches(mit)) { + context.reportIssue(this, mit, STUBBER_WHEN_MESSAGE); + return true; + } + return false; + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/tests/MockitoStubbingChainCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/tests/MockitoStubbingChainCheckTest.java new file mode 100644 index 00000000000..40deb31dba0 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/tests/MockitoStubbingChainCheckTest.java @@ -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 MockitoStubbingChainCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(testCodeSourcesPath("checks/tests/MockitoStubbingChainCheckSample.java")) + .withCheck(new MockitoStubbingChainCheck()) + .verifyIssues(); + } + + @Test + void testWithoutSemantics() { + CheckVerifier.newVerifier() + .onFile(testCodeSourcesPath("checks/tests/MockitoStubbingChainCheckSample.java")) + .withCheck(new MockitoStubbingChainCheck()) + .withoutSemantic() + .verifyNoIssues(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9017.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9017.html new file mode 100644 index 00000000000..ac778803e52 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9017.html @@ -0,0 +1,94 @@ +

This rule raises an issue when test mock stubbing is syntactically incomplete.

+

In Mockito, this means: a when() call must be followed by thenReturn(), thenThrow(), +thenAnswer(), or thenCallRealMethod(); and a doReturn(), doThrow(), doAnswer(), +doNothing(), or doCallRealMethod() call must be followed by .when(mock).method().

+

Why is this an issue?

+

Test double frameworks allow you to configure behavior through chained API calls. These chains must be syntactically complete to take effect.

+

When you write the first part of a stub configuration (specifying which method call to intercept) without completing the chain with a behavior +specification (what value to return or action to take), the configuration is discarded. The method continues to return its default value (null for +objects, 0 for numbers, false for booleans).

+

Similarly, when you write the behavior specification first (what to return) without completing it with the method specification (which method to +stub), the configured behavior is never attached to any method. It is silently dropped.

+

Both patterns compile and run without errors in lenient testing frameworks. This makes them hard to spot during code review or testing. The test +may pass because it never actually exercises the intended stub, or it may fail with a confusing error message that doesn’t point to the incomplete +stubbing.

+

The incomplete stub creates a gap between what you intended to test and what the test actually verifies. This undermines the reliability of your +test suite.

+

What is the potential impact?

+

Incomplete mock object stubs can cause:

+
    +
  • False passes: Tests pass because they’re checking the wrong behavior (default return values from unconfigured mock methods + instead of your intended stub behavior)
    • +
  • Misleading failures: Tests fail with null pointer exceptions or assertion errors that don’t indicate the real problem
    • +
  • Maintenance burden: Developers waste time debugging test failures that stem from incomplete mock configurations rather than + actual code issues
    • +
  • Reduced confidence: The test suite becomes less trustworthy when tests don’t verify what they appear to verify
    • +
+

How to fix it in Mockito

+

Complete the when() chain by adding a behavior method such as thenReturn(), thenThrow(), +thenAnswer(), or thenCallRealMethod().

+

Code examples

+

Noncompliant code example

+
+@Test
+void testFindUser() {
+  UserRepository mock = mock(UserRepository.class);
+  when(mock.findUser(42)); // Noncompliant
+
+  User user = service.getUser(42);
+  assertNotNull(user); // Will fail - findUser returns null
+}
+
+

Compliant solution

+
+@Test
+void testFindUser() {
+  UserRepository mock = mock(UserRepository.class);
+  when(mock.findUser(42)).thenReturn(new User("Alice"));
+
+  User user = service.getUser(42);
+  assertNotNull(user); // Now passes correctly
+}
+
+

Complete the do*() chain by adding .when(mock).method() to specify which method should have the configured behavior.

+

Noncompliant code example

+
+@Test
+void testDeleteUser() {
+  UserRepository mock = mock(UserRepository.class);
+  doThrow(new RuntimeException()); // Noncompliant
+
+  assertThrows(RuntimeException.class, () -> {
+    service.deleteUser(42);
+  }); // Will fail - no exception is thrown
+}
+
+

Compliant solution

+
+@Test
+void testDeleteUser() {
+  UserRepository mock = mock(UserRepository.class);
+  doThrow(new RuntimeException()).when(mock).deleteUser(42);
+
+  assertThrows(RuntimeException.class, () -> {
+    service.deleteUser(42);
+  }); // Now passes correctly
+}
+
+

Resources

+

Documentation

+ +

Related rules

+
    +
  • {rule:java:S2970} - Assertions should be complete - similar pattern of incomplete test code
    • +
+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9017.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9017.json new file mode 100644 index 00000000000..b15d6e7e983 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9017.json @@ -0,0 +1,26 @@ +{ + "title": "Mockito stubbing chains should be complete", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5 min" + }, + "tags": [ + "mockito", + "tests", + "junit" + ], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-9017", + "sqKey": "S9017", + "scope": "Tests", + "quickfix": "unknown", + "code": { + "impacts": { + "RELIABILITY": "HIGH", + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "COMPLETE" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9017 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9017 new file mode 100644 index 00000000000..e69de29bb2d