Skip to content

Commit 080ce83

Browse files
asya-vorobevaasya-vorobeva
committed
SONARJAVA-5820 S2698: should suggest using assertThrows and expectThrows with message
1 parent 93a20b9 commit 080ce83

6 files changed

Lines changed: 91 additions & 23 deletions

File tree

‎java-checks-test-sources/default/pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,7 @@
534534
<dependency>
535535
<groupId>org.testng</groupId>
536536
<artifactId>testng</artifactId>
537-
<version>7.5.1</version>
537+
<version>7.12.0</version>
538538
<type>jar</type>
539539
<scope>provided</scope>
540540
</dependency>

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,11 @@ void foo() {
3030
org.testng.Assert.assertEquals("abc", "abc"); // Noncompliant
3131
org.testng.Assert.assertEquals("abc", "abc", "msg for strings"); // Compliant
3232

33-
org.testng.Assert.assertThrows(() -> {}); // Compliant
33+
org.testng.Assert.assertThrows(() -> {}); // Noncompliant
34+
org.testng.Assert.assertThrows(Exception.class, () -> {}); // Noncompliant
35+
org.testng.Assert.assertThrows("msg", Exception.class, () -> {}); // Compliant
36+
org.testng.Assert.expectThrows(Exception.class, () -> {}); // Noncompliant
37+
org.testng.Assert.expectThrows("msg", Exception.class, () -> {}); // Compliant
3438

3539
org.assertj.core.api.Assertions.assertThat("").usingComparator(null).as("a").isEqualTo(222); // Compliant
3640
org.junit.Assert.assertTrue(true); // Noncompliant {{Add a message to this assertion.}}

‎java-checks-test-sources/default/src/test/java/checks/tests/AssertionsWithoutMessageCheckSample_Testng75.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
package checks.tests;
2+
3+
public class AssertionsWithoutMessageCheckSample_Testng75 {
4+
void foo() {
5+
org.testng.Assert.assertThrows(() -> {}); // Compliant
6+
org.testng.Assert.assertThrows(Exception.class, () -> {}); // Compliant
7+
org.testng.Assert.expectThrows(Exception.class, () -> {}); // Compliant
8+
}
9+
}

‎java-checks-test-sources/pom.xml‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,24 @@
4848
<outputFile>${project.build.directory}/test-classpath.txt</outputFile>
4949
</configuration>
5050
</execution>
51+
<execution>
52+
<id>copy</id>
53+
<phase>test-compile</phase>
54+
<goals>
55+
<goal>copy</goal>
56+
</goals>
57+
<configuration>
58+
<artifactItems>
59+
<artifactItem>
60+
<groupId>org.testng</groupId>
61+
<artifactId>testng</artifactId>
62+
<version>7.5.1</version>
63+
<type>jar</type>
64+
</artifactItem>
65+
</artifactItems>
66+
<outputDirectory>${project.build.directory}/test-jars</outputDirectory>
67+
</configuration>
68+
</execution>
5169
</executions>
5270
</plugin>
5371
</plugins>

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

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,15 @@
1717
package org.sonar.java.checks.tests;
1818

1919
import java.util.List;
20+
import java.util.Optional;
2021
import java.util.Set;
22+
import java.util.function.Function;
23+
import java.util.function.Predicate;
24+
2125
import org.sonar.check.Rule;
2226
import org.sonar.java.checks.methods.AbstractMethodDetection;
27+
import org.sonar.plugins.java.api.DependencyVersionAware;
28+
import org.sonar.plugins.java.api.Version;
2329
import org.sonarsource.analyzer.commons.collections.SetUtils;
2430
import org.sonar.java.model.ExpressionUtils;
2531
import org.sonar.plugins.java.api.semantic.MethodMatchers;
@@ -35,55 +41,58 @@
3541
import static org.sonar.plugins.java.api.semantic.Type.Primitives.FLOAT;
3642

3743
@Rule(key = "S2698")
38-
public class AssertionsWithoutMessageCheck extends AbstractMethodDetection {
44+
public class AssertionsWithoutMessageCheck extends AbstractMethodDetection implements DependencyVersionAware {
3945

4046
private static final String MESSAGE = "Add a message to this assertion.";
4147
private static final String MESSAGE_FEST_LIKE = "Add a message to this assertion chain before the predicate method.";
42-
private static final String ASSERT = "assert";
4348

4449
private static final String JAVA_LANG_STRING = "java.lang.String";
4550

4651
private static final String FEST_GENERIC_ASSERT = "org.fest.assertions.GenericAssert";
4752
private static final String ASSERTJ_ABSTRACT_ASSERT = "org.assertj.core.api.AbstractAssert";
48-
private static final MethodMatchers FEST_LIKE_MESSAGE_METHODS = MethodMatchers.or(
49-
MethodMatchers.create()
50-
.ofSubTypes(FEST_GENERIC_ASSERT).names("as", "describedAs", "overridingErrorMessage")
51-
.addParametersMatcher(types -> matchFirstParameterWithAnyOf(types, JAVA_LANG_STRING, "org.fest.assertions.Description")).build(),
52-
MethodMatchers.create()
53-
.ofSubTypes(ASSERTJ_ABSTRACT_ASSERT).names("as", "describedAs", "withFailMessage", "overridingErrorMessage")
54-
.addParametersMatcher(types -> matchFirstParameterWithAnyOf(types, JAVA_LANG_STRING, "org.assertj.core.description.Description"))
55-
.build()
56-
);
5753

5854
private static final Set<String> ASSERT_METHODS_WITH_ONE_PARAM = SetUtils.immutableSetOf("assertNull", "assertNotNull");
5955
private static final Set<String> ASSERT_METHODS_WITH_TWO_PARAMS = SetUtils.immutableSetOf("assertEquals", "assertSame", "assertNotSame", "assertThat");
6056
private static final Set<String> JUNIT5_ASSERT_METHODS_IGNORED = SetUtils.immutableSetOf("assertAll", "assertLinesMatch");
6157
private static final Set<String> JUNIT5_ASSERT_METHODS_WITH_ONE_PARAM = SetUtils.immutableSetOf("assertTrue", "assertFalse", "assertNull", "assertNotNull", "assertDoesNotThrow");
6258
private static final Set<String> JUNIT5_ASSERT_METHODS_WITH_DELTA = SetUtils.immutableSetOf("assertArrayEquals", "assertEquals");
59+
private static final Set<String> TESTNG_THROWS_METHODS = SetUtils.immutableSetOf("assertThrows", "expectThrows");
60+
61+
private static final Predicate<String> INVOCATION_PREDICATE = name ->
62+
name.startsWith("assert") || name.startsWith("expect") || "fail".equals(name);
6363

6464
private static final MethodMatchers FEST_LIKE_ABSTRACT_ASSERT = MethodMatchers.create()
6565
.ofSubTypes(FEST_GENERIC_ASSERT, ASSERTJ_ABSTRACT_ASSERT).anyName().withAnyParameters().build();
66-
66+
private static final MethodMatchers FEST_LIKE_MESSAGE_METHODS = MethodMatchers.or(
67+
MethodMatchers.create()
68+
.ofSubTypes(FEST_GENERIC_ASSERT).names("as", "describedAs", "overridingErrorMessage")
69+
.addParametersMatcher(types -> matchFirstParameterWithAnyOf(types, JAVA_LANG_STRING, "org.fest.assertions.Description")).build(),
70+
MethodMatchers.create()
71+
.ofSubTypes(ASSERTJ_ABSTRACT_ASSERT).names("as", "describedAs", "withFailMessage", "overridingErrorMessage")
72+
.addParametersMatcher(types -> matchFirstParameterWithAnyOf(types, JAVA_LANG_STRING, "org.assertj.core.description.Description"))
73+
.build()
74+
);
6775
private static final MethodMatchers ASSERT_THAT_MATCHER = MethodMatchers.create()
6876
.ofSubTypes("org.assertj.core.api.Assertions",
6977
"org.assertj.core.api.AssertionsForInterfaceTypes",
7078
"org.assertj.core.api.AssertionsForClassTypes",
7179
"org.fest.assertions.Assertions")
7280
.names("assertThat", "assertThatObject").withAnyParameters().build();
73-
7481
private static final MethodMatchers ASSERT_SETTING_CONTEXT = MethodMatchers.create()
7582
.ofSubTypes(ASSERTJ_ABSTRACT_ASSERT)
7683
.name(name -> name.startsWith("extracting") || name.startsWith("using") || name.startsWith("filtered"))
7784
.withAnyParameters()
7885
.build();
7986

87+
private boolean isTestng77orLater = false;
88+
8089
@Override
8190
protected MethodMatchers getMethodInvocationMatchers() {
8291
return MethodMatchers.or(
8392
MethodMatchers.create()
8493
.ofTypes("org.junit.jupiter.api.Assertions", "org.junit.Assert", "junit.framework.Assert", "org.fest.assertions.Fail",
8594
"org.assertj.core.api.Fail", "org.testng.Assert", "org.testng.AssertJUnit")
86-
.name(name -> name.startsWith(ASSERT) || "fail".equals(name)).withAnyParameters().build(),
95+
.name(INVOCATION_PREDICATE).withAnyParameters().build(),
8796
FEST_LIKE_ABSTRACT_ASSERT
8897
);
8998
}
@@ -114,7 +123,7 @@ protected void onMethodInvocationFound(MethodInvocationTree mit) {
114123
* True if the call has a message argument. Such an argument is a string
115124
* and it is the first of the last argument (depending on the assertion library).
116125
*/
117-
private static boolean hasMessageArg(MethodInvocationTree mit, Type type) {
126+
private boolean hasMessageArg(MethodInvocationTree mit, Type type) {
118127
List<ExpressionTree> args = mit.arguments();
119128

120129
// `fail` needs one argument. Assume it is a string.
@@ -124,13 +133,13 @@ private static boolean hasMessageArg(MethodInvocationTree mit, Type type) {
124133

125134
// In TestNG, the message is the last argument.
126135
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())) {
136+
// Depending on the version of TestNG, assertThrows /expectThrows may or may not have the variant with a message.
137+
boolean isThrowsMethod = TESTNG_THROWS_METHODS.contains(mit.methodSymbol().name());
138+
if (!isTestng77orLater && isThrowsMethod) {
130139
return true;
131140
}
132-
int expectedMessageArgIndex = args.size() - 1;
133-
return expectedMessageArgIndex > 0 && isString(args.get(expectedMessageArgIndex));
141+
int expectedMessageArgIndex = isThrowsMethod ? 0 : (args.size() - 1);
142+
return expectedMessageArgIndex >= 0 && isString(args.get(expectedMessageArgIndex));
134143
}
135144

136145
// In JUnit and others, the message is the first argument.
@@ -230,4 +239,10 @@ private static boolean isString(ExpressionTree expressionTree) {
230239
return expressionTree.symbolType().is(JAVA_LANG_STRING);
231240
}
232241

242+
@Override
243+
public boolean isCompatibleWithDependencies(Function<String, Optional<Version>> dependencyFinder) {
244+
Optional<Version> testngVersion = dependencyFinder.apply("testng");
245+
isTestng77orLater = testngVersion.map(v -> v.isGreaterThanOrEqualTo("7.7")).orElse(false);
246+
return true;
247+
}
233248
}

‎java-checks/src/test/java/org/sonar/java/checks/tests/AssertionsWithoutMessageCheckTest.java‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,38 @@
1818

1919
import org.junit.jupiter.api.Test;
2020
import org.sonar.java.checks.verifier.CheckVerifier;
21+
import org.sonar.java.checks.verifier.FilesUtils;
22+
import org.sonar.java.test.classpath.TestClasspathUtils;
23+
24+
import java.io.File;
25+
import java.nio.file.Path;
26+
import java.util.ArrayList;
27+
import java.util.List;
2128

2229
import static org.sonar.java.checks.verifier.TestUtils.testCodeSourcesPath;
2330

2431
class AssertionsWithoutMessageCheckTest {
2532

2633
@Test
27-
void test() {
34+
void test_Testng77() {
35+
List<File> classPath = new ArrayList<>(TestClasspathUtils.DEFAULT_MODULE.getClassPath());
2836
CheckVerifier.newVerifier()
2937
.onFile(testCodeSourcesPath("checks/tests/AssertionsWithoutMessageCheckSample.java"))
3038
.withCheck(new AssertionsWithoutMessageCheck())
39+
.withClassPath(classPath)
3140
.verifyIssues();
3241
}
42+
43+
@Test
44+
void test_Testng75() {
45+
List<File> classPath = new ArrayList<>(TestClasspathUtils.DEFAULT_MODULE.getClassPath());
46+
classPath.removeIf(file -> file.getName().contains("testng"));
47+
List<File> list = FilesUtils.getFilesRecursively(Path.of("..", "java-checks-test-sources", "target/test-jars"), "jar");
48+
classPath.addAll(list);
49+
CheckVerifier.newVerifier()
50+
.onFile(testCodeSourcesPath("checks/tests/AssertionsWithoutMessageCheckSample_Testng75.java"))
51+
.withCheck(new AssertionsWithoutMessageCheck())
52+
.withClassPath(classPath)
53+
.verifyNoIssues();
54+
}
3355
}

0 commit comments

Comments
 (0)