Remove the end-to-end test suite - #133
Merged
Merged
Conversation
The Playwright/PTY e2e harness (tests/e2e) carried real infrastructure cost — a compiled cyrusd binary rebuilt per run, wrangler dev + local D1 + Vite orchestrated through process-compose, a PTY driver, Playwright browsers — but only ever ran on manual workflow_dispatch, never on PRs or main, and never reached the stable pass history its own docs said cron was waiting on. That's pure maintenance surface with no day-to-day safety net, so it's removed along with the nightly workflow that ran it and the process-compose/shell-use tooling that only existed to run it. tooling/test keeps the two pieces genuinely shared outside tests/e2e (cyrus-home.ts, used by apps/cli's own tests; auth-env.ts, used by the root @cyrus/server Vitest project) and drops everything else that was e2e-only. See docs/adr/0019-remove-end-to-end-test-suite.md for the full record, including which OpenSpecs lose cross-process/browser coverage as a result and how to recover the removed harness from git history if a future need re-justifies its cost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request removes the E2E test suite and nightly workflow, updates test execution configuration and documentation, adds shared test fixtures and authentication bindings, and migrates selected CLI tests to shared temporary-home utilities. ChangesE2E retirement and test execution updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0019-remove-end-to-end-test-suite.md`:
- Line 3: Correct the decision date in ADR 0019’s opening metadata to reflect
the actual decision date, ensuring it is not later than the review date; update
the superseding-reference text only if needed to keep the ADR metadata
consistent.
In `@docs/guides/TESTING_FRAMEWORK.md`:
- Line 67: Update the deployment smoke-test documentation to reference the
environment variables consumed by tooling/test/smoke/deploy.ts:
SMOKE_BEARER_TOKEN and SMOKE_ROOM_ID, replacing DEPLOY_SMOKE_TOKEN and
DEPLOY_SMOKE_ROOM_ID while preserving the described optional WebSocket check.
In `@docs/guides/VERIFY_LOOP.md`:
- Line 87: The verification guide still references the deleted “canonical
harness flow” and documents an unactionable email/password path. Update the
relevant section of VERIFY_LOOP to remove that harness reference and describe
the surviving authentication sequence, including sign-up/sign-in, device claim,
and approval, or link to an existing document that covers those concrete steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cad63df7-311e-445a-bc23-ab52a9fdce26
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
.github/workflows/nightly.ymlapps/cli/__tests__/integration/wiring.test.tsapps/cli/package.jsonapps/cli/src/commands/service/status.test.tsapps/cli/src/store/health.test.tsbiome.jsondocs/adr/0017-vitest-default-bun-only-cli-desktop.mddocs/adr/0019-remove-end-to-end-test-suite.mddocs/guides/TESTING_FRAMEWORK.mddocs/guides/VERIFY_LOOP.mdknip.jsonmise.tomlpackage.jsontests/e2e/harness/auth.tstests/e2e/harness/cli-doctor-terminal.test.tstests/e2e/harness/cli-login-terminal.test.tstests/e2e/harness/cli-login.test.tstests/e2e/harness/cli-login.tstests/e2e/harness/cli-service-terminal.test.tstests/e2e/harness/cli-worker.tstests/e2e/harness/database.test.tstests/e2e/harness/database.tstests/e2e/harness/dev-servers.tstests/e2e/harness/env.tstests/e2e/harness/process-compose.test.tstests/e2e/harness/process-compose.tstests/e2e/harness/process.tstests/e2e/harness/shell-use.tstests/e2e/harness/stack.tstests/e2e/package.jsontests/e2e/process-compose.yamltests/e2e/tsconfig.jsontests/e2e/web/device-auth.tstests/e2e/web/fixtures.tstests/e2e/web/helpers.tstests/e2e/web/playwright.config.tstests/e2e/web/prepare-database.tstests/e2e/web/specs/catalog.spec.tstests/e2e/web/specs/cold-resume.spec.tstests/e2e/web/specs/smoke.spec.tstests/e2e/web/specs/thread-lifecycle.spec.tstests/e2e/web/specs/thread-sync.spec.tstests/e2e/web/specs/worker-connects.spec.tstooling/test/fixtures/cyrus-home.tstooling/test/mocks/auth-env.tstooling/test/mocks/data-channel.tstooling/test/package.jsontooling/test/setup/bun.setup.tsturbo.jsonvitest.config.ts
💤 Files with no reviewable changes (35)
- .github/workflows/nightly.yml
- tests/e2e/harness/cli-login.test.ts
- tests/e2e/package.json
- tests/e2e/harness/cli-service-terminal.test.ts
- tests/e2e/web/specs/smoke.spec.ts
- tests/e2e/web/specs/worker-connects.spec.ts
- tests/e2e/harness/process.ts
- tests/e2e/web/specs/thread-lifecycle.spec.ts
- tests/e2e/web/specs/thread-sync.spec.ts
- tests/e2e/web/specs/cold-resume.spec.ts
- tooling/test/mocks/data-channel.ts
- tests/e2e/web/prepare-database.ts
- tests/e2e/web/specs/catalog.spec.ts
- tests/e2e/web/playwright.config.ts
- tests/e2e/harness/process-compose.test.ts
- tests/e2e/harness/cli-doctor-terminal.test.ts
- mise.toml
- tests/e2e/web/fixtures.ts
- tests/e2e/web/device-auth.ts
- tests/e2e/harness/dev-servers.ts
- tests/e2e/harness/cli-worker.ts
- tests/e2e/web/helpers.ts
- tests/e2e/harness/cli-login.ts
- turbo.json
- tests/e2e/harness/shell-use.ts
- tests/e2e/harness/database.test.ts
- tests/e2e/harness/cli-login-terminal.test.ts
- tooling/test/setup/bun.setup.ts
- tests/e2e/process-compose.yaml
- tests/e2e/harness/auth.ts
- tests/e2e/tsconfig.json
- tests/e2e/harness/process-compose.ts
- tests/e2e/harness/database.ts
- tests/e2e/harness/stack.ts
- tests/e2e/harness/env.ts
…oke secrets VERIFY_LOOP.md still pointed at the deleted "canonical harness flow" section for the manual email/password verification step; inline the actual sign-up/sign-in/device-approve steps instead (already described earlier in the same section). TESTING_FRAMEWORK.md's smoke-test secret names were flagged as mismatching tooling/test/smoke/deploy.ts's env vars. Checked .github/workflows/deploy.yml: DEPLOY_SMOKE_TOKEN/DEPLOY_SMOKE_ROOM_ID are the actual GitHub secret names a maintainer configures, mapped to the script's SMOKE_BEARER_TOKEN/SMOKE_ROOM_ID inside the workflow — the doc was right to lead with the secret names, just needed the mapping spelled out. The ADR 0019 date finding was already withdrawn on the PR (the review timestamp isn't a valid reference point for the decision date). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Addressed the CodeRabbit review (46342c1):
Pre-push hooks (typecheck, knip, syncpack, unit tests) passed on this commit too. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tests/e2ein full: the harness-driven Vitest + shell-use terminal tier (login/start/stop/status/agents doctorover a real PTY) and the Playwright cross-peer browser suite (worker-connects,catalog,thread-lifecycle,thread-sync,cold-resume)..github/workflows/nightly.ymlentirely (all three jobs: the e2e run, build smoke, real-WebRTC check).mise.toml'sprocess-composeandgithub:microsoft/shell-use, the e2e-only files undertooling/test/(fixtures/wrangler-env-file.ts,fixtures/terminal-session.ts,fixtures/cli-worker-state.ts,mocks/cli-worker-config.ts,helpers/process.ts), and the workspace/CI wiring (package.jsonworkspace entry +test:e2escript,turbo.jsontask, thee2eVitest project, relatedknip.json/biome.jsonentries).tooling/test/fixtures/cyrus-home.tsandtooling/test/mocks/auth-env.ts— both are genuinely shared outsidetests/e2e(apps/cli's own unit tests, and the root@cyrus/serverVitest project's Miniflare bindings respectively).docs/guides/TESTING_FRAMEWORK.mdanddocs/guides/VERIFY_LOOP.mdto drop the now-stale e2e/Playwright/nightly content; the manual full-stack verification loop is unaffected and is now the only loop.docs/adr/0019-remove-end-to-end-test-suite.mdrecording the decision (why, what coverage is lost, how to recover the suite from git history if ever re-justified), and links it fromdocs/adr/0017-*.mdas a partial supersession.Why
The suite's infrastructure cost (compiled
cyrusdbinary rebuilt per run,wrangler dev+ local D1 + Vite via process-compose, a PTY driver, Playwright browsers) was carried entirely off the path contributors feel day to day — it only ran on manualworkflow_dispatch, never on PRs ormain, and never reached the "stable pass history" its own docs said cron was waiting on. Full reasoning in the ADR.Test plan
bun run check:types— all packages passbun check:knip— cleanbun check:syncpack— cleanbun run test:unit— 81 passed / 1 skipped (Vitest) + 101 passed (Bun,apps/cli)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit
Testing
Documentation