fix(ci): update vitest e2e-brev include to .ts after rename#1743
Conversation
The brev-e2e test was renamed from .js to .ts but vitest.config.ts still pointed to the old .js path, causing "No test files found". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitches the Vitest e2e project to run the TypeScript test file and centralizes beforeAll gating into a single Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Remote as Remote Host / Launchable
participant Setup as Local Setup (build / npm link)
participant Onboard as Onboard Background
participant Registry as Registry Workaround
Note over Runner,Remote: Start e2e run
Runner->>Remote: waitForSsh (retries reduced, progress logs)
Runner->>Remote: run remote rsync
Runner->>Remote: run remote npm install
alt needsBeforeAllSetup = true (TEST_SUITE !== "full")
Runner->>Setup: build plugins & npm link
Runner->>Onboard: start onboard background process
Runner->>Registry: apply registry workaround
else needsBeforeAllSetup = false (TEST_SUITE === "full")
Runner--xSetup: skip plugin build & npm link
Runner--xOnboard: skip onboard background
Runner--xRegistry: skip registry workaround
end
Runner->>Runner: run tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
❌ Brev E2E (full): FAILED on branch |
When TEST_SUITE=full, test-full-e2e.sh runs install.sh which handles Node.js, OpenShell, NemoClaw install, and onboard from scratch. The beforeAll was redundantly doing npm install, plugin build, and npm link before that — wasting ~10 min on work install.sh redoes. Now beforeAll only provisions the Brev instance, waits for SSH + launchable, and rsyncs the code when TEST_SUITE=full. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
❌ Brev E2E (full): FAILED on branch |
wscurran
added
CI/CD
|
✨ Possibly related open PRs: |
wscurran
added
Platform: Brev
|
❌ Brev E2E (full): FAILED on branch |
Move npm install back outside the needsBeforeAllSetup guard. The launchable caches node_modules for main's lockfile, but rsync brings the PR branch's lockfile — without a reconciling npm install, install.sh hits a cold cache and takes longer. Still skip plugin build, npm link, and onboard for full suite. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
90 attempts × (5s sleep + 10s SSH timeout) = ~22.5 min wasted when the instance never comes up. GCP instances are typically SSH-ready within 5 min — 40 attempts (~10 min) is sufficient. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/brev-e2e.test.ts`:
- Around line 168-175: The waitForSsh readiness budget is hard-coded to 40
attempts; make it configurable via an env var (e.g., SSH_READINESS_ATTEMPTS or
BREV_SSH_MAX_ATTEMPTS) and use a safer default (higher than 40). Update the
waitForSsh function to derive its default maxAttempts from process.env
(parseInt/Number with a fallback safe default and clamp to a positive integer),
optionally allow intervalMs to be env-configurable too, and ensure the thrown
error and log messages reference the actual resolved maxAttempts so CI can be
tuned without code changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36999b43-b145-4d91-adb1-90ff91d4b2a4
📒 Files selected for processing (1)
test/e2e/brev-e2e.test.ts
| function waitForSsh(maxAttempts = 40, intervalMs = 5_000) { | ||
| for (let i = 1; i <= maxAttempts; i++) { | ||
| try { | ||
| ssh("echo ok", { timeout: 10_000 }); | ||
| return; | ||
| } catch { | ||
| if (i === maxAttempts) throw new Error(`SSH not ready after ${maxAttempts} attempts`); | ||
| if (i === maxAttempts) throw new Error(`SSH not ready after ${maxAttempts} attempts (~${Math.round(maxAttempts * (intervalMs + 10_000) / 60_000)} min)`); | ||
| console.log(` SSH attempt ${i}/${maxAttempts} failed, retrying in ${intervalMs / 1000}s...`); |
There was a problem hiding this comment.
Make SSH readiness budget configurable to avoid flaky failures
Line 168 hard-caps SSH readiness to 40 attempts (~10 min total wall time with current timeout/interval). In busy/cloud-constrained runs, this can fail before the instance is actually usable and create false negatives in CI. Please make the budget env-configurable (with a safer default) instead of hard-coding this lower ceiling.
🔧 Proposed change
-function waitForSsh(maxAttempts = 40, intervalMs = 5_000) {
+const BREV_SSH_MAX_ATTEMPTS = parseInt(process.env.BREV_SSH_MAX_ATTEMPTS || "90", 10);
+const BREV_SSH_INTERVAL_MS = parseInt(process.env.BREV_SSH_INTERVAL_MS || "5000", 10);
+
+function waitForSsh(maxAttempts = BREV_SSH_MAX_ATTEMPTS, intervalMs = BREV_SSH_INTERVAL_MS) {📝 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.
| function waitForSsh(maxAttempts = 40, intervalMs = 5_000) { | |
| for (let i = 1; i <= maxAttempts; i++) { | |
| try { | |
| ssh("echo ok", { timeout: 10_000 }); | |
| return; | |
| } catch { | |
| if (i === maxAttempts) throw new Error(`SSH not ready after ${maxAttempts} attempts`); | |
| if (i === maxAttempts) throw new Error(`SSH not ready after ${maxAttempts} attempts (~${Math.round(maxAttempts * (intervalMs + 10_000) / 60_000)} min)`); | |
| console.log(` SSH attempt ${i}/${maxAttempts} failed, retrying in ${intervalMs / 1000}s...`); | |
| const BREV_SSH_MAX_ATTEMPTS = parseInt(process.env.BREV_SSH_MAX_ATTEMPTS || "90", 10); | |
| const BREV_SSH_INTERVAL_MS = parseInt(process.env.BREV_SSH_INTERVAL_MS || "5000", 10); | |
| function waitForSsh(maxAttempts = BREV_SSH_MAX_ATTEMPTS, intervalMs = BREV_SSH_INTERVAL_MS) { | |
| for (let i = 1; i <= maxAttempts; i++) { | |
| try { | |
| ssh("echo ok", { timeout: 10_000 }); | |
| return; | |
| } catch { | |
| if (i === maxAttempts) throw new Error(`SSH not ready after ${maxAttempts} attempts (~${Math.round(maxAttempts * (intervalMs + 10_000) / 60_000)} min)`); | |
| console.log(` SSH attempt ${i}/${maxAttempts} failed, retrying in ${intervalMs / 1000}s...`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/brev-e2e.test.ts` around lines 168 - 175, The waitForSsh readiness
budget is hard-coded to 40 attempts; make it configurable via an env var (e.g.,
SSH_READINESS_ATTEMPTS or BREV_SSH_MAX_ATTEMPTS) and use a safer default (higher
than 40). Update the waitForSsh function to derive its default maxAttempts from
process.env (parseInt/Number with a fallback safe default and clamp to a
positive integer), optionally allow intervalMs to be env-configurable too, and
ensure the thrown error and log messages reference the actual resolved
maxAttempts so CI can be tuned without code changes.
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
|
❌ Brev E2E (full): FAILED on branch |
brev delete is flaky and shouldn't fail the entire test run when the actual tests passed. Log a warning with manual cleanup instructions instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
❌ Brev E2E (full): FAILED on branch |
deleteBrevInstance was retrying 5x with sleeps because brev ls still showed the instance in DELETING state. This took 35-40s and exceeded vitest's 10s hookTimeout. Now: fire brev delete once, treat DELETING/STOPPING as success since Brev finishes teardown asynchronously. No retries needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
✅ Brev E2E (full): PASSED on branch |
deleteBrevInstance treats DELETING/STOPPING as success, so the warning is unreachable in practice. Remove it for clean logs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Summary
brev-e2etest file was renamed from.jsto.tsin perf(e2e): skip redundant beforeAll onboard when TEST_SUITE=full #1631, butvitest.config.tsstill pointed to the old.jspathNo test files foundwhen runningnpx vitest run --project e2e-brevTest plan
TEST_SUITE=fulland confirm it finds the test file🤖 Generated with Claude Code
Summary by CodeRabbit