Reduce browser fixture overhead and establish behavior-based test layers - #63
Conversation
…esting-library/dom@10 @testing-library/user-event@14.6.7 @testing-library/jest-dom@7.0.1 jsdom@30.0.1 Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Verdict
No material correctness, isolation, or coverage-regression findings in the reviewed diff. This is a comment review, not approval or a claim that all merge gates have completed.
Reviewed head: 9f59272a2f2accd5fe46e54784011963188e817c
Reviewed base / merge base: 7ce898eb6f77ca4fb95f06a7c7789f63245dee28.
Review scope
- Verified that the five changed browser specs retain their literal test declarations and all 338 normalized assertion expressions. Traced their changed setup and consumers rather than relying on counts: only stateless source serving/cache is worker-scoped; browser contexts, route interception, identities, relay state, and mutable app fixtures remain test-owned. Scale/scroll history defaults remain 640/80; smaller histories are opted into only by Settings and navigation-authority cases.
- Mapped the former composer and emoji assertions to the real React Strict Mode/RTL replacement and extracted Node tests. Checked channel/thread sends, recipient identity, rejection/retry, draft retargeting/remounting, subscription disposal, stale tool commands, emoji serialization, and edit limits. Native editing/layout coverage remains in Playwright; jsdom is not used as evidence for those contracts.
- Checked dependency/lockfile compatibility and the test-layer documentation. No production-source delta against the reviewed base;
git diff --checkpassed for those explicit SHAs. Source review only locally: I did not rerun broad CI-equivalent suites or repeat the author's mutation experiment.
Validation provenance and remaining gates
Current-head CI run executes synthetic merge f0d70e6fb9806792617b7dd0e756609866d1f58d, combining this head with main 8925c92b8837f4709343c4eb65f834829b086a84. I compared that merge to the PR head: its only additional changes are the inline-reference styling fix and its composer-link fixture/test. This is integration-merge evidence, not literal exact-head execution.
The JavaScript job passed 147 files / 1,549 tests, including 15 mounted composer tests and 2 extracted emoji tests. Rust/tool integration, Windows notification compilation/tests, browser measurements, DCO, and security checks also report success. At my last check, all four functional browser shards were still running; no complete current-run browser pass is claimed.
The updated PR description reports exact-head focused checks and a running full local scan; I have not independently verified that scan's completion. The three documented local-only WebKit measurement cases and remaining required local/native/platform checks are not waived by hosted CI. Older-head timing and suite totals in the description remain historical evidence, not current-head measurements.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: no actionable defects found
Reviewed head 9f59272a2f2accd5fe46e54784011963188e817c against base 7ce898eb6f77ca4fb95f06a7c7789f63245dee28. The contract is lower fixture overhead without production behavior changes or loss of existing browser assertions, failure paths, or engine coverage.
- Traced the mounted composer tests through real React StrictMode, draft persistence and retargeting, synchronous rejection/recovery, disabled/read-only behavior, exact mention recipients, stale tool callbacks, and emoji subscription cleanup. The extracted pure emoji assertions remain discovered by the existing Node/Vitest glob; jsdom is confined to the composer file, not a replacement for browser editing/layout coverage.
- Compared all five changed browser files with the exact base: declarations and assertion sequences remain intact. Profile/mention/typeahead changes move stateless source serving to a worker-owned server while Playwright retains fresh per-test contexts. The existing optimizer helper owns separate caches and cleanup. Settings and navigation-authority do not consume the removed histories; large-history defaults and measurement datasets remain unchanged.
- Checked the dependency/discovery boundary and full changed-file inventory. Added dependencies are development-only with lockfile entries; production application source, browser engine selection, retries, and timing budgets are unchanged. Independent composer and fixture reviews were integrated with this end-to-end check.
Validation limits: source/metadata-only review on the authorized laptop; no PR code was checked out or executed, and no tests were run locally. The one-shot check of PR CI associated with this head showed successful JavaScript, Windows and browser-measurement jobs, but other lanes were not yet complete. Check-rollup metadata is not evidence of literal head-only execution; earlier timing/test claims are not reattributed to this head. This is not approval, a measured-speedup certification, or native/live-account acceptance; required CI and human/code-owner approval remain separate gates.
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
* origin/main: Reduce browser fixture overhead and establish behavior-based test layers (#63) Signed-off-by: Clay Delk <clay.delk@gmail.com>
September 15 conflict resolution — current status
🤖 Conflicts resolved by merging main, retaining mounted RTL coverage and rich-editor DOM semantics. The final tree matches the green merge build in run 34983091405, verified by Git tree IDs. The final commit includes #74 so stacked children inherit its typing-fixture fix. TypeScript and all 1,549 Vitest tests passed through the unchanged push hook; 17 targeted unit checks and 32 affected browser cases passed. The earlier local scan ran before #74: 387 browser cases passed and three failed (composer-links in both engines, fixed by #74; WebKit emoji bounds, reproduced on unchanged earlier main). The scan stopped before its remaining recipes; no full local scan pass is claimed. Fresh final-head CI is running; DCO passed. No production-code delta.
Behavior and scope
Reduce unnecessary browser fixture setup and establish behavior-based test-layer governance in AGENTS.md. Mount the composer with React Testing Library and jsdom rather than a substitute React lifecycle. Keep pure emoji assertions in Node Vitest. No production behavior changes, removed browser cases, or PR-template changes.
Source-only typeahead/profile/mention-edit journeys reuse a stateless Vite server and isolated optimizer per worker. Browser contexts remain fresh. Settings and navigation-authority fixtures generate only their required histories; scale/scroll datasets remain unchanged.
Measured cost
Local macOS arm64, pinned Node 24.18.0 / Playwright 1.60.0, two workers, Chromium and WebKit. Baseline is main at 3f824df; after is this branch's test changes. One diagnostic sample each, not a hosted CI claim.
Command:
bin/pnpm test:browser settings.spec.mjs navigation-authority.spec.mjs typeahead.spec.mjs profiles.spec.mjs mention-edit.spec.mjs --project chromium --project webkit --no-deps --reporter=json.No test cases, assertions, engine coverage, or failure paths were removed to achieve this improvement. No retries added. Original CI context: https://github.com/block/buzz-app/actions/runs/34883211270/job/104107327638?pr=53 (a different snapshot, not the timing baseline).
Verification and remaining gates
just scanclaim.🤖 AI-assisted implementation and PR description.
Hosted CI evidence — September 14
🤖 CI verification: this completed run at PR head, compared with main baseline at baseline head. All required CI lanes, DCO and security checks passed. No PR has merged yet; post-merge main verification remains pending.
Coverage: 364 unique functional browser cases passed (182 per engine), five CI-selected measurement cases passed, and 1,377 Vitest tests passed. Runner totals contain no skipped or flaky cases. Test identities were compared across logs, including colocated workflow journeys; #64's two intentional title changes were normalized. Three documented local-only WebKit measurement cases remain excluded from CI, unchanged. Measurement artifacts retain the existing narrow WebKit ResizeObserver warning exception; no broader allowance was added.
Times are Actions step durations, not summed test durations. The overall interval includes setup/build and scheduling between jobs but excludes initial queue time. Linux lanes use the pinned Ubuntu 24.04 workflow; runner/cache variation and workload distribution still affect comparisons.
Shard allocation changed from 92/90 to 95/87 tests per engine, although the complete test identity set is unchanged; individual-shard deltas are not directly attributable to test speed. The five changed files total approximately 263.7s → 170.3s summed test execution across both engines (rounded runner-log durations). Browser-engine cache hits on baseline became misses because the lockfile changed the cache key; native fixture builds remain 13–18s versus 14–19s on baseline. One hosted sample does not establish the 52% local elapsed improvement across the whole CI lane.