Conversation
Make flaky integration tests deterministic by avoiding interactive shell selection, using dynamic dev-server target ports, and suppressing telemetry subprocesses when tests explicitly set CI mode. Made-with: Cursor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes introduce an optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
📊 Benchmark resultsComparing with 0415e87
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/completion/index.ts (1)
10-10: Optional: move shell validation to Commander via.choices().The description already enumerates the supported shells. Declaring
.choices(['bash','fish','pwsh','zsh'])(e.g. viaaddOption(new Option(...).choices([...]))) would let Commander reject invalid values before the action runs and keep the help text and accepted values in one place, instead of duplicating that list incompletion.ts'sisShellSupportedguard.Feel free to skip if you prefer keeping validation close to the tabtab call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/completion/index.ts` at line 10, Replace the free-form .option('--shell <shell>') with a Commander Option that specifies allowed values so invalid shells are rejected before action runs: import Option from commander and use addOption(new Option('--shell <shell>').choices(['bash','fish','pwsh','zsh'])); then remove or bypass the duplicate validation in isShellSupported in completion.ts so the supported-shell list is maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/commands/dev/redirects.test.ts`:
- Around line 50-53: The current replace call may be a silent no-op if the exact
substring 'targetPort = 6123' isn't present; update the logic around
readFile/replace/writeFile (referencing netlifyTomlPath and targetPort) to
perform a more robust regex-based replacement (e.g., allow flexible spacing) and
then assert that the replacement actually occurred by comparing the original and
replaced contents or by checking a replace count; if no replacement happened,
throw or fail the test immediately before calling writeFile so the test fails
loudly rather than silently using the unchanged fixture.
---
Nitpick comments:
In `@src/commands/completion/index.ts`:
- Line 10: Replace the free-form .option('--shell <shell>') with a Commander
Option that specifies allowed values so invalid shells are rejected before
action runs: import Option from commander and use addOption(new Option('--shell
<shell>').choices(['bash','fish','pwsh','zsh'])); then remove or bypass the
duplicate validation in isShellSupported in completion.ts so the supported-shell
list is maintained in one place.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e9f17c5-ffeb-429a-97c0-ec0eeb993f05
⛔ Files ignored due to path filters (1)
tests/integration/commands/help/__snapshots__/help.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
src/commands/completion/completion.tssrc/commands/completion/index.tssrc/utils/telemetry/report-error.tstests/integration/commands/completion/completion-install.test.tstests/integration/commands/dev/dev.test.tstests/integration/commands/dev/redirects.test.ts
| await writeFile( | ||
| netlifyTomlPath, | ||
| (await readFile(netlifyTomlPath, 'utf8')).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`), | ||
| ) |
There was a problem hiding this comment.
Silent no-op risk: assert the targetPort replacement actually happened.
String.prototype.replace returns the original string unchanged if 'targetPort = 6123' isn't present (e.g., the fixture's default port changes, someone reformats to targetPort=6123, or the line is moved/removed). The test would then silently fall back to whatever port is checked into the fixture, re-introducing a flaky/unexpected port binding without any signal.
Either pattern-match more robustly or assert that the replacement occurred.
🛡️ Proposed fix
- packageJson.scripts.dev = `next dev -p ${targetPort.toString()}`
- await writeFile(packageJsonPath, `${JSON.stringify(packageJson, null, 2)}\n`)
- await writeFile(
- netlifyTomlPath,
- (await readFile(netlifyTomlPath, 'utf8')).replace('targetPort = 6123', `targetPort = ${targetPort.toString()}`),
- )
+ packageJson.scripts.dev = `next dev -p ${targetPort.toString()}`
+ await writeFile(packageJsonPath, `${JSON.stringify(packageJson, null, 2)}\n`)
+
+ const originalToml = await readFile(netlifyTomlPath, 'utf8')
+ const updatedToml = originalToml.replace(/targetPort\s*=\s*\d+/, `targetPort = ${targetPort.toString()}`)
+ if (updatedToml === originalToml) {
+ throw new Error(`Failed to rewrite targetPort in ${netlifyTomlPath}; fixture may have changed.`)
+ }
+ await writeFile(netlifyTomlPath, updatedToml)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/commands/dev/redirects.test.ts` around lines 50 - 53, The
current replace call may be a silent no-op if the exact substring 'targetPort =
6123' isn't present; update the logic around readFile/replace/writeFile
(referencing netlifyTomlPath and targetPort) to perform a more robust
regex-based replacement (e.g., allow flexible spacing) and then assert that the
replacement actually occurred by comparing the original and replaced contents or
by checking a replace count; if no replacement happened, throw or fail the test
immediately before calling writeFile so the test fails loudly rather than
silently using the unchanged fixture.
1 participant
Summary
--shelloption forcompletion:installand use it in the zsh completion integration tests so they no longer drive the upstream tabtab shell picker interactively.process.env.CIin error telemetry reporting so expected CLI failures in integration tests do not spawn telemetry subprocesses that Vitest reports as unhandled rejections.netlify help completionsnapshot to match the current option ordering.Test plan
npm run test:initnpm run typechecknpm run lintnpm exec vitest -- run --retry=3 --reporter=default tests/integration/frameworks/hugo.test.ts tests/integration/commands/dev/redirects.test.ts tests/integration/commands/build/build.test.ts tests/integration/commands/completion/completion-install.test.ts tests/integration/commands/dev/dev.test.ts tests/integration/commands/help/help.test.tsnpm exec vitest -- run --retry=3 --reporter=default tests/integration/commands/sites/sites.test.tsnpm run test:integration— 60 files passed, 554 passed / 3 skipped / 7 todoMade with Cursor