diff --git a/.changeset/tree-contract-envelope-waits-6839.md b/.changeset/tree-contract-envelope-waits-6839.md new file mode 100644 index 0000000000..a3f4ed734e --- /dev/null +++ b/.changeset/tree-contract-envelope-waits-6839.md @@ -0,0 +1,6 @@ +--- +--- + +Test-only change in `@object-ui/plugin-tree`: the `contractEnvelope-6839` pin now +splits its waits by expected outcome instead of reading the row count the instant +the tree-grid wrapper mounts. No published behaviour changes. diff --git a/packages/plugin-tree/src/ObjectTree.contractEnvelope-6839.test.tsx b/packages/plugin-tree/src/ObjectTree.contractEnvelope-6839.test.tsx index 03a74cec1f..c896254303 100644 --- a/packages/plugin-tree/src/ObjectTree.contractEnvelope-6839.test.tsx +++ b/packages/plugin-tree/src/ObjectTree.contractEnvelope-6839.test.tsx @@ -28,10 +28,64 @@ * ⚠️ The refusal case is ALSO satisfied by an `extractRecords` that returns * `[]` for everything — an implementation strictly worse than the bug. The * `data` and bare-array cases refuse it: same rows, same mount. + * + * ## ⭐ Why the two outcomes wait DIFFERENTLY + * + * This file used to hand all three cases ONE wait, and it went red on `main` + * on PRs that cannot reach `plugin-tree` at all — `expected 1 to be 2`. + * + * That wait was `container.querySelector('[data-testid="object-tree"]') ?? + * queryByText('No records')`, and the row count was read the instant it passed. + * The testid is on the TABLE WRAPPER, so it is a MOUNT signal, not a rows + * signal — and the rows this file counts arrive at a LATER commit than the + * table does. + * + * MEASURED, the mechanism is one race, in `ObjectTree` itself: expansion is a + * `useState>(new Set())` MIRROR that a `useEffect` keyed on + * `[roots, defaultExpandedDepth]` re-seeds from the forest. Rows are then + * `flattenVisible(roots, expanded)`. So when `find()`'s rows land, the commit + * that first paints the table still carries the PREVIOUS (empty) mirror: the + * root draws, its child does not, and `tbody tr` is 1. The mirror is seeded in + * the passive effect that follows, and a second commit takes it to 2. Probed + * on this file's own fixture, the DOM sequence is exactly + * `loading → table:1rows → table:2rows`, and the OLD wait's first passing state + * was `table:1rows`, where it yielded 1 — the CI failure, by construction and + * not by luck. Which of the two commits the read lands on is decided by machine + * load, which is why it was green locally and red on a saturated shard. + * + * ⚠️ NOT the same shape as the `plugin-kanban` twin (objectui#8532 / PR #8533), + * although it is the same family. That board had TWO independent races — + * `React.lazy(() => import('./KanbanImpl'))` chunk reveal AND a prop-mirrored + * `boardColumns` — and its symptom was `expected +0 to be 2`, nothing drawn at + * all. `plugin-tree` has NO lazy boundary anywhere in its source (`index.tsx` + * imports `./ObjectTree` eagerly), so only the mirrored-state half transfers, + * and the symptom is a PARTIAL draw: the root without its child. + * + * The two outcomes therefore no longer share a wait: + * + * - the POSITIVE arms wait FOR the DESCENDANT row — the row that only exists + * once the mirror has been seeded. That is the condition the race is + * about, so waiting on it is what makes these arms immune to which commit + * the read lands on. It is NOT a wider window on the same race, which is + * all a raised timeout would have bought. They then assert the drawn SHAPE + * (label + depth per row) plus the root's toggle reading `Collapse`, so the + * pin is the seeded-open hierarchy and not merely "eventually 2 rows" — a + * count alone is also satisfied by a tree that flattens everything. + * - the REFUSAL arm cannot wait for an absence, so it takes a SETTLED read + * anchored on something that DOES appear in that scenario: the tree's own + * "No records" panel, which `ObjectTree` renders only once `loading` has + * flipped false. Probed on the `records` fixture the sequence is + * `loading → empty-state` with no table at any point, and the panel is + * absent while loading — so this arm cannot pass by timing out on an + * absence, which is the failure mode every absence-shaped pin has. + * + * ⛔ Do not fold these back into one wait, and ⛔ do not "fix" a future red here + * with a longer timeout: the failure was never slowness, it was reading a + * signal that does not carry the answer. */ import React from 'react'; -import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { render, waitFor, cleanup, within, act } from '@testing-library/react'; import { describe, it, expect, vi, afterEach } from 'vitest'; import { ObjectTree } from './ObjectTree'; @@ -66,21 +120,87 @@ const asData: Envelope = (rows) => ({ data: rows, total: rows.length }); const asBareArray: Envelope = (rows) => rows; const asRecords: Envelope = (rows) => ({ records: rows, total: rows.length }); +/** One row the forest actually painted, as the tree-grid describes it. */ +interface DrawnRow { + readonly label: string; + readonly depth: number; +} + /** - * Mount the tree over a `find()` answering `envelope`, and hand back what it - * settled on: the number of rows it drew, or `'empty-state'` when it drew its - * own "No records" panel instead. + * Everything this file reads off a settled tree. * - * The two outcomes are DIFFERENT DOM, not a count of 0 — `ObjectTree` returns - * early on an empty forest and never mounts the table. Reporting which one - * happened is what keeps a refused envelope distinguishable from a mount that - * never rendered. + * `tableDrawn` and `emptyPanelDrawn` are carried SEPARATELY rather than + * collapsed into a row count: `ObjectTree` returns early on an empty forest and + * never mounts the table, so a refused envelope and a mount that never rendered + * are DIFFERENT DOM, not both "zero rows". Reporting which one happened is what + * keeps them distinguishable. + */ +interface Settled { + readonly rows: readonly DrawnRow[]; + readonly tableDrawn: boolean; + readonly emptyPanelDrawn: boolean; + /** The root row's toggle, by accessible name — the mirror's state, named. */ + readonly rootToggle: string | null; +} + +/** The rows the tree-grid has painted, in document order. */ +function drawnRows(container: HTMLElement): DrawnRow[] { + return Array.from( + container.querySelectorAll('tbody tr[data-testid="object-tree-row"]'), + ).map((tr) => ({ + label: (tr.querySelector('td')?.textContent ?? '').trim(), + depth: Number(tr.getAttribute('data-depth')), + })); +} + +/** + * Has the tree drawn its own "No records" panel? + * + * ⚠️ `queryAllByText`, not `queryByText`: the singular form THROWS on multiple + * matches as well as answering `null` on none, so it cannot express "how many" + * without the throw becoming the result. + */ +function emptyPanelDrawn(container: HTMLElement): boolean { + return within(container).queryAllByText('No records').length > 0; +} + +function readSettled(container: HTMLElement): Settled { + const root = container.querySelector('tbody tr[data-depth="0"]'); + return { + rows: drawnRows(container), + tableDrawn: container.querySelector('[data-testid="object-tree"]') !== null, + emptyPanelDrawn: emptyPanelDrawn(container), + rootToggle: root?.querySelector('button')?.getAttribute('aria-label') ?? null, + }; +} + +/** + * What a case expects the tree to settle on — which is also what decides HOW it + * waits. See this file's header. + */ +type Outcome = + /** Wait FOR that shape. `because` is carried into the timeout message. */ + | { readonly draws: readonly DrawnRow[]; readonly because: string } + /** No absence to wait for: anchor on the empty panel, settle, then read. */ + | { readonly refuses: true }; + +const REFUSES: Outcome = { refuses: true }; + +/** + * Mount the tree over a `find()` answering `envelope`, and hand back what it + * settled on once `outcome` says it has settled. * * ⛔ Call ONCE per case, never inside a `waitFor` predicate (objectui#7802): * it renders, and `waitFor` re-runs its callback on DOM mutations, so a * predicate that renders feeds itself and leaks a container div per run. + * + * ⚠️ The predicates BELOW are inside `waitFor` on purpose and stay sound under + * that same rule: `drawnRows` and `emptyPanelDrawn` are pure reads of a + * container that is already mounted. They mount nothing, so re-running them on + * a DOM mutation is free — which is exactly the property this helper itself + * does not have. */ -async function settledOn(envelope: Envelope): Promise { +async function settledOn(envelope: Envelope, outcome: Outcome): Promise { const find = vi.fn(async () => envelope(ROWS)); const ds: any = { find, @@ -103,17 +223,38 @@ async function settledOn(envelope: Envelope): Promise { // touches no DOM. Without it "no rows" is satisfied by the mount's initial // empty state, which every arm renders identically. await find.mock.results[0].value; + + if ('refuses' in outcome) { + // A refusal has no arrival to wait for, so this is a SETTLED read built + // from the two things that CAN be observed: `find` has answered (above), + // and the tree has drawn the "No records" panel — a node it renders only + // after `loading` flips false, so it is a COMPLETION ANCHOR and not the + // mere passage of time. `act` then flushes what React still had queued; it + // is the opposite of widening a timeout. + await waitFor(() => + expect( + emptyPanelDrawn(container), + 'the tree must have drawn its own "No records" panel before it is read — without that anchor an absence assertion passes by timing out', + ).toBe(true), + ); + await act(async () => {}); + return readSettled(container); + } + + // Wait on the SHAPE, whose deepest row is the one the expansion mirror gates. + // The table wrapper appears a commit earlier, carrying the root alone. await waitFor(() => - expect( - container.querySelector('[data-testid="object-tree"]') ?? - screen.queryByText('No records'), - ).not.toBeNull(), + expect(drawnRows(container), outcome.because).toEqual(outcome.draws), ); - return container.querySelector('[data-testid="object-tree"]') - ? container.querySelectorAll('tbody tr').length - : 'empty-state'; + return readSettled(container); } +/** Both positive arms draw the same seeded-open hierarchy. */ +const OPEN_FOREST: readonly DrawnRow[] = [ + { label: 'Root', depth: 0 }, + { label: 'Child', depth: 1 }, +]; + afterEach(() => { cleanup(); vi.clearAllMocks(); @@ -121,11 +262,24 @@ afterEach(() => { describe('ObjectTree — the find() envelope it reads (objectui#6839)', () => { it("still reads the contract's `data` member", async () => { - expect(await settledOn(asData), 'the declared rows member must still draw').toBe(2); + const because = 'the declared rows member must still draw the whole forest'; + const settled = await settledOn(asData, { draws: OPEN_FOREST, because }); + expect(settled.rows, because).toEqual(OPEN_FOREST); + // The CONDITION the wait above is keyed to, asserted rather than assumed: + // the child is on screen because the root was seeded OPEN, not because the + // tree flattens its forest regardless of expansion. + expect(settled.rootToggle, 'the root must have settled open').toBe('Collapse'); + expect(settled.tableDrawn, 'the tree-grid must be the node that drew').toBe(true); + expect(settled.emptyPanelDrawn, 'and not the empty panel').toBe(false); }); it('still reads a bare array — the live non-envelope shape fakes answer with', async () => { - expect(await settledOn(asBareArray), 'the bare-array arm must still draw').toBe(2); + const because = 'the bare-array arm must still draw the whole forest'; + const settled = await settledOn(asBareArray, { draws: OPEN_FOREST, because }); + expect(settled.rows, because).toEqual(OPEN_FOREST); + expect(settled.rootToggle, 'the root must have settled open').toBe('Collapse'); + expect(settled.tableDrawn, 'the tree-grid must be the node that drew').toBe(true); + expect(settled.emptyPanelDrawn, 'and not the empty panel').toBe(false); }); it('does NOT read `records` — not a QueryResult member', async () => { @@ -133,9 +287,18 @@ describe('ObjectTree — the find() envelope it reads (objectui#6839)', () => { // declare, and did so AHEAD of `data`. The tree now settles on its own // "No records" panel — a DIFFERENT node from the table, which is what makes // this a reading of the refusal rather than of a mount that never happened. + const settled = await settledOn(asRecords, REFUSES); expect( - await settledOn(asRecords), + settled.emptyPanelDrawn, + 'the refusal must be OBSERVED: the tree drew its "No records" panel', + ).toBe(true); + expect( + settled.rows, 'a `records` envelope must reach the forest as no rows at all, not as the rows it names', - ).toBe('empty-state'); + ).toEqual([]); + expect( + settled.tableDrawn, + 'a refused envelope must not mount the tree-grid at all', + ).toBe(false); }); });