JS-1971: Extend S8785 coverage

[guillemsarda]

---

Summary by Gitar

• Rule S8785 enhancements:
  • Extended detection to async helper functions that silently drop tests when called within test suites.
  • Added TypeScript-based resolution for callbacks passed by reference to identify async declarations.
• Refactored analysis logic:
  • Extracted analysis logic into async-analysis.ts and call-to-declaration.ts for improved maintainability.
  • Updated rule.ts to orchestrate detection of both top-level await and unawaited async helper calls.
• Rule metadata updates:
  • Renamed rule title to "Tests should not be registered asynchronously in suite callbacks" and updated quick-fix status.
  • Adjusted S5914 and S6582 rule metadata (titles, severity, and quick-fix indicators).

_{This will update automatically on new commits.}

[hashicorp-vault-sonar-prod]

JS-1971

[gitar-bot]

💡 Edge Case: Referenced async callback may be reported once per suite

getRaisingSuiteCallback now resolves a callback passed by reference (an identifier) to its function-like declaration and returns it as byReference. When the same async function is shared across multiple suite registrations (e.g. describe('a', register); describe('b', register);), each CallExpression visit resolves to the identical declaration node, and visitRaisingSuiteCallback will call reportRemoveAsync/reportUnawaitedAsyncHelperCalls on that same declaration for every referencing suite. Since the report helper does not deduplicate and ESLint core does not dedupe identical descriptors, this emits duplicate issues at the same location on the shared declaration. Consider deduplicating reports on referenced declarations (e.g. track already-reported declaration nodes in the rule's per-file state) so a shared helper is flagged only once. This is an edge case (multiple suites sharing one referenced callback), hence minor.

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

[gitar-bot]

💡 Quality: Unrelated S6582 severity change bundled in S8785 PR

This PR is titled "Extend S8785 coverage" yet also changes S6582.json, downgrading its impact from MAINTAINABILITY MEDIUM to LOW and defaultSeverity from Major to Minor. This behavioral metadata change for an unrelated rule is easy to miss during review and complicates rollback/bisection if it turns out to be unintended. If this is an intentional RSPEC metadata sync it is fine to keep, but it would be cleaner to split it into a separate commit/PR or note it explicitly in the description.

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

[github-actions]

Ruling Report

✅ No changes to ruling expected issues in this PR

[nathsou]

Reviewed the S8785 extension logic (async-analysis.ts, call-to-declaration.ts, rule.ts). Found a real false-negative gap, an overlapping-secondaries bug, a false-negative from a bare-name check, a reuse opportunity, and a minor efficiency note. Details inline.

[gitar-bot]

💡 Edge Case: Secondary-location overlap cap misses nested awaited helpers

findNextTopLevelAwaitAfter (rule.ts:433-449) only scans the direct statements of body.body for the next awaited expression, but reportUnawaitedAsyncHelperCalls descends into nested blocks (if/try/loops) to find awaited helper calls, and findTestsRegisteredAfter likewise descends into nested blocks when collecting dropped tests.

Consequently, if an awaited helper call lives inside a nested block and the following awaited helper is also nested (not a top-level statement of the describe body), findNextTopLevelAwaitAfter returns undefined, so the beforeNode cap becomes Infinity. The first helper's report then collects dropped tests all the way to the end of the callback, overlapping with the second helper's secondaries — the exact duplication this commit aims to prevent.

This is an uncommon shape (awaited helpers nested inside conditionals/loops), so severity is minor. If desired, findNextTopLevelAwaitAfter could walk the same node set (descending into non-function children) used by the other helpers so the cap is consistent.

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

[gitar-bot]

💡 Quality: Typo 'qexpression' in reportUnawaitedAsyncHelperCalls comment

The comment at rule.ts:363 reads "seed with the qexpression itself" — 'qexpression' should be 'expression'. Minor wording cleanup in a newly added comment.

Fix typo 'qexpression' -> 'expression'.:

// Concise-body arrow (`() => expr`): seed with the expression itself so it falls through the

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

[nathsou]

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

[gitar-bot]

Code Review 👍 Approved with suggestions 2 resolved / 6 findings

Extends S8785 coverage to async helper functions with improved analysis modularity and metadata updates. Consider isolating the unrelated S6582 severity change and addressing the potential reporting duplication of referenced async callbacks.

💡 Edge Case: Referenced async callback may be reported once per suite

📄 packages/analysis/src/jsts/rules/S8785/rule.ts:120-134 📄 packages/analysis/src/jsts/rules/S8785/rule.ts:242-256

getRaisingSuiteCallback now resolves a callback passed by reference (an identifier) to its function-like declaration and returns it as byReference. When the same async function is shared across multiple suite registrations (e.g. describe('a', register); describe('b', register);), each CallExpression visit resolves to the identical declaration node, and visitRaisingSuiteCallback will call reportRemoveAsync/reportUnawaitedAsyncHelperCalls on that same declaration for every referencing suite. Since the report helper does not deduplicate and ESLint core does not dedupe identical descriptors, this emits duplicate issues at the same location on the shared declaration. Consider deduplicating reports on referenced declarations (e.g. track already-reported declaration nodes in the rule's per-file state) so a shared helper is flagged only once. This is an edge case (multiple suites sharing one referenced callback), hence minor.

💡 Quality: Unrelated S6582 severity change bundled in S8785 PR

📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6582.json:5 📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6582.json:18

This PR is titled "Extend S8785 coverage" yet also changes S6582.json, downgrading its impact from MAINTAINABILITY MEDIUM to LOW and defaultSeverity from Major to Minor. This behavioral metadata change for an unrelated rule is easy to miss during review and complicates rollback/bisection if it turns out to be unintended. If this is an intentional RSPEC metadata sync it is fine to keep, but it would be cleaner to split it into a separate commit/PR or note it explicitly in the description.

💡 Edge Case: Secondary-location overlap cap misses nested awaited helpers

📄 packages/analysis/src/jsts/rules/S8785/rule.ts:433-447 📄 packages/analysis/src/jsts/rules/S8785/rule.ts:384-396 📄 packages/analysis/src/jsts/rules/S8785/rule.ts:517-525 📄 packages/analysis/src/jsts/rules/S8785/async-analysis.ts:118-132

findNextTopLevelAwaitAfter (rule.ts:433-449) only scans the direct statements of body.body for the next awaited expression, but reportUnawaitedAsyncHelperCalls descends into nested blocks (if/try/loops) to find awaited helper calls, and findTestsRegisteredAfter likewise descends into nested blocks when collecting dropped tests.

Consequently, if an awaited helper call lives inside a nested block and the following awaited helper is also nested (not a top-level statement of the describe body), findNextTopLevelAwaitAfter returns undefined, so the beforeNode cap becomes Infinity. The first helper's report then collects dropped tests all the way to the end of the callback, overlapping with the second helper's secondaries — the exact duplication this commit aims to prevent.

This is an uncommon shape (awaited helpers nested inside conditionals/loops), so severity is minor. If desired, findNextTopLevelAwaitAfter could walk the same node set (descending into non-function children) used by the other helpers so the cap is consistent.

💡 Quality: Typo 'qexpression' in reportUnawaitedAsyncHelperCalls comment

📄 packages/analysis/src/jsts/rules/S8785/rule.ts:362-364

The comment at rule.ts:363 reads "seed with the qexpression itself" — 'qexpression' should be 'expression'. Minor wording cleanup in a newly added comment.

Fix typo 'qexpression' -> 'expression'.

// Concise-body arrow (`() => expr`): seed with the expression itself so it falls through the

✅ 2 resolved

✅ Performance: Type checker resolves signature for every call statement in suite

📄 packages/analysis/src/jsts/rules/S8785/rule.ts:343-357 📄 packages/analysis/src/jsts/rules/S8785/rule.ts:391-401 📄 packages/analysis/src/jsts/rules/S8785/call-to-declaration.ts:49-58
reportUnawaitedAsyncHelperCalls walks every descendant of the suite callback body (descending into nested blocks) and, for each ExpressionStatement whose expression is a CallExpression (or AwaitExpression → CallExpression), unconditionally calls reportAsyncHelperCall, which immediately invokes followCallToDeclaration → checker.getResolvedSignature(...).

This means that for a typical typed test file, the TypeScript type checker is asked to resolve a signature for every call statement in every suite body — including the very common it(...), beforeEach(...), expect(...).toBe(...), etc. A describe block with hundreds of it/assertion statements produces hundreds of getResolvedSignature calls per suite, and this runs on every CallExpression matched by the rule. getResolvedSignature is a relatively expensive checker operation, so this can be a noticeable slowdown on large typed test files where previously the rule did only lightweight AST work.

Most of these calls are registration/assertion calls that can be cheaply rejected before touching the type checker. Filtering out calls whose callee is a known test/suite/lifecycle name (or otherwise obviously not a local helper) via getMochaCalleeParts before calling followCallToDeclaration would eliminate the bulk of the resolutions.

✅ Edge Case: reportUnawaitedAsyncHelperCalls reads callback.body.type without null guard

📄 packages/analysis/src/jsts/rules/S8785/rule.ts:349 📄 packages/analysis/src/jsts/rules/S8785/async-analysis.ts:53-59 📄 packages/analysis/src/jsts/rules/S8785/async-analysis.ts:99-107
reportUnawaitedAsyncHelperCalls accesses callback.body.type directly, whereas the sibling helpers findFirstTopLevelAwait (guards callback.body === null) and findTestsRegisteredAfter (uses callback.body?.type) defensively handle a null body. callback is a FunctionLike that, for the by-reference path, is produced from a resolved declaration (tsNodeToESTreeNodeMap.get(decl)), so its body type is not fully controlled by this rule.

In practice the current isFunctionLike guard filters TSDeclareFunction (bodyless declarations) out, so this is likely not reachable today, but the inconsistency is a latent crash risk (TypeError: Cannot read properties of null) if a bodyless function-like node ever reaches this function. Aligning the guard with the other two helpers (callback.body?.type !== 'BlockStatement') removes the divergence at no cost.

🤖 Prompt for agents

Code Review: Extends S8785 coverage to async helper functions with improved analysis modularity and metadata updates. Consider isolating the unrelated S6582 severity change and addressing the potential reporting duplication of referenced async callbacks.

1. 💡 Edge Case: Referenced async callback may be reported once per suite
   Files: packages/analysis/src/jsts/rules/S8785/rule.ts:120-134, packages/analysis/src/jsts/rules/S8785/rule.ts:242-256

   `getRaisingSuiteCallback` now resolves a callback passed by reference (an identifier) to its function-like declaration and returns it as `byReference`. When the *same* async function is shared across multiple suite registrations (e.g. `describe('a', register); describe('b', register);`), each `CallExpression` visit resolves to the identical declaration node, and `visitRaisingSuiteCallback` will call `reportRemoveAsync`/`reportUnawaitedAsyncHelperCalls` on that same declaration for every referencing suite. Since the `report` helper does not deduplicate and ESLint core does not dedupe identical descriptors, this emits duplicate issues at the same location on the shared declaration. Consider deduplicating reports on referenced declarations (e.g. track already-reported declaration nodes in the rule's per-file state) so a shared helper is flagged only once. This is an edge case (multiple suites sharing one referenced callback), hence minor.

2. 💡 Quality: Unrelated S6582 severity change bundled in S8785 PR
   Files: sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6582.json:5, sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S6582.json:18

   This PR is titled "Extend S8785 coverage" yet also changes S6582.json, downgrading its impact from MAINTAINABILITY MEDIUM to LOW and defaultSeverity from Major to Minor. This behavioral metadata change for an unrelated rule is easy to miss during review and complicates rollback/bisection if it turns out to be unintended. If this is an intentional RSPEC metadata sync it is fine to keep, but it would be cleaner to split it into a separate commit/PR or note it explicitly in the description.

3. 💡 Edge Case: Secondary-location overlap cap misses nested awaited helpers
   Files: packages/analysis/src/jsts/rules/S8785/rule.ts:433-447, packages/analysis/src/jsts/rules/S8785/rule.ts:384-396, packages/analysis/src/jsts/rules/S8785/rule.ts:517-525, packages/analysis/src/jsts/rules/S8785/async-analysis.ts:118-132

   `findNextTopLevelAwaitAfter` (rule.ts:433-449) only scans the direct statements of `body.body` for the next awaited expression, but `reportUnawaitedAsyncHelperCalls` descends into nested blocks (`if`/`try`/loops) to find awaited helper calls, and `findTestsRegisteredAfter` likewise descends into nested blocks when collecting dropped tests.
   
   Consequently, if an awaited helper call lives inside a nested block and the following awaited helper is also nested (not a top-level statement of the describe body), `findNextTopLevelAwaitAfter` returns `undefined`, so the `beforeNode` cap becomes `Infinity`. The first helper's report then collects dropped tests all the way to the end of the callback, overlapping with the second helper's secondaries — the exact duplication this commit aims to prevent.
   
   This is an uncommon shape (awaited helpers nested inside conditionals/loops), so severity is minor. If desired, `findNextTopLevelAwaitAfter` could walk the same node set (descending into non-function children) used by the other helpers so the cap is consistent.

4. 💡 Quality: Typo 'qexpression' in reportUnawaitedAsyncHelperCalls comment
   Files: packages/analysis/src/jsts/rules/S8785/rule.ts:362-364

   The comment at rule.ts:363 reads "seed with the qexpression itself" — 'qexpression' should be 'expression'. Minor wording cleanup in a newly added comment.

   Fix (Fix typo 'qexpression' -> 'expression'.):
   // Concise-body arrow (`() => expr`): seed with the expression itself so it falls through the

Options

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

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}