test(harness): mount hand-written wrappers into the document too, and unmount them - #516
Merged
Merged
Conversation
… unmount them #512 attached the cases `renderEach` builds. The 293 hand-written `mountSuspended` calls across 112 files stayed detached, which is what kept the dialog family in `console-gate.ts`'s register with nothing left to fix in it. None of the 293 call sites is edited. Each project sets the default in the one place it already had a seam: the `vue` project in its shim, `test/utils/mount.ts`, and the `nuxt` project through a `resolveId` in `vitest.config.ts` pointing `@vue/test-utils` at the new `test/utils/attach-mount.ts`. `defu` and a spread leave `attachTo` overridable, so a case that wants a detached tree still can. `@nuxt/test-utils/runtime` is deliberately not redirected: `mockNuxtImport` and `mockComponent` are macros keyed on that specifier and would stop being transpiled. No spec imports both, which is why aliasing the Vue package rather than the Nuxt one is safe. `enableAutoUnmount(afterEach)` in both setup files is the other half, and is not optional: attaching without it leaves every case's tree in `document.body`, and reka-ui's focus scope from an earlier case then steals the focus a later one just set. `Modal`'s two focus tests failed exactly that way on the first attempt. `componentRender` no longer unmounts by hand — doing both would call `app.unmount()` twice and Vue warns on the second. Two things only became reachable once trees were in the document: - `getComputedStyle` returns a real declaration instead of an empty one, and reka-ui's `usePresence` keeps it in a `ref`, which deep-wraps it in a proxy. happy-dom's getters check their receiver, so `Drawer` threw `TypeError: Receiver must be an instance of class CSSStyleDeclaration` four times per run — uncaught, so it failed the run without failing a test. The same shape as `Countdown`'s `ref`-wrapped `Immediate` in #512, this time in third-party code. `test/utils/patchComputedStyle.ts` hands the declaration back with `markRaw`; removing that call brings all four back. - reka-ui's `hideOthers` marks the trigger `aria-hidden` while a popup is open and leaves it focusable, which axe reports as `aria-hidden-focus` in `Select` and `Table`. Measured: with the production default `portal: true` there is no violation — it needs `portal: false`, which these specs pass so the content lands inside the wrapper and can be asserted on. Auditing `document.body` instead is worse: it then trips on reka-ui's own `data-reka-focus-guard` spans, which carry `tabindex="0"` beside `aria-hidden="true"` by design. The rule is disabled in those two cases with that measurement recorded. The register drops from 37 entries to 25. The dialog-family group is gone entirely: every `nuxt` entry in it went quiet, and its `vue` entries remain only because the plain-Vue test router does not declare the routes those fixtures navigate to. Message volume moved the other way — 366 to 569 — because the two files that stay noisiest, `DropdownMenu` and `ContextMenu`, now really mount their menu content, so reka-ui's `textValue` warning fires once per item rather than once per test. Same third-party causes, more items reaching them. Six snapshot files moved. The `#454` baseline is regenerated at 292 groups and 880 entries; the entries that joined it are reference renders, not props that stopped rendering — `DashboardSearch > renders with groups` passes the shared fixture that already carries `groups`. Closes #513
… reasoning Review of #516. Two findings were mine, and one of them was a false claim. The gate stopped watching before teardown ran. `enableAutoUnmount(afterEach)` was registered before `installConsoleGate()`, and vitest runs same-level `afterEach` hooks last-registered-first — so the gate restored `console` and made its check while the wrapper was still mounted, and a `console.warn` from `onUnmounted` was never seen. Probed with a component that warns while unmounting: it passed. The two calls are swapped, the probe now goes red, and the whole suite stays green — nothing warns during teardown today, but it would be caught from here on. The reason given for disabling axe's `aria-hidden-focus` was wrong. It said the violation is absent under the production default `portal: true`. Measured directly on the DOM rather than through axe: with `portal: false` the trigger itself carries `aria-hidden` while the popup is open; with `portal: true` the `[data-v-app]` ancestor carries it instead and the trigger is still focusable inside. The arrangement is the same either way — the rule only stops firing because `axe(wrapper.element)` cannot see an ancestor above its own root. The comment now says that, and says the underlying question is open. Raised with the maintainer, who decided against an issue and against an upstream report. Also from review: - `attach-mount.ts` took `attachTo` from a plain spread, so an explicit `attachTo: undefined` mounted detached there and attached in the `vue` project, where `defu` drops it. Now `?? document.body` in both. - The alias guard compared `importer` with `endsWith('test/utils/attach-mount.ts')`. On Windows `importer` arrives with backslashes, the guard never matches and the module resolves to itself — invisible here, fatal there. Now a resolved path comparison. - Benchmarks inherit both defaults, and `afterEach` never fires in bench mode, so every wrapper stayed in Vue Test Utils' tracking array — measured at 711 iterations with 0 `afterEach` calls. `disableAutoUnmount()` at the top of the bench file, with a note that timings are not comparable to ones recorded before mounts were attached. - Four comments still credited `componentRender` with the unmounting it no longer does, one of them contradicting a paragraph thirty lines below it. - `patchComputedStyle.ts` carried 24 lines of comment on 6 lines of code. `main` is merged in: the branch was cut before #515 and would have reverted it. The claim in the previous commit message that Vue warns on a second `app.unmount()` does not hold on the versions this repo pins — two reviewers measured it independently. `componentRender` still leaves teardown to `enableAutoUnmount`, because one mechanism is better than two, but that is the reason and the warning is not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Closes #513.
Type of change
revert(Scope): ...)No
src/changes — test harness only.Description
#512 attached the cases
renderEachbuilds. The 293 hand-writtenmountSuspendedcalls across 112 files stayed detached, which is what kept the dialog family inconsole-gate.ts's register with nothing left to fix in it.None of the 293 call sites is edited. Each project sets the default where it already had a seam:
vuetest/utils/mount.tsnuxtresolveIdinvitest.config.tspointing@vue/test-utilsat the newtest/utils/attach-mount.tsA call site that wants a detached tree still can — both seams take the caller's
attachTowhen it gives one, and both treat an explicitundefinedas "no opinion".@nuxt/test-utils/runtimeis deliberately not redirected:mockNuxtImportandmockComponentare macros keyed on that specifier and would stop being transpiled. No spec imports both, which is why aliasing the Vue package rather than the Nuxt one is safe.enableAutoUnmount(afterEach)in both setup files is the other half, and is not optional: attaching without it leaves every case's tree indocument.body, and reka-ui's focus scope from an earlier case then steals the focus a later one just set. Removing that one call reddens 50 ofModal's 60 tests.componentRenderno longer unmounts by hand, because one mechanism is better than two.Two things only became reachable once trees were in the document
getComputedStylereturns a real declaration instead of an empty one, and reka-ui'susePresencekeeps it in aref, which deep-wraps it in a proxy. happy-dom's getters check their receiver, soDrawerthrewTypeError: Receiver must be an instance of class CSSStyleDeclarationfour times per run — uncaught, so it failed the run without failing a test. Same shape asCountdown'sref-wrappedImmediatein #512, this time in third-party code.test/utils/patchComputedStyle.tshands the declaration back withmarkRaw; removing that call brings all four back.reka-ui's
hideOthersputs the trigger inside anaria-hiddenregion while a popup is open and leaves it focusable, which axe reports asaria-hidden-focusinSelectandTable. Measured on both configurations by reading the DOM rather than through axe:aria-hiddenon the triggerportal: falseportal: true(production)[data-v-app]So the arrangement is the same either way; the rule stops firing only because
axe(wrapper.element)cannot see an ancestor above its own root. The rule is disabled in those two cases with that measurement recorded beside it, and with the note that this does not mean the condition is absent. Whether it is a real defect for screen-reader users or the ordinary price of a focus-trapped popup was raised with the maintainer, who decided against an issue and against an upstream report.What it bought, honestly
The dialog-family group is gone entirely: every
nuxtentry in it went quiet, and itsvueentries remain only because the plain-Vue test router does not declare the routes those fixtures navigate to.Message volume moved the other way, and that is not a regression being hidden:
DropdownMenuandContextMenunow really mount their menu content, so reka-ui'stextValuewarning fires once per item rather than once per test. Same third-party causes, more items reaching them. Both files stay registered either way, so this does not change what the gate lets through.Review
/reviewplus the five-reviewer panel. Findings acted on, all in this PR:enableAutoUnmount(afterEach)was registered beforeinstallConsoleGate(), and vitest runs same-levelafterEachhooks last-registered-first — so the gate restoredconsoleand made its check while the wrapper was still mounted. Probed with a component that warns fromonUnmounted: it passed. The calls are swapped, the probe now goes red, and the suite stays green. This was a blind spot introduced by this PR's first commit.aria-hidden-focuswas false — it claimed the violation is absent underportal: true. Re-measured directly on the DOM; see the table above. Corrected everywhere it was written down.mainwas merged in. The branch was cut before chore(sync): close the ledger's decision vocabulary and guard its shape #515 and would have reverted it.attach-mount.tstookattachTofrom a plain spread, soattachTo: undefinedbehaved differently in the two projects.importerwithendsWith('test/utils/attach-mount.ts'); on Windows that never matches and the module resolves to itself. Now a resolved-path comparison.afterEachnever fires in bench mode — 711 iterations, 0afterEachcalls, every wrapper retained.disableAutoUnmount()at the top of the bench file, with a note that timings are not comparable to ones recorded before mounts were attached.componentRenderwith the unmounting it no longer does, one contradicting a paragraph thirty lines below it.One claim from the first commit message does not hold: that Vue warns on a second
app.unmount(). Two reviewers measured it independently on the pinned versions — it does not. The reasoncomponentRenderleaves teardown toenableAutoUnmountis that one mechanism beats two, not that one.Snapshots
Six files moved; the corpus is unchanged at 26.4 MB. The #454 baseline is regenerated at 292 groups / 880 entries. The entries that joined it are reference renders rather than props that stopped rendering —
DashboardSearch > renders with groupspasses the shared fixture that already carriesgroups, the same shape asDropdownMenu > with itemsin #512.Gate
lint,typecheck, 320 test files / 7490 tests,test:module,vitest bench, and coverage 71.89 / 69.93 / 71.19 / 71.44 against thresholds of 70 / 68 / 70 / 70.Checklist