Skip to content

JS-1736 Clarify /tests skill guidance for false-positive fixes#7013

Draft
francois-mora-sonarsource wants to merge 3 commits into
masterfrom
codex/sync-js-tests-skill-guidance
Draft

JS-1736 Clarify /tests skill guidance for false-positive fixes#7013
francois-mora-sonarsource wants to merge 3 commits into
masterfrom
codex/sync-js-tests-skill-guidance

Conversation

@francois-mora-sonarsource
Copy link
Copy Markdown
Contributor

Summary

This updates the /tests skill for SonarJS to better reflect how false-positive fixes are represented in rule tests.

  • add guidance for converting existing FP reproducers from invalid to failing valid cases
  • clarify that autofix expectations such as output and suggestions should be removed during that conversion while keeping setup fields like filename and options when needed
  • add decorator upstream sentinel guidance and explicit guidance to move suppressed cases from invalid to valid instead of deleting them

Why

The SonarJS skill and the mirrored Nigel guidance had drifted.
SonarJS already had the richer explanation for errors objects used in quickfix assertions, while Nigel had accumulated useful guidance for FP reproducers and decorator sentinels.
This change merges those pieces so the SonarJS-side skill is the more complete reference again.

Validation

  • git diff --check
  • no automated SonarJS test currently validates .claude/skills/tests/SKILL.md

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod Bot changed the title [codex] Clarify /tests skill guidance for false-positive fixes JS-1736 [codex] Clarify /tests skill guidance for false-positive fixes May 11, 2026
@hashicorp-vault-sonar-prod
Copy link
Copy Markdown

hashicorp-vault-sonar-prod Bot commented May 11, 2026

JS-1736

@francois-mora-sonarsource francois-mora-sonarsource changed the title JS-1736 [codex] Clarify /tests skill guidance for false-positive fixes JS-1736 Clarify /tests skill guidance for false-positive fixes May 11, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 11, 2026

Ruling Report

Code no longer flagged (73 issues)

S6606

Joust/ts/Entity.ts:140

   138 | 			return this;
   139 | 		}
>  140 | 		if (value === null) {
   141 | 			value = 0;
   142 | 		}

Joust/ts/state/GameStateTracker.ts:33

    31 | 		let oldState = this.gameState;
    32 | 		this.gameState = this.plugins.reduce((state: GameState, plugin: GameStateTrackerPlugin): GameState => {
>   33 | 			return plugin.onBeforeMutate(mutator, state) || state;
    34 | 		}, oldState);
    35 | 		this.gameState = this.gameState.apply(mutator);

Joust/ts/state/GameStateTracker.ts:38

    36 | 		this.push(this.gameState);
    37 | 		this.gameState = this.plugins.reduce((state: GameState, plugin: GameStateTrackerPlugin): GameState => {
>   38 | 			return plugin.onAfterMutate(mutator, this.gameState) || state;
    39 | 		}, this.gameState);
    40 | 		this.push(this.gameState);

S7755

Joust/ts/components/EventLog.tsx:96

    94 | 		}
    95 | 		if (input.length) {
>   96 | 			output.push(input[input.length - 1]);
    97 | 		}
    98 | 		return output;

S7764

Joust/ts/components/GameWidget.tsx:75

    73 | 			this.setState({fullscreenError: true});
    74 | 			this.clearFullscreenErrorTimeout();
>   75 | 			this.fullscreenErrorTimeout = window.setTimeout(() => {
    76 | 				this.setState({fullscreenError: false});
    77 | 			}, 3000);

Joust/ts/components/GameWidget.tsx:102

   100 | 	private clearFullscreenErrorTimeout() {
   101 | 		if (this.fullscreenErrorTimeout) {
>  102 | 			window.clearTimeout(this.fullscreenErrorTimeout);
   103 | 			this.fullscreenErrorTimeout = null;
   104 | 		}

Joust/ts/components/GameWidget.tsx:187

   185 | 		try {
   186 | 			let event = document.createEvent('UIEvents');
>  187 | 			event.initUIEvent('resize', true, false, window, 0);
   188 | 			window.dispatchEvent(event);
   189 | 		} catch (e) {

Joust/ts/components/GameWidget.tsx:188

   186 | 			let event = document.createEvent('UIEvents');
   187 | 			event.initUIEvent('resize', true, false, window, 0);
>  188 | 			window.dispatchEvent(event);
   189 | 		} catch (e) {
   190 | 		}

Joust/ts/components/game/visuals/CardArt.tsx:40

    38 | 		window.removeEventListener("resize", this.onResize);
    39 | 		if (this.request) {
>   40 | 			window.cancelAnimationFrame(this.request);
    41 | 		}
    42 | 	}

Joust/ts/components/game/visuals/CardArt.tsx:126

   124 | 				return;
   125 | 			}
