SONARJAVA-6482 Change number of issues raised for S5786 to one per class by asya-vorobeva · Pull Request #5685 · SonarSource/sonar-java

asya-vorobeva · 2026-06-22T10:21:12Z

Instead of reporting a separate issue on each noncompliant modifier, the rule
now reports a single issue on the class name listing all offending modifiers as
secondaries. The quick fix removes all of them in one click.

See SONARJAVA-6482.

hashicorp-vault-sonar-prod · 2026-06-22T10:21:27Z

SONARJAVA-6482

gitar-bot · 2026-06-22T10:47:27Z

+    String description = modifiers.size() == 1
+      ? String.format(MODIFIER_MESSAGE, modifiers.get(0).keyword().text())


💡 Quality: Inconsistent single-modifier quick-fix wording between S5786 and S5810

For the single-modifier removal quick fix, S5786 now uses the description String.format(MODIFIER_MESSAGE, ...) → "Remove this 'protected' modifier." (JUnit5DefaultPackageClassAndMethodCheck.java:116-117), while S5810 still uses JavaQuickFix.newQuickFix("Remove "%s" modifier", ...) → "Remove "protected" modifier" (JUnit5SilentlyIgnoreClassAndMethodCheck.java:107). These two closely-related rules now present differently-worded quick-fix labels for what is the same single-modifier removal action. This is purely cosmetic (it does not affect behavior), but aligning the wording would give users a consistent experience across the two test-modifier rules. Consider using the same description string (e.g. the MODIFIER_MESSAGE form) in both checks, or extracting a shared constant.

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

…dant visibility modifiers at once Instead of reporting a separate issue on each noncompliant modifier, the rule now reports a single issue on the class name listing all offending modifiers as secondaries. The quick fix removes all of them in one click. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

sonarqube-next · 2026-06-22T11:55:19Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
97.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

rombirli

I’m requesting changes because I don’t understand why these two rules diverged this much, or why the common abstraction was removed.

They still seem to share the same core traversal and JUnit 5 classification logic, so I would expect them to follow the same direction and keep the shared parent class.

Could you either keep the common abstraction, or explain why this level of divergence is necessary?

The other comments are personal preferences and optional.

rombirli · 2026-06-22T11:55:22Z

  <properties>
    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
    <maven.compiler.release>21</maven.compiler.release>
+    <slf4j.version>1.7.30</slf4j.version>


rombirli · 2026-06-22T11:59:10Z

