JS-2014: Implement S8980: react testing library calls should not be wrapped in act()#7480
JS-2014: Implement S8980: react testing library calls should not be wrapped in act()#7480nathsou wants to merge 9 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
…te-external-rules-docs
…e render import-resolution check
…sleading comment about query detection
# Conflicts: # packages/analysis/src/jsts/rules/README.md
…eta.url usage eslint-plugin-testing-library reads its own name/version off package.json via createRequire(import.meta.url)(...). esbuild's CJS output makes import.meta.url undefined, so createRequire(undefined) throws at bridge startup - the same class of bug already patched here for @babel/core, css-tree, and stylelint. Verified locally: reproduced the crash by building this branch's bridge bundle and running bin/server.cjs directly (TypeError [ERR_INVALID_ARG_VALUE]: Received undefined), then confirmed the patched bundle starts cleanly.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Upstream's non-strict mode treats a callback as "entirely Testing Library calls" whenever it finds no statement it can positively classify as non-Testing-Library. Its per-statement identifier extraction only understands a few shapes (a bare call, an awaited call, a return); an awaited `new Promise(...)` (the shape of a manual timer delay, e.g. a `sleep()` test helper) is invisible to that check, so a callback made entirely of such statements was flagged even though it contains no Testing Library call at all. Found via ruling on ant-design (tests/utils.ts), which wraps a raw `setTimeout`-backed Promise in `act()` from 'react-dom/test-utils' - a legitimate, necessary use of act() with nothing to do with Testing Library. Guard against this in the decorator by independently confirming the callback contains at least one call that resolves to a real Testing Library (or react-dom/test-utils) export before letting the report through. Also syncs the one genuine new ruling finding this rule introduces (searchkit, a true positive: act() wrapping fireEvent.change/screen both imported from @testing-library/react).
| function collectCallExpressions( | ||
| node: estree.Node, | ||
| results: estree.CallExpression[] = [], | ||
| ): estree.CallExpression[] { | ||
| if (node.type === 'CallExpression') { | ||
| results.push(node); | ||
| } | ||
| for (const key of Object.keys(node)) { | ||
| if (key === 'parent' || key === 'loc' || key === 'range') { | ||
| continue; | ||
| } | ||
| const value = (node as unknown as Record<string, unknown>)[key]; | ||
| if (Array.isArray(value)) { | ||
| for (const item of value) { | ||
| if (item && typeof (item as estree.Node).type === 'string') { |
There was a problem hiding this comment.
💡 Edge Case: collectCallExpressions descends into nested callbacks
actCallbackHasNoRecognizedTestingLibraryCall calls collectCallExpressions(callback.body), which recurses into every nested node, including inner function/arrow expressions that are not executed synchronously as part of the act() body (e.g. a call passed to setTimeout, Array.forEach, a Promise executor, or an event handler). If such a nested, non-synchronous callback happens to contain a recognized Testing Library call while the act body's top-level statements do not, the guard would see a TL call and decline to suppress the upstream report, potentially re-introducing the exact false-positive class this commit is meant to remove.
In practice this is narrow: the upstream non-strict rule only emits noUnnecessaryActTestingLibraryUtil when it cannot positively classify any statement as non-Testing-Library, so most such shapes won't reach this guard. Still, restricting the scan to the callback's own top-level/synchronously-reachable statements (rather than all descendants) would make the guard's notion of "contains a TL call" match upstream's per-statement model more precisely. Consider documenting or tightening the traversal scope.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 1 resolved / 2 findingsImplements S8980 to flag redundant act() wrappers around React Testing Library calls, addressing issues with import resolution, test fixture coverage, and bridge server stability. Ensure collectCallExpressions correctly handles nested callbacks to avoid potential false positives. 💡 Edge Case: collectCallExpressions descends into nested callbacks📄 packages/analysis/src/jsts/rules/S8980/decorator.ts:59-73 📄 packages/analysis/src/jsts/rules/S8980/decorator.ts:97-111
In practice this is narrow: the upstream non-strict rule only emits ✅ 1 resolved✅ Quality: textReplace silently no-ops if upstream expression changes
🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
2 participants
Summary
Implements S8980: flags
act(...)calls (from@testing-library/reactorreact-dom/test-utils) whose callback consists entirely of React Testing Library /user-eventcalls, since those utilities already wrap themselves inact()internally.eslint-plugin-testing-library'sno-unnecessary-actrule (implementation: 'decorated') rather than reimplementing Testing Library import/call resolution from scratch.isStrict: false(upstream default istrue) so only callbacks made entirely of Testing Library calls are flagged; mixed content stays compliant.context.settingslocally in the rule's owndecorator.ts(not the shared linter settings) to disable the upstream plugin's "aggressive" name-based heuristic detection, forcing precise import-resolution-only matching.noUnnecessaryActEmptyFunctionreport (act(() => {})) viainterceptReport, since that case is already covered by S1186.requiredDependency: ['@testing-library/react'].eslint-plugin-testing-library@7.16.2as a devDependency (consistent with the recenteslint-plugin-vueprecedent for S8950/JS-1952).Known upstream limitation
Found and worked around (not patched) a real ordering bug in
eslint-plugin-testing-library@7.16.2: importing@testing-library/user-eventbeforereact-dom/test-utilsin the same file causes the plugin's ownreact-dom/test-utils-importedactdetection to be silently skipped, due to a stray!importedUserEventLibraryNodeguard instead of!importedReactDomTestUtilsNode. This is an inherited false-negative risk in that narrow scenario, tracked as a follow-up rather than a blocker.Testing
Comment-based fixtures cover:
render/fireEvent/implicit-returnact(() => screen.getByRole(...))/userEvent/waitFornoncompliant cases,actimported fromreact-dom/test-utils(isolated fixture, see limitation above), plain RTL calls with noactwrapper (compliant),actwrapping only non-Testing-Library code (compliant — proves the rule won't mask genuine act warnings), mixed TL/non-TL content (compliant, verifiesisStrict: false), and emptyact(() => {})(not reported by this rule, verifies no overlap with S1186).Links