>  126 | 			this.request = window.requestAnimationFrame(() => {
   127 | 				this.request = null;
   128 | 				this.setState({height: 0});

...and 63 more

New issues flagged (150 issues)

S7728

TypeScript/Jakefile.js:1133

  1131 | desc("Compiles tslint rules to js");
  1132 | task("build-rules", ["build-rules-start"].concat(tslintRulesOutFiles).concat(["build-rules-end"]));
> 1133 | tslintRulesFiles.forEach(function (ruleFile, i) {
  1134 |     compileFile(tslintRulesOutFiles[i], [ruleFile], [ruleFile], [], /*useBuiltCompiler*/ false,
  1135 |         { noOutFile: true, generateDeclarations: false, outDir: path.join(builtLocalDirectory, "tslint"), lib: "es6" });

TypeScript/scripts/link-hooks.js:8

     6 | ];
     7 | 
>    8 | hooks.forEach(function (hook) {
     9 |     var hookInSourceControl = path.resolve(__dirname, "hooks", hook);
    10 | 

TypeScript/scripts/authors.ts:62

    60 |     const authorsByName: AuthorMap = {};
    61 |     const authorsByEmail: AuthorMap = {};
>   62 |     knownAuthors.forEach(author => {
    63 |         author.displayNames.forEach(n => authorsByName[n] = author);
    64 |         author.emails.forEach(e => authorsByEmail[e.toLocaleLowerCase()] = author);

TypeScript/scripts/authors.ts:63

    61 |     const authorsByEmail: AuthorMap = {};
    62 |     knownAuthors.forEach(author => {
>   63 |         author.displayNames.forEach(n => authorsByName[n] = author);
    64 |         author.emails.forEach(e => authorsByEmail[e.toLocaleLowerCase()] = author);
    65 |     });

TypeScript/scripts/authors.ts:64

    62 |     knownAuthors.forEach(author => {
    63 |         author.displayNames.forEach(n => authorsByName[n] = author);
>   64 |         author.emails.forEach(e => authorsByEmail[e.toLocaleLowerCase()] = author);
    65 |     });
    66 |     return {

TypeScript/scripts/authors.ts:108

   106 | 
   107 |     export const listKnownAuthors: Command = function () {
>  108 |         deduplicate(getKnownAuthors().map(getAuthorName)).filter(a => !!a).sort(sortAuthors).forEach(log);
   109 |     };
   110 |     listKnownAuthors.description = "List known authors as listed in .mailmap file.";

TypeScript/scripts/authors.ts:155

   153 |                     console.log("Found known authors: ");
   154 |                     console.log("=====================");
>  155 |                     deduplicate(knownAuthors).sort(sortAuthors).forEach(log);
   156 |                 }
   157 | 

TypeScript/scripts/authors.ts:162

   160 |                     console.log("Found unknown authors: ");
   161 |                     console.log("=====================");
>  162 |                     deduplicate(unknownAuthors).sort(sortAuthors).forEach(log);
   163 |                 }
   164 |             }

TypeScript/scripts/authors.ts:174

   172 |     console.log('Usage: node authors.js [command]');
   173 |     console.log('List of commands: ');
>  174 |     Object.keys(Commands).forEach(k => console.log(`     ${k}: ${(Commands as any)[k]['description']}`));
   175 | } else {
   176 |     var cmd: Function = (Commands as any)[args[0]];

TypeScript/scripts/createBenchmark.ts:30

    28 | // .ts sources for the compiler, used as a test input
    29 | var rawCompilerSources = "";
>   30 | sourceFiles.forEach(f=> {
    31 |     rawCompilerSources += "\r\n" + fs.readFileSync(path.join(tsSourceDir, f)).toString();
    32 | });

...and 140 more

📋 View full report

Code no longer flagged (73)

S6606

S7755

S7764

S7765

S7728

S7781

S6653

New issues flagged (150)

S7728

S7755

S7764

S7781

S7751

S6653

@francois-mora-sonarsource francois-mora-sonarsource marked this pull request as ready for review May 11, 2026 12:47
@sonar-review-alpha
Copy link
Copy Markdown
Contributor

sonar-review-alpha Bot commented May 11, 2026

Summary

This PR adds three substantial new sections to the /tests skill guidance document (.claude/skills/tests/SKILL.md):

  1. Converting FP Reproducers to Failing Tests — clarifies that when fixing false positives, test entries move from invalid to valid, with autofix fields (output, suggestions) removed but setup fields (filename, options) retained

  2. Upstream Sentinel Tests — comprehensive guidance for decorator rules: how to identify suppressed FP patterns, import and reference the upstream ESLint rule, structure the sentinel block, and when to include autofix expectations

  3. Preventing Silent Test Deletions — emphasizes moving invalid cases to valid instead of deleting them, preserving visibility and regression detection

These sections align SonarJS guidance with companion documentation that had accumulated richer detail on FP handling and decorator patterns. The changes codify practices already in use on the team and help prevent common mistakes like losing coverage of now-valid code.

What reviewers should know

What to look for: Pure documentation updates to an internal skill file — focus on clarity, accuracy of the patterns, and completeness of the guidance rather than code logic.

Key points for reviewers:

  • All three sections are additions, no existing guidance removed or modified
  • The patterns described reflect real workflows: FP fixes do move tests from invalidvalid, decorator rules do need sentinel tests, and moving (not deleting) test cases prevents regressions
  • Upstream sentinel section includes important detail: always use the external/ import shims, never direct imports from eslint packages
  • No automated tests validate .claude/skills/tests/SKILL.md yet, so validation is manual review
  • Author confirmed whitespace/line-ending integrity with git diff --check

Where to start: Read the three new sections in order (they appear consecutively at the end of the file) and verify each pattern matches SonarJS conventions you're familiar with. Pay special attention to the sentinel imports guidance — those constraints matter for test isolation.


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

sonar-review-alpha[bot]

This comment was marked as resolved.

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 resolved.

Co-authored-by: sonar-review-alpha[bot] <266116024+sonar-review-alpha[bot]@users.noreply.github.com>
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.

LGTM! ✅

🗣️ Give feedback

@sonarqube-next
Copy link
Copy Markdown

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@francois-mora-sonarsource francois-mora-sonarsource requested a review from a team May 11, 2026 16:00
@francois-mora-sonarsource francois-mora-sonarsource marked this pull request as draft May 28, 2026 08:34
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.

1 participant