SONARJAVA-6833: S2699 - Fix false positives for disabled tests, Java assert, and AssertJ methods by asya-vorobeva · Pull Request #6096 · SonarSource/sonar-java

asya-vorobeva · 2026-09-08T14:58:08Z

Add missing AssertJ assertion method prefixes (accepts, matches, startsWith) to ASSERTJ_ASSERTION_METHODS_PREDICATE so methods like accepts(), startsWith(), and matches() are recognized when called on AssertJ assertion types
Skip assertion checking for test methods annotated with @disabled (JUnit 5) or @ignore (JUnit 4) since those tests are never executed
Recognize Java built-in assert statement as a valid assertion in AbstractAssertionVisitor, fixing FPs for both S2699 and S6103

@disabled

…assert, and AssertJ methods - Add missing AssertJ assertion method prefixes (accepts, matches, startsWith) to ASSERTJ_ASSERTION_METHODS_PREDICATE so methods like accepts(), startsWith(), and matches() are recognized when called on AssertJ assertion types - Skip assertion checking for test methods annotated with @disabled (JUnit 5) or @ignore (JUnit 4) since those tests are never executed - Recognize Java built-in assert statement as a valid assertion in AbstractAssertionVisitor, fixing FPs for both S2699 and S6103 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

hashicorp-vault-sonar-prod · 2026-09-08T14:58:23Z

SONARJAVA-6833

gitar-bot · 2026-09-08T15:05:35Z

+  @Test
+  public void assertj_predicate_accepts() {
+    Assertions.assertThat(longPredicateMethod()).accepts(1L, 2L);
+  }
+
+  @Test
+  public void assertj_string_starts_with() {
+    Assertions.assertThat("hello world").startsWith("hello");
+  }
+
+  @Test
+  public void assertj_string_matches() {
+    Assertions.assertThat("hello").matches("[a-z]+");
+  }
+


⚠️ Quality: New AssertJ samples don't exercise the predicate change

All five new sample tests start their chain with Assertions.assertThat(...), whose name already matches ASSERTION_METHODS_PATTERN ((assert|verify|fail|...).*) in methodNameMatchesAssertionMethodPattern, so AbstractAssertionVisitor sets hasAssertion = true on the assertThat invocation itself and never needs to match the terminal accepts/startsWith/matches call. These samples therefore pass identically with or without the ASSERTJ_ASSERTION_METHODS_PREDICATE change, leaving the PR's main code change with zero coverage (the existing bdd_assertions_* samples in the same file use BDDAssertions.then(...) precisely because then is not name-matched). Use the BDD entry point (or a bare AbstractAssert receiver) so the terminal method is what decides, and/or extend UnitTestUtilsTest.testAssertJAssertionMethodPattern with the three new alternatives.

Drive the samples through BDDAssertions.then(...) so the new terminal-method names are what makes them compliant:

@Test public void assertj_predicate_accepts() { // Compliant BDDAssertions.then(longPredicateMethod()).accepts(1L, 2L); } @Test public void assertj_string_starts_with() { // Compliant BDDAssertions.then("hello world").startsWith("hello"); } @Test public void assertj_string_matches() { // Compliant BDDAssertions.then("hello").matches("[a-z]+"); }

Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

gitar-bot · 2026-09-08T15:05:37Z

@ignore

+  private static boolean isDisabledTest(MethodTree methodTree) {
+    SymbolMetadata metadata = methodTree.symbol().metadata();
+    return metadata.isAnnotatedWith(JUNIT5_DISABLED_ANNOTATION) || metadata.isAnnotatedWith(JUNIT4_IGNORE_ANNOTATION);
+  }


💡 Edge Case: @Disabled/@ignore skipped only at method level, not class level

isDisabledTest only inspects the method symbol's metadata, so a test class annotated with @Disabled (JUnit 5) or @Ignore (JUnit 4) — whose methods are equally never executed, which is the PR's stated rationale — still raises S2699 on every assertion-less method. The same gap applies to a @Disabled enclosing @Nested class. Extend the check to the enclosing class(es).

Also consider the enclosing class annotations:

private static boolean isDisabledTest(MethodTree methodTree) { Symbol.TypeSymbol enclosingClass = methodTree.symbol().enclosingClass(); return isDisabled(methodTree.symbol().metadata()) || (enclosingClass != null && isDisabled(enclosingClass.metadata())); } private static boolean isDisabled(SymbolMetadata metadata) { return metadata.isAnnotatedWith(JUNIT5_DISABLED_ANNOTATION) || metadata.isAnnotatedWith(JUNIT4_IGNORE_ANNOTATION); }

Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

gitar-bot · 2026-09-08T15:05:38Z

+  private static final String JUNIT5_DISABLED_ANNOTATION = "org.junit.jupiter.api.Disabled";
+  private static final String JUNIT4_IGNORE_ANNOTATION = "org.junit.Ignore";


💡 Quality: Disabled-test annotation names duplicated from IgnoredTestsCheck

IgnoredTestsCheck (S1607) already encodes the same pair of fully qualified names (org.junit.Ignore, org.junit.jupiter.api.Disabled), and JUnit4AnnotationsCheck maps between them; the new private constants and isDisabledTest re-implement that knowledge in a third place, so a future addition (e.g. TestNG's org.testng.annotations.Ignore) has to be applied several times. Move the annotation list and the predicate into UnitTestUtils, next to the other test-annotation sets, and reuse it from both checks.

Add a shared helper in UnitTestUtils and call it from AssertionsInTestsCheck:

// UnitTestUtils.java public static final List<String> SKIPPED_TEST_ANNOTATIONS = List.of("org.junit.Ignore", "org.junit.jupiter.api.Disabled"); public static boolean isSkippedTest(MethodTree methodTree) { SymbolMetadata metadata = methodTree.symbol().metadata(); return SKIPPED_TEST_ANNOTATIONS.stream().anyMatch(metadata::isAnnotatedWith); } // AssertionsInTestsCheck.java, replacing the two constants and isDisabledTest if (isSkippedTest(methodTree) || isSpringBootAssertableContext(methodTree)) {

Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

gitar-bot · 2026-09-08T15:05:39Z

@disabled

+  @Override
+  public void visitAssertStatement(AssertStatementTree tree) {
+    hasAssertion = true;
+  }


💡 Quality: Rule descriptions not updated for assert/@disabled exemptions

S2699's description enumerates exactly which assertion mechanisms are recognised ("assertions from any of the following known frameworks") and says nothing about Java's assert statement or about skipped tests being exempt, so after this PR the documented behaviour no longer matches the implementation for both S2699 and S6103 (the latter now accepts a bare assert inside an AssertJ consumer). Since these HTML files are generated from RSPEC, the corresponding RSPEC entries need updating along with the code change.

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

gitar-bot · 2026-09-08T15:05:42Z

Reviewing your code

Code Review ⚠️ Changes requested 0 resolved / 4 findings

Fixes false positives in S2699 by recognizing Java's assert statement, adding missing AssertJ method prefixes, and skipping disabled tests. Three issues must be addressed: new AssertJ samples don't exercise the predicate change (they use assertThat(...) which matches by name, leaving the terminal method names uncovered), disabled-test checks only apply at method level and should extend to class level, and annotation constants are duplicated across three checks instead of being centralized in UnitTestUtils.

⚠️

Quality: New AssertJ samples don't exercise the predicate change

📄 java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/AssertJ.java:274-288 📄 java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java:61-62 📄 java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java:295-304 📄 java-checks/src/main/java/org/sonar/java/checks/helpers/AbstractAssertionVisitor.java:37-42

All five new sample tests start their chain with Assertions.assertThat(...), whose name already matches ASSERTION_METHODS_PATTERN ((assert|verify|fail|...).*) in methodNameMatchesAssertionMethodPattern, so AbstractAssertionVisitor sets hasAssertion = true on the assertThat invocation itself and never needs to match the terminal accepts/startsWith/matches call. These samples therefore pass identically with or without the ASSERTJ_ASSERTION_METHODS_PREDICATE change, leaving the PR's main code change with zero coverage (the existing bdd_assertions_* samples in the same file use BDDAssertions.then(...) precisely because then is not name-matched). Use the BDD entry point (or a bare AbstractAssert receiver) so the terminal method is what decides, and/or extend UnitTestUtilsTest.testAssertJAssertionMethodPattern with the three new alternatives.

Drive the samples through BDDAssertions.then(...) so the new terminal-method names are what makes them compliant

@Test
public void assertj_predicate_accepts() { // Compliant
  BDDAssertions.then(longPredicateMethod()).accepts(1L, 2L);
}

@Test
public void assertj_string_starts_with() { // Compliant
  BDDAssertions.then("hello world").startsWith("hello");
}

@Test
public void assertj_string_matches() { // Compliant
  BDDAssertions.then("hello").matches("[a-z]+");
}

💡 Edge Case: @Disabled/@Ignore skipped only at method level, not class level

📄 java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java:102-105

isDisabledTest only inspects the method symbol's metadata, so a test class annotated with @Disabled (JUnit 5) or @Ignore (JUnit 4) — whose methods are equally never executed, which is the PR's stated rationale — still raises S2699 on every assertion-less method. The same gap applies to a @Disabled enclosing @Nested class. Extend the check to the enclosing class(es).

Also consider the enclosing class annotations

private static boolean isDisabledTest(MethodTree methodTree) {
  Symbol.TypeSymbol enclosingClass = methodTree.symbol().enclosingClass();
  return isDisabled(methodTree.symbol().metadata())
    || (enclosingClass != null && isDisabled(enclosingClass.metadata()));
}

private static boolean isDisabled(SymbolMetadata metadata) {
  return metadata.isAnnotatedWith(JUNIT5_DISABLED_ANNOTATION) || metadata.isAnnotatedWith(JUNIT4_IGNORE_ANNOTATION);
}

💡 Quality: Disabled-test annotation names duplicated from IgnoredTestsCheck

📄 java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java:83-84 📄 java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java:102-105

IgnoredTestsCheck (S1607) already encodes the same pair of fully qualified names (org.junit.Ignore, org.junit.jupiter.api.Disabled), and JUnit4AnnotationsCheck maps between them; the new private constants and isDisabledTest re-implement that knowledge in a third place, so a future addition (e.g. TestNG's org.testng.annotations.Ignore) has to be applied several times. Move the annotation list and the predicate into UnitTestUtils, next to the other test-annotation sets, and reuse it from both checks.

Add a shared helper in UnitTestUtils and call it from AssertionsInTestsCheck

// UnitTestUtils.java
  public static final List<String> SKIPPED_TEST_ANNOTATIONS = List.of("org.junit.Ignore", "org.junit.jupiter.api.Disabled");

  public static boolean isSkippedTest(MethodTree methodTree) {
    SymbolMetadata metadata = methodTree.symbol().metadata();
    return SKIPPED_TEST_ANNOTATIONS.stream().anyMatch(metadata::isAnnotatedWith);
  }

// AssertionsInTestsCheck.java, replacing the two constants and isDisabledTest
      if (isSkippedTest(methodTree) || isSpringBootAssertableContext(methodTree)) {

💡 Quality: Rule descriptions not updated for assert/@Disabled exemptions

📄 java-checks/src/main/java/org/sonar/java/checks/helpers/AbstractAssertionVisitor.java:60-63 📄 java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java:102-105

S2699's description enumerates exactly which assertion mechanisms are recognised ("assertions from any of the following known frameworks") and says nothing about Java's assert statement or about skipped tests being exempt, so after this PR the documented behaviour no longer matches the implementation for both S2699 and S6103 (the latter now accepts a bare assert inside an AssertJ consumer). Since these HTML files are generated from RSPEC, the corresponding RSPEC entries need updating along with the code change.

🤖 Prompt for agents

Code Review: Fixes false positives in S2699 by recognizing Java's `assert` statement, adding missing AssertJ method prefixes, and skipping disabled tests. Three issues must be addressed: new AssertJ samples don't exercise the predicate change (they use `assertThat(...)` which matches by name, leaving the terminal method names uncovered), disabled-test checks only apply at method level and should extend to class level, and annotation constants are duplicated across three checks instead of being centralized in `UnitTestUtils`.

1. ⚠️ Quality: New AssertJ samples don't exercise the predicate change
   Files: java-checks-test-sources/default/src/test/java/checks/tests/AssertionsInTestsCheck/AssertJ.java:274-288, java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java:61-62, java-checks/src/main/java/org/sonar/java/checks/helpers/UnitTestUtils.java:295-304, java-checks/src/main/java/org/sonar/java/checks/helpers/AbstractAssertionVisitor.java:37-42

   All five new sample tests start their chain with `Assertions.assertThat(...)`, whose name already matches `ASSERTION_METHODS_PATTERN` (`(assert|verify|fail|...).*`) in `methodNameMatchesAssertionMethodPattern`, so `AbstractAssertionVisitor` sets `hasAssertion = true` on the `assertThat` invocation itself and never needs to match the terminal `accepts`/`startsWith`/`matches` call. These samples therefore pass identically with or without the `ASSERTJ_ASSERTION_METHODS_PREDICATE` change, leaving the PR's main code change with zero coverage (the existing `bdd_assertions_*` samples in the same file use `BDDAssertions.then(...)` precisely because `then` is not name-matched). Use the BDD entry point (or a bare `AbstractAssert` receiver) so the terminal method is what decides, and/or extend `UnitTestUtilsTest.testAssertJAssertionMethodPattern` with the three new alternatives.

   Fix (Drive the samples through BDDAssertions.then(...) so the new terminal-method names are what makes them compliant):
   @Test
   public void assertj_predicate_accepts() { // Compliant
     BDDAssertions.then(longPredicateMethod()).accepts(1L, 2L);
   }
   
   @Test
   public void assertj_string_starts_with() { // Compliant
     BDDAssertions.then("hello world").startsWith("hello");
   }
   
   @Test
   public void assertj_string_matches() { // Compliant
     BDDAssertions.then("hello").matches("[a-z]+");
   }

2. 💡 Edge Case: @Disabled/@Ignore skipped only at method level, not class level
   Files: java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java:102-105

   `isDisabledTest` only inspects the method symbol's metadata, so a test class annotated with `@Disabled` (JUnit 5) or `@Ignore` (JUnit 4) — whose methods are equally never executed, which is the PR's stated rationale — still raises S2699 on every assertion-less method. The same gap applies to a `@Disabled` enclosing `@Nested` class. Extend the check to the enclosing class(es).

   Fix (Also consider the enclosing class annotations):
   private static boolean isDisabledTest(MethodTree methodTree) {
     Symbol.TypeSymbol enclosingClass = methodTree.symbol().enclosingClass();
     return isDisabled(methodTree.symbol().metadata())
       || (enclosingClass != null && isDisabled(enclosingClass.metadata()));
   }
   
   private static boolean isDisabled(SymbolMetadata metadata) {
     return metadata.isAnnotatedWith(JUNIT5_DISABLED_ANNOTATION) || metadata.isAnnotatedWith(JUNIT4_IGNORE_ANNOTATION);
   }

3. 💡 Quality: Disabled-test annotation names duplicated from IgnoredTestsCheck
   Files: java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java:83-84, java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java:102-105

   `IgnoredTestsCheck` (S1607) already encodes the same pair of fully qualified names (`org.junit.Ignore`, `org.junit.jupiter.api.Disabled`), and `JUnit4AnnotationsCheck` maps between them; the new private constants and `isDisabledTest` re-implement that knowledge in a third place, so a future addition (e.g. TestNG's `org.testng.annotations.Ignore`) has to be applied several times. Move the annotation list and the predicate into `UnitTestUtils`, next to the other test-annotation sets, and reuse it from both checks.

   Fix (Add a shared helper in UnitTestUtils and call it from AssertionsInTestsCheck):
   // UnitTestUtils.java
     public static final List<String> SKIPPED_TEST_ANNOTATIONS = List.of("org.junit.Ignore", "org.junit.jupiter.api.Disabled");
   
     public static boolean isSkippedTest(MethodTree methodTree) {
       SymbolMetadata metadata = methodTree.symbol().metadata();
       return SKIPPED_TEST_ANNOTATIONS.stream().anyMatch(metadata::isAnnotatedWith);
     }
   
   // AssertionsInTestsCheck.java, replacing the two constants and isDisabledTest
         if (isSkippedTest(methodTree) || isSpringBootAssertableContext(methodTree)) {

4. 💡 Quality: Rule descriptions not updated for assert/@Disabled exemptions
   Files: java-checks/src/main/java/org/sonar/java/checks/helpers/AbstractAssertionVisitor.java:60-63, java-checks/src/main/java/org/sonar/java/checks/tests/AssertionsInTestsCheck.java:102-105

   S2699's description enumerates exactly which assertion mechanisms are recognised ("assertions from any of the following known frameworks") and says nothing about Java's `assert` statement or about skipped tests being exempt, so after this PR the documented behaviour no longer matches the implementation for both S2699 and S6103 (the latter now accepts a bare `assert` inside an AssertJ consumer). Since these HTML files are generated from RSPEC, the corresponding RSPEC entries need updating along with the code change.

Implementation Status ✅ 3 of 3 objectives covered

✅ SONARJAVA-6833 - 3 of 3 objectives covered

This PR successfully implements all objectives: it adds missing AssertJ assertion methods, skips assertion checking for disabled or ignored tests, and recognizes Java built-in assert statements as valid assertions.

✅ 3 covered here

✅ Add missing AssertJ assertion methods (startsWith, accepts, matches, isLowerCase, doesNotHaveDuplicates) to the set of recognized AssertJ assertions
✅ Skip assertion checking for test methods annotated with JUnit 5's @Disabled or JUnit 4's @Ignored
✅ Recognize Java's built-in assert statement as a valid assertion

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.
Unblock → Override a blocking verdict and allow merging.

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

`Auto-apply`	`Compact`	`Unblock`
`gitar auto-apply:on`	`gitar display:verbose`	`gitar unblock`

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

github-actions · 2026-09-08T15:16:42Z

❌ Ruling needs updating. A fix PR has been created: #6097

Please review and merge it into your branch.

github-actions · 2026-09-08T15:21:51Z

Ruling Diff Summary

Detected changes in 2 rule files: 2 issues removed, 0 issues added.

S2699 (java) on commons-beanutils - 1 issues removed, 0 issues added

Removed src/test/java/org/apache/commons/beanutils2/bugs/Jira509TestCase.java (line 55)

        50 |     /**
        51 |      * The bug makes the {@link WrapDynaClass#createDynaClass} method run in an infinite loop and acquire locks. The
        52 |      * test case adds a timeout. The test case may pass even without this fix because this is a rare scenario.
        53 |      */
        54 |     @Test(timeout = 60_000)
>>>     55 |     public void test_concurrent() throws InterruptedException {
        56 |         final List<Class<?>> classList = Arrays.asList(Map.class, HashMap.class, Collections.class, Arrays.class,
        57 |                 Collection.class, Set.class, ArrayList.class, List.class, HashSet.class);
        58 | 
        59 |         // All daemon threads.
        60 |         final ExecutorService executor = Executors.newFixedThreadPool(100, new ThreadFactory() {

S2699 (java) on eclipse-jetty - 1 issues removed, 0 issues added

Removed jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SelectChannelServerSslTest.java (line 237)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SelectChannelServerSslTest.java)

sonarqube-next · 2026-09-08T15:30:10Z

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 Bot reviewed Sep 8, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

github-actions Bot mentioned this pull request Sep 8, 2026

Update ruling results for PR #6096 #6097

Closed

Updated ruling results and rspec

d8094bc

Navigation Menu

Uh oh!

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

SONARJAVA-6833: S2699 - Fix false positives for disabled tests, Java assert, and AssertJ methods#6096
asya-vorobeva wants to merge 2 commits into
masterfrom
asya/fix-s2699-fps

asya-vorobeva commented Sep 8, 2026

Uh oh!

hashicorp-vault-sonar-prod Bot commented Sep 8, 2026 •

edited by atlassian Bot

Loading

Uh oh!

gitar-bot Bot Sep 8, 2026

Uh oh!

gitar-bot Bot Sep 8, 2026

Uh oh!

gitar-bot Bot Sep 8, 2026

Uh oh!

gitar-bot Bot Sep 8, 2026

Uh oh!

gitar-bot Bot commented Sep 8, 2026 •

edited

Loading

Uh oh!

This comment has been minimized.

github-actions Bot commented Sep 8, 2026

Uh oh!

github-actions Bot commented Sep 8, 2026

Uh oh!

sonarqube-next Bot commented Sep 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		private static final String JUNIT5_DISABLED_ANNOTATION = "org.junit.jupiter.api.Disabled";
		private static final String JUNIT4_IGNORE_ANNOTATION = "org.junit.Ignore";

Uh oh!

Conversation

asya-vorobeva commented Sep 8, 2026

Uh oh!

hashicorp-vault-sonar-prod Bot commented Sep 8, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitar-bot Bot Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

github-actions Bot commented Sep 8, 2026

Uh oh!

github-actions Bot commented Sep 8, 2026

Ruling Diff Summary

Uh oh!

sonarqube-next Bot commented Sep 8, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

hashicorp-vault-sonar-prod Bot commented Sep 8, 2026 •

edited by atlassian Bot

Loading

gitar-bot Bot commented Sep 8, 2026 •

edited

Loading