Skip to content

SONARJAVA-6208 S2699: Add approve to assertion method name pattern#5570

Merged
NoemieBenard merged 7 commits intomasterfrom
nb/sonarjava-6208-assertions-in-tests
Apr 15, 2026
Merged

SONARJAVA-6208 S2699: Add approve to assertion method name pattern#5570
NoemieBenard merged 7 commits intomasterfrom
nb/sonarjava-6208-assertions-in-tests

Conversation

@NoemieBenard
Copy link
Copy Markdown
Contributor

Add approve keyword to the assertion methods pattern to support ApproveJ library

@hashicorp-vault-sonar-prod
Copy link
Copy Markdown
Contributor

hashicorp-vault-sonar-prod Bot commented Apr 14, 2026

SONARJAVA-6208

@sonar-review-alpha
Copy link
Copy Markdown
Contributor

sonar-review-alpha Bot commented Apr 14, 2026

Summary

This PR extends the S2699 rule ("Tests should include assertions") to recognize assertion calls from the ApproveJ library. ApproveJ uses an approve() method for snapshot-style assertions, which was not previously recognized by SonarJava's assertion detection pattern.

The change adds "approve" as an additional assertion method prefix to the existing pattern that already recognizes assert, verify, fail, should, check, expect, validate, andExpect, and others. A new test file with ApproveJ usage examples is included, along with documentation updates listing ApproveJ as a supported framework.

What reviewers should know

Where to start: Review the core change in UnitTestUtils.java (line 44) where "approve" is added to the regex pattern. This is the single-line change that enables ApproveJ support.

Test coverage: The new ApproveJ.java test file demonstrates the rule behavior with ApproveJ—one test with no assertions (should be flagged as noncompliant) and two tests using approve() calls (should pass). Check that test cases cover the library's common usage patterns.

Metrics update: The false negatives count in diff_S2699.json increases by 1 (157→158), indicating one more false negative was found and is now fixed by this change.

Why this matters: ApproveJ is a snapshot testing library that provides approval-style assertions. Without recognizing approve(), the rule would incorrectly flag tests using this library as having no assertions.


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

sonar-review-alpha[bot]

This comment was marked as resolved.

NoemieBenard and others added 2 commits April 14, 2026 13:07
Co-authored-by: sonar-review-alpha[bot] <266116024+sonar-review-alpha[bot]@users.noreply.github.com>
sonar-review-alpha[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this change looks good, but we should also update the documentation in RSpec. Let me know when you have time and I'll show you how to do this.

sonar-review-alpha[bot]

This comment was marked as outdated.

<artifactId>core</artifactId>
<version>1.6.0</version>
<scope>test</scope>
</dependency>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix formatting.

Copy link
Copy Markdown
Contributor

@sonar-review-alpha sonar-review-alpha Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, focused PR overall. One question worth discussing regarding the autoscan metrics change.

🗣️ Give feedback

"hasTruePositives": true,
"falseNegatives": 157,
"falseNegatives": 158,
"falsePositives": 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The false negatives count went up by 1 (157 → 158) after adding approve to the pattern. This suggests the autoscan corpus contains at least one test method that calls something like approveOrder() or approvePayment() — a business-logic domain method that the rule now silently treats as an assertion, causing it to miss a genuine missing-assertion issue.

The word approve is broad enough to appear in non-assertion domain methods inside test code. Is this increase expected/acceptable as a deliberate trade-off, or is it a signal that the pattern should be narrowed (e.g. via a type-based check anchored to org.approvej, similar to how other frameworks are handled)?

  • Mark as noise

@sonarqube-next
Copy link
Copy Markdown

@NoemieBenard NoemieBenard merged commit 0d3bc92 into master Apr 15, 2026
14 of 15 checks passed
@NoemieBenard NoemieBenard deleted the nb/sonarjava-6208-assertions-in-tests branch April 15, 2026 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants