Skip to content

Commit 227f78b

Browse files
SONARJAVA-5797 Fix FP in S2698 on fail and assertEquals with a message (#5332)
1 parent 991bf5f commit 227f78b

3 files changed

Lines changed: 36 additions & 4 deletions

File tree

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"ruleKey": "S3415",
33
"hasTruePositives": false,
4-
"falseNegatives": 280,
4+
"falseNegatives": 286,
55
"falsePositives": 0
6-
}
6+
}

java-checks-test-sources/default/src/test/java/checks/tests/AssertionsWithoutMessageCheckSample.java

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,21 @@ void foo() {
1717
org.testng.Assert.assertTrue(true, "msg"); // Compliant
1818
org.testng.Assert.assertTrue(true); // Noncompliant
1919
org.testng.AssertJUnit.assertTrue(true); // Noncompliant
20+
21+
org.testng.Assert.fail(); // Noncompliant
22+
org.testng.Assert.fail("msg"); // Compliant
23+
24+
org.testng.Assert.assertEquals(2, 3); // Noncompliant
25+
org.testng.Assert.assertEquals(2, 3, "two is not equal three"); // Compliant
26+
27+
org.testng.Assert.assertEquals(2.0, 3.1, 0.01); // Noncompliant
28+
org.testng.Assert.assertEquals(2.1, 3.2, 0.01, "diff more than delta"); // Compliant
29+
30+
org.testng.Assert.assertEquals("abc", "abc"); // Noncompliant
31+
org.testng.Assert.assertEquals("abc", "abc", "msg for strings"); // Compliant
32+
33+
org.testng.Assert.assertThrows(() -> {}); // Compliant
34+
2035
org.assertj.core.api.Assertions.assertThat("").usingComparator(null).as("a").isEqualTo(222); // Compliant
2136
org.junit.Assert.assertTrue(true); // Noncompliant {{Add a message to this assertion.}}
2237
// ^^^^^^^^^^

java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsWithoutMessageCheck.java

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,9 +115,26 @@ protected void onMethodInvocationFound(MethodInvocationTree mit) {
115115
* and it is the first of the last argument (depending on the assertion library).
116116
*/
117117
private static boolean hasMessageArg(MethodInvocationTree mit, Type type) {
118-
int expectedMessageArgIndex = (type.is("org.testng.Assert") || type.is("org.testng.AssertJUnit")) ? 1 : 0;
119118
List<ExpressionTree> args = mit.arguments();
120-
return expectedMessageArgIndex < args.size() && isString(args.get(expectedMessageArgIndex));
119+
120+
// `fail` needs one argument. Assume it is a string.
121+
if ("fail".equals(mit.methodSymbol().name())) {
122+
return !args.isEmpty();
123+
}
124+
125+
// In TestNG, the message is the last argument.
126+
if (type.is("org.testng.Assert") || type.is("org.testng.AssertJUnit")) {
127+
// Depending on the version of TestNG, assertThrows may or may not have the variant with a message.
128+
// For simplicity, do not raise.
129+
if ("assertThrows".equals(mit.methodSymbol().name())) {
130+
return true;
131+
}
132+
int expectedMessageArgIndex = args.size() - 1;
133+
return expectedMessageArgIndex > 0 && isString(args.get(expectedMessageArgIndex));
134+
}
135+
136+
// In JUnit and others, the message is the first argument.
137+
return !args.isEmpty() && isString(args.get(0));
121138
}
122139

123140
private void checkFestLikeAssertion(MethodInvocationTree mit, Symbol symbol, IdentifierTree reportLocation) {

0 commit comments

Comments
 (0)