test(e2e): add outside-in walkthrough of the prisma example README#472
Open
auxesis wants to merge 11 commits into
Open
test(e2e): add outside-in walkthrough of the prisma example README#472auxesis wants to merge 11 commits into
auxesis wants to merge 11 commits into
Conversation
|
📝 WalkthroughWalkthroughAdds an end-to-end Vitest that parses and executes the Prisma example README "Run it" commands (skipping ChangesPrisma README E2E Test Suite
Sequence Diagram(s)sequenceDiagram
participant Vitest as Vitest(Test Runner)
participant Bash as bash
participant DockerCompose as DockerCompose
participant Postgres as Postgres
participant FS as Filesystem
Vitest->>Bash: execute README command (bash -c)
Bash->>FS: read/write contract, migrations, .env
Bash->>DockerCompose: start/stop services
DockerCompose->>Postgres: provide DB service
Vitest->>DockerCompose: docker compose down -v (teardown)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (1)
e2e/tests/prisma-example-readme.e2e.test.ts (1)
66-94: 💤 Low valueRemove unnecessary
asynckeywords.The functions
snapshotTransientOutputs,restoreTransientOutputs, andwipeTransientOutputsare declaredasyncbut contain only synchronous operations (noawaitstatements). Theasynckeyword is unnecessary and can be removed along with thePromise<...>return types.♻️ Simplify function signatures
-async function snapshotTransientOutputs(): Promise<string> { +function snapshotTransientOutputs(): string { const snap = mkdtempSync(join(tmpdir(), 'prisma-readme-e2e-snap-')) // ... } -async function restoreTransientOutputs(snap: string): Promise<void> { +function restoreTransientOutputs(snap: string): void { for (const rel of TRANSIENT_PATHS) { // ... } -async function wipeTransientOutputs(): Promise<void> { +function wipeTransientOutputs(): void { for (const rel of TRANSIENT_PATHS) { // ... }Then remove
awaitfrom the call sites on lines 111-112, 170, if these changes are applied.🤖 Prompt for 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. In `@e2e/tests/prisma-example-readme.e2e.test.ts` around lines 66 - 94, The three functions snapshotTransientOutputs, restoreTransientOutputs, and wipeTransientOutputs are declared async but use only synchronous fs operations; remove the async keyword and change their signatures from Promise<string>/Promise<void> to plain string/void (e.g., function snapshotTransientOutputs(): string { ... }), and update all call sites that currently await them (calls to snapshotTransientOutputs(), restoreTransientOutputs(), wipeTransientOutputs()) to remove the unnecessary await so they are invoked synchronously.
🤖 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 @.github/workflows/prisma-example-readme-e2e.yml:
- Around line 45-46: Replace the unpinned checkout action reference `uses:
actions/checkout@v6` with a pinned commit SHA (e.g., `uses:
actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683`) and add
`persist-credentials: false` under the checkout step to disable credential
persistence; update the SHA to the latest v6.x commit as appropriate while
keeping the `actions/checkout` identifier to locate the step in the workflow.
In `@e2e/tests/prisma-example-readme.e2e.test.ts`:
- Around line 109-159: The beforeAll timeout (600_000ms) is smaller than the sum
of the per-step timeouts used in the setup steps (outcomes.cpEnv,
outcomes.dockerUp, outcomes.pnpmInstall, outcomes.pnpmEmit, outcomes.pnpmPlan,
outcomes.pnpmApply, outcomes.pnpmStart) and may be exceeded if several steps
reach their individual timeouts; increase the beforeAll timeout to 900_000ms (15
minutes) or otherwise raise it to exceed the total step timeouts so the
beforeAll wrapper around snapshotTransientOutputs() and the runStep calls has
sufficient headroom.
- Around line 28-36: The describeSpawnFailure function currently includes raw
result.stdout and result.stderr (and other StepResult fields) in logs;
redact/sanitize sensitive plaintext before joining: in describeSpawnFailure,
sanitize result.stdout and result.stderr (and include the error.message if kept)
by replacing emails (e.g., /\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}\b/i) and
common connection-string patterns (e.g., postgres://, mysql://, mongodb://, and
URI-like host:port credentials) with masked placeholders, or truncate long
outputs to a safe summary, then use those sanitized strings when building
lines.join('\n'); keep the function signature and other fields (result.label,
result.signal, typeof result.status) intact while ensuring no raw plaintext
sensitive data is emitted.
---
Nitpick comments:
In `@e2e/tests/prisma-example-readme.e2e.test.ts`:
- Around line 66-94: The three functions snapshotTransientOutputs,
restoreTransientOutputs, and wipeTransientOutputs are declared async but use
only synchronous fs operations; remove the async keyword and change their
signatures from Promise<string>/Promise<void> to plain string/void (e.g.,
function snapshotTransientOutputs(): string { ... }), and update all call sites
that currently await them (calls to snapshotTransientOutputs(),
restoreTransientOutputs(), wipeTransientOutputs()) to remove the unnecessary
await so they are invoked synchronously.
🪄 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: ce84e65c-6676-4e9d-b346-ae7004f0aff5
📒 Files selected for processing (3)
.github/workflows/prisma-example-readme-e2e.ymle2e/README.mde2e/tests/prisma-example-readme.e2e.test.ts
Comment on lines
+109
to
+159
| describe.skipIf(!authConfigured)('examples/prisma README "Run it" walkthrough', () => { | ||
| beforeAll(async () => { | ||
| snapDir = await snapshotTransientOutputs() | ||
| await wipeTransientOutputs() | ||
|
|
||
| // Step 1: cp .env.example .env | ||
| // Run via `cp` for fidelity to the README; falls back to ENOENT spawn | ||
| // error on Windows runners (we only target Linux/macOS in CI). | ||
| outcomes.cpEnv = runStep('cp .env.example .env', 'cp', ['.env.example', '.env'], { | ||
| timeoutMs: 5_000, | ||
| }) | ||
|
|
||
| // Step 2: docker compose up -d | ||
| outcomes.dockerUp = runStep( | ||
| 'docker compose up -d', | ||
| 'docker', | ||
| ['compose', 'up', '-d', '--wait'], | ||
| { timeoutMs: 180_000 }, | ||
| ) | ||
|
|
||
| // Step 3: pnpm install | ||
| outcomes.pnpmInstall = runStep('pnpm install', 'pnpm', ['install'], { | ||
| timeoutMs: 180_000, | ||
| }) | ||
|
|
||
| // Step 4: pnpm emit | ||
| outcomes.pnpmEmit = runStep('pnpm emit', 'pnpm', ['emit'], { | ||
| timeoutMs: 60_000, | ||
| }) | ||
|
|
||
| // Step 5: pnpm migration:plan --name initial | ||
| outcomes.pnpmPlan = runStep( | ||
| 'pnpm migration:plan --name initial', | ||
| 'pnpm', | ||
| ['migration:plan', '--name', 'initial'], | ||
| { timeoutMs: 60_000 }, | ||
| ) | ||
|
|
||
| // Step 6: pnpm migration:apply | ||
| outcomes.pnpmApply = runStep( | ||
| 'pnpm migration:apply', | ||
| 'pnpm', | ||
| ['migration:apply'], | ||
| { timeoutMs: 120_000 }, | ||
| ) | ||
|
|
||
| // Step 7: pnpm start | ||
| outcomes.pnpmStart = runStep('pnpm start', 'pnpm', ['start'], { | ||
| timeoutMs: 120_000, | ||
| }) | ||
| }, 600_000) // 10 min total budget for the cold path |
There was a problem hiding this comment.
Potential timeout mismatch in beforeAll.
The beforeAll timeout is 600,000ms (10 minutes), but the sum of individual step timeouts is 725 seconds (~12 minutes): cp (5s) + docker up (180s) + install (180s) + emit (60s) + plan (60s) + apply (120s) + start (120s). While commands typically complete much faster than their timeout limits, if multiple steps hit their timeouts, the beforeAll could exceed its budget.
Consider increasing the beforeAll timeout to 15 minutes (900,000ms) to provide headroom, or document that the 10-minute limit assumes normal execution.
🤖 Prompt for 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.
In `@e2e/tests/prisma-example-readme.e2e.test.ts` around lines 109 - 159, The
beforeAll timeout (600_000ms) is smaller than the sum of the per-step timeouts
used in the setup steps (outcomes.cpEnv, outcomes.dockerUp,
outcomes.pnpmInstall, outcomes.pnpmEmit, outcomes.pnpmPlan, outcomes.pnpmApply,
outcomes.pnpmStart) and may be exceeded if several steps reach their individual
timeouts; increase the beforeAll timeout to 900_000ms (15 minutes) or otherwise
raise it to exceed the total step timeouts so the beforeAll wrapper around
snapshotTransientOutputs() and the runStep calls has sufficient headroom.
… steps
Replace the hardcoded list of 7 'Run it' commands in
e2e/tests/prisma-example-readme.e2e.test.ts with a parseRunItCommands
helper that reads examples/prisma/README.md at test-collection time
and executes whatever's in the bash fence under '## Run it'. The
README is now the single source of truth — if a command gets renamed
the test runs the new wording and fails clearly, instead of silently
passing on the old wording.
Per-step exit-zero assertion uses it.each over the parsed lines so
each parsed command shows up as its own row in the vitest reporter.
'stash auth login' (interactive PKCE) is skipped via an exact-match
SKIP_COMMANDS set; brittle to wording changes by design so README
drift surfaces loudly. Per-command timeouts mapped via a small
prefix table.
Side-effect assertions (.env exists, contract.{json,d.ts}, the
migrations/app/<...>_initial directory, pg_isready) and the 'Expected
output' string list remain hardcoded — they catch 'exit 0 but
produced no output' regressions and aren't parseable from the README.
Verified:
- 13/13 pass in ~12s (warm cache)
- drift simulation: renamed 'pnpm emit' -> 'pnpm emiit' in README, test
failed with 'README "Run it" step exited 0: pnpm emiit' surfacing
pnpm's 'Did you mean "pnpm emit"?' stderr
- working tree clean after run, container torn down
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/prisma-example-readme.e2e.test.ts (1)
170-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard restore when snapshot creation failed early.
Line 170 assumes
snapDiris always assigned. IfbeforeAllfails before assignment,afterAllcan throw here and mask the root failure.Suggested minimal fix
-let snapDir: string +let snapDir: string | undefined ... - await restoreTransientOutputs(snapDir) + if (snapDir) await restoreTransientOutputs(snapDir)🤖 Prompt for 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. In `@e2e/tests/prisma-example-readme.e2e.test.ts` at line 170, The afterAll cleanup unconditionally calls restoreTransientOutputs(snapDir) which can throw if snapDir was never assigned due to a beforeAll failure; modify the afterAll to guard the call by checking that snapDir is defined/non-empty (or that the snapshot was created) before invoking restoreTransientOutputs, e.g., wrap the restoreTransientOutputs(snapDir) call in an if (snapDir) / truthy check so failures in beforeAll are not masked by a cleanup error.
♻️ Duplicate comments (2)
e2e/tests/prisma-example-readme.e2e.test.ts (2)
145-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncrease
beforeAlltimeout to match step-budget worst case.Line 159 keeps
600_000ms, but configured/default step timeouts can exceed that in slow CI paths, causing wrapper timeout before step-level failures are reported.Suggested minimal fix
- }, 600_000) // 10 min total budget for the cold path + }, 900_000) // 15 min budget to cover worst-case step timeout sum🤖 Prompt for 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. In `@e2e/tests/prisma-example-readme.e2e.test.ts` around lines 145 - 159, The beforeAll wrapper timeout (currently the fixed 600_000) can be shorter than the sum of per-step timeouts and causes premature test termination; replace the hardcoded 600_000 with a computed total budget based on the README_COMMANDS step timeouts (e.g. const totalBudget = README_COMMANDS.reduce((acc, line) => acc + timeoutFor(line), 0) + 60_000) and use totalBudget as the beforeAll timeout so the outer hook always exceeds the worst-case step budget when running runStep over README_COMMANDS (skip logic with SKIP_COMMANDS and outcomes map remains unchanged).
33-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact process output before appending failure diagnostics.
Line 33 and Line 34 still include raw
stderr/stdout, which can emit plaintext values in CI logs. Please sanitize (or summarize/truncate) these fields beforelines.push(...).Suggested minimal fix
+function redactPlaintext(s: string): string { + return s + .replace(/\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}\b/gi, '[redacted-email]') + .replace(/\b(?:postgres|mysql|mongodb):\/\/\S+/gi, '[redacted-conn-string]') + .slice(0, 4000) +} + function describeSpawnFailure(result: StepResult): string { const lines = [`step \`${result.label}\` failed.`] if (result.error) lines.push(` spawn error: ${result.error.message}`) if (result.signal) lines.push(` killed by signal: ${result.signal}`) if (typeof result.status === 'number') lines.push(` exit status: ${result.status}`) - if (result.stderr.trim()) lines.push(`--- stderr ---\n${result.stderr.trim()}`) - if (result.stdout.trim()) lines.push(`--- stdout ---\n${result.stdout.trim()}`) + const safeStderr = redactPlaintext(result.stderr).trim() + const safeStdout = redactPlaintext(result.stdout).trim() + if (safeStderr) lines.push(`--- stderr ---\n${safeStderr}`) + if (safeStdout) lines.push(`--- stdout ---\n${safeStdout}`) return lines.join('\n') }As per coding guidelines,
Never log plaintext data; the library by design never logs plaintext sensitive information.🤖 Prompt for 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. In `@e2e/tests/prisma-example-readme.e2e.test.ts` around lines 33 - 34, The test currently appends raw process output (result.stderr and result.stdout) into diagnostics via lines.push, which can leak plaintext; replace those direct appends with sanitized or truncated summaries: for example compute a redacted string from result.stderr and result.stdout (e.g., mask sensitive patterns, limit length, or replace with "<redacted>"/first-N-chars + "...") and push that sanitized string instead; update the lines.push calls that reference result.stderr, result.stdout so they use the sanitized variables and include contextual labels like "--- stderr ---" / "--- stdout ---" as before.
🤖 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.
Outside diff comments:
In `@e2e/tests/prisma-example-readme.e2e.test.ts`:
- Line 170: The afterAll cleanup unconditionally calls
restoreTransientOutputs(snapDir) which can throw if snapDir was never assigned
due to a beforeAll failure; modify the afterAll to guard the call by checking
that snapDir is defined/non-empty (or that the snapshot was created) before
invoking restoreTransientOutputs, e.g., wrap the
restoreTransientOutputs(snapDir) call in an if (snapDir) / truthy check so
failures in beforeAll are not masked by a cleanup error.
---
Duplicate comments:
In `@e2e/tests/prisma-example-readme.e2e.test.ts`:
- Around line 145-159: The beforeAll wrapper timeout (currently the fixed
600_000) can be shorter than the sum of per-step timeouts and causes premature
test termination; replace the hardcoded 600_000 with a computed total budget
based on the README_COMMANDS step timeouts (e.g. const totalBudget =
README_COMMANDS.reduce((acc, line) => acc + timeoutFor(line), 0) + 60_000) and
use totalBudget as the beforeAll timeout so the outer hook always exceeds the
worst-case step budget when running runStep over README_COMMANDS (skip logic
with SKIP_COMMANDS and outcomes map remains unchanged).
- Around line 33-34: The test currently appends raw process output
(result.stderr and result.stdout) into diagnostics via lines.push, which can
leak plaintext; replace those direct appends with sanitized or truncated
summaries: for example compute a redacted string from result.stderr and
result.stdout (e.g., mask sensitive patterns, limit length, or replace with
"<redacted>"/first-N-chars + "...") and push that sanitized string instead;
update the lines.push calls that reference result.stderr, result.stdout so they
use the sanitized variables and include contextual labels like "--- stderr ---"
/ "--- stdout ---" as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d899ed80-c819-4805-b175-540c6c4792e1
📒 Files selected for processing (1)
e2e/tests/prisma-example-readme.e2e.test.ts
Drop the six brittle assertions beyond the per-command exit-zero
check: two that inspect pnpm start stdout for specific codec headings
and row counts, and four side-effect checks (.env exists, pg_isready,
contract.{json,d.ts} exist, migrations/app/*_initial exists).
These broke for reasons unrelated to the test's contract — any
wording change in src/index.ts tripped the stdout checks, and the
side-effect checks hardcoded the CLI's migration-naming convention,
the container name, and the emit output paths.
The test is an outside-in smoke test: it verifies every command in
the README's documented walkthrough runs successfully. It now asserts
exactly that and nothing more — parse the README, run each command,
assert exit 0.
Accepted tradeoff: a command that exits 0 but silently produces
nothing is no longer caught.
Verified: 7/7 exit-zero rows pass (~12s warm cache), test file
typechecks clean, working tree clean after run.
Since the test runs every command via `bash -c` and asserts only
exit-zero, parts of runStep's signature are vestigial:
- the cmd + args split existed for the old hardcoded
runStep('pnpm', ['emit']) calls — every call is now bash -c <line>
- opts.cwd / opts.env were never passed — always EXAMPLE_DIR /
process.env
- the timeoutMs default was never reached — both call sites pass an
explicit timeout
Collapse to runStep(commandLine, timeoutMs). Drop the redundant
SpawnSyncReturns annotation (TS infers it) and its now-unused import.
afterAll's teardown runs via bash -c too, consistent with the
walkthrough — bash is already a hard dependency of every step.
Also fix the e2e/README.md coverage row, which still claimed the test
asserts pnpm start matches the documented Expected output — removed in
2cd61ab.
No behaviour change. Verified: 7/7 exit-zero rows pass (~12s warm),
typecheck clean, working tree clean after run.
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.
Add an e2e test and CI workflow that runs the steps documented in the
README.mdfor the Prisma example app.Background:
examples/prismaapp ships a README with a "Run it" section documenting the commands to run the example app