Skip to content

test: centralize stream and extension lifecycle fixtures - #412

Closed
steipete wants to merge 2 commits into
codex/sidepanel-state-ownerfrom
codex/shared-test-fixtures
Closed

test: centralize stream and extension lifecycle fixtures#412
steipete wants to merge 2 commits into
codex/sidepanel-state-ownerfrom
codex/shared-test-fixtures

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Owner

What changed

Replace 65 copied Writable stream factories across 60 CLI/daemon test files with two shared helpers. Move the ordinary browser-extension harness lifetime into a lazy, test-scoped Playwright fixture for 84 tests; each requesting test still gets a fresh profile, runtime-error checks, and guaranteed cleanup.

Live prerequisites, custom/native launchers, and multi-resource cleanup remain explicit. No production code or behavioral assertions were removed. An AST check confirms all 87 scenario names and direct expectation counts in the 14 migrated browser files are unchanged. Three focused tests cover stream capture, isolation, and discard behavior.

Net reduction: 1,450 lines. Most of the displayed browser diff is indentation from removing repeated try/finally blocks.

Proof

  • Full project gate passes: formatting, lint, core/CLI type checks, and 3,073 tests (43 skipped); coverage remains above all thresholds.
  • Supported Chromium command: all three native/local-browser scenarios pass; the HTTP run passes 108 scenarios with seven explicit skips and four sidepanel-startup timeouts. Both affected slide suites then pass unchanged: all 12 scenarios, including all four failures. No timeout or assertion was relaxed.
  • Independent Codex review: no actionable P0–P2 findings across the complete diff.

This is test infrastructure only; the packaged UI is unchanged. Stacked on #411; land only after its parent and this PR's hosted CI are green.

@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 4, 2026, 11:43 PM ET / September 5, 2026, 03:43 UTC.

ClawSweeper review

What this changes

Consolidates CLI and daemon test output streams and browser-extension setup and cleanup into shared helpers, adds three helper tests, and records the cleanup in the changelog.

Merge readiness

Blocked before merge - 2 items remain

Keep open: the fixture cleanup remains unlanded, and repository policy protects owner-authored work. No actionable correctness or security defect was found.

Priority: P3
Reviewed head: 36181b374a319c8fdb3f7d5ddef73d796642921d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, maintainable consolidation with preserved assertions and no actionable findings.
Proof confidence 🌊 off-meta tidepool Not applicable: Owner-authored test infrastructure is exempt from contributor runtime proof; the reported Chromium runs exercise the migrated lifecycle, and no production authority boundary changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Owner-authored test infrastructure is exempt from contributor runtime proof; the reported Chromium runs exercise the migrated lifecycle, and no production authority boundary changes.
Evidence reviewed 10 items Repository policy: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-notes directory was found. Applied the supported Chromium and diagnostic-only Firefox guidance.
Verified patch boundary: The exact head records fetched main as its parent. Comparing that parent with head isolates 78 files: 77 test files and one changelog entry. Production code, dependencies, manifests, and workflows are unchanged; the sidepanel refactor belongs to the merged parent.
Browser lifecycle preserved: The test-scoped fixture retains the Firefox opt-in guard, invokes the existing fresh-profile launcher, checks runtime errors after the scenario, and closes the extension in finally. Specialized native and live-resource launchers remain explicit.
Findings None None.
Security None None.

How this fits together

These helpers support Summarize’s regression tests: CLI tests capture command output, while extension tests exercise browser scenarios in isolated profiles. Their results feed assertions and runtime-error checks.

flowchart TD
  A[Regression tests] --> B[Shared stream helpers]
  B --> C[CLI output assertions]
  A --> D[Per-test browser fixture]
  D --> E{Browser enabled?}
  E -->|Yes| F[Fresh profile and scenario]
  F --> G[Runtime checks and cleanup]
  E -->|No| H[Explicit skip]
Loading

Before merge

  • Resolve merge risk (P1) - Captured GitHub metadata still targets the deleted stacked base and reports a dirty merge; standalone landing is not validated.
  • Complete next step (P2) - Complete the owner-designated landing through refactor: land reviewed architecture cleanup stack #415 and reconcile this PR after that integration merges.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code footprint Production +0/-0; tests +5,460/-6,911; changelog +1 The net reduction is 1,450 lines without production growth.
Browser scenario preservation 84 fixture migrations; 87 scenario names and 445 expectation calls unchanged Source counts corroborate preservation of the existing browser assertions.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Keep test lifecycle ownership centralized while preserving isolated profiles, output captures, runtime checks, and explicit specialized launchers.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep test lifecycle ownership centralized while preserving isolated profiles, output captures, runtime checks, and explicit specialized launchers.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR consolidates existing test infrastructure rather than reporting a product bug; its behavior-preservation claims were checked in source.

Is this the best way to solve the issue?

Yes: extracting the identical streams and ordinary browser lifecycle is a focused approach that preserves assertions and leaves specialized resource handling explicit.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6e20e4493ffa.

Labels

Label justifications:

  • P3: This is test-infrastructure cleanup with no introduced production behavior change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Owner-authored test infrastructure is exempt from contributor runtime proof; the reported Chromium runs exercise the migrated lifecycle, and no production authority boundary changes.

Evidence

What I checked:

  • Repository policy: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-notes directory was found. Applied the supported Chromium and diagnostic-only Firefox guidance. (AGENTS.md:1, 36181b374a31)
  • Verified patch boundary: The exact head records fetched main as its parent. Comparing that parent with head isolates 78 files: 77 test files and one changelog entry. Production code, dependencies, manifests, and workflows are unchanged; the sidepanel refactor belongs to the merged parent. (36181b374a31)
  • Browser lifecycle preserved: The test-scoped fixture retains the Firefox opt-in guard, invokes the existing fresh-profile launcher, checks runtime errors after the scenario, and closes the extension in finally. Specialized native and live-resource launchers remain explicit. (apps/chrome-extension/tests/helpers/extension-fixtures.ts:11, 36181b374a31)
  • Stream behavior preserved: The shared helpers preserve the removed Writable implementations. Three added tests cover ordered string/buffer capture, independent captures without invented terminal properties, and normal completion of discarded output. (tests/helpers.streams.test.ts:5, 36181b374a31)
  • Scope and scenario preservation: Read-only source comparison confirms 65 removed stream factories across 60 existing test files, 84 migrated browser launches, and unchanged names and direct expectation-call counts for all 87 scenarios across 14 browser files. This was source inspection, not test execution. (36181b374a31)
  • Still absent from main and latest release: Fetched main retains copied stream factories and explicit browser lifecycles. Both new shared helper paths are absent from fetched main and v0.21.11, whose release commit is 1dcbff7. (tests/cli-main.integration.test.ts:8, 6e20e4493ffa)

Likely related people:

  • Peter Steinberger: Raw commit ec28bb6 adds tests/cli-main.integration.test.ts:47 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: ec28bb678fe3; files: tests/cli-main.integration.test.ts)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-05T03:00:36.241Z sha 42c84fa :: needs changes before merge. :: none

@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Landed through #415 in 8308778. The resulting main tree was verified byte-for-byte against the complete reviewed stack (33b957d), with green combined review, Node 24, Chromium E2E, Firefox smoke, and security checks. Closing this constituent PR as superseded; its changes and review history are preserved by the integration.

@steipete steipete closed this Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant