From 3df35c6682b6ebfcc3b8676dd705f90b04a6d9c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 00:09:11 +0000 Subject: [PATCH] test(plugin-map,plugin-timeline): pin what these two waits were standing on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both `contractEnvelope-6839` pins were green for reasons their wait expressions did not state. `ObjectMap` carries the family's only absence-shaped wait — `queryByText('Loading map...')` is null. An absence nothing entered is satisfied by a mount that never started: with the component mutated to `return null` unconditionally, an implementation strictly worse than the bug, the refusal case still passed. And the panel is a settle signal, not a rows signal — it stands in for "the rows are on screen" only because `setData` and `setLoading(false)` land in one React commit, which nothing asserted. With `setData` deferred by 50ms and the `records` arm restored to `extractRecords`, the refusal case read zero markers and passed while the settled map plotted two. `ObjectTimeline` waited for `getByTestId('timeline-renderer')`'s `data-item-count` to be not-null. That clause is inert: the renderer double writes the attribute unconditionally, so once `getByTestId` stops throwing the attribute is always a string, and the throw was the entire gate. It named a row count and gated on a mount, so `"0"` satisfied it — under the same deferral the wait was satisfied at `data-item-count="0"` while the settled timeline drew two rows. `markersThrough` now asserts the loading panel is on screen before waiting for it to go, and that the marker count read at the transition survives a 50ms settle window. `itemsThrough` takes the count its arm claims, gates the wait on that value, requires it to survive the same window, and observes the loading skeleton first so a renderer mounted empty on the first paint is not read as a settled zero. 50ms rather than 0ms because RTL's `asyncWrapper` drains one macrotask, so a 0ms deferral hides inside the window meant to detect it. Test-only; no component was changed. Refs objectui#8709 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- .../pin-unpinned-wait-preconditions-8709.md | 10 ++ .../ObjectMap.contractEnvelope-6839.test.tsx | 68 +++++++++++++- ...ectTimeline.contractEnvelope-6839.test.tsx | 91 +++++++++++++++++-- 3 files changed, 159 insertions(+), 10 deletions(-) create mode 100644 .changeset/pin-unpinned-wait-preconditions-8709.md diff --git a/.changeset/pin-unpinned-wait-preconditions-8709.md b/.changeset/pin-unpinned-wait-preconditions-8709.md new file mode 100644 index 0000000000..674e3819cb --- /dev/null +++ b/.changeset/pin-unpinned-wait-preconditions-8709.md @@ -0,0 +1,10 @@ +--- +--- + +Test-only (no release). `ObjectMap.contractEnvelope-6839` and +`ObjectTimeline.contractEnvelope-6839` each waited on something that could not +fail: the map's absence-shaped wait was satisfied by a mount that never showed +the loading panel and by a `setData` that had not committed yet, and the +timeline's `data-item-count` not-null clause was inert — `getByTestId` throwing +was the whole gate, so `"0"` satisfied it. Both waits now observe the +transition and gate on a settled row count. No package source changed. diff --git a/packages/plugin-map/src/ObjectMap.contractEnvelope-6839.test.tsx b/packages/plugin-map/src/ObjectMap.contractEnvelope-6839.test.tsx index 829e417b5f..ea263b3e6e 100644 --- a/packages/plugin-map/src/ObjectMap.contractEnvelope-6839.test.tsx +++ b/packages/plugin-map/src/ObjectMap.contractEnvelope-6839.test.tsx @@ -34,10 +34,30 @@ * ⚠️ 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. + * + * ## The two things this file's wait used to STAND ON without saying so (objectui#8709) + * + * This is the family's only ABSENCE-shaped wait, and an absence carries two + * unstated preconditions. Both are now assertions in `markersThrough`, and both + * were measured failing first: + * + * 1. **The panel was ever on screen.** An absence nothing entered is + * satisfied by a mount that never started. With the component mutated to + * `return null` unconditionally the refusal case PASSED — "render nothing, + * ever" is strictly worse than the bug and cleared the old bar. + * 2. **`setData` and `setLoading(false)` commit TOGETHER.** The panel is a + * settle signal, not a rows signal; it is only a usable proxy for "the + * rows are on screen" because the two `setState` calls land in one commit. + * With `setData` deferred by 50ms and the `records` arm restored to + * `extractRecords`, the refusal case read zero markers and PASSED while + * the settled map plotted two. + * + * ⭐ Neither gate subsumes the other: (1) alone still lets a split commit + * through, and (2) alone still passes a component that renders nothing. */ import React from 'react'; -import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { act, render, screen, waitFor, cleanup } from '@testing-library/react'; import { describe, it, expect, vi, afterEach } from 'vitest'; // The MapLibre canvas, replaced by DOM the test can count. Copied from @@ -72,6 +92,17 @@ const schema: any = { data: { provider: 'object', object: 'store' }, }; +/** + * The window held open AFTER the loading panel clears, to prove no markers + * arrive behind it. + * + * ⚠️ 50ms, not 0ms. RTL's `asyncWrapper` drains one macrotask before it + * returns, so a commit scheduled with `setTimeout(…, 0)` lands INSIDE that + * drain window — a 0ms window cannot tell a deferred commit from a + * same-commit one, and reports "stable" for both (measured on objectui#8664). + */ +const POST_SETTLE_MS = 50; + /** How one case wraps its rows on the way back out of `find()`. */ type Envelope = (rows: unknown[]) => unknown; @@ -105,6 +136,17 @@ async function markersThrough(envelope: Envelope): Promise { })), }; render(); + // ① The transition's START, observed — the half an ABSENCE-shaped wait cannot + // supply for itself. "The panel has cleared" and "nothing was ever rendered" + // are the SAME DOM, so without this line the wait below is satisfied by a + // mount that never started. MEASURED, not argued: with `ObjectMap` mutated to + // `return null` unconditionally — an implementation strictly worse than the + // bug — the refusal case below still PASSED (objectui#8709 leg B). It does + // not survive this line. + expect( + screen.getByText('Loading map...'), + 'the loading panel must be on screen BEFORE the absence wait — an absence nothing entered is satisfied by never having started', + ).toBeInTheDocument(); await waitFor(() => expect(find).toHaveBeenCalled()); // `find`'s OWN answer, settled — a pure read of the mock's call record that // touches no DOM. Without it "no markers" is satisfied by the mount's @@ -114,7 +156,29 @@ async function markersThrough(envelope: Envelope): Promise { // rather than a rows signal, which is exactly what makes it usable as the // one wait shared by the live cases and the refusal case. await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull()); - return screen.queryAllByTestId('map-marker').length; + const atPanelClear = screen.queryAllByTestId('map-marker').length; + // ② The unstated precondition the line above STANDS ON, now pinned: + // `setData(capped.rows)` and `setLoading(false)` reach the DOM in ONE commit. + // React 18 batches them today because they sit in one `await` continuation — + // true, load-bearing, and asserted by nothing else in this repo. The day they + // split (a transition, an async boundary, a `startTransition`) the panel + // clears over an EMPTY `data` and the count read above is the INTERMEDIATE + // state. MEASURED: with `setData` deferred by 50ms and the `records` arm + // restored to `extractRecords`, the refusal case read zero markers and PASSED + // while the settled map plotted two — the bug went undetected (leg A). + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, POST_SETTLE_MS)); + }); + // A dependency landing mid-window (`objectSchema`, `perms`) re-runs the fetch + // effect and re-raises the panel, so re-settle before re-reading: the claim is + // about the SETTLED count, not about a moment inside a refetch. + await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull()); + const settled = screen.queryAllByTestId('map-marker').length; + expect( + settled, + 'markers must not arrive AFTER the loading panel clears — the count read at the transition must already be the settled one', + ).toBe(atPanelClear); + return settled; } afterEach(() => { diff --git a/packages/plugin-timeline/src/ObjectTimeline.contractEnvelope-6839.test.tsx b/packages/plugin-timeline/src/ObjectTimeline.contractEnvelope-6839.test.tsx index e5e244554e..569c8f14b6 100644 --- a/packages/plugin-timeline/src/ObjectTimeline.contractEnvelope-6839.test.tsx +++ b/packages/plugin-timeline/src/ObjectTimeline.contractEnvelope-6839.test.tsx @@ -32,10 +32,27 @@ * The `data` and bare-array cases are the ones that refuse it: they push the * SAME rows through the SAME mount, so an arm that delivers nothing delivered * nothing because the envelope was refused. + * + * ## What this file's wait used to STAND ON without saying so (objectui#8709) + * + * The wait named `data-item-count` and did not gate on it. `getByTestId` + * THROWING inside `waitFor` was the entire gate; by the time the attribute was + * read the node existed and the renderer double writes the attribute + * unconditionally, so `.not.toBeNull()` could never be the clause that failed. + * ⭐ That is worse than no clause: it reads like a row-count gate and is a + * mount signal, satisfied the instant the renderer appears — `"0"` included. + * + * MEASURED, not argued: with `setFetchedData` deferred by 50ms and the + * `records` arm restored to `extractRecords`, the wait was satisfied at + * `data-item-count="0"` and the refusal case PASSED while the settled timeline + * drew two rows. `itemsThrough` now gates on the count the wait names AND on + * that count surviving a settle window, with the loading skeleton observed + * first so a renderer mounted empty on the first paint is not read as a + * settled zero. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { render, waitFor, cleanup, screen } from '@testing-library/react'; +import { act, render, waitFor, cleanup, screen } from '@testing-library/react'; import React from 'react'; // The timeline's own visualisation is orthogonal to what this file observes @@ -55,6 +72,17 @@ const ROWS = [ { id: 't2', subject: 'Ship it again', starts_at: '2026-01-02T09:00:00Z' }, ]; +/** + * The window held open AFTER the renderer reports the expected count, to prove + * no further rows arrive behind it. + * + * ⚠️ 50ms, not 0ms. RTL's `asyncWrapper` drains one macrotask before it + * returns, so a commit scheduled with `setTimeout(…, 0)` lands INSIDE that + * drain window — a 0ms window cannot tell a deferred commit from a same-commit + * one, and reports "stable" for both (measured on objectui#8664). + */ +const POST_SETTLE_MS = 50; + /** How one case wraps its rows on the way back out of `find()`. */ type Envelope = (rows: unknown[]) => unknown; @@ -69,8 +97,14 @@ const schema: any = { startDateField: 'starts_at', }; -/** Mount the timeline over a `find()` answering `envelope`, return rows drawn. */ -async function itemsThrough(envelope: Envelope): Promise { +/** + * Mount the timeline over a `find()` answering `envelope`, return rows drawn. + * + * `expectedItems` is the count this arm claims the envelope produces, and it is + * what the wait GATES on — see the comments in the body for why the count has + * to be named here rather than merely read at the end. + */ +async function itemsThrough(envelope: Envelope, expectedItems: number): Promise { const ds: Record = { find: vi.fn(async () => envelope(ROWS)), findOne: vi.fn(), @@ -87,15 +121,56 @@ async function itemsThrough(envelope: Envelope): Promise { })), }; render(); + // ① The transition's START, observed. Without it a component that mounts the + // renderer EMPTY on its first paint and fetches afterwards clears every bar + // below on the refusal arm — `"0"`, stable, forever. The skeleton is the + // proof that this mount actually went through a loading phase. + // + // ⚠️ `toBeTruthy`, not `toBeInTheDocument`: this package's + // `tsconfig.test.json` does not name `@testing-library/jest-dom` in `types` + // (the sibling `plugin-map` one does), so that matcher is green under vitest + // and TS2339 under `tsc -p tsconfig.test.json`. `getByTestId` THROWING when + // the skeleton is absent is the assertion either way; the matcher only + // attaches the message. + expect( + screen.getByTestId('timeline-loading'), + 'the loading skeleton must be on screen first — a renderer mounted empty from the first paint is not a settled zero', + ).toBeTruthy(); await waitFor(() => expect(ds.find).toHaveBeenCalled()); // `find`'s OWN answer, settled — a pure read of the mock's call record that // touches no DOM. Without it the assertion can be satisfied by the mount's // initial empty state, which every arm renders identically. await ds.find.mock.results[0].value; + // ② Gate on the count this wait NAMES, at the value this arm claims. + // + // The clause here used to be `.not.toBeNull()`, and it was INERT: the + // attribute is written unconditionally by the renderer double, so once + // `getByTestId` stops throwing the attribute is always a string. The THROW + // was the whole gate, which made this a bare mount signal wearing the + // vocabulary of a row count — `"0"` satisfied it, so an arm expecting rows + // and an arm expecting none waited on exactly the same event. await waitFor(() => - expect(screen.getByTestId('timeline-renderer').getAttribute('data-item-count')).not.toBeNull(), + expect( + screen.getByTestId('timeline-renderer').getAttribute('data-item-count'), + 'the renderer must report the row count this envelope produces, not merely exist', + ).toBe(String(expectedItems)), ); - return Number(screen.getByTestId('timeline-renderer').getAttribute('data-item-count')); + // ③ … and it must STILL be that count after the settle window. This is the + // half ② cannot supply on the refusal arm, where the expected value is `"0"` + // and `"0"` is also what an unpopulated renderer reports. MEASURED: with + // `setFetchedData` deferred by 50ms and the `records` arm restored to + // `extractRecords`, the old wait was satisfied at `data-item-count="0"` and + // the refusal case PASSED while the settled timeline drew two rows + // (objectui#8709 leg A). + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, POST_SETTLE_MS)); + }); + const settled = screen.getByTestId('timeline-renderer').getAttribute('data-item-count'); + expect( + settled, + 'rows must not arrive AFTER the count was read — a count that moves in the settle window was an intermediate state', + ).toBe(String(expectedItems)); + return Number(settled); } beforeEach(() => { @@ -113,18 +188,18 @@ describe('ObjectTimeline — the find() envelope it reads (objectui#6839)', () = // (objectui#7802): it renders, and `waitFor` re-runs its callback on DOM // mutations, so the predicate feeds itself and leaks a container div per // run. The render happens once, out here, and the case reads its answer. - expect(await itemsThrough(asData), 'the declared rows member must still draw').toBe(2); + expect(await itemsThrough(asData, 2), 'the declared rows member must still draw').toBe(2); }); it('still reads a bare array — the live non-envelope shape fakes answer with', async () => { - expect(await itemsThrough(asBareArray), 'the bare-array arm must still draw').toBe(2); + expect(await itemsThrough(asBareArray, 2), 'the bare-array arm must still draw').toBe(2); }); it('does NOT read `records` — not a QueryResult member', async () => { // Before the fix these two rows drew off a key `QueryResult` does not // declare, and did so AHEAD of `data`. expect( - await itemsThrough(asRecords), + await itemsThrough(asRecords, 0), 'a `records` envelope must reach the rail as zero rows, not as the rows it names', ).toBe(0); });