Skip to content

Commit 8314a41

Browse files
bloveclaude
andauthored
test(react): extract the shared rejected-write harness (#563)
* test(react): extract the shared rejected-write harness The four "an invalid X update is rejected, not fatal" suites each carried a near-verbatim copy of the same machinery: the `vi.mock("@pretable/core", ...)` proxy that intercepts one row-model method and can be armed to throw, the `Holding`/`COLUMNS`/`getRowId`/`ROWS` fixture, the `dataRowCount` probe, the warn-spy lifecycle, and — the signal that prompted this — the "TRAP IF THIS FILE GROWS" paragraph, re-pasted by each author rather than re-derived. The proxy and its two traps now live in `rejected-write-core-proxy.ts`, and everything else in `rejected-write-harness.ts`. The split is load-bearing, not cosmetic: `vi.mock` is hoisted above the test file's imports, so its factory reaches the registry through a dynamic `import()` that runs while the mocked `@pretable/core` module is still being constructed. The proxy module therefore imports nothing, while the harness imports `@pretable/core` for the shared `COLUMNS` fixture. Measured: adding a `@pretable/core` import to the proxy module hangs the run. `installWarnSpy` takes no handles. It sweeps the whole per-file proxy registry, so a suite cannot intercept a method, forget to hand its handle over, and silently lose its per-test reset. Each suite keeps its own faults — the invalid `Query` shapes, the grouped/aggregate columns, the bad-row arrays and the `FAULTS` tables — since those genuinely differ and sharing them would make a failure harder to localise. Test-only: 61 tests before, 61 after, every name unchanged, no assertion weakened, and nothing outside `packages/react/src/__tests__/` touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(react): close the counter vacuity in the attempted-once pins NOT a no-op, unlike the harness extraction it sits on. Both "a rejected update is attempted once, not retried every render" tests compared a delta against ITSELF: const afterRejection = setRows.callCount(); // ... same-identity rerenders ... expect(setRows.callCount()).toBe(afterRejection); A counter frozen at zero satisfies both sides, so the pin passed while observing nothing. Measured on the pre-refactor files as well as these: with the increment removed, `invalid-local-rows-rejected` stayed fully green and `invalid-rows-rejected` reddened only on an unrelated test. The vacuity is older than the harness; the harness only made it easy to see. Each test now baselines BEFORE the rejecting rerender and asserts the count actually moved, before asserting the same-identity rerenders do not move it. Verified by mutation: freezing `callCount()` at 0 now reds both tests, where both were green before this commit. Also verified, and recorded in the comment so it is not mistaken for a second vacuity: both still survive a broken `armThrow`, correctly — they drive a real duplicate-id fault through the rows prop/option and never touch the armed seam. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 0e12b48 commit 8314a41

6 files changed

Lines changed: 505 additions & 487 deletions

‎packages/react/src/__tests__/invalid-derivations-rejected.test.tsx‎

Lines changed: 50 additions & 126 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,24 @@
11
// @vitest-environment jsdom
22
import "@testing-library/jest-dom/vitest";
3-
import { act, cleanup, render, waitFor } from "@testing-library/react";
4-
import { afterEach, beforeEach, describe, expect, test, vi } from "vitest";
3+
import { act, render, waitFor } from "@testing-library/react";
4+
import { describe, expect, test, vi } from "vitest";
55

6-
import { createColumnHelper, type PretableQueryFor } from "@pretable/core";
6+
import { type PretableQueryFor } from "@pretable/core";
77

8-
import { resetDevWarnings } from "../dev-warn";
98
import { PretableSurface, type PretableSurfaceGrid } from "../pretable-surface";
10-
11-
type Holding = {
12-
id: string;
13-
sector: string;
14-
qty: number;
15-
};
16-
17-
const helper = createColumnHelper<Holding>();
18-
19-
/**
20-
* `qty` declares `sum`. Over the Tech rows sum is 30 and count is 2 — two
21-
* distinct numbers, so "the rejected update did NOT land" and "the recovery
22-
* update DID land" are distinguishable at the pixel. A fixture whose
23-
* aggregates agreed would pass either way.
24-
*/
25-
const COLUMNS = [
26-
helper.accessor("sector", { type: "text" }),
27-
helper.accessor("qty", { type: "number", aggregate: "sum" }),
28-
] as const;
9+
import {
10+
COLUMNS,
11+
columnHelper,
12+
getRowId,
13+
type Holding,
14+
installWarnSpy,
15+
ROWS,
16+
rowModelMethodProxy,
17+
} from "./rejected-write-harness";
2918

3019
const COUNT_COLUMNS = [
31-
helper.accessor("sector", { type: "text" }),
32-
helper.accessor("qty", { type: "number", aggregate: "count" }),
20+
columnHelper.accessor("sector", { type: "text" }),
21+
columnHelper.accessor("qty", { type: "number", aggregate: "count" }),
3322
] as const;
3423

3524
/*
@@ -47,7 +36,7 @@ const INVALID_COLUMNS = [
4736
* Identity matters: the derivations gate compares the merged list by identity,
4837
* so re-passing one constant would be a no-op rather than a second attempt —
4938
* and a latching test that never makes a second attempt passes vacuously. The
50-
* `setDerivationsCallCount` guards below are what hold that invariant; give
39+
* `setDerivations.callCount()` guards below are what hold that invariant; give
5140
* this function an identity cache and they fail.
5241
*/
5342
function invalidColumns(aggregate: string): typeof COLUMNS {
@@ -80,7 +69,7 @@ function invalidColumnsReordered(aggregate: string): typeof COLUMNS {
8069
* A getter on the column itself (`type`, `value`, `compare`, `accessor`,
8170
* `aggregate`) does NOT reach here: measured, each throws a plain `Error`
8271
* during React render, ahead of the guarded call, exactly as this file's
83-
* seam comment above describes. The aggregate's members are read only by the
72+
* seam comment below describes. The aggregate's members are read only by the
8473
* compiler, which is what makes this fault class reachable at all.
8574
*/
8675
function throwingAggregateMember(member: string): typeof COLUMNS {
@@ -113,96 +102,31 @@ function throwingAggregateMember(member: string): typeof COLUMNS {
113102
* `setDerivations`. Without this, "swallowed" and "rethrown" look identical
114103
* from outside the module, and a narrow catch nobody can verify is a blanket
115104
* one. The same proxy counts calls, which is how the "attempted once" pin
116-
* observes a recompile.
105+
* observes a recompile. It is disarmed by default, so every other test in this
106+
* file runs the real model.
117107
*
118-
* Disarmed by default, so every other test in this file runs the real model.
108+
* `setQuery` is intercepted too, for its COUNT only — never armed — which is
109+
* what "a rejected update does not force a query re-apply" reads.
119110
*
120-
* TWO TRAPS IF THIS FILE GROWS.
121-
* 1. The mock is module-wide, unlike the narrower per-test seam one directory
122-
* over (`vi.spyOn(core, "createLocalRowModel")` in
123-
* `row-model-mode.test.tsx`). Every test here gets the proxy.
124-
* 2. The proxy is NOT identity-transparent. `ɵsetLocalRowModelFilterAuthority`
125-
* and `ɵsetLocalRowModelSortAuthority` look the model up in WeakMaps keyed
126-
* by the RAW object and swallow a miss with `?.`, so those writes are
127-
* silent no-ops for every test in this file. Nothing here depends on
128-
* filter/sort authority; a test that does would pass vacuously.
111+
* The proxy itself, and the two traps it carries, live in
112+
* `rejected-write-core-proxy.ts`. READ THEM BEFORE ADDING A TEST HERE.
129113
*/
130-
const NON_VALIDATION_ERROR = new Error("boom");
131-
let throwNonValidationOnNextSetDerivations = false;
132-
let setDerivationsCallCount = 0;
133-
let setQueryCallCount = 0;
134-
135114
vi.mock("@pretable/core", async (importOriginal) => {
136-
const actual = await importOriginal<typeof import("@pretable/core")>();
137-
return {
138-
...actual,
139-
createLocalRowModel: (...args: readonly unknown[]) => {
140-
const model = (
141-
actual.createLocalRowModel as unknown as (
142-
...a: readonly unknown[]
143-
) => object
144-
)(...args);
145-
return new Proxy(model, {
146-
get(target, property, receiver) {
147-
const value = Reflect.get(target, property, receiver) as unknown;
148-
if (property === "setQuery") {
149-
return (...callArgs: readonly unknown[]) => {
150-
setQueryCallCount += 1;
151-
return (value as (...a: readonly unknown[]) => unknown)(
152-
...callArgs,
153-
);
154-
};
155-
}
156-
if (property !== "setDerivations") return value;
157-
return (...callArgs: readonly unknown[]) => {
158-
setDerivationsCallCount += 1;
159-
if (throwNonValidationOnNextSetDerivations) {
160-
throwNonValidationOnNextSetDerivations = false;
161-
throw NON_VALIDATION_ERROR;
162-
}
163-
return (value as (...a: readonly unknown[]) => unknown)(
164-
...callArgs,
165-
);
166-
};
167-
},
168-
});
169-
},
170-
};
115+
const { proxiedCoreModule } = await import("./rejected-write-core-proxy");
116+
return proxiedCoreModule(importOriginal, "setDerivations", "setQuery");
171117
});
172118

173-
const ROWS: readonly Holding[] = [
174-
{ id: "h1", sector: "Tech", qty: 10 },
175-
{ id: "h2", sector: "Tech", qty: 20 },
176-
{ id: "h3", sector: "Energy", qty: 5 },
177-
];
119+
const NON_VALIDATION_ERROR = new Error("boom");
120+
const setDerivations = rowModelMethodProxy("setDerivations");
121+
const setQuery = rowModelMethodProxy("setQuery");
122+
const warnSpy = installWarnSpy();
178123

179124
const GROUPED_QUERY: PretableQueryFor<typeof COLUMNS> = {
180125
filters: [],
181126
sort: [],
182127
rowGroups: [{ columnId: "sector" }],
183128
};
184129

185-
const getRowId = (row: Holding) => row.id;
186-
187-
/*
188-
* `warnOnce` keeps its emitted keys in MODULE state, so without the reset the
189-
* second test to provoke the same fault would see no warning at all.
190-
*/
191-
let warnSpy: ReturnType<typeof vi.spyOn>;
192-
beforeEach(() => {
193-
resetDevWarnings();
194-
warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
195-
});
196-
197-
afterEach(() => {
198-
throwNonValidationOnNextSetDerivations = false;
199-
// `cleanup()` FIRST: unmount runs with the spy still installed, so a warning
200-
// emitted on the way down is captured rather than escaping to the real
201-
// console. No such warning exists on this path today.
202-
cleanup();
203-
warnSpy.mockRestore();
204-
});
205-
206130
type Grid = PretableSurfaceGrid<Holding, string, typeof COLUMNS>;
207131

208132
function groupedElement(props: {
@@ -288,7 +212,7 @@ describe("an invalid derivations update is rejected, not fatal", () => {
288212

289213
// Widen the catch to every error and this is the assertion that fails:
290214
// the plain error is swallowed and the rerender completes quietly.
291-
throwNonValidationOnNextSetDerivations = true;
215+
setDerivations.armThrow(() => NON_VALIDATION_ERROR);
292216
expect(() => {
293217
view.rerender(
294218
groupedElement({
@@ -304,12 +228,12 @@ describe("an invalid derivations update is rejected, not fatal", () => {
304228
expect(techAggregateText(view.container)).toBe("30");
305229
});
306230

307-
const beforeRejection = setDerivationsCallCount;
231+
const beforeRejection = setDerivations.callCount();
308232
view.rerender(groupedElement({ columns: INVALID_COLUMNS }));
309233
await waitFor(() => {
310234
expect(techAggregateText(view.container)).toBe("30");
311235
});
312-
expect(setDerivationsCallCount).toBe(beforeRejection + 1);
236+
expect(setDerivations.callCount()).toBe(beforeRejection + 1);
313237

314238
/*
315239
* The half of decision 4 that recovery alone cannot pin: the rejected
@@ -324,7 +248,7 @@ describe("an invalid derivations update is rejected, not fatal", () => {
324248
await waitFor(() => {
325249
expect(techAggregateText(view.container)).toBe("30");
326250
});
327-
expect(setDerivationsCallCount).toBe(beforeRejection + 1);
251+
expect(setDerivations.callCount()).toBe(beforeRejection + 1);
328252
});
329253

330254
test("a rejected update does not force a query re-apply", async () => {
@@ -363,12 +287,12 @@ describe("an invalid derivations update is rejected, not fatal", () => {
363287
* `query` prop in the SAME commit still reaches `setQuery`, through
364288
* `controlledQueryChanged`, which is independent of this gate.
365289
*/
366-
const beforeRejection = setQueryCallCount;
290+
const beforeRejection = setQuery.callCount();
367291
view.rerender(groupedElement({ columns: INVALID_COLUMNS }));
368292
await waitFor(() => {
369293
expect(techAggregateText(view.container)).toBe("30");
370294
});
371-
expect(setQueryCallCount).toBe(beforeRejection);
295+
expect(setQuery.callCount()).toBe(beforeRejection);
372296
});
373297

374298
test("the rejection warns once, naming the column and the value", async () => {
@@ -379,21 +303,21 @@ describe("an invalid derivations update is rejected, not fatal", () => {
379303

380304
view.rerender(groupedElement({ columns: invalidColumns("nonsense") }));
381305
await waitFor(() => {
382-
expect(warnSpy).toHaveBeenCalledTimes(1);
306+
expect(warnSpy()).toHaveBeenCalledTimes(1);
383307
});
384308

385309
/*
386310
* Content, not just count: a warning naming neither the column nor the
387311
* offending value would satisfy a bare call-count assertion while telling
388312
* a developer nothing about what to fix.
389313
*/
390-
const message = String(warnSpy.mock.calls[0]?.[0]);
314+
const message = String(warnSpy().mock.calls[0]?.[0]);
391315
expect(message).toContain("qty");
392316
expect(message).toContain("nonsense");
393317

394318
// A second attempt at the SAME fault must stay silent — that is what
395319
// `warnOnce` is for.
396-
const beforeSecondAttempt = setDerivationsCallCount;
320+
const beforeSecondAttempt = setDerivations.callCount();
397321
view.rerender(groupedElement({ columns: invalidColumns("nonsense") }));
398322
await waitFor(() => {
399323
expect(techAggregateText(view.container)).toBe("30");
@@ -405,8 +329,8 @@ describe("an invalid derivations update is rejected, not fatal", () => {
405329
* — because the derivations gate compares identity and never calls
406330
* `setDerivations` again. This line is what fails under that mutation.
407331
*/
408-
expect(setDerivationsCallCount).toBeGreaterThan(beforeSecondAttempt);
409-
expect(warnSpy).toHaveBeenCalledTimes(1);
332+
expect(setDerivations.callCount()).toBeGreaterThan(beforeSecondAttempt);
333+
expect(warnSpy()).toHaveBeenCalledTimes(1);
410334
});
411335

412336
test("two faults differing only in the property each warn", async () => {
@@ -423,18 +347,18 @@ describe("an invalid derivations update is rejected, not fatal", () => {
423347
*/
424348
view.rerender(groupedElement({ columns: throwingAggregateMember("init") }));
425349
await waitFor(() => {
426-
expect(warnSpy).toHaveBeenCalledTimes(1);
350+
expect(warnSpy()).toHaveBeenCalledTimes(1);
427351
});
428352
view.rerender(
429353
groupedElement({ columns: throwingAggregateMember("merge") }),
430354
);
431355
await waitFor(() => {
432-
expect(warnSpy).toHaveBeenCalledTimes(2);
356+
expect(warnSpy()).toHaveBeenCalledTimes(2);
433357
});
434-
expect(String(warnSpy.mock.calls[0]?.[0])).toContain(
358+
expect(String(warnSpy().mock.calls[0]?.[0])).toContain(
435359
"derivations[1].aggregate.init",
436360
);
437-
expect(String(warnSpy.mock.calls[1]?.[0])).toContain(
361+
expect(String(warnSpy().mock.calls[1]?.[0])).toContain(
438362
"derivations[1].aggregate.merge",
439363
);
440364
});
@@ -447,7 +371,7 @@ describe("an invalid derivations update is rejected, not fatal", () => {
447371

448372
view.rerender(groupedElement({ columns: invalidColumns("nonsense") }));
449373
await waitFor(() => {
450-
expect(warnSpy).toHaveBeenCalledTimes(1);
374+
expect(warnSpy()).toHaveBeenCalledTimes(1);
451375
});
452376

453377
/*
@@ -456,15 +380,15 @@ describe("an invalid derivations update is rejected, not fatal", () => {
456380
* fault. Key on the raw `path` and this re-fires; the index-stripping is
457381
* what keeps a reorder from re-reporting a fault already reported.
458382
*/
459-
const beforeMove = setDerivationsCallCount;
383+
const beforeMove = setDerivations.callCount();
460384
view.rerender(
461385
groupedElement({ columns: invalidColumnsReordered("nonsense") }),
462386
);
463387
await waitFor(() => {
464388
expect(techAggregateText(view.container)).toBe("30");
465389
});
466-
expect(setDerivationsCallCount).toBeGreaterThan(beforeMove);
467-
expect(warnSpy).toHaveBeenCalledTimes(1);
390+
expect(setDerivations.callCount()).toBeGreaterThan(beforeMove);
391+
expect(warnSpy()).toHaveBeenCalledTimes(1);
468392
});
469393

470394
test("a DIFFERENT invalid value still warns — the key is not a constant", async () => {
@@ -475,7 +399,7 @@ describe("an invalid derivations update is rejected, not fatal", () => {
475399

476400
view.rerender(groupedElement({ columns: invalidColumns("nonsense") }));
477401
await waitFor(() => {
478-
expect(warnSpy).toHaveBeenCalledTimes(1);
402+
expect(warnSpy()).toHaveBeenCalledTimes(1);
479403
});
480404

481405
/*
@@ -486,9 +410,9 @@ describe("an invalid derivations update is rejected, not fatal", () => {
486410
*/
487411
view.rerender(groupedElement({ columns: invalidColumns("bogus") }));
488412
await waitFor(() => {
489-
expect(warnSpy).toHaveBeenCalledTimes(2);
413+
expect(warnSpy()).toHaveBeenCalledTimes(2);
490414
});
491-
expect(String(warnSpy.mock.calls[1]?.[0])).toContain("bogus");
415+
expect(String(warnSpy().mock.calls[1]?.[0])).toContain("bogus");
492416
});
493417

494418
test("a valid update after a rejected one still lands", async () => {

0 commit comments

Comments
 (0)