test(cli): add node-pty E2E suite for the stash CLI#370
Conversation
Adds a second Vitest config (`vitest.integration.config.ts`) that drives the built `dist/bin/stash.js` through a real pseudo-terminal via `node-pty`, so we can cover @clack/prompts flows that piped-stdin mocks can't reach (raw mode, escape sequences, cancellation). - `tests/helpers/pty.ts` exposes `render`/`waitFor`/`key`/`exit` and auto-fixes the exec bit pnpm strips from node-pty's macOS spawn-helper prebuild. - `tests/e2e/smoke.e2e.test.ts` covers help/version/unknown-command exits and the `db migrate` stub (7 tests). - `tests/e2e/auth-login-cancel.e2e.test.ts` waits for the region `p.select` prompt and asserts ctrl-c cancels cleanly (1 test). - Default `vitest.config.ts` now excludes `tests/e2e/**` so `pnpm --filter @cipherstash/cli test` stays fast and build-free. - New `test:e2e` package script and matching turbo task that depends on `^build` and `build`. - `packages/cli/AGENTS.md` documents when to add/update E2E tests, the harness API, and the gotchas (build-first, spawn-helper chmod, prompt string stability). Root `AGENTS.md` gets a one-line pointer. - `docs/plans/cli-pty-integration-tests.md` captures the rationale and the planned phase-2 messages-handle refactor.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces a node-pty-backed end-to-end test framework for the CLI package. It adds test configuration files, a PTY-based test helper for spawning the built CLI in a pseudo-terminal environment, initial smoke and auth-interaction test files, and comprehensive documentation explaining the testing infrastructure and contributions workflow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 54 seconds.Comment |
Adds a step after the existing `pnpm test` that runs `turbo run test:e2e --filter @cipherstash/cli`. Going via turbo honors the `^build` and `build` deps declared on the task so the CLI binary is built before E2E runs. The runner is Linux x64 / Node 20 — node-pty 1.1.0 ships prebuilds for that combo, but if resolution misses we'll see a node-gyp fallback build (Blacksmith Ubuntu 24.04 has the toolchain).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/cli/tests/helpers/pty.ts (1)
125-129: Consider resetting regexlastIndexto prevent potential issues with stateful patterns.The
matchesfunction on line 127–128 callsmatch.test(s)which mutateslastIndexfor regexes with/gor/yflags. While currently allwaitForcalls pass strings, this could be a source of subtle bugs if regex literals with stateful flags are passed in the future. Defensively resettinglastIndex = 0before callingtest()ensures predictable behavior:Suggested patch
const waitFor = async (match: string | RegExp, timeoutMs = 5_000) => { const deadline = Date.now() + timeoutMs - const matches = (s: string) => - typeof match === 'string' ? s.includes(match) : match.test(s) + const matches = (s: string) => { + if (typeof match === 'string') return s.includes(match) + match.lastIndex = 0 + return match.test(s) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/helpers/pty.ts` around lines 125 - 129, The matches helper inside waitFor can call match.test(s) which mutates RegExp.lastIndex for stateful regexes; before invoking test, detect when match is a RegExp and reset match.lastIndex = 0 to avoid surprising stateful behavior — update the matches function inside waitFor (referencing the match variable and matches function and leaving stripAnsi(raw) usage unchanged) to clear lastIndex on RegExp inputs before calling test().packages/cli/tests/e2e/smoke.e2e.test.ts (1)
30-36: ⚡ Quick winPrefer stable unknown-command substrings in E2E assertions
At Line 34, Line 50, and Line 57, the checks currently lock onto full phrasing. To reduce churn from copy tweaks, assert on stable fragments (
Unknown command,Unknown auth command,Unknown db subcommand) plus the subcommand token separately.Suggested diff
- expect(r.output).toContain('Unknown command: definitely-not-a-command') + expect(r.output).toContain('Unknown command') + expect(r.output).toContain('definitely-not-a-command') @@ - expect(r.output).toContain('Unknown auth command: bogus-sub') + expect(r.output).toContain('Unknown auth command') + expect(r.output).toContain('bogus-sub') @@ - expect(r.output).toContain('Unknown db subcommand: bogus-sub') + expect(r.output).toContain('Unknown db subcommand') + expect(r.output).toContain('bogus-sub')As per coding guidelines:
packages/cli/tests/e2e/**/*.e2e.test.{ts,js}— “In E2E tests, prefer asserting on stable prompt substrings rather than full prompt strings to survive copy changes and formatting variations.”Also applies to: 46-51, 53-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/e2e/smoke.e2e.test.ts` around lines 30 - 36, The assertions in the smoke E2E test lock onto full error messages; update them to assert on stable substrings plus the token separately: for the test using render(...) and r.output, replace expect(r.output).toContain('Unknown command: definitely-not-a-command') with two assertions—expect(r.output).toContain('Unknown command') and expect(r.output).toContain('definitely-not-a-command')—and do the same pattern for the other failing checks that reference 'Unknown auth command' and 'Unknown db subcommand' so they assert the stable phrase and the specific subcommand token separately while keeping the existing exitCode check on r.exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/cli-pty-integration-tests.md`:
- Line 43: Update the plan doc so the cancel flow refers to the implemented
`auth login` scenario instead of `stash init`: change the table row label "init
cancel" and the command cell containing `stash init` to reflect `auth login`,
and update the related sequencing and deliverables text blocks (including the
sections around the table and the lines referenced at ~68-70 and ~203-206) to
describe cancelling `auth login` and expected output (e.g., exit 0, output
contains "Setup cancelled."). Ensure all instances of `stash init` in this doc
are replaced or aligned with `auth login` to match the tests.
In `@packages/cli/tests/helpers/pty.ts`:
- Around line 99-103: The test PTY environment sets CI to '1' but the CLI's
non-interactive guard in packages/cli/src/commands/db/install.ts checks
process.env.CI !== 'true'; update the env object in
packages/cli/tests/helpers/pty.ts (the object containing NO_COLOR, FORCE_COLOR,
CI, ...(opts.env ?? {})) to set CI: 'true' so the test process runs in the same
non-interactive CI mode, ensuring any overrides in opts.env still apply.
---
Nitpick comments:
In `@packages/cli/tests/e2e/smoke.e2e.test.ts`:
- Around line 30-36: The assertions in the smoke E2E test lock onto full error
messages; update them to assert on stable substrings plus the token separately:
for the test using render(...) and r.output, replace
expect(r.output).toContain('Unknown command: definitely-not-a-command') with two
assertions—expect(r.output).toContain('Unknown command') and
expect(r.output).toContain('definitely-not-a-command')—and do the same pattern
for the other failing checks that reference 'Unknown auth command' and 'Unknown
db subcommand' so they assert the stable phrase and the specific subcommand
token separately while keeping the existing exitCode check on r.exit.
In `@packages/cli/tests/helpers/pty.ts`:
- Around line 125-129: The matches helper inside waitFor can call match.test(s)
which mutates RegExp.lastIndex for stateful regexes; before invoking test,
detect when match is a RegExp and reset match.lastIndex = 0 to avoid surprising
stateful behavior — update the matches function inside waitFor (referencing the
match variable and matches function and leaving stripAnsi(raw) usage unchanged)
to clear lastIndex on RegExp inputs before calling test().
🪄 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
Run ID: 84abd26a-e423-403a-994b-b75a9d6a9e8f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
AGENTS.mddocs/plans/cli-pty-integration-tests.mdpackages/cli/AGENTS.mdpackages/cli/package.jsonpackages/cli/tests/e2e/auth-login-cancel.e2e.test.tspackages/cli/tests/e2e/smoke.e2e.test.tspackages/cli/tests/helpers/pty.tspackages/cli/vitest.config.tspackages/cli/vitest.integration.config.tsturbo.json
- Add `node-pty` to root `pnpm.onlyBuiltDependencies` so its install
script runs on the Linux CI runner. pnpm 10 blocks lifecycle scripts
by default, which is why the previous run failed loading
`prebuilds/linux-x64/pty.node` — node-pty doesn't ship a Linux
prebuild, so the source build via node-gyp must be allowed.
- `tests/helpers/pty.ts`: set `CI: 'true'` (not '1') to match the
convention used by `commands/db/install.ts:502`.
- `tests/helpers/pty.ts`: reset `match.lastIndex = 0` before
`match.test()` in `waitFor` so stateful regex flags (`/g`, `/y`)
don't drift across calls.
- `tests/e2e/smoke.e2e.test.ts`: split full error strings into stable
phrase + token assertions ("Unknown command" / "definitely-not-a-
command" etc.) so wording tweaks don't break tests.
- `docs/plans/cli-pty-integration-tests.md`: update the cancel-test
references to match the implemented `auth login` flow rather than
the originally-planned `stash init`.
2 participants
Summary
Adds a pty-driven integration test suite for
@cipherstash/cliso we can cover@clack/promptsflows (raw mode, escape sequences, cancellation) that piped-stdin mocks can't reach. Drives the builtdist/bin/stash.jsthrough a real pseudo-terminal vianode-pty.vitest.integration.config.ts(forks pool, 30s timeout) andtest:e2escript + turbo task that depend onbuild.vitest.config.tsnow excludestests/e2e/**so the existingpnpm teststays fast and build-free.tests/helpers/pty.ts— smallrender/waitFor/key/exitwrapper. Auto-fixes the exec bit pnpm strips from node-pty's macOS spawn-helper prebuild.db migratestub) + 1 interactive cancel test (auth login→ ctrl-c at the region prompt).packages/cli/AGENTS.mddocuments when to add/update E2E tests, the harness API, and the gotchas. RootAGENTS.mdgets a one-line pointer.docs/plans/cli-pty-integration-tests.mdcaptures the rationale, alternatives considered, and a planned phase-2 messages-handle refactor.Why node-pty and not a mock
@clack/promptsswitches stdin to raw mode and renders differently when stdout isn't a TTY.mock-stdinandcli-testing-librarydon't drive a real pty so the high-fidelity prompt flows (select/multiselect, ctrl-c) aren't exercised.expect(1)adds a Tcl runtime and lives outside Vitest.node-ptykeeps tests inside Vitest with no second toolchain.Test plan
pnpm --filter @cipherstash/cli test— 76 unit tests pass, ~400ms, no e2e bleed-throughpnpm --filter @cipherstash/cli test:e2e— 8 e2e tests pass, ~1s, all spawning the built binary through a real ptypnpm --filter @cipherstash/cli build && pnpm --filter @cipherstash/cli test:e2elocallySummary by CodeRabbit
Release Notes
New Features
Tests
Documentation
Chores