test(react): give the JSDOM harness one home - #3908
Conversation
Twelve UI test files each carried their own copy of the same harness: snapshot globals, install them off a JSDOM window, stub ResizeObserver and matchMedia, stub requestAnimationFrame, restore on teardown. Identical code, twelve times. That is how one defect ended up in all twelve. The rAF stub is a real timer, and nothing drained the frames still queued at teardown, so a frame React scheduled just before unmount became a pending timer -- which the op sanitizer reports as a leak, at suite level, with no step and no stack, on whatever branch happens to be pushing. Move the mechanics into one module and have each file call it. The helper owns snapshot, install, drain and restore. It deliberately does not decide which globals a test installs: callers opt in to matchMedia and to any extra window constructors they need, so every file keeps exactly the globals it had and this change alters no behaviour. Net 798 lines removed, and no file stubs requestAnimationFrame by hand any more. The export is named installComponentDom rather than installDomGlobals because fourteen other test files already define a local function by that name with a different signature. An import would have shadowed those declarations, and with --no-check on the test tasks the suite would still have run green.
|
Warning Review limit reached
Next review available in: 23 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
The repository's anti-slop ratchet rejected four `as unknown as` casts I carried over from the per-file harnesses. Baselining them would have made a shared module the place new copies come from, which is the opposite of the point. - matchMedia now returns an interface describing what the stub actually provides, instead of asserting an object literal into the full DOM type. - Globals are read and written through Reflect, so there is no assertion about the shape of globalThis. - Frame ids are plain numbers here, so the Set is typed accordingly and both casts disappear.
The shared JSDOM helper must install the same constructors each test previously exposed, and framework imports must remain in the repository-defined group. Constraint: Preserve every migrated test global exactly. Rejected: Remove the local harness wrappers | Their local options document per-component browser requirements. Confidence: high Scope-risk: narrow Directive: Keep per-test windowGlobals aligned with the globals replaced by this harness. Tested: Deno 2.7.7 fmt and check on all migrated files; 16 suites and 97 test steps passed. Not-tested: Full repository suite.
1 participant
Description
Twelve UI test files each carried their own copy of the same harness — snapshot globals, install them off a JSDOM window, stub
ResizeObserverandmatchMedia, stubrequestAnimationFrame, restore on teardown. Identical code, twelve times.That duplication is how one defect ended up in all twelve. The rAF stub is a real timer, and nothing drained the frames still queued at teardown, so a frame React scheduled just before unmount became a pending timer — reported by the op sanitizer as a leak, at suite level, with no step and no stack, on whatever branch happened to be pushing.
Fixing twelve copies (#3890) closed the instances. This closes the source.
Design
The helper owns the mechanics — snapshot, install, drain, restore — and deliberately does not decide which globals a test installs. Callers opt in:
That matters because the twelve were not uniform: eight shared a key set,
drawer.conformancedeliberately omitsmatchMedia, and three add keyboard/focus constructors. Installing a superset would have changed behaviour for components that branch on a global's absence. Every file keeps exactly the globals it had.Why the export is called
installComponentDomFourteen other test files already define a local
function installDomGlobalswith a different signature. I named the export that first, added the import broadly, and the suite ran 241 passed / 0 failed while those files had an import shadowing their own declaration — invisible because the test tasks run--no-check.Reverted and renamed. Worth recording: a green test run does not tell you a test file typechecks in this repo.
deno task lint:test-typecheckis the gate that does.Evidence
No production code changed.
Related Issue(s)
Tracked internally.
Type of Change
Checklist