diff --git a/its/ruling/src/test/resources/commons-beanutils/java-S2699.json b/its/ruling/src/test/resources/commons-beanutils/java-S2699.json index 3710b0b4eca..17449298ca3 100644 --- a/its/ruling/src/test/resources/commons-beanutils/java-S2699.json +++ b/its/ruling/src/test/resources/commons-beanutils/java-S2699.json @@ -36,9 +36,6 @@ "commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/bugs/Jira411TestCase.java": [ 40 ], -"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/bugs/Jira509TestCase.java": [ -55 -], "commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/bugs/Jira92TestCase.java": [ 35 ], diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S2699.json b/its/ruling/src/test/resources/eclipse-jetty/java-S2699.json index 6b8e0cf5704..986b6565356 100644 --- a/its/ruling/src/test/resources/eclipse-jetty/java-S2699.json +++ b/its/ruling/src/test/resources/eclipse-jetty/java-S2699.json @@ -18,9 +18,6 @@ 105, 158 ], -"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SelectChannelServerSslTest.java": [ -237 -], "org.eclipse.jetty:jetty-project:jetty-slf4j-impl/src/test/java/org/eclipse/jetty/logging/JettyLoggerTest.java": [ 221 ], diff --git a/java-checks-test-sources/default/src/test/java/checks/tests/AssertJAssertionsInConsumerCheckSample.java b/java-checks-test-sources/default/src/test/java/checks/tests/AssertJAssertionsInConsumerCheckSample.java index db7c9f75c29..be012c1266d 100644 --- a/java-checks-test-sources/default/src/test/java/checks/tests/AssertJAssertionsInConsumerCheckSample.java +++ b/java-checks-test-sources/default/src/test/java/checks/tests/AssertJAssertionsInConsumerCheckSample.java @@ -96,6 +96,13 @@ public void testIterableSatisfyMethods(Consumer unknownRequirements) { assertThat(myList).zipSatisfy(myList, (a, b) -> assertThat(a).isEqualTo(b)); } + @Test + public void testJavaAssertKeyword() { + List myList = getSomeList(); + assertThat(myList).allSatisfy(s -> { assert s != null; }); + assertThat("a").satisfies(s -> { assert !s.isEmpty(); }); + } + private void localMethodWithAssertion(Object objectToAssert) { assertThat(objectToAssert).isEqualTo("b"); } diff --git a/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/AssertJ.java b/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/AssertJ.java index 47fdbbf9bbb..f9dc2dea727 100644 --- a/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/AssertJ.java +++ b/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/AssertJ.java @@ -193,6 +193,7 @@ public void assertion_method_reference_in_helper_method() { abstract boolean booleanMethod(); abstract Object[] arrayMethod(); abstract java.util.List listStringMethod(); + abstract java.util.function.LongPredicate longPredicateMethod(); @Test public void bdd_assertions_with_boolean() { // Compliant @@ -269,4 +270,29 @@ public void bdd_assertions_then_no_exception_as() { // Noncompliant public void bdd_assertions_then_true_is_true(){ org.assertj.core.api.BDDAssertions.then(true).isTrue(); // Compliant } + + @Test + public void assertj_predicate_accepts() { + BDDAssertions.then(longPredicateMethod()).accepts(1L, 2L); + } + + @Test + public void assertj_string_starts_with() { + BDDAssertions.then("hello world").startsWith("hello"); + } + + @Test + public void assertj_string_matches() { + BDDAssertions.then("hello").matches("[a-z]+"); + } + + @Test + public void assertj_string_is_lower_case() { + BDDAssertions.then("hello").isLowerCase(); + } + + @Test + public void assertj_list_does_not_have_duplicates() { + BDDAssertions.then(listStringMethod()).doesNotHaveDuplicates(); + } } diff --git a/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/Junit4.java b/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/Junit4.java index 01073ac3f26..37e1e5afa8c 100644 --- a/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/Junit4.java +++ b/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/Junit4.java @@ -3,6 +3,7 @@ import java.util.List; import javax.annotation.Nullable; import junit.framework.TestCase; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -123,6 +124,16 @@ public void junit_test_annotated_with_expected() { throw new IllegalStateException("message"); } + @Ignore + @Test + public void ignored_test_without_assertion() { // Compliant - @Ignored tests are skipped + } + + @Test + public void java_assert_statement() { // Compliant - Java assert keyword is a valid assertion + assert true; + } + @Test public void mockito_assertion_verify() { Mockito.verify(Mockito.mock(List.class)).clear(); @@ -143,6 +154,20 @@ public void mockito_assertion_verify_no_more_interactions() { Mockito.verifyNoMoreInteractions(Mockito.mock(List.class)); } + @Ignore + static class IgnoredTestClass { + @Test + public void test_without_assertion() { // Compliant - enclosing class is @Ignored + } + + // JUnit still runs static nested classes inside @Ignored + static class StaticNestedInsideIgnored { + @Test + public void test_without_assertion() { // Noncompliant + } + } + } + static abstract class AbstractTest { @Test public abstract void unit_test(); diff --git a/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/Junit5.java b/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/Junit5.java index d29639fee13..f8ccffd3440 100644 --- a/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/Junit5.java +++ b/java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/Junit5.java @@ -3,7 +3,9 @@ import java.util.Arrays; import java.util.Collection; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.DynamicTest; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestFactory; @@ -29,6 +31,11 @@ class ATest extends Junit5Test { void test_method_parent() { // Ok - not considered as test method as it is overridden } + @Disabled + @Test + void disabled_test_without_assertion() { // Compliant - @Disabled tests are skipped + } + @Test void test_1_no_assertion() { // Noncompliant {{Add at least one assertion to this test case.}} } @@ -88,4 +95,39 @@ void bar() { interface CustomStringContextProvider extends Extension { } + @Nested + @Disabled + class DisabledNestedClass { + @Test + void test_without_assertion() { // Compliant - enclosing @Nested class is @Disabled + } + } + +} + +@Disabled +class DisabledClassTest { + @Test + void test_without_assertion() { // Compliant - enclosing class is @Disabled + } + + // JUnit still runs static nested classes inside @Disabled + static class StaticNestedInsideDisabled { + @Test + void test_without_assertion() { // Noncompliant + } + } + + @Nested + class NestedInsideDisabled { + @Test + void test_without_assertion() { // Compliant - @Disabled propagates to @Nested classes + } + } + + class NonStaticInnerInsideDisabled { // non-static, no @Nested: not a valid JUnit test container + @Test + void test_without_assertion() { // Compliant + } + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/AbstractAssertionVisitor.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/AbstractAssertionVisitor.java index 3c5b13d9e94..46baa926c51 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/AbstractAssertionVisitor.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/AbstractAssertionVisitor.java @@ -24,6 +24,8 @@ import org.sonar.plugins.java.api.tree.MethodReferenceTree; import org.sonar.plugins.java.api.tree.NewClassTree; +import org.sonar.plugins.java.api.tree.AssertStatementTree; + import static org.sonar.java.checks.helpers.UnitTestUtils.ASSERTION_INVOCATION_MATCHERS; import static org.sonar.java.checks.helpers.UnitTestUtils.methodNameMatchesAssertionMethodPattern; import static org.sonar.java.model.ExpressionUtils.methodName; @@ -55,6 +57,11 @@ public void visitNewClass(NewClassTree tree) { } } + @Override + public void visitAssertStatement(AssertStatementTree tree) { + hasAssertion = true; + } + public boolean hasAssertion() { return hasAssertion; } diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java index 1516dd283f7..a55ff3c4237 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java @@ -59,7 +59,7 @@ public final class UnitTestUtils { @VisibleForTesting static final Predicate ASSERTJ_ASSERTION_METHODS_PREDICATE = Pattern.compile( - "(allMatch|assert|contains|doesNot|has|is|returns|satisfies)([A-Z].*)?").asMatchPredicate(); + "(accepts|allMatch|assert|contains|doesNot|has|is|matches|returns|satisfies|startsWith)([A-Z].*)?").asMatchPredicate(); private static final Pattern ASSERTJ_ASSERTION_CLASSNAME_PATTERN = Pattern.compile("org\\.assertj\\.core\\.api\\.[a-zA-Z]+Assert"); private static final Predicate ASSERTJ_ASSERTION_TYPE_PREDICATE = type -> ASSERTJ_ASSERTION_CLASSNAME_PATTERN.matcher(type.fullyQualifiedName()).matches() @@ -145,6 +145,12 @@ public final class UnitTestUtils { public static final MethodMatchers COMMON_ASSERTION_MATCHER = MethodMatchers.or( FAIL_METHOD_MATCHER, ASSERTIONS_METHOD_MATCHER); + public static final Set SKIPPED_TEST_ANNOTATIONS = Set.of("org.junit.Ignore", "org.junit.jupiter.api.Disabled"); + + public static boolean isAnnotatedWithSkippedTestAnnotation(SymbolMetadata metadata) { + return SKIPPED_TEST_ANNOTATIONS.stream().anyMatch(metadata::isAnnotatedWith); + } + private static final Set TEST_ANNOTATIONS = new HashSet<>(asList(ORG_JUNIT_TEST, "org.testng.annotations.Test")); public static final Set JUNIT5_TEST_ANNOTATIONS = Set.of( "org.junit.jupiter.api.Test", diff --git a/java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java index 77553fc1ec5..95651fa2151 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java @@ -43,6 +43,7 @@ import org.sonar.plugins.java.api.tree.Tree; import static org.apache.commons.lang3.StringUtils.isEmpty; +import static org.sonar.java.checks.helpers.UnitTestUtils.isAnnotatedWithSkippedTestAnnotation; import static org.sonar.java.checks.helpers.UnitTestUtils.isUnitTest; @Rule(key = "S2699") @@ -87,7 +88,7 @@ public void visitMethod(MethodTree methodTree) { } if (isUnitTest(methodTree)) { - if (isSpringBootAssertableContext(methodTree)) { + if (isAnnotatedWithSkippedTestAnnotation(methodTree.symbol().metadata()) || isSpringBootAssertableContext(methodTree)) { return; } if (!isSpringBootSanityTest(methodTree) && !expectAssertion(methodTree) && !isLocalMethodWithAssertion(methodTree.symbol())) { @@ -96,6 +97,21 @@ public void visitMethod(MethodTree methodTree) { } } + @Override + public void visitClass(ClassTree classTree) { + if (isAnnotatedWithSkippedTestAnnotation(classTree.symbol().metadata())) { + // Static nested classes are separate test containers; + // JUnit does not propagate the disabled state to them, so we still visit them. + classTree.members().stream() + .filter(member -> member.is(Tree.Kind.CLASS)) + .map(ClassTree.class::cast) + .filter(nestedClass -> ModifiersUtils.hasModifier(nestedClass.modifiers(), Modifier.STATIC)) + .forEach(this::visitClass); + return; + } + super.visitClass(classTree); + } + private boolean isSpringBootAssertableContext(MethodTree methodTree) { var runMethodInvocation = SPRING_BOOT_APP_CTX_RUNNER_VISITOR.findMethodInvocation(methodTree); if (runMethodInvocation != null) { diff --git a/java-checks/src/main/java/org/sonar/java/checks/tests/IgnoredTestsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/tests/IgnoredTestsCheck.java index c61cb8721c1..d004395bff7 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/tests/IgnoredTestsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/tests/IgnoredTestsCheck.java @@ -21,6 +21,7 @@ import java.util.List; import java.util.Optional; import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.UnitTestUtils; import org.sonar.java.model.ExpressionUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; @@ -56,7 +57,7 @@ public void visitNode(Tree tree) { SymbolMetadata symbolMetadata = methodTree.symbol().metadata(); // check for @Ignore or @Disabled annotations - for (String annotationName : List.of("org.junit.Ignore", "org.junit.jupiter.api.Disabled")) { + for (String annotationName : UnitTestUtils.SKIPPED_TEST_ANNOTATIONS) { getSilentlyIgnoredAnnotation(symbolMetadata, annotationName) .ifPresent(annotationTree -> { String shortName = annotationTypeIdentifier(annotationName); diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2699.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2699.html index b567b1b2a32..8fdbb52238a 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2699.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2699.html @@ -10,6 +10,7 @@

Why is this an issue?

  • Eclipse Vert.x
  • Fest 1.x and 2.x
  • Hamcrest
  • +
  • Java built-in assert statement
  • JMock
  • JMockit
  • JUnit
  • @@ -22,6 +23,8 @@

    Why is this an issue?

  • Truth Framework
  • WireMock
  • +

    Tests annotated with @Disabled (JUnit 5) or @Ignore (JUnit 4) are excluded from this rule, as they are not executed by +the test framework.

    Furthermore, as new or custom assertion frameworks may be used, the rule can be parametrized to define specific methods that will also be considered as assertions. No issue will be raised when such methods are found in test cases. The parameter value should have the following format <FullyQualifiedClassName>#<MethodName>, where MethodName can end with the wildcard character. For constructors,