SONARJAVA-6833: S2699 - Fix false positives for disabled tests, Java assert, and AssertJ methods

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
 ],

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
 ],

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

@@ -96,6 +96,13 @@ public void testIterableSatisfyMethods(Consumer<Object> unknownRequirements) {
     assertThat(myList).zipSatisfy(myList, (a, b) -> assertThat(a).isEqualTo(b));
   }
 
+  @Test
+  public void testJavaAssertKeyword() {
+    List<Object> 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");
   }

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<String> 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();
+  }
 }

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();

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
+    }
+  }
 }

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;
   }

java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java

@@ -59,7 +59,7 @@ public final class UnitTestUtils {
 
   @VisibleForTesting
   static final Predicate<String> 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<Type> 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<String> 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<String> TEST_ANNOTATIONS = new HashSet<>(asList(ORG_JUNIT_TEST, "org.testng.annotations.Test"));
   public static final Set<String> JUNIT5_TEST_ANNOTATIONS = Set.of(
     "org.junit.jupiter.api.Test",

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) {

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);

sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S2699.html

@@ -10,6 +10,7 @@ <h2>Why is this an issue?</h2>
   <li>Eclipse Vert.x</li>
   <li>Fest 1.x and 2.x</li>
   <li>Hamcrest</li>
+  <li>Java built-in <code>assert</code> statement</li>
   <li>JMock</li>
   <li>JMockit</li>
   <li>JUnit</li>
@@ -22,6 +23,8 @@ <h2>Why is this an issue?</h2>
   <li>Truth Framework</li>
   <li>WireMock</li>
 </ul>
+<p>Tests annotated with <code>@Disabled</code> (JUnit 5) or <code>@Ignore</code> (JUnit 4) are excluded from this rule, as they are not executed by
+the test framework.</p>
 <p>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
 <code>&lt;FullyQualifiedClassName&gt;#&lt;MethodName&gt;</code>, where <code>MethodName</code> can end with the wildcard character. For constructors,