RC-350 Unify ruling expectation file locations. by romainbrenguier · Pull Request #7922 · SonarSource/SonarJS

romainbrenguier · 2026-09-09T12:03:09Z

Summary

Move ruling expectation files from its/ruling/src/test/expected/<project>/<language>-<ruleId>.json to its/ruling/src/test/resources/expected/<project>/<ruleId>.json
Update Java ruling test (LITS-based) to merge per-language expectation dirs into a flat directory at build time
Update JS/TS ruling pipeline (lits.ts, testProject.ts) to write and compare results using the new layout
Update CI scripts (sync-results.mjs, generate-report.mjs, build.yml) to use the new paths

Motivation

Normalize ruling expectation file locations so that it is consistent across all analyzers.

Test plan

JS/TS ruling tests pass (npm run ruling)
Java ruling tests pass (mvn test -Dtest=RulingTest)
lits.test.ts unit tests pass

🤖 Generated with Claude Code

Agent workflow

Tool link: Tool link: https://github.com/SonarSource/languages-experimental-tooling/tree/romain/my-tickets/personal/romain-brenguier

Iterated on the PR with uv run ci_loop.py for 3 iterations.
✔️ The PR is now ready for review.

hashicorp-vault-sonar-prod · 2026-09-09T12:03:21Z

RC-350

github-actions · 2026-09-09T12:09:10Z

Ruling Report

New issues flagged (624124 issues)

CommentRegexTest

ace/demo/kitchen-sink/doclist.js:126

   124 |         return callback(doc.session);
   125 | 
>  126 |     // TODO: show load screen while waiting
   127 |     var path = doc.path;
   128 |     var parts = path.split("/");

ace/lib/ace/mode/css/csslint.js:1605

  1603 |                 tokenStream.mustMatch(Tokens.IDENT);
  1604 | 
> 1605 |                 // TODO: CSS3 Paged Media says only "left", "center", and "right" are allowed
  1606 | 
  1607 |                 return tokenStream.token().value;

ace/lib/ace/mode/css/csslint.js:4869

  4867 | 
  4868 |             // make sure this token is wanted
> 4869 |             // TODO: check channel
  4870 |             break;
  4871 |         }

ace/lib/ace/mode/css/csslint.js:5797

  5795 |     { name: "CHAR" },
  5796 | 
