Skip to content

fix(ci): update vitest e2e-brev include to .ts after rename #1743

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cv merged 10 commits into from
Apr 10, 2026
+41 −45
Merged

fix(ci): update vitest e2e-brev include to .ts after rename #1743

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2319dc5
fix(ci): update vitest e2e-brev include to .ts after rename
cjagwani Apr 10, 2026
4e042f0
perf(e2e): skip npm install, plugin build, and npm link for full suite
cjagwani Apr 10, 2026
8eb57c9
perf(e2e): keep npm install for full suite to prime dep cache
cjagwani Apr 10, 2026
4b3e8fd
perf(e2e): reduce SSH wait timeout from 90 to 40 attempts
cjagwani Apr 10, 2026
574d64b
fix(e2e): log each SSH retry attempt for visibility
cjagwani Apr 10, 2026
25aec90
Merge branch 'main' into fix/vitest-e2e-brev-ts-include
cv Apr 10, 2026
d76aca5
fix(e2e): warn instead of throw on afterAll instance delete failure
cjagwani Apr 10, 2026
427fd59
fix(e2e): treat DELETING status as successful delete
cjagwani Apr 10, 2026
012147d
fix(e2e): simplify afterAll delete to fire-and-forget
cjagwani Apr 10, 2026
8858ba3
Merge branch 'main' into fix/vitest-e2e-brev-ts-include
cjagwani Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions test/e2e/brev-e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,14 +165,16 @@ function sshEnv(cmd, { timeout = 600_000, stream = false } = {}) {
return ssh(`${envPrefix} && ${cmd}`, { timeout, stream });
}

function waitForSsh(maxAttempts = 90, intervalMs = 5_000) {
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...`);
Comment on lines +163 to +170
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

if (i % 5 === 0) {
console.log(` Refreshing brev SSH config...`);
try {
brev("refresh");
} catch {
Expand Down Expand Up @@ -418,6 +420,8 @@ describe.runIf(hasRequiredVars && hasAuthenticatedBrev)("Brev E2E", () => {
// may have a package.json/package-lock.json that are slightly out of sync
// (e.g. new transitive deps). npm install is more forgiving and still
// benefits from the launchable's pre-cached node_modules.
// Always run this even for TEST_SUITE=full — it primes the cache so
// install.sh's npm install is a fast no-op.
console.log(`[${elapsed()}] Running npm install to sync dependencies...`);
ssh(
[
Expand All @@ -430,6 +434,18 @@ describe.runIf(hasRequiredVars && hasAuthenticatedBrev)("Brev E2E", () => {
);
console.log(`[${elapsed()}] Dependencies synced`);

// When TEST_SUITE=full, test-full-e2e.sh runs install.sh which handles
// plugin build, npm link, and onboard from scratch. Skip those steps
// to avoid ~8 min of redundant work.
const needsBeforeAllSetup = TEST_SUITE !== "full";

if (!needsBeforeAllSetup) {
console.log(
`[${elapsed()}] Skipping plugin build, npm link, and onboard (TEST_SUITE=full — install.sh handles it)`,
);
}

if (needsBeforeAllSetup) {
// Rebuild TS plugin for our branch (reinstall plugin deps in case they changed)
console.log(`[${elapsed()}] Building TypeScript plugin...`);
ssh(
Expand All @@ -455,19 +471,6 @@ describe.runIf(hasRequiredVars && hasAuthenticatedBrev)("Brev E2E", () => {
);
console.log(`[${elapsed()}] nemoclaw CLI linked`);

// The full E2E test (test-full-e2e.sh) immediately destroys any sandbox
// in Phase 0, then runs install.sh which creates its own from scratch.
// Skip the beforeAll onboard for that suite — it would be thrown away
// and wastes ~6 min of image build + upload time.
const needsBeforeAllSandbox = TEST_SUITE !== "full";

if (!needsBeforeAllSandbox) {
console.log(
`[${elapsed()}] Skipping beforeAll onboard (TEST_SUITE=full — test does its own)`,
);
}

if (needsBeforeAllSandbox) {
// Run onboard in the background. The `nemoclaw onboard` process hangs
// after sandbox creation because `openshell sandbox create` keeps a
// long-lived SSH connection to the sandbox entrypoint, and the dashboard
Expand Down Expand Up @@ -620,7 +623,7 @@ describe.runIf(hasRequiredVars && hasAuthenticatedBrev)("Brev E2E", () => {
{ timeout: 15_000 },
);
console.log(`[${elapsed()}] Registry written, onboard workaround complete`);
} // end if (needsBeforeAllSandbox)
} // end if (needsBeforeAllSetup)
}

// Verify sandbox registry (only when beforeAll created a sandbox)
Expand Down
2 changes: 1 addition & 1 deletion vitest.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export default defineConfig({
{
test: {
name: "e2e-brev",
include: ["test/e2e/brev-e2e.test.js"],
include: ["test/e2e/brev-e2e.test.ts"],
// Only run when explicitly targeted: npx vitest run --project e2e-brev
enabled: !!process.env.BREV_API_TOKEN,
},
Expand Down
Loading