SONARJAVA-6605 Implement S9017: Mockito stubbing chains should be complete

[NoemieBenard]

---

Summary by Gitar

• New rule implementation:
  • Implemented S9017 to detect incomplete Mockito stubbing chains using when() or do*() methods.
  • Added rule metadata, documentation, and comprehensive test cases in MockitoStubbingChainCheckSample.java.

_{This will update automatically on new commits.}

[hashicorp-vault-sonar-prod]

SONARJAVA-6605

[romainbrenguier]

Using an instance variable isChained and calling scan directly doesn't fill clean. Couldn't we have a recursive version of visitMethodInvocation that takes the equivalent of isChained as an argument.

[romainbrenguier]

It looks better. Thanks

[gitar-bot]

Code Review ✅ Approved 3 resolved / 3 findings

Implements S9017 to detect incomplete Mockito stubbing chains with full test coverage, addressing previous issues regarding redundant parameters, variable assignments, and lambda scoping.

✅ 3 resolved

✅ Edge Case: False positive: stub assigned to pre-declared variable

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:66-68 📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:91-93
The check suppresses reporting when a stub is stored in a variable by making visitVariable a no-op, which skips the initializer of a VariableTree (e.g. OngoingStubbing<User> stub = when(repo.findUser(42));). However, this only covers variable declarations. When the stub is stored via a plain assignment to a previously declared variable, e.g.:

OngoingStubbing<User> stub;
stub = when(repo.findUser(42));
stub.thenReturn(new User("Alice"));

the RHS is reached through AssignmentExpressionTree, not VariableTree. visitMethodInvocation then sees when(...) as the top-level invocation with isChained == null and raises a false positive, even though the stubbing is completed later. Because S9017 is a Critical/HIGH-reliability rule, a false positive on valid test code erodes trust.

Consider also skipping assignment right-hand sides (override visitAssignmentExpression similarly, or detect that the invocation is an assignment RHS) so the behavior matches the variable-declaration case. Add a test covering the reassignment pattern.

✅ Edge Case: Incomplete stubbings inside lambda/anonymous args not detected

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:82-83
visitMethodInvocation only recurses into mit.methodSelect() and intentionally does not scan the arguments. While this correctly avoids reprocessing chain receivers, it also means any code nested inside argument expressions is never visited. As a result, incomplete stubbings inside lambda bodies or anonymous classes passed as arguments are missed, for example:

assertThrows(RuntimeException.class, () -> {
  when(repo.findUser(1)); // incomplete, not flagged
  service.deleteUser(1);
});

This is a false negative that reduces the rule's effectiveness in common test patterns (lambdas passed to assertThrows, doAnswer, etc.). If this limitation is intentional, consider documenting it; otherwise, scan the bodies of lambda/anonymous-class arguments (while still not treating them as part of the current chain, i.e. reset isChained to null when descending into them).

✅ Quality: Redundant isChained parameter in isIncompleteStubbing

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:97-110
isIncompleteStubbing is only ever called at line 98 inside a !isChained guard, so isChained is always false there and the if (isChained) return false; branch (lines 108-110) is dead code. Drop the parameter and that early-return to simplify the method and avoid confusing maintainers into thinking the guard is reachable.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[gitar-bot]

Code Review ✅ Approved 3 resolved / 3 findings

Implements S9017 to detect incomplete Mockito stubbing chains with full test coverage, addressing previous issues regarding redundant parameters, variable assignments, and lambda scoping.

✅ 3 resolved

✅ Edge Case: False positive: stub assigned to pre-declared variable

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:66-68 📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:91-93
The check suppresses reporting when a stub is stored in a variable by making visitVariable a no-op, which skips the initializer of a VariableTree (e.g. OngoingStubbing<User> stub = when(repo.findUser(42));). However, this only covers variable declarations. When the stub is stored via a plain assignment to a previously declared variable, e.g.:

OngoingStubbing<User> stub;
stub = when(repo.findUser(42));
stub.thenReturn(new User("Alice"));

the RHS is reached through AssignmentExpressionTree, not VariableTree. visitMethodInvocation then sees when(...) as the top-level invocation with isChained == null and raises a false positive, even though the stubbing is completed later. Because S9017 is a Critical/HIGH-reliability rule, a false positive on valid test code erodes trust.

Consider also skipping assignment right-hand sides (override visitAssignmentExpression similarly, or detect that the invocation is an assignment RHS) so the behavior matches the variable-declaration case. Add a test covering the reassignment pattern.

✅ Edge Case: Incomplete stubbings inside lambda/anonymous args not detected

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:82-83
visitMethodInvocation only recurses into mit.methodSelect() and intentionally does not scan the arguments. While this correctly avoids reprocessing chain receivers, it also means any code nested inside argument expressions is never visited. As a result, incomplete stubbings inside lambda bodies or anonymous classes passed as arguments are missed, for example:

assertThrows(RuntimeException.class, () -> {
  when(repo.findUser(1)); // incomplete, not flagged
  service.deleteUser(1);
});

This is a false negative that reduces the rule's effectiveness in common test patterns (lambdas passed to assertThrows, doAnswer, etc.). If this limitation is intentional, consider documenting it; otherwise, scan the bodies of lambda/anonymous-class arguments (while still not treating them as part of the current chain, i.e. reset isChained to null when descending into them).

✅ Quality: Redundant isChained parameter in isIncompleteStubbing

📄 java-checks/src/main/java/org/sonar/java/checks/tests/MockitoStubbingChainCheck.java:97-110
isIncompleteStubbing is only ever called at line 98 inside a !isChained guard, so isChained is always false there and the if (isChained) return false; branch (lines 108-110) is dead code. Drop the parameter and that early-return to simplify the method and avoid confusing maintainers into thinking the guard is reachable.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}