> 5797 |     // TODO: Needed?
  5798 |     // Not defined as tokens, but might as well be
  5799 |     {

ace/lib/ace/mode/css/csslint.js:7308

  7306 |             tt;
  7307 |         if (index > 0) {
> 7308 |             // TODO: Store 5 somewhere
  7309 |             if (index > 5) {
  7310 |                 throw new Error("Too much lookahead.");

ace/lib/ace/mode/html/saxparser.js:2462

  2460 | 	this.insertionModeName = "";
  2461 | 	this.originalInsertionMode = "";
> 2462 | 	this.inQuirksMode = false; // TODO quirks mode
  2463 | 	this.compatMode = "no quirks";
  2464 | 	this.framesetOk = true;

ace/lib/ace/mode/html/saxparser.js:2980

  2978 | 	modes.inHead.startTagMeta = function(name, attributes) {
  2979 | 		tree.insertSelfClosingElement(name, attributes);
> 2980 | 		// @todo process charset attributes
  2981 | 	};
  2982 |

ace/lib/ace/mode/html/saxparser.js:3273

  3271 | 		var characters = buffer.takeRemaining();
  3272 | 		characters = characters.replace(/\u0000/g, function(match, index){
> 3273 | 			// @todo position
  3274 | 			tree.parseError("invalid-codepoint");
  3275 | 			return '';

ace/lib/ace/mode/html/saxparser.js:3354

  3352 | 
  3353 | 	modes.inBody.startTagListItem = function(name, attributes) {
> 3354 | 		/// @todo: Fix according to current spec. http://www.w3.org/TR/html5/tree-construction.html#parsing-main-inbody
  3355 | 		var stopNames = {li: ['li'], dd: ['dd', 'dt'], dt: ['dd', 'dt']};
  3356 | 		var stopName = stopNames[name];

ace/lib/ace/mode/html/saxparser.js:3366

  3364 | 			}
  3365 | 
> 3366 | 			// todo isScoping()
  3367 | 			if (node.isSpecial() && node.localName !== 'p' && node.localName !== 'address' && node.localName !== 'div')
  3368 | 				break;

...and 624114 more

📋 View full report

New issues flagged (624124)

CommentRegexTest

Move ruling expectation files from its/ruling/src/test/expected/<project>/<language>-<ruleId>.json to its/ruling/src/test/resources/expected/<language>/<project>/<ruleId>.json This makes the layout consistent across analyzers. The Java ruling test (LITS-based) reconstructs the flat per-project directory that LITS expects at build time under target/expected/. The JS/TS ruling pipeline (lits.ts) and the CI ruling bot scripts are updated to read and write the new structure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ge>-<rule>.json Move expectation files from the language-first hierarchy (expected/<language>/<project>/<rule>.json) to a flat per-project structure (expected/<project>/<language>-<rule>.json). Simplify RulingTest.java by removing the mergeExpectedDir step, and testProject.ts by replacing compareByLanguage with a direct directory comparison. Replace syncTransposed in sync-results.mjs with a simple mirrorDirectory that copies the flat layout as-is. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The mergeExpectedDir step was removed, so target/expected no longer exists. The diff command should compare against target/actual instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The earlier commit flattened expected ruling files to <project>/<language>-<rule>.json but several components still used the old nested <project>/<language>/<rule>.json layout: - lits.ts writeIssues: remove per-language subdirectory, write flat filenames - lits.test.ts: update assertions to read from flat paths - generate-report.mjs: parse 2-segment paths (<project>/<lang>-<rule>.json) instead of 3-segment (<language>/<project>/<rule>.json) - SKILL.md: update expected path documentation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

✅ All code review findings resolved.

_{Configure merge blocking}

guillemsarda

Approving with 2 minor concerns.

guillemsarda · 2026-09-21T15:19:20Z

@@ -32,7 +32,7 @@ sh tools/ruling-debug-script.sh
 Results:


For file packages/ruling/lits.ts:

Could we keep creating actualPath unconditionally in writeResults()? With mkdir inside writeIssues(), a project that produces zero issues (including parsing errors) leaves no actual-results directory, so dir-compare throws ENOENT instead of comparing the results. This doesn't affect the current projects, but could affect a future zero-issue project even if its expected directory exists. Keeping the directory creation here and adding an empty-results test would cover that edge case.

guillemsarda · 2026-09-21T15:21:20Z

@@ -44,13 +44,13 @@ mvn verify -Dtest=RulingTest -Dmaven.test.redirectTestOutputToFile=false
 Copy actual to expected:


Could we keep target/actual/ as the source in the copy command and as the comparison directory in the diff command? RulingTest.java (lines 280-282) still sets sonar.lits.dump.new to target/actual/<project>, so changing these instructions to target/expected makes them point to the wrong output directory. Only the baseline path needs to change to src/test/resources/expected/.

That's not changed by this PR, it was already packages/ruling/actual before, and it is still what the github action use to update the ruling expectations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Prompt: "address review comment: keep creating actualPath unconditionally in writeResults() so a zero-issue project doesn't cause dir-compare ENOENT, and add a test for that edge case" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gitar-bot · 2026-09-22T12:59:57Z

Code Review ✅ Approved 10 closed / 10 findings

🟡 Medium risk · Ruling infrastructure changes paths, report parsing, and CI synchronization behavior.

Refactors ruling expectation file locations to a unified flat layout across JS/TS and Java analyzers. Resolved 10 issues including path handling in comparison logic, documentation updates, build-time directory merging, CI script synchronization, and report parsing to align with the new expected/<project>/<ruleId>.json structure. No issues remain.

✅ 10 closed

✅ Bug: compareByLanguage crashes with ENOENT when one side is missing

📄 packages/ruling/testProject.ts:104-118 📄 packages/ruling/lits.ts:52-54 📄 packages/ruling/lits.ts:153-158
dir-compare@5.0.0's compare() starts with fs.promises.realpath() on both paths (node_modules/dir-compare/build/src/index.js, compare), so it rejects with ENOENT ... realpath as soon as one of the two directories does not exist. compareByLanguage only skips a language when both sides are missing, and writeIssues creates actual/<project>/<language>/ only when that language produced at least one issue (the unconditional fs.mkdir(actualPath) was also removed from writeResults). Concrete trigger: Ghost has expected/javascript/Ghost but no expected/typescript/Ghost; the moment the analyzer raises a single TS/CSS issue there, actual/Ghost/typescript exists while the expected dir does not and npm run ruling dies with a raw ENOENT instead of reporting the new issues — and symmetrically, if all CSS issues for tailwindcss disappear, actual/tailwindcss/css is never created and the existing expected/css/tailwindcss comparison throws. Compare against an empty placeholder directory instead of throwing so the whole side is reported as left-only/right-only entries.

✅ Bug: ruling-debug-script.sh diffs mismatched layouts, prints nothing

📄 tools/ruling-debug-script.sh:5-19 📄 .claude/skills/ruling/SKILL.md:30-35
The script only had its expected_dir path updated, but the two trees no longer have comparable shapes: expectations are now expected/<language>/<project>/<rule>.json while packages/ruling/actual is <project>/<language>/<rule>.json. diff -rq therefore emits only top-level Only in ...: javascript / Only in ...: Ghost lines — no differ lines at all — and the second loop's [ -f "$full_path" ] guard rejects those directory entries, so the script silently reports nothing even when ruling has real differences. Transpose the actual tree (or walk language/project pairs) before diffing.

✅ Bug: Docs tell you to copy target/expected back over the expectations

📄 .claude/skills/ruling/SKILL.md:44-54 📄 docs/DEV.md:192-197 📄 its/ruling/src/test/java/org/sonar/javascript/it/RulingTest.java:275 📄 its/ruling/src/test/java/org/sonar/javascript/it/RulingTest.java:285-289 📄 its/ruling/src/test/java/org/sonar/javascript/it/RulingTest.java:303-317
RulingTest writes fresh LITS results to target/actual/<project>/<language>-<rule>.json and uses target/expected/<project>/ only as a flat input copy of the committed expectations. So cp -R target/expected/ src/test/resources/expected/ copies the baseline back in the old flat shape (creating expected/<project>/javascript-S100.json next to the new expected/<language>/<project>/S100.json tree) and never picks up the new results, while diff -rq src/test/resources/expected target/expected compares the expectations against a rearranged copy of themselves — it can never show a real ruling change. DEV.md's surviving instruction to copy its/ruling/target/actual/ into its/ruling/src/test/resources/expected/ is wrong for the same reason (flat <language>-<rule>.json names, project-first nesting). Document the actual transposing step (or add a helper/ruling-sync-style script for the Java flow).

✅ Bug: mergeExpectedDir keeps stale files from previous runs

📄 its/ruling/src/test/java/org/sonar/javascript/it/RulingTest.java:307-321
mergeExpectedDir only calls Files.createDirectories(targetDir) and copies with REPLACE_EXISTING, so any file left in target/expected/<project>/ from an earlier run survives. On a repeated local run after an expectation file was deleted or renamed (exactly the workflow of updating ruling), the stale <language>-<rule>.json is still handed to LITS as sonar.lits.dump.old and reported as a missing-issue difference, failing the test for a file that is no longer in the repository. Clear the directory before copying.

✅ Quality: Stray consecutive blank lines will fail check-format

📄 its/ruling/src/test/java/org/sonar/javascript/it/RulingTest.java:294-298 📄 package.json:6-7
Removing mergeExpectedDir left three consecutive blank lines between runRulingTest and instantiateTemplateRule. The repo formats Java with prettier-plugin-java and gates on npm run check-format (prettier --list-different .), which collapses repeated blank lines, so this file will be reported as different and the format check fails. Also, void ruling(...) throws Exception no longer needs the throws now that the merge step (which threw IOException) is gone.

...and 5 more closed from earlier reviews

Review coverage

🧪 Functional validation No results

📋 Rules No rules evaluated

🤖 Auto-approval Not enabled · Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

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

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

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

sonarqube-next · 2026-09-22T13:12:39Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

romainbrenguier requested a review from a team September 9, 2026 12:03

This comment has been minimized.

Sign in to view

gitar-bot Bot reviewed Sep 9, 2026

View reviewed changes

Comment thread .claude/skills/ruling/SKILL.md

gitar-bot Bot reviewed Sep 9, 2026

View reviewed changes

Comment thread its/ruling/src/test/java/org/sonar/javascript/it/RulingTest.java Outdated

gitar-bot Bot reviewed Sep 16, 2026

View reviewed changes

Comment thread .github/actions/ruling_bot/sync-results.mjs Outdated

gitar-bot Bot reviewed Sep 16, 2026

View reviewed changes

Comment thread docs/DEV.md Outdated

gitar-bot Bot reviewed Sep 16, 2026

View reviewed changes

Comment thread its/ruling/src/test/java/org/sonar/javascript/it/RulingTest.java

This comment was marked as resolved.

Sign in to view

romainbrenguier and others added 2 commits September 21, 2026 14:54

romainbrenguier force-pushed the romain/normalize-ruling-expectations branch from 842d4d7 to f0097bb Compare September 21, 2026 13:00

gitar-bot Bot reviewed Sep 21, 2026

View reviewed changes

Comment thread docs/DEV.md Outdated

romainbrenguier and others added 2 commits September 21, 2026 15:09

RC-350 Fix diff command in DEV.md to compare against target/actual

2957e6c

The mergeExpectedDir step was removed, so target/expected no longer exists. The diff command should compare against target/actual instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

romainbrenguier marked this pull request as ready for review September 21, 2026 13:51

romainbrenguier requested a review from guillemsarda September 21, 2026 14:03

guillemsarda approved these changes Sep 21, 2026

View reviewed changes

romainbrenguier and others added 2 commits September 22, 2026 14:29

RC-350 Fix misnamed ruling expectation files for console project

f226f0f

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

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

Update ruling results for PR #7922 #8016

Closed

Fix console ruling expectation filenames

29e8037

romainbrenguier merged commit 9ce6b79 into master Sep 22, 2026
42 checks passed

romainbrenguier deleted the romain/normalize-ruling-expectations branch September 22, 2026 13:49

Navigation Menu

RC-350 Unify ruling expectation file locations. - #7922

RC-350 Unify ruling expectation file locations.#7922
romainbrenguier merged 7 commits into
masterfrom
romain/normalize-ruling-expectations

romainbrenguier commented Sep 9, 2026 •

edited

Loading

Uh oh!

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

edited by atlassian Bot

Loading

Uh oh!

This comment has been minimized.

github-actions Bot commented Sep 9, 2026 •

edited

Loading

New issues flagged (624124)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

guillemsarda left a comment

Uh oh!

guillemsarda Sep 21, 2026

Uh oh!

guillemsarda Sep 21, 2026

Uh oh!

romainbrenguier Sep 22, 2026

Uh oh!

gitar-bot Bot commented Sep 22, 2026

Uh oh!

sonarqube-next Bot commented Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -44,13 +44,13 @@ mvn verify -Dtest=RulingTest -Dmaven.test.redirectTestOutputToFile=false
		Copy actual to expected:

Conversation

romainbrenguier commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Test plan

Agent workflow

Uh oh!

hashicorp-vault-sonar-prod Bot commented Sep 9, 2026 • edited by atlassian Bot 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 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ruling Report

New issues flagged (624124 issues)

CommentRegexTest

New issues flagged (624124)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

guillemsarda left a comment

Choose a reason for hiding this comment

Uh oh!

guillemsarda Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

guillemsarda Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

romainbrenguier Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

gitar-bot Bot commented Sep 22, 2026

Uh oh!

sonarqube-next Bot commented Sep 22, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

romainbrenguier commented Sep 9, 2026 •

edited

Loading

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

edited by atlassian Bot

Loading

github-actions Bot commented Sep 9, 2026 •

edited

Loading