Skip to content

Reduce terminal test overhead while preserving full parity checks - #25151

Merged
nwparker merged 3 commits into
mainfrom
nwparker/ci-terminal-test-oracles-20261004
Oct 4, 2026
Merged

nwparker merged 3 commits into
mainfrom
nwparker/ci-terminal-test-oracles-20261004

Conversation

@nwparker

@nwparker nwparker commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​339 $\color{#cf222e}{\Huge{\mathbf{−}}}$​116 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​223
Prod 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​214 $\color{#cf222e}{\Huge{\mathbf{−}}}$​35 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​179

ELI5

Terminal regression tests spend less time allocating cell descriptions and waiting between input chunks. Three hosted comparisons retain their meaningful checks and reduce complete focused invocations by 25.6–77.5%.

What Changed

Compare serializer cells in place and format the first differing row for diagnostics. Queue every original parity-test chunk in order and await the final parser callback. Memoize the independent legacy frame oracle's byte measurements. Remove its algebra-only case, which never called production.

Why

These are measured costs in our slowest CI tests. Reusing scratch cells and parser scheduling preserves seeds, chunk boundaries, full buffers and failure detection. The frame case we removed still passed when its production generator always threw.

Linked Issue

N/A — internal CI optimization.

Visual Proof

N/A — test support only; application behavior is unchanged.

Testing

  • Local focused tests, controls, lint and formatting passed.
  • Added independent stream/cell and callback/parser controls.
  • Three alternating one-worker hosted ARM pairs: serializer median 71.675 → 46.581s, parity workload 24.095 → 5.411s, frame equivalence 18.472 → 13.736s.
  • Serializer retained 116 timed passes; separate complete captures matched all 190,796,645 bytes over 1,611 scenarios and 8,617 checkpoints. Twenty candidate controls and visual-state fault controls passed. Serializer run.
  • Parity captures matched every original chunk and terminal/buffer/style/serialization result across 2,325 batches, 28,182 chunks and 1,698,285 input bytes. All 73 controls/other-consumer tests passed; four ordering/completion faults failed. Frame retained 4,000 short and 800 near-cap trials, matched the original uncached oracle, and detected four production faults. Parity/frame run.
  • Hosted Node typecheck passed. Final-head required CI must pass before merge. These measurements do not establish whole-shard duration or queue improvements.

Review

Cell comparison retains the original effective-color/blank/clipped policy and exact first-mismatch diagnostics. Scratch cells stay local. The parity helper waits through asynchronous parser continuation; independent terminal completion order is handled only when comparing capture records.

Agent skill upstream boundary

  • Not applicable; no upstream skill source is copied.

Checklist

  • Focused test-support performance changes.
  • Explained before/after, mechanism and preservation of meaningful coverage.
  • N/A visual proof has a reason.
  • Reviewed full capture parity and negative controls.
  • No platform or SSH execution behavior changes.
  • Focused tests, lint, format and hosted Node typecheck passed; CI covers the final head.

@nwparker
nwparker marked this pull request as ready for review October 4, 2026 07:07
@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3d6b53f6-ff1c-402a-87f8-43a15ca99096
📥 Commits

Reviewing files that changed from the base of the PR and between 3cc5e1d and 4ec6438.

📒 Files selected for processing (7)
  • docs/reference/ci-runner-efficiency.md
  • src/main/daemon/serialize-grid-cell-descriptors.test.ts
  • src/main/daemon/serialize-grid-cell-descriptors.ts
  • src/main/daemon/serialize-grid-roundtrip.ts
  • src/main/runtime/rpc/terminal-output-frame-chunks-equivalence.test.ts
  • src/shared/terminal-restore-parity-fixture.ts
  • src/shared/terminal-restore-parity-writes.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The terminal oracle now compares buffer ranges directly and formats only the first differing row. Parity tests queue terminal chunks in FIFO order and wait for parser completion. The frame-chunking reference caches measured byte lengths for repeated code points. A report records hosted timing comparisons and parity evidence.

Priority: ⬇️ Low

Estimated code review effort:

Merge Risk: ⚪ Minimal · up to 4ec64

No merge-blocking issue is established for these test-support changes. Merge after the required CI checks pass.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 6 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: reducing terminal test overhead while preserving parity checks.
Description check ✅ Passed The description covers the required change, rationale, testing, visual-proof status, review, and checklist sections. It explains that no linked issue is needed because this is an internal optimization…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 6 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • In-place grid comparison — serialize-grid-cell-descriptors.ts replaces the bufferRows + compareRowSets string-array path with compareBufferRows, which reuses one scratch Cell per buffer, trims trailing blank rows, and formats only the first differing row. A sameCellFields fast path skips descriptor construction when public cell getters agree and falls back to the frozen cellDescriptor policy on any difference or clipping.
  • Callers migrated — serialize-grid-roundtrip.ts updated to the new 8-argument signature; the old exports have no remaining consumers.
  • Batched parity writes — writeChunksToTerminal queues every chunk and resolves on the final write callback instead of awaiting each chunk.
  • Frame oracle memoization — the legacy splitter caches pure per-part byte counts; an algebra-only test that never invoked production was deleted.
  • Tests — the serializer tests now validate compareBufferRows against the frozen allocating oracle with flag-loss / blank / clipped / color-policy controls, and a new file covers the batched writer (FIFO order, empty batch, async OSC barrier, serial equivalence).

The fast path was traced field-by-field against cellDescriptor (including blank-cell, inverse-foreground, and clipping gates) and the installed @xterm/headless cell semantics — no case where sameCellFields returns true while the descriptors differ. The deleted frame test was genuinely tautological: reaching the chunk loop already guarantees rawLength === data.length, making shouldDelayFinalSeq unreachable. The new tests can fail: a fast path that missed a real difference would return null while expectComparisonParity asserts equality against the non-null frozen oracle diff.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker merged commit 2b3b692 into main Oct 4, 2026
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant