Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions .github/contributing/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,44 @@ often a different vitest project — from the one being re-rendered.
A full `pnpm run test:update` fixes it in one pass. `test:update` exists so
that the safe command is the short one, and it takes no path on purpose.

## Every case is mounted into the document, and unmounted after

`componentRender` mounts with `attachTo: document.body` and calls `unmount()`
as soon as it has the markup. Both halves matter, and neither is the default.

Vue Test Utils renders into a detached element. That is invisible until
something asks the document a question, and reka-ui's dialog family does: it
checks its own accessibility with `document.getElementById(titleId)` in
`onMounted`. Against a detached tree the lookup fails, so every dialog spec
rendering with `portal: false` warned that `DialogContent` requires a
`DialogTitle` — with the title sitting in the markup it had just produced.
Measured across the suite, with the register in `test/utils/console-gate.ts`
emptied on both sides: 623 console messages from 62 tests detached, 366 from 56
attached.

Attached, the snapshots also record what a browser would: `Header` and `Table`
gain the `aria-hidden` reka-ui puts on everything behind an open dialog, and
`CheckboxGroup`/`RadioGroup` gain the `aria-label` it reads off the associated
`<label>` rather than the raw value.

Unmounting is the half that keeps cases independent, and it had never run
before. Without it reka-ui's **module-global** layer stack kept every layer any
case had ever mounted, so `DismissableLayer` rendered `pointer-events: auto`
according to how many earlier cases were still on the stack — not according to
anything under test. Four `#454` baseline groups were held apart by exactly
that byte.

Teardown running for the first time is also why a mock that implements only the
setup half now fails: `EditorDragHandle` and `EditorToolbar` mocked tiptap's
`registerPlugin` without `unregisterPlugin`, which nothing reached until
`beforeUnmount` did. If a component you are testing acquires something on
mount, the mock owes you the release too.

Tests that call `mountSuspended` by hand do not go through `componentRender`
and are still detached — 293 call sites across 112 files. That is why the
dialog specs stay in the register below even though their snapshot matrices
went quiet.

## Console warnings fail the test

A component that renders while logging a warning is a defect the suite already
Expand Down
45 changes: 37 additions & 8 deletions src/runtime/components/Countdown.vue
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,20 @@ const endTime = ref<number>(0)
*/
const totalMilliseconds = ref<number>(0)
/**
* The request ID of the requestAnimationFrame.
* The handle of the pending `requestAnimationFrame`, or `0` when none is
* scheduled.
*
* Deliberately not a `ref`. Nothing renders or watches it, and a `ref` deep-
* wraps an object value in `reactive()` — which is exactly what this holds
* outside a browser: a DOM spec says `requestAnimationFrame` returns a `long`,
* happy-dom returns the Node `Immediate` it scheduled. Stored in a `ref`, the
* handle came back out as a Proxy, and `clearImmediate` unlinked a node that
* was not identical to anything in Node's queue, leaving `processImmediate` to
* read `_idleNext` off `undefined`. Four uncaught `TypeError`s in
* `Countdown.spec.ts`, invisible until the harness began unmounting wrappers
* and `pause` ran for the first time.
*/
const requestId = ref<number>(0)
let requestId: number = 0
// endregion ////

// region events ////
Expand Down Expand Up @@ -323,16 +334,29 @@ function continueProcess(): void {
// props watcher's `start()` and `onActivated`'s `resumeCounting()` both land
// here. Measured: one extra uncancellable frame chain, and `progress` firing
// twice for a single tick. Guarding here rather than at the two call sites
// covers any future third caller too. `cancelAnimationFrame` on a stale or
// already-fired handle is a no-op, so the recursive call from `step` is safe.
cancelAnimationFrame(requestId.value)
// covers any future third caller too.
//
// A spent handle never reaches here: `step` drops it as it fires, and `pause`
// drops it as it cancels, so `requestId` holds only a frame that is still
// pending. That used to rest on `cancelAnimationFrame` being a no-op for a
// handle that already ran, which is true of a browser and not of happy-dom:
// its handle is a Node `Immediate`, and `clearImmediate` on one that has
// already executed unlinks a node that is no longer in the queue, leaving
// Node's `processImmediate` to read `_idleNext` off `undefined`. Four
// uncaught `TypeError`s in `Countdown.spec.ts` — invisible until the harness
// began unmounting wrappers, because nothing ever called `pause`.
cancelAnimationFrame(requestId)
requestId = 0

const delay = Math.min(totalMilliseconds.value, props.interval!)

if (delay > 0) {
let init: number
let prev: number
const step = (now: number) => {
// This frame has fired, so its handle is spent — see `pause`.
requestId = 0

if (!init) {
init = now
}
Expand All @@ -350,13 +374,13 @@ function continueProcess(): void {
) {
progress()
} else {
requestId.value = requestAnimationFrame(step)
requestId = requestAnimationFrame(step)
}

prev = now
}

requestId.value = requestAnimationFrame(step)
requestId = requestAnimationFrame(step)
} else {
stop()
}
Expand All @@ -366,7 +390,12 @@ function continueProcess(): void {
* Pauses the countdown.
*/
function pause(): void {
cancelAnimationFrame(requestId.value)
if (!requestId) {
return
}

cancelAnimationFrame(requestId)
requestId = 0
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/components/Modal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,11 @@ const b24ui = computed(() => tv({ extend: theme, ...(appConfig.b24ui?.modal || {

<slot name="content" :close="close">
<div
v-if="!!slots.header || (props.title || !!slots.title) || (props.description || !!slots.description) || (props.close || !!slots.close) || !!slots.body"
v-if="!!slots.header || (props.title || !!slots.title) || (props.description || !!slots.description) || (props.close || !!slots.close) || !!slots.body || !!slots.actions"
data-slot="contentWrapper"
:class="b24ui.contentWrapper({ class: props.b24ui?.contentWrapper })"
>
<div v-if="!!slots.header || (props.title || !!slots.title) || (props.description || !!slots.description) || (props.close || !!slots.close)" data-slot="header" :class="b24ui.header({ class: props.b24ui?.header })">
<div v-if="!!slots.header || (props.title || !!slots.title) || (props.description || !!slots.description) || (props.close || !!slots.close) || !!slots.actions" data-slot="header" :class="b24ui.header({ class: props.b24ui?.header })">
<slot name="header" :close="close">
<div
v-if="props.title || !!slots.title || props.description || !!slots.description"
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/components/Slideover.vue
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ const isBtnCloseExternal = computed(() => (!props.inset && ['left', 'right', 'bo

<slot name="content" :close="close">
<div
v-if="!!slots.header || (props.title || !!slots.title) || (props.description || !!slots.description) || (!isBtnCloseExternal && (props.close || !!slots.close))"
v-if="!!slots.header || (props.title || !!slots.title) || (props.description || !!slots.description) || (!isBtnCloseExternal && (props.close || !!slots.close)) || !!slots.actions"
data-slot="header"
:class="b24ui.header({ class: props.b24ui?.header })"
>
Expand Down
18 changes: 16 additions & 2 deletions test/component-render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,18 @@ import { it, expect } from 'vitest'

type MountSuspendedOptions<T> = Parameters<typeof mountSuspended<T>>[1]

/**
* Mounted into the document, and unmounted once the markup is taken.
*
* Vue Test Utils renders into a detached element, which is invisible to
* anything that asks the document a question — and reka-ui's dialogs do. Not
* unmounting left reka-ui's module-global layer stack holding every case ever
* mounted, so cases were not independent.
*
* Both halves changed what the corpus records, and unmounting surfaced three
* defects. `.github/contributing/testing.md` has the measurements and the
* consequences for writing mocks.
*/
async function componentRender<T>(nameOrHtml: string, options: MountSuspendedOptions<T>, component: T) {
let html: string
const name = component && typeof component === 'object' && '__file' in component && typeof component.__file === 'string'
Expand All @@ -14,11 +26,13 @@ async function componentRender<T>(nameOrHtml: string, options: MountSuspendedOpt
template: nameOrHtml,
components: { [`B24${name}`]: component }
}
const result = await mountSuspended(app)
const result = await mountSuspended(app, { attachTo: document.body })
html = result.html()
result.unmount()
} else {
const cResult = await mountSuspended<T>(component, options)
const cResult = await mountSuspended<T>(component, { ...options, attachTo: document.body })
html = cResult.html()
cResult.unmount()
}
return html
}
Expand Down
4 changes: 4 additions & 0 deletions test/components/Countdown.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ describe('Countdown', () => {
props: { seconds: 10, needStartImmediately: false }
})
const vm = wrapper.vm as any
// `start()` first, because there is nothing to cancel otherwise. This
// used to pass without it: `pause` cancelled unconditionally, so the spy
// recorded a call that handed `cancelAnimationFrame` the initial `0`.
vm.start()
vm.pause()
expect(cancelAnimationFrameSpy).toHaveBeenCalled()
})
Expand Down
6 changes: 5 additions & 1 deletion test/components/EditorDragHandle.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import { renderEach } from '../component-render'
import EditorDragHandle from '../../src/runtime/components/EditorDragHandle.vue'

describe('EditorDragHandle', () => {
const props = { editor: { registerPlugin: vi.fn() } as unknown as Editor }
// `unregisterPlugin` is the teardown half. It was missing for as long as
// nothing unmounted the wrapper — `componentRender` now does, so tiptap's
// `DragHandleVue` reaches its `beforeUnmount` and calls it. A mock that
// implements only the setup half is a fixture that never sees teardown.
const props = { editor: { registerPlugin: vi.fn(), unregisterPlugin: vi.fn() } as unknown as Editor }

renderEach(EditorDragHandle, [
// Props
Expand Down
4 changes: 3 additions & 1 deletion test/components/EditorToolbar.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ describe('EditorToolbar', () => {
'icon': EncloseTextInCodeTagIcon,
'aria-label': 'Code'
}]]
const props = { editor: { registerPlugin: vi.fn() } as unknown as Editor, items }
// `unregisterPlugin` for the same reason as in EditorDragHandle: the wrapper
// is unmounted now, so tiptap's BubbleMenu reaches its teardown.
const props = { editor: { registerPlugin: vi.fn(), unregisterPlugin: vi.fn() } as unknown as Editor, items }

renderEach(EditorToolbar, [
// Props
Expand Down
7 changes: 7 additions & 0 deletions test/components/Modal.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ describe('Modal', () => {
['with title slot', { props, slots: { title: () => 'Title slot' } }],
['with description slot', { props, slots: { description: () => 'Description slot' } }],
['with actions slot', { props, slots: { actions: () => 'Actions slot' } }],
// The header is what holds the `actions` slot, and every other reason to
// render it is off here. It used to be missing from that `v-if`, so an
// `actions` slot on its own rendered nothing — the case above did not show
// it, because `close` defaults to `true` and kept the header alive.
// `Slideover` had the same hole and was reached by its default fixture:
// with `side: 'bottom'` its close button is external, so no header.
['with actions slot and without close', { props: { ...props, close: false }, slots: { actions: () => 'Actions slot' } }],
['with close slot', { props, slots: { close: () => 'Close slot' } }],
['with body slot', { props, slots: { body: () => 'Body slot' } }],
['with footer slot', { props, slots: { footer: () => 'Footer slot' } }]
Expand Down
Loading
Loading