-    // Handled by S5810
+  public void visitNode(Tree tree) {
+    ClassTree classTree = (ClassTree) tree;
+    if (classTree.symbol().isAbstract() || (classTree.simpleName() == null)) {


redundant parentheses

Suggested change

if (classTree.symbol().isAbstract() || (classTree.simpleName() == null)) {

if (classTree.symbol().isAbstract() || classTree.simpleName() == null) {

rombirli · 2026-06-22T12:27:10Z

A few things are now duplicated :
MODIFIER_MESSAGE, nodesToVisit, UnitTestUtils.groupJUnit5Methods and ClassPatternsUtils.shouldBePublicClass are only used in JUnit5DefaultPackageClassAndMethodCheck and JUnit5SilentlyIgnoreClassAndMethodCheck
IMO it makes sense to keep this superclass even if it only contains a few things.

I think both rules should follow the same direction here.

S5786 was moved to a bulk quick-fix model, while S5810 still keeps a separate per-issue/per-modifier behavior. In my view, that creates unnecessary divergence between two checks that still share the same core traversal and JUnit 5 classification logic.

I would prefer:

keeping AbstractJUnit5NotCompliantModifierChecker with the shared visit / grouping / class-handling logic;

aligning S5810 with S5786 on the reporting model, with a bulk quick fix for the noncompliant modifiers in the same test class.

The shared abstraction still looks valid to me, and keeping it would make future changes safer.

Was clarified in Slack discussion.

rombirli

Approved after discussion on why these two rules need to diverge for business reasons.

gitar-bot · 2026-06-23T11:25:15Z

Code Review 👍 Approved with suggestions 3 resolved / 4 findings

Refactors S5786 to report a single issue per class with a consolidated quick fix, addressing several issues including missing file newlines and test coverage. Consider aligning the quick-fix description with S5810 for consistent messaging.

💡 Quality: Inconsistent single-modifier quick-fix wording between S5786 and S5810

📄 java-checks/src/main/java/org/sonar/java/checks/tests/JUnit5DefaultPackageClassAndMethodCheck.java:116-117 📄 java-checks/src/main/java/org/sonar/java/checks/tests/JUnit5SilentlyIgnoreClassAndMethodCheck.java:107

For the single-modifier removal quick fix, S5786 now uses the description String.format(MODIFIER_MESSAGE, ...) → "Remove this 'protected' modifier." (JUnit5DefaultPackageClassAndMethodCheck.java:116-117), while S5810 still uses JavaQuickFix.newQuickFix("Remove "%s" modifier", ...) → "Remove "protected" modifier" (JUnit5SilentlyIgnoreClassAndMethodCheck.java:107). These two closely-related rules now present differently-worded quick-fix labels for what is the same single-modifier removal action. This is purely cosmetic (it does not affect behavior), but aligning the wording would give users a consistent experience across the two test-modifier rules. Consider using the same description string (e.g. the MODIFIER_MESSAGE form) in both checks, or extracting a shared constant.

✅ 3 resolved

✅ Quality: Missing newline at end of JUnit5SilentlyIgnoreClassAndMethodCheck.java

📄 java-checks/src/main/java/org/sonar/java/checks/tests/JUnit5SilentlyIgnoreClassAndMethodCheck.java:157
JUnit5SilentlyIgnoreClassAndMethodCheck.java no longer ends with a trailing newline (the diff shows \ No newline at end of file). This is a minor style/POSIX consistency issue and may trip linters or produce noisy diffs later. Add a trailing newline.

✅ Edge Case: No test for merged class+method modifier quick fix

📄 java-checks/src/main/java/org/sonar/java/checks/tests/JUnit5DefaultPackageClassAndMethodCheck.java:80-94
In JUnit5DefaultPackageClassAndMethodCheck, the branch where the class itself has a noncompliant visibility modifier AND there are noncompliant method modifiers (the class modifier is appended to noncompliantModifiers at line 80 and merged into a single class-level issue with secondaries + a multi-edit quick fix) is not exercised by JUnit5DefaultPackageClassAndMethodCheckSample. The top-level sample class is package-private (no class modifier), and the only class-modifier case (PublicWithOneTest) has no method violations and goes through the early-return single-modifier path. Consider adding a sample with e.g. public (or protected) class containing noncompliant @Test methods and no public-static members, to cover the combined quick fix (description "Remove all redundant visibility modifiers" with the class-modifier edit appended out of source order). This also validates that the appended class-modifier edit applies correctly together with the method edits.

✅ Quality: Developer-specific absolute path committed in command doc

📄 .claude/commands/quickfix-tests.md:5
The new file .claude/commands/quickfix-tests.md embeds a hardcoded, developer-specific absolute path: /Users/asya.vorobeva/Projects/sonar-analyzer-commons/test-commons. This path is meaningless to anyone else and looks like accidentally committed local/AI tooling notes. If this file is intended to be shared, replace the absolute path with a repo-relative reference (or a placeholder); otherwise consider not committing it at all.

🤖 Prompt for agents

Code Review: Refactors S5786 to report a single issue per class with a consolidated quick fix, addressing several issues including missing file newlines and test coverage. Consider aligning the quick-fix description with S5810 for consistent messaging.

1. 💡 Quality: Inconsistent single-modifier quick-fix wording between S5786 and S5810
   Files: java-checks/src/main/java/org/sonar/java/checks/tests/JUnit5DefaultPackageClassAndMethodCheck.java:116-117, java-checks/src/main/java/org/sonar/java/checks/tests/JUnit5SilentlyIgnoreClassAndMethodCheck.java:107

   For the single-modifier removal quick fix, S5786 now uses the description `String.format(MODIFIER_MESSAGE, ...)` → "Remove this 'protected' modifier." (JUnit5DefaultPackageClassAndMethodCheck.java:116-117), while S5810 still uses `JavaQuickFix.newQuickFix("Remove "%s" modifier", ...)` → "Remove "protected" modifier" (JUnit5SilentlyIgnoreClassAndMethodCheck.java:107). These two closely-related rules now present differently-worded quick-fix labels for what is the same single-modifier removal action. This is purely cosmetic (it does not affect behavior), but aligning the wording would give users a consistent experience across the two test-modifier rules. Consider using the same description string (e.g. the `MODIFIER_MESSAGE` form) in both checks, or extracting a shared constant.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

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

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

gitar-bot Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread .claude/commands/quickfix-tests.md

gitar-bot Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread ...hecks/src/main/java/org/sonar/java/checks/tests/JUnit5SilentlyIgnoreClassAndMethodCheck.java Outdated

gitar-bot Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread ...hecks/src/main/java/org/sonar/java/checks/tests/JUnit5DefaultPackageClassAndMethodCheck.java

asya-vorobeva force-pushed the asya/improve-s5786 branch from bbd6182 to 660de09 Compare June 22, 2026 10:41

gitar-bot Bot reviewed Jun 22, 2026

View reviewed changes

asya-vorobeva force-pushed the asya/improve-s5786 branch from 660de09 to f59eb2c Compare June 22, 2026 11:00

asya-vorobeva requested a review from francois-mora-sonarsource June 22, 2026 11:03

asya-vorobeva force-pushed the asya/improve-s5786 branch from f59eb2c to f05ed3d Compare June 22, 2026 11:47

rombirli requested changes Jun 22, 2026

View reviewed changes

rombirli approved these changes Jun 22, 2026

View reviewed changes

francois-mora-sonarsource approved these changes Jun 23, 2026

View reviewed changes

asya-vorobeva merged commit d08f917 into master Jun 23, 2026
23 checks passed

asya-vorobeva deleted the asya/improve-s5786 branch June 23, 2026 11:23

		String description = modifiers.size() == 1
		? String.format(MODIFIER_MESSAGE, modifiers.get(0).keyword().text())

	if (classTree.symbol().isAbstract() \|\| (classTree.simpleName() == null)) {
	if (classTree.symbol().isAbstract() \|\| classTree.simpleName() == null) {

Uh oh!

Conversation

asya-vorobeva commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gitar-bot Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqube-next Bot commented Jun 22, 2026

Quality Gate passed

Uh oh!

rombirli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rombirli Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rombirli Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rombirli Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

rombirli Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

asya-vorobeva Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

rombirli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gitar-bot Bot commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asya-vorobeva commented Jun 22, 2026 •

edited

Loading

hashicorp-vault-sonar-prod Bot commented Jun 22, 2026 •

edited by atlassian Bot

Loading