Skip to content

Commit fe26b4a

Browse files
fix(test): create-batch --run without --wait exits 0 on a fully successful dispatch
The no-wait fan-out returns each result with the trigger response's status, which is `queued` by design (non-terminal — the field is documented as "Terminal status if --wait; queued if no --wait"). But the exit-code logic only recognized terminal statuses: const allPassed = batchRunResults.every(r => r.status === 'passed'); Every no-wait result is `queued`, so `allPassed` was always false and the command fell through to exit 1 with a misleading "N of N run(s) did not pass" — even when every trigger was dispatched successfully. This broke the documented exit-code contract (the DOCUMENTATION.md create-batch example uses `--run --max-concurrency 4 --output json`, no `--wait`), failed CI on fully successful dispatches, and was internally inconsistent with single `test run <id>` without `--wait`, which exits 0 on a successful `queued` dispatch. In the no-wait case "success" means every trigger dispatched without error (statuses are non-terminal by definition). The exit-code block now branches on `opts.wait`: const failing = opts.wait ? batchRunResults.filter(r => r.status !== 'passed') : batchRunResults.filter(r => r.error !== undefined); if (failing.length === 0) return; // exit 0 Trigger errors in no-wait mode keep today's aggregation (uniform specific code when shared, else exit 1), and the "did not pass" message becomes "N of N trigger(s) failed" when `--wait` is absent. The stderr text summary is likewise corrected for no-wait mode — it now reports "N/N triggered" instead of misreading a successful dispatch as "0/N passed". Adds three specs in test.create-batch-run.spec.ts: all-queued (json) → resolves without error and never polls; all-queued (text) → "N/N triggered" summary, no "passed"/"did not pass" wording; partial trigger failure → still exits non-zero with the full results envelope. The two success specs fail against the pre-fix exit-code block and pass with it. Existing no-wait specs only exercised error scenarios and never asserted the success exit code, which is how this slipped through. Fixes #161 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent e53257d commit fe26b4a

3 files changed

Lines changed: 250 additions & 15 deletions

File tree

DOCUMENTATION.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ testsprite test create --plan-from ./checkout.plan.json --dry-run --output json
271271

272272
#### `testsprite test create-batch`
273273

274-
Bulk-create frontend tests from a JSONL plan-steps file (or a directory of plan files with `--plan-from-dir`). Optional `--run --max-concurrency <N>` fans out triggers.
274+
Bulk-create frontend tests from a JSONL plan-steps file (or a directory of plan files with `--plan-from-dir`). Optional `--run --max-concurrency <N>` fans out triggers. Without `--wait`, each run is dispatched (`status: "queued"`) and the command exits 0 when every trigger is accepted — mirroring single `test run` without `--wait`; a trigger error still exits non-zero. With `--wait`, it polls every run to terminal and exits non-zero if any run does not pass.
275275

276276
```bash
277277
testsprite test create-batch --plans ./plans.jsonl --run --max-concurrency 4 --output json

src/commands/test.create-batch-run.spec.ts

Lines changed: 219 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,225 @@ describe('runCreateBatch --run --wait: mixed outcomes', () => {
333333
});
334334
});
335335

336+
// ---------------------------------------------------------------------------
337+
// No --wait: exit-code contract (issue #161)
338+
//
339+
// Without --wait, every trigger response is non-terminal ('queued') by design.
340+
// A fully successful dispatch must exit 0 — success means every trigger was
341+
// dispatched without error, mirroring single `test run` (no --wait). A trigger
342+
// error must still exit non-zero. Existing no-wait specs only exercised error
343+
// scenarios and never asserted the success exit code, which is how the
344+
// "always exits 1 even when every trigger succeeds" bug slipped through.
345+
// ---------------------------------------------------------------------------
346+
347+
describe('runCreateBatch --run (no --wait): exit-code contract', () => {
348+
let logSpy: ReturnType<typeof vi.spyOn>;
349+
let errorSpy: ReturnType<typeof vi.spyOn>;
350+
351+
beforeEach(() => {
352+
logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
353+
errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
354+
});
355+
356+
afterEach(() => {
357+
logSpy.mockRestore();
358+
errorSpy.mockRestore();
359+
});
360+
361+
it('all triggers succeed (queued) → resolves without error, exit 0; results all queued, no error (json)', async () => {
362+
const { credentialsPath } = makeCreds();
363+
const testIds = ['test_q1', 'test_q2', 'test_q3'];
364+
const plansFile = writePlansJsonl([FE_SPEC, FE_SPEC, FE_SPEC]);
365+
366+
let pollCount = 0;
367+
const fetchImpl = makeFetch(url => {
368+
if (url.includes('/tests/batch')) {
369+
return { body: makeBatchCreateResponse(testIds) };
370+
}
371+
const triggerMatch = /\/tests\/(test_[a-z0-9]+)\/runs$/.exec(url);
372+
if (triggerMatch?.[1]) {
373+
const testId = triggerMatch[1];
374+
return { body: makeTriggerResponse(testId, `run_${testId}`) };
375+
}
376+
// No --wait must NOT poll — count any GET /runs as a violation.
377+
if (/\/runs\/run_test_[a-z0-9]+/.exec(url)) {
378+
pollCount++;
379+
}
380+
return {
381+
status: 404,
382+
body: {
383+
error: { code: 'NOT_FOUND', message: 'not found', nextAction: '', requestId: 'r1' },
384+
},
385+
};
386+
});
387+
388+
const stdout: string[] = [];
389+
const stderrLines: string[] = [];
390+
391+
// Must NOT throw — a fully successful no-wait dispatch exits 0.
392+
// (If runBatchRun threw a CLIError, this await would reject and fail the test.)
393+
await runCreateBatch(
394+
{
395+
profile: 'default',
396+
output: 'json',
397+
debug: false,
398+
dryRun: false,
399+
plans: plansFile,
400+
run: true,
401+
wait: false,
402+
timeoutSeconds: 60,
403+
},
404+
{
405+
credentialsPath,
406+
fetchImpl,
407+
stdout: line => stdout.push(line),
408+
stderr: line => stderrLines.push(line),
409+
sleep: instantSleep,
410+
},
411+
);
412+
413+
expect(pollCount).toBe(0); // no polling without --wait
414+
415+
const printed = JSON.parse(stdout.join('')) as {
416+
results: Array<{ testId: string; status: string; error?: unknown }>;
417+
};
418+
expect(printed.results).toHaveLength(3);
419+
expect(printed.results.every(r => r.status === 'queued')).toBe(true);
420+
expect(printed.results.every(r => r.error === undefined)).toBe(true);
421+
});
422+
423+
it('all triggers succeed (queued) → text summary reports "3/3 triggered", exit 0 (no "0/N passed")', async () => {
424+
const { credentialsPath } = makeCreds();
425+
const testIds = ['test_t1', 'test_t2', 'test_t3'];
426+
const plansFile = writePlansJsonl([FE_SPEC, FE_SPEC, FE_SPEC]);
427+
428+
const fetchImpl = makeFetch(url => {
429+
if (url.includes('/tests/batch')) {
430+
return { body: makeBatchCreateResponse(testIds) };
431+
}
432+
const triggerMatch = /\/tests\/(test_[a-z0-9]+)\/runs$/.exec(url);
433+
if (triggerMatch?.[1]) {
434+
const testId = triggerMatch[1];
435+
return { body: makeTriggerResponse(testId, `run_${testId}`) };
436+
}
437+
return {
438+
status: 404,
439+
body: {
440+
error: { code: 'NOT_FOUND', message: 'not found', nextAction: '', requestId: 'r1' },
441+
},
442+
};
443+
});
444+
445+
const stdout: string[] = [];
446+
const stderrLines: string[] = [];
447+
448+
// Must NOT throw — a fully successful no-wait dispatch exits 0.
449+
await runCreateBatch(
450+
{
451+
profile: 'default',
452+
output: 'text',
453+
debug: false,
454+
dryRun: false,
455+
plans: plansFile,
456+
run: true,
457+
wait: false,
458+
timeoutSeconds: 60,
459+
},
460+
{
461+
credentialsPath,
462+
fetchImpl,
463+
stdout: line => stdout.push(line),
464+
stderr: line => stderrLines.push(line),
465+
sleep: instantSleep,
466+
},
467+
);
468+
469+
const summary = stderrLines.find(l => l.startsWith('batch-run summary:'));
470+
expect(summary).toBeDefined();
471+
expect(summary).toContain('3/3 triggered');
472+
// The pre-fix bug printed a pass/fail summary ("0/3 passed") for a fully
473+
// successful no-wait dispatch — assert that misleading wording is gone.
474+
expect(summary).not.toContain('passed');
475+
expect(stderrLines.some(l => l.includes('did not pass'))).toBe(false);
476+
});
477+
478+
it('partial trigger failure (2 queued, 1 errors) → still exits non-zero; all 3 results in envelope', async () => {
479+
const { credentialsPath } = makeCreds();
480+
const testIds = ['test_p1', 'test_p2', 'test_p3'];
481+
const plansFile = writePlansJsonl([FE_SPEC, FE_SPEC, FE_SPEC]);
482+
483+
// test_p3's trigger returns 404 NOT_FOUND — a non-retryable error (exit 4)
484+
// that surfaces immediately as an error result. The other two dispatch fine.
485+
const fetchImpl = makeFetch(url => {
486+
if (url.includes('/tests/batch')) {
487+
return { body: makeBatchCreateResponse(testIds) };
488+
}
489+
const triggerMatch = /\/tests\/(test_[a-z0-9]+)\/runs$/.exec(url);
490+
if (triggerMatch?.[1]) {
491+
const testId = triggerMatch[1];
492+
if (testId === 'test_p3') {
493+
return {
494+
status: 404,
495+
body: {
496+
error: {
497+
code: 'NOT_FOUND',
498+
message: 'test not found',
499+
nextAction: '',
500+
requestId: 'req_p3',
501+
},
502+
},
503+
};
504+
}
505+
return { body: makeTriggerResponse(testId, `run_${testId}`) };
506+
}
507+
return {
508+
status: 404,
509+
body: {
510+
error: { code: 'NOT_FOUND', message: 'not found', nextAction: '', requestId: 'r1' },
511+
},
512+
};
513+
});
514+
515+
const stdout: string[] = [];
516+
const stderrLines: string[] = [];
517+
518+
const err = await runCreateBatch(
519+
{
520+
profile: 'default',
521+
output: 'json',
522+
debug: false,
523+
dryRun: false,
524+
plans: plansFile,
525+
run: true,
526+
wait: false,
527+
timeoutSeconds: 60,
528+
},
529+
{
530+
credentialsPath,
531+
fetchImpl,
532+
stdout: line => stdout.push(line),
533+
stderr: line => stderrLines.push(line),
534+
sleep: instantSleep,
535+
},
536+
).catch(e => e);
537+
538+
// A dispatch with any trigger error must exit non-zero.
539+
expect(err).toBeInstanceOf(CLIError);
540+
expect((err as CLIError).exitCode).not.toBe(0);
541+
542+
const printed = JSON.parse(stdout.join('')) as {
543+
results: Array<{ testId: string; status: string; error?: { code: string } }>;
544+
};
545+
expect(printed.results).toHaveLength(3);
546+
const queued = printed.results.filter(r => r.status === 'queued');
547+
const errored = printed.results.filter(r => r.error !== undefined);
548+
expect(queued).toHaveLength(2);
549+
expect(errored).toHaveLength(1);
550+
expect(errored[0]?.testId).toBe('test_p3');
551+
expect(errored[0]?.error?.code).toBe('NOT_FOUND');
552+
});
553+
});
554+
336555
// ---------------------------------------------------------------------------
337556
// --max-concurrency: verify only N in-flight at any time
338557
// ---------------------------------------------------------------------------

src/commands/test.ts

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2888,6 +2888,17 @@ async function runBatchRun(
28882888
})
28892889
: batchRunResults;
28902890
out.print({ results: enrichedResults });
2891+
} else if (!opts.wait) {
2892+
// Text mode, no --wait: statuses are non-terminal by design ('queued'),
2893+
// so a pass/fail summary would misread a successful dispatch as
2894+
// "0/N passed". Report what actually happened: triggers.
2895+
const total = batchRunResults.length;
2896+
const erroredCount = batchRunResults.filter(r => r.error !== undefined).length;
2897+
const parts = [`${total - erroredCount}/${total} triggered`];
2898+
if (erroredCount > 0) {
2899+
parts.push(`${erroredCount} trigger error${erroredCount !== 1 ? 's' : ''}`);
2900+
}
2901+
stderrFn(`batch-run summary: ${parts.join(', ')}`);
28912902
} else {
28922903
// Text mode: print summary line.
28932904
const passed = batchRunResults.filter(r => r.status === 'passed').length;
@@ -2906,29 +2917,32 @@ async function runBatchRun(
29062917
stderrFn(`batch-run summary: ${parts.join(', ')}`);
29072918
}
29082919

2909-
// Determine exit code.
2910-
const allPassed = batchRunResults.every(r => r.status === 'passed');
2911-
if (allPassed) return; // exit 0
2920+
// Determine exit code. With --wait, success means every run reached the
2921+
// terminal status 'passed'. Without --wait, statuses are non-terminal by
2922+
// design ('queued' per CliBatchRunResult), so success means every trigger
2923+
// dispatched without error — mirroring single `test run` (no --wait),
2924+
// which exits 0 on a successful queued dispatch.
2925+
const failing = opts.wait
2926+
? batchRunResults.filter(r => r.status !== 'passed')
2927+
: batchRunResults.filter(r => r.error !== undefined);
2928+
if (failing.length === 0) return; // exit 0
29122929

2913-
// Check for a uniform non-pass exit code across all non-passed results.
2914-
const errorExitCodes = batchRunResults
2915-
.filter(r => r.error !== undefined)
2916-
.map(r => r.error!.exitCode);
2917-
const nonPassedStatuses = batchRunResults.filter(r => r.status !== 'passed');
2930+
// Check for a uniform non-pass exit code across all failing results.
2931+
const errorExitCodes = failing.filter(r => r.error !== undefined).map(r => r.error!.exitCode);
29182932
// Exit 7 only when EVERY run timed out — a mix of pass + timeout is "mixed
2919-
// outcomes" (exit 1), not "all timed out". `nonPassedStatuses.every(...)`
2933+
// outcomes" (exit 1), not "all timed out". `failing.every(...)` alone
29202934
// would incorrectly fire exit 7 when 1 of N passed and the rest timed out.
29212935
const allTimeout =
2922-
batchRunResults.length > 0 &&
2936+
failing.length === batchRunResults.length &&
29232937
batchRunResults.every(r => r.status === 'timeout' || r.error?.exitCode === 7);
29242938
if (allTimeout) {
29252939
throw new CLIError(
29262940
`All ${batchRunResults.length} batch run(s) timed out after ${timeoutSeconds}s.`,
29272941
7,
29282942
);
29292943
}
2930-
// If all non-passed results share the same specific exit code (6 or 11), use it.
2931-
if (errorExitCodes.length > 0 && errorExitCodes.length === nonPassedStatuses.length) {
2944+
// If all failing results share the same specific exit code (6 or 11), use it.
2945+
if (errorExitCodes.length > 0 && errorExitCodes.length === failing.length) {
29322946
const uniformCode = errorExitCodes[0];
29332947
if (
29342948
uniformCode !== undefined &&
@@ -2937,14 +2951,16 @@ async function runBatchRun(
29372951
uniformCode !== 7
29382952
) {
29392953
throw new CLIError(
2940-
`Batch run finished: ${nonPassedStatuses.length} run(s) failed with exit code ${uniformCode}.`,
2954+
`Batch run finished: ${failing.length} run(s) failed with exit code ${uniformCode}.`,
29412955
uniformCode,
29422956
);
29432957
}
29442958
}
29452959
// Default: mixed outcomes or generic failure → exit 1.
29462960
throw new CLIError(
2947-
`Batch run finished: ${batchRunResults.filter(r => r.status !== 'passed').length} of ${batchRunResults.length} run(s) did not pass.`,
2961+
opts.wait
2962+
? `Batch run finished: ${failing.length} of ${batchRunResults.length} run(s) did not pass.`
2963+
: `Batch run trigger finished: ${failing.length} of ${batchRunResults.length} trigger(s) failed.`,
29482964
1,
29492965
);
29502966
}

0 commit comments

Comments
 (0)