Merged
Conversation
📊 Benchmark resultsComparing with 2453221
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request updates CI workflows and test suites: GitHub Actions e2e job timeouts are increased and an integration test script now fails the job on non-201 HTTP responses. The e2e install tests are refactored to use a single shared Verdaccio registry with centralized publish, setup, and teardown logic, plus improved error handling. Two integration tests replace hardcoded ports with dynamic allocation via get-port. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/install.e2e.ts`:
- Around line 41-154: Prettier formatting failed for e2e/install.e2e.ts; run
Prettier and commit the formatted file. Open the file that contains the
beforeAll block (which defines shared, calls runServer, creates
publishWorkspace, uses shouldCopyCLIFile, and runs execa/npm publish), run your
project's Prettier configuration (e.g. npx prettier --write e2e/install.e2e.ts)
or your repo's formatting script, verify the file is reformatted, then stage and
commit the updated e2e/install.e2e.ts.
- Around line 52-154: The beforeAll hook in e2e/install.e2e.ts needs an explicit
timeout because Vitest's default 10s is too short for the Verdaccio setup and
npm publish steps; update the existing beforeAll(async () => { ... }) invocation
to pass a longer timeout as the second argument (for example 120000 ms or
another CI-appropriate value) so the setup (functions like runServer, execa
publishes, fs operations) can complete without timing out.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/workflows/e2e-tests.yml.github/workflows/integration-tests.ymle2e/install.e2e.tstests/integration/commands/dev/dev.test.tstests/integration/commands/dev/redirects.test.ts
eduardoboucas
commented
Feb 23, 2026
| '"dist" directory does not exist or is not a directory. The project must be built before running E2E tests.', | ||
| { cause: err }, | ||
| ) | ||
| // Shared registry state: one verdaccio server + one publish for all tests. |
Member
Author
There was a problem hiding this comment.
Main change here is that we publish once and reuse that registry for all the tests, as opposed to doing it once per test.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/install.e2e.ts (3)
164-176: Consider adding a timeout toafterAllhook.If the server fails to close cleanly,
events.once(shared.server, 'close')could hang indefinitely. While this is non-CI only, it could affect local development.♻️ Suggested fix with timeout
- afterAll(async () => { + afterAll(async () => { if (shared.server) { await Promise.all([ events.once(shared.server, 'close'), shared.server.close(), // eslint-disable-next-line `@typescript-eslint/no-confusing-void-expression` shared.server.closeAllConnections(), ]) } if (shared.verdaccioStorageDir) { await fs.rm(shared.verdaccioStorageDir, { force: true, recursive: true, maxRetries: 3, retryDelay: 1000 }) } - }) + }, 30_000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/install.e2e.ts` around lines 164 - 176, The afterAll teardown can hang waiting on events.once(shared.server, 'close'); modify the afterAll to race the close sequence (events.once(shared.server, 'close'), shared.server.close(), shared.server.closeAllConnections()) against a timeout promise (e.g., Promise.race with a short timeout like 5–10s) and ensure the timeout path logs a warning and proceeds to cleanup (still remove shared.verdaccioStorageDir). Update the block that references afterAll, shared.server, events.once(..., 'close'), shared.server.close(), and shared.server.closeAllConnections() to use the timeout race and handle the timeout case gracefully so the test suite doesn't hang.
108-116: Simplify the server startup pattern.The
Promise.allwrappingPromise.raceandserver.listen()is slightly convoluted. Sinceserver.listen()triggers the 'listening' event asynchronously, you can simplify by callinglisten()first, then awaiting the race.♻️ Suggested simplification
- await Promise.all([ - Promise.race([ - events.once(server, 'listening'), - events.once(server, 'error').then(() => { - throw new Error('Verdaccio server failed to start') - }), - ]), - server.listen(), - ]) + server.listen() + await Promise.race([ + events.once(server, 'listening'), + events.once(server, 'error').then(([err]) => { + throw new Error(`Verdaccio server failed to start: ${err instanceof Error ? err.message : String(err)}`) + }), + ])Additionally, the error handler discards the actual error from the 'error' event. Including it would help diagnose startup failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/install.e2e.ts` around lines 108 - 116, The startup logic for the test server is overcomplicated and drops the real error; change the pattern to call server.listen() first and then await a Promise.race between events.once(server, 'listening') and events.once(server, 'error') so we wait for whichever happens next, and when the 'error' event wins capture and include the actual error object in the thrown/propagated error message. Update the code around server.listen(), events.once(server, 'listening'), and events.once(server, 'error') to reflect this simpler ordering and include the error value instead of discarding it.
329-329: Unused temporary directory.The
cwddirectory is created at line 329 but never used as a working directory for anyexecacalls in this test (lines 336, 347, 370 all omitcwd). The directory is only referenced in the cleanup at line 384, making the creation and cleanup overhead unnecessary.♻️ Suggested fix: remove unused temp directory
it.skipIf(skipOnWindows)('runs commands without errors', async () => { - const cwd = await fs.mkdtemp(path.join(os.tmpdir(), tempdirPrefix)) const [cmd, args] = config.run const env = { npm_config_registry: shared.registryURL, } try { // ... test body unchanged ... - } finally { - if (!process.env.CI) { - await fs.rm(cwd, { force: true, recursive: true, maxRetries: 3, retryDelay: 1000 }) - } - } + } finally {} })Or remove the try/finally entirely if there's no cleanup needed.
Also applies to: 382-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/install.e2e.ts` at line 329, The temp directory created by const cwd = await fs.mkdtemp(path.join(os.tmpdir(), tempdirPrefix)) is never used for any execa calls (the execa invocations in this test omit a cwd option) and is only cleaned up, so either remove the fs.mkdtemp call and the associated cleanup/try-finally, or actually pass cwd into the execa calls (e.g., include { cwd } in their options) so the temp dir is used as the working directory; update references in e2e/install.e2e.ts accordingly and remove any now-unnecessary cleanup if you choose to delete the temp-dir creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/install.e2e.ts`:
- Around line 379-381: The assertion message for the unlink test is misleading;
change the failure message passed to expect on the unlinkOutput variable so it
accurately describes the failure (e.g., "Unlink output should include the
suggested link command" or "Unlink command missing command context") instead of
"includes command context". Update the expect call that uses unlinkOutput and
cmd in e2e/install.e2e.ts to use the clearer message so test failures indicate
that the unlink output didn’t contain the expected "Run ${cmd} netlify link"
suggestion.
- Around line 296-298: The assertion message is misleading because it reads as
if the output includes the command context even though the message is shown only
on failure; update the failure message used with expect(unlinkOutput,
`...`).toContain(...) to indicate the negative case (e.g., "Unlink command
missing command context" or "expected unlink output to include 'Run netlify
link' but it did not") so it accurately describes the failure when the variable
unlinkOutput does not contain the expected string.
---
Nitpick comments:
In `@e2e/install.e2e.ts`:
- Around line 164-176: The afterAll teardown can hang waiting on
events.once(shared.server, 'close'); modify the afterAll to race the close
sequence (events.once(shared.server, 'close'), shared.server.close(),
shared.server.closeAllConnections()) against a timeout promise (e.g.,
Promise.race with a short timeout like 5–10s) and ensure the timeout path logs a
warning and proceeds to cleanup (still remove shared.verdaccioStorageDir).
Update the block that references afterAll, shared.server, events.once(...,
'close'), shared.server.close(), and shared.server.closeAllConnections() to use
the timeout race and handle the timeout case gracefully so the test suite
doesn't hang.
- Around line 108-116: The startup logic for the test server is overcomplicated
and drops the real error; change the pattern to call server.listen() first and
then await a Promise.race between events.once(server, 'listening') and
events.once(server, 'error') so we wait for whichever happens next, and when the
'error' event wins capture and include the actual error object in the
thrown/propagated error message. Update the code around server.listen(),
events.once(server, 'listening'), and events.once(server, 'error') to reflect
this simpler ordering and include the error value instead of discarding it.
- Line 329: The temp directory created by const cwd = await
fs.mkdtemp(path.join(os.tmpdir(), tempdirPrefix)) is never used for any execa
calls (the execa invocations in this test omit a cwd option) and is only cleaned
up, so either remove the fs.mkdtemp call and the associated cleanup/try-finally,
or actually pass cwd into the execa calls (e.g., include { cwd } in their
options) so the temp dir is used as the working directory; update references in
e2e/install.e2e.ts accordingly and remove any now-unnecessary cleanup if you
choose to delete the temp-dir creation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
e2e/install.e2e.ts
Comment on lines
+296
to
298
| expect(unlinkOutput, `Unlink command includes command context':\n\n${unlinkOutput}`).toContain( | ||
| `Run netlify link to link it`, | ||
| ) |
There was a problem hiding this comment.
Misleading assertion message.
The assertion message says "Unlink command includes command context" but the expect().toContain() call is checking that the output does contain the expected text. The message appears if the assertion fails, so it should indicate what's wrong (i.e., the output does not include the expected text).
✏️ Suggested fix
- expect(unlinkOutput, `Unlink command includes command context':\n\n${unlinkOutput}`).toContain(
+ expect(unlinkOutput, `Unlink command does not include expected context:\n\n${unlinkOutput}`).toContain(
`Run netlify link to link it`,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(unlinkOutput, `Unlink command includes command context':\n\n${unlinkOutput}`).toContain( | |
| `Run netlify link to link it`, | |
| ) | |
| expect(unlinkOutput, `Unlink command does not include expected context:\n\n${unlinkOutput}`).toContain( | |
| `Run netlify link to link it`, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/install.e2e.ts` around lines 296 - 298, The assertion message is
misleading because it reads as if the output includes the command context even
though the message is shown only on failure; update the failure message used
with expect(unlinkOutput, `...`).toContain(...) to indicate the negative case
(e.g., "Unlink command missing command context" or "expected unlink output to
include 'Run netlify link' but it did not") so it accurately describes the
failure when the variable unlinkOutput does not contain the expected string.
Comment on lines
+379
to
381
| expect(unlinkOutput, `Unlink command includes command context':\n\n${unlinkOutput}`).toContain( | ||
| `Run ${cmd} netlify link to link it`, | ||
| ) |
There was a problem hiding this comment.
Misleading assertion message (same issue as install tests).
The assertion message says "includes command context" but should indicate what's wrong when the assertion fails.
✏️ Suggested fix
- expect(unlinkOutput, `Unlink command includes command context':\n\n${unlinkOutput}`).toContain(
+ expect(unlinkOutput, `Unlink command does not include expected context:\n\n${unlinkOutput}`).toContain(
`Run ${cmd} netlify link to link it`,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(unlinkOutput, `Unlink command includes command context':\n\n${unlinkOutput}`).toContain( | |
| `Run ${cmd} netlify link to link it`, | |
| ) | |
| expect(unlinkOutput, `Unlink command does not include expected context:\n\n${unlinkOutput}`).toContain( | |
| `Run ${cmd} netlify link to link it`, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/install.e2e.ts` around lines 379 - 381, The assertion message for the
unlink test is misleading; change the failure message passed to expect on the
unlinkOutput variable so it accurately describes the failure (e.g., "Unlink
output should include the suggested link command" or "Unlink command missing
command context") instead of "includes command context". Update the expect call
that uses unlinkOutput and cmd in e2e/install.e2e.ts to use the clearer message
so test failures indicate that the unlink output didn’t contain the expected
"Run ${cmd} netlify link" suggestion.
minivan
approved these changes
Feb 23, 2026
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
3 participants
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.
Follow-up to #7958.