From 82d09022b3a7c96e10dfffc8c14cdbc1a79730a2 Mon Sep 17 00:00:00 2001 From: Alisue Date: Thu, 8 Jan 2026 22:41:32 +0900 Subject: [PATCH 1/2] refactor(@probitas/runner): improve timeout error handling and retry logic Simplified timeout signal creation and enhanced error context tracking through static factory methods and step information enrichment. Changes: - Add timeoutSignal() static methods to timeout error classes with Disposable interface for automatic cleanup via 'using' declarations - Enhance ScenarioTimeoutError to track which step was executing when timeout occurred (name and index) - Refactor retry() to accept shouldRetry callback for caller-controlled error classification without coupling to specific error types - Preserve raw error types through retry (unknown instead of Error) to maintain type information and avoid lossy conversions - Remove isTimeoutError() and ErrorWithRetryMetadata as they're no longer needed with the new architecture - Add comprehensive tests for timeout signal creation, error enrichment, and retry behavior (19 new test cases) This separates concerns between timeout signal creation (error classes), retry logic (retry utility), and error classification (callers via shouldRetry), improving maintainability and testability. --- packages/probitas-runner/errors.ts | 99 ++++++++++- packages/probitas-runner/errors_test.ts | 166 ++++++++++++++++++ packages/probitas-runner/runner.ts | 67 ++----- packages/probitas-runner/runner_test.ts | 120 +++++++++++-- packages/probitas-runner/scenario_runner.ts | 30 +++- .../probitas-runner/scenario_runner_test.ts | 4 +- packages/probitas-runner/step_runner.ts | 108 ++++++++---- packages/probitas-runner/step_runner_test.ts | 6 +- packages/probitas-runner/utils/retry.ts | 58 +++--- packages/probitas-runner/utils/retry_test.ts | 128 +++++++++++++- 10 files changed, 642 insertions(+), 144 deletions(-) create mode 100644 packages/probitas-runner/errors_test.ts diff --git a/packages/probitas-runner/errors.ts b/packages/probitas-runner/errors.ts index dc0b4e6..b1143f8 100644 --- a/packages/probitas-runner/errors.ts +++ b/packages/probitas-runner/errors.ts @@ -31,35 +31,128 @@ export class StepTimeoutError extends Error { this.attemptNumber = attemptNumber; this.elapsedMs = elapsedMs; } + + /** + * Creates an AbortSignal that aborts with StepTimeoutError after the specified timeout. + * + * @param timeout - Timeout duration in milliseconds + * @param params - Step information for the error + * @returns AbortSignal with Disposable interface for cleanup + */ + static timeoutSignal( + timeout: number, + params: { + stepName: string; + attemptNumber: number; + }, + ): AbortSignal & Disposable { + const controller = new AbortController(); + const startTime = performance.now(); + const timeoutId = setTimeout(() => { + const elapsedMs = Math.round(performance.now() - startTime); + const error = new StepTimeoutError( + params.stepName, + timeout, + params.attemptNumber, + elapsedMs, + ); + controller.abort(error); + }, timeout); + + // Cleanup timeout when signal is aborted by other means + controller.signal.addEventListener("abort", () => { + clearTimeout(timeoutId); + }, { once: true }); + + // Add Disposable interface for manual cleanup + return Object.assign(controller.signal, { + [Symbol.dispose]: () => { + clearTimeout(timeoutId); + }, + }); + } } /** * Error thrown when a scenario execution times out. * * Contains structured information about the timeout including - * the scenario name, timeout duration, and total elapsed time. + * the scenario name, timeout duration, total elapsed time, + * and optionally the step being executed when timeout occurred. */ export class ScenarioTimeoutError extends Error { override readonly name = "ScenarioTimeoutError"; readonly scenarioName: string; readonly timeoutMs: number; readonly elapsedMs: number; + readonly currentStepName?: string; + readonly currentStepIndex?: number; constructor( scenarioName: string, timeoutMs: number, elapsedMs: number, - options?: ErrorOptions, + options?: ErrorOptions & { + currentStepName?: string; + currentStepIndex?: number; + }, ) { + const stepMsg = options?.currentStepName + ? ` while executing step "${options.currentStepName}"${ + options.currentStepIndex !== undefined + ? ` (step ${options.currentStepIndex + 1})` + : "" + }` + : ""; const elapsedMsg = elapsedMs !== timeoutMs ? `, total elapsed: ${elapsedMs}ms` : ""; super( - `Scenario "${scenarioName}" timed out after ${timeoutMs}ms${elapsedMsg}`, + `Scenario "${scenarioName}" timed out after ${timeoutMs}ms${stepMsg}${elapsedMsg}`, options, ); this.scenarioName = scenarioName; this.timeoutMs = timeoutMs; this.elapsedMs = elapsedMs; + this.currentStepName = options?.currentStepName; + this.currentStepIndex = options?.currentStepIndex; + } + + /** + * Creates an AbortSignal that aborts with ScenarioTimeoutError after the specified timeout. + * + * @param timeout - Timeout duration in milliseconds + * @param params - Scenario information for the error + * @returns AbortSignal with Disposable interface for cleanup + */ + static timeoutSignal( + timeout: number, + params: { + scenarioName: string; + }, + ): AbortSignal & Disposable { + const controller = new AbortController(); + const startTime = performance.now(); + const timeoutId = setTimeout(() => { + const elapsedMs = Math.round(performance.now() - startTime); + const error = new ScenarioTimeoutError( + params.scenarioName, + timeout, + elapsedMs, + ); + controller.abort(error); + }, timeout); + + // Cleanup timeout when signal is aborted by other means + controller.signal.addEventListener("abort", () => { + clearTimeout(timeoutId); + }, { once: true }); + + // Add Disposable interface for manual cleanup + return Object.assign(controller.signal, { + [Symbol.dispose]: () => { + clearTimeout(timeoutId); + }, + }); } } diff --git a/packages/probitas-runner/errors_test.ts b/packages/probitas-runner/errors_test.ts new file mode 100644 index 0000000..aeefedb --- /dev/null +++ b/packages/probitas-runner/errors_test.ts @@ -0,0 +1,166 @@ +import { expect } from "@std/expect"; +import { delay } from "@std/async/delay"; +import { ScenarioTimeoutError, StepTimeoutError } from "./errors.ts"; + +Deno.test("ScenarioTimeoutError - constructor creates error with correct properties", () => { + const error = new ScenarioTimeoutError("test-scenario", 5000, 5100); + + expect(error.name).toBe("ScenarioTimeoutError"); + expect(error.scenarioName).toBe("test-scenario"); + expect(error.timeoutMs).toBe(5000); + expect(error.elapsedMs).toBe(5100); + expect(error.currentStepName).toBeUndefined(); + expect(error.currentStepIndex).toBeUndefined(); + expect(error.message).toBe( + 'Scenario "test-scenario" timed out after 5000ms, total elapsed: 5100ms', + ); +}); + +Deno.test("ScenarioTimeoutError - constructor with step information", () => { + const error = new ScenarioTimeoutError("test-scenario", 5000, 5100, { + currentStepName: "slow-step", + currentStepIndex: 2, + }); + + expect(error.currentStepName).toBe("slow-step"); + expect(error.currentStepIndex).toBe(2); + expect(error.message).toBe( + 'Scenario "test-scenario" timed out after 5000ms while executing step "slow-step" (step 3), total elapsed: 5100ms', + ); +}); + +Deno.test("ScenarioTimeoutError - constructor with only step name", () => { + const error = new ScenarioTimeoutError("test-scenario", 5000, 5000, { + currentStepName: "slow-step", + }); + + expect(error.currentStepName).toBe("slow-step"); + expect(error.currentStepIndex).toBeUndefined(); + expect(error.message).toBe( + 'Scenario "test-scenario" timed out after 5000ms while executing step "slow-step"', + ); +}); + +Deno.test("ScenarioTimeoutError.timeoutSignal - creates signal that aborts after timeout", async () => { + using signal = ScenarioTimeoutError.timeoutSignal(50, { + scenarioName: "test-scenario", + }); + + expect(signal.aborted).toBe(false); + + await delay(100); + + expect(signal.aborted).toBe(true); + expect(signal.reason).toBeInstanceOf(ScenarioTimeoutError); + + const error = signal.reason as ScenarioTimeoutError; + expect(error.scenarioName).toBe("test-scenario"); + expect(error.timeoutMs).toBe(50); + expect(error.elapsedMs).toBeGreaterThanOrEqual(50); + expect(error.elapsedMs).toBeLessThan(200); +}); + +Deno.test("ScenarioTimeoutError.timeoutSignal - cleanup via dispose", async () => { + const signal = ScenarioTimeoutError.timeoutSignal(1000, { + scenarioName: "test-scenario", + }); + + expect(signal.aborted).toBe(false); + + // Dispose immediately to cleanup timer + signal[Symbol.dispose](); + + await delay(50); + + // Should not abort because timer was cleaned up + expect(signal.aborted).toBe(false); +}); + +Deno.test("ScenarioTimeoutError.timeoutSignal - cleanup via using declaration", async () => { + { + using signal = ScenarioTimeoutError.timeoutSignal(1000, { + scenarioName: "test-scenario", + }); + + expect(signal.aborted).toBe(false); + } // signal is disposed here + + await delay(50); + + // Timer should be cleaned up, no leak +}); + +Deno.test("StepTimeoutError - constructor creates error with correct properties", () => { + const error = new StepTimeoutError("test-step", 3000, 2, 3200); + + expect(error.name).toBe("StepTimeoutError"); + expect(error.stepName).toBe("test-step"); + expect(error.timeoutMs).toBe(3000); + expect(error.attemptNumber).toBe(2); + expect(error.elapsedMs).toBe(3200); + expect(error.message).toBe( + 'Step "test-step" timed out after 3000ms (attempt 2), total elapsed: 3200ms', + ); +}); + +Deno.test("StepTimeoutError - constructor for first attempt", () => { + const error = new StepTimeoutError("test-step", 3000, 1, 3000); + + expect(error.attemptNumber).toBe(1); + expect(error.message).toBe( + 'Step "test-step" timed out after 3000ms', + ); +}); + +Deno.test("StepTimeoutError.timeoutSignal - creates signal that aborts after timeout", async () => { + using signal = StepTimeoutError.timeoutSignal(50, { + stepName: "test-step", + attemptNumber: 1, + }); + + expect(signal.aborted).toBe(false); + + await delay(100); + + expect(signal.aborted).toBe(true); + expect(signal.reason).toBeInstanceOf(StepTimeoutError); + + const error = signal.reason as StepTimeoutError; + expect(error.stepName).toBe("test-step"); + expect(error.timeoutMs).toBe(50); + expect(error.attemptNumber).toBe(1); + expect(error.elapsedMs).toBeGreaterThanOrEqual(50); + expect(error.elapsedMs).toBeLessThan(200); +}); + +Deno.test("StepTimeoutError.timeoutSignal - cleanup via dispose", async () => { + const signal = StepTimeoutError.timeoutSignal(1000, { + stepName: "test-step", + attemptNumber: 1, + }); + + expect(signal.aborted).toBe(false); + + // Dispose immediately to cleanup timer + signal[Symbol.dispose](); + + await delay(50); + + // Should not abort because timer was cleaned up + expect(signal.aborted).toBe(false); +}); + +Deno.test("StepTimeoutError.timeoutSignal - measures elapsed time correctly", async () => { + using signal = StepTimeoutError.timeoutSignal(50, { + stepName: "test-step", + attemptNumber: 1, + }); + + await delay(100); + + const error = signal.reason as StepTimeoutError; + // Elapsed time should be approximately 50ms (actual timeout duration) + // with some tolerance for timer precision + expect(error.elapsedMs).toBeGreaterThanOrEqual(50); + expect(error.elapsedMs).toBeLessThan(150); +}); diff --git a/packages/probitas-runner/runner.ts b/packages/probitas-runner/runner.ts index e70d394..666572c 100644 --- a/packages/probitas-runner/runner.ts +++ b/packages/probitas-runner/runner.ts @@ -10,7 +10,7 @@ import { ScenarioRunner } from "./scenario_runner.ts"; import { toScenarioMetadata } from "./metadata.ts"; import { timeit } from "./utils/timeit.ts"; import { mergeSignals } from "./utils/signal.ts"; -import { ScenarioTimeoutError, StepTimeoutError } from "./errors.ts"; +import { ScenarioTimeoutError } from "./errors.ts"; /** * Top-level test runner that orchestrates execution of multiple scenarios. @@ -118,13 +118,16 @@ export class Runner { timeout: number, signal?: AbortSignal, ): Promise { - const timeoutSignal = mergeSignals( - signal, - AbortSignal.timeout(timeout), - ); + // Create timeout signal that aborts with ScenarioTimeoutError + using timeoutSignal = ScenarioTimeoutError.timeoutSignal(timeout, { + scenarioName: scenario.name, + }); + + // Merge with external signal + const mergedSignal = mergeSignals(signal, timeoutSignal); const result = await timeit(() => - scenarioRunner.run(scenario, { signal: timeoutSignal }) + scenarioRunner.run(scenario, { signal: mergedSignal }) ); // Handle timeit result @@ -133,25 +136,7 @@ export class Runner { throw result.error; } - const scenarioResult = result.value; - - // Check if scenario failed due to timeout - if ( - scenarioResult.status === "failed" && - isTimeoutError(scenarioResult.error, timeoutSignal) - ) { - return { - ...scenarioResult, - error: new ScenarioTimeoutError( - scenario.name, - timeout, - result.duration, - { cause: scenarioResult.error }, - ), - }; - } - - return scenarioResult; + return result.value; } async #run( @@ -199,35 +184,3 @@ export class Runner { } } } - -function isTimeoutError(error: unknown, signal?: AbortSignal): boolean { - // Direct TimeoutError - if (error instanceof DOMException && error.name === "TimeoutError") { - return true; - } - - // StepTimeoutError - if (error instanceof StepTimeoutError) { - return true; - } - - // AbortError caused by timeout - if ( - error instanceof DOMException && - error.name === "AbortError" && - signal?.aborted && - signal.reason instanceof DOMException && - signal.reason.name === "TimeoutError" - ) { - return true; - } - - return false; -} - -/** - * @internal - */ -export const _internal = { - isTimeoutError, -}; diff --git a/packages/probitas-runner/runner_test.ts b/packages/probitas-runner/runner_test.ts index b52462d..51e8d38 100644 --- a/packages/probitas-runner/runner_test.ts +++ b/packages/probitas-runner/runner_test.ts @@ -7,29 +7,12 @@ import { TestReporter, } from "./_testutils.ts"; import { Skip } from "./skip.ts"; -import { _internal, Runner } from "./runner.ts"; +import { Runner } from "./runner.ts"; import { createScopedSignal } from "./utils/signal.ts"; import { ScenarioTimeoutError } from "./errors.ts"; const reporter = new TestReporter(); -Deno.test("isTimeoutError detects TimeoutError", () => { - const error = new DOMException("Timeout", "TimeoutError"); - expect(_internal.isTimeoutError(error)).toBe(true); -}); - -Deno.test("isTimeoutError should detect AbortError caused by timeout", async () => { - const signal = AbortSignal.timeout(10); - - // Wait for signal to abort - await delay(20); - - const error = new DOMException("Aborted", "AbortError"); - - // AbortError should be detected as timeout error when signal.reason is TimeoutError - expect(_internal.isTimeoutError(error, signal)).toBe(true); -}); - Deno.test("Runner run runs single scenario", async () => { const runner = new Runner(reporter); const scenarios = [ @@ -453,3 +436,104 @@ Deno.test({ } }, }); + +Deno.test({ + name: "Runner run with timeout includes step info in ScenarioTimeoutError", + sanitizeResources: false, + sanitizeOps: false, + async fn() { + const runner = new Runner(reporter); + const scenarios = [ + createTestScenario({ + name: "Timeout with step info", + steps: [ + createTestStep({ + name: "fast-step", + fn: async (ctx) => { + await delay(10, { signal: ctx.signal }); + return "done"; + }, + }), + createTestStep({ + name: "slow-step", + fn: async (ctx) => { + await delay(200, { signal: ctx.signal }); + return "should-timeout"; + }, + }), + createTestStep({ + name: "never-reached", + fn: () => "not-reached", + }), + ], + }), + ]; + + const summary = await runner.run(scenarios, { + timeout: 50, + }); + + expect(summary).toMatchObject({ + total: 1, + passed: 0, + skipped: 0, + failed: 1, + }); + + const scenarioResult = summary.scenarios[0]; + expect(scenarioResult.status).toBe("failed"); + if (scenarioResult.status === "failed") { + expect(scenarioResult.error).toBeInstanceOf(ScenarioTimeoutError); + const timeoutError = scenarioResult.error as ScenarioTimeoutError; + expect(timeoutError.scenarioName).toBe("Timeout with step info"); + expect(timeoutError.timeoutMs).toBe(50); + expect(timeoutError.currentStepName).toBe("slow-step"); + expect(timeoutError.currentStepIndex).toBe(1); + expect(timeoutError.message).toContain( + 'while executing step "slow-step"', + ); + expect(timeoutError.message).toContain("(step 2)"); + } + }, +}); + +Deno.test({ + name: "Runner run with timeout on first step includes step index 0", + sanitizeResources: false, + sanitizeOps: false, + async fn() { + const runner = new Runner(reporter); + const scenarios = [ + createTestScenario({ + name: "First step timeout", + steps: [ + createTestStep({ + name: "first-slow-step", + fn: async (ctx) => { + await delay(200, { signal: ctx.signal }); + return "should-timeout"; + }, + }), + createTestStep({ + name: "never-reached", + fn: () => "not-reached", + }), + ], + }), + ]; + + const summary = await runner.run(scenarios, { + timeout: 50, + }); + + const scenarioResult = summary.scenarios[0]; + expect(scenarioResult.status).toBe("failed"); + if (scenarioResult.status === "failed") { + expect(scenarioResult.error).toBeInstanceOf(ScenarioTimeoutError); + const timeoutError = scenarioResult.error as ScenarioTimeoutError; + expect(timeoutError.currentStepName).toBe("first-slow-step"); + expect(timeoutError.currentStepIndex).toBe(0); + expect(timeoutError.message).toContain("(step 1)"); + } + }, +}); diff --git a/packages/probitas-runner/scenario_runner.ts b/packages/probitas-runner/scenario_runner.ts index 11e9430..35b9bdc 100644 --- a/packages/probitas-runner/scenario_runner.ts +++ b/packages/probitas-runner/scenario_runner.ts @@ -5,6 +5,7 @@ import { StepRunner } from "./step_runner.ts"; import { toScenarioMetadata } from "./metadata.ts"; import { timeit } from "./utils/timeit.ts"; import { createScenarioContext } from "./context.ts"; +import { ScenarioTimeoutError } from "./errors.ts"; export interface RunOptions { readonly signal?: AbortSignal; @@ -61,14 +62,33 @@ export class ScenarioRunner { this.#stepOptions, ); - for (const step of scenario.steps) { + for (let stepIndex = 0; stepIndex < scenario.steps.length; stepIndex++) { + const step = scenario.steps[stepIndex]; + signal?.throwIfAborted(); - const stepResult = await stepRunner.run(step, stack); - stepResults.push(stepResult); + try { + const stepResult = await stepRunner.run(step, stack); + stepResults.push(stepResult); - if (stepResult.status !== "passed") { - throw stepResult.error; + if (stepResult.status !== "passed") { + throw stepResult.error; + } + } catch (error) { + // Enrich ScenarioTimeoutError with current step information + if (error instanceof ScenarioTimeoutError) { + throw new ScenarioTimeoutError( + error.scenarioName, + error.timeoutMs, + error.elapsedMs, + { + cause: error, + currentStepName: step.name, + currentStepIndex: stepIndex, + }, + ); + } + throw error; } } } diff --git a/packages/probitas-runner/scenario_runner_test.ts b/packages/probitas-runner/scenario_runner_test.ts index 2475de7..855e0b4 100644 --- a/packages/probitas-runner/scenario_runner_test.ts +++ b/packages/probitas-runner/scenario_runner_test.ts @@ -64,7 +64,7 @@ Deno.test("ScenarioRunner run runs scenario with single step (fail)", async () = status: "failed", steps: [{ status: "failed", - error: new Error("ng"), + error: "ng", }], metadata: { name: "Test Scenario", @@ -139,7 +139,7 @@ Deno.test("ScenarioRunner run runs scenario with multiple steps (fail)", async ( }, { status: "failed", - error: new Error("2"), + error: "2", }, ], metadata: { diff --git a/packages/probitas-runner/step_runner.ts b/packages/probitas-runner/step_runner.ts index 804d3a7..a225beb 100644 --- a/packages/probitas-runner/step_runner.ts +++ b/packages/probitas-runner/step_runner.ts @@ -13,7 +13,7 @@ import { retry } from "./utils/retry.ts"; import { createStepContext } from "./context.ts"; import { mergeSignals } from "./utils/signal.ts"; import { Skip } from "./skip.ts"; -import { StepTimeoutError } from "./errors.ts"; +import { ScenarioTimeoutError, StepTimeoutError } from "./errors.ts"; const DEFAULT_STEP_TIMEOUT = 30000; const DEFAULT_STEP_RETRY_MAX_ATTEMPTS = 1; @@ -23,14 +23,6 @@ type ResourceStep = StepDefinition & { kind: "resource" }; type SetupStep = StepDefinition & { kind: "setup" }; type ExecutionStep = StepDefinition & { kind: "step" }; -/** - * Error with retry metadata attached - * @internal - */ -interface ErrorWithRetryMetadata extends Error { - __retryAttemptNumber?: number; -} - export class StepRunner { #reporter: Reporter; #scenarioMetadata: ScenarioMetadata; @@ -61,32 +53,92 @@ export class StepRunner { const timeout = this.#resolveTimeout(step); const retryConfig = this.#resolveRetry(step); - const signal = mergeSignals( - ctx.signal, - AbortSignal.timeout(timeout), - ); const result = await timeit(() => { return retry( - () => deadline(this.#run(ctx, step, stack), timeout, { signal }), + async (attempt) => { + // Create timeout signal for each retry attempt + using attemptTimeoutSignal = StepTimeoutError.timeoutSignal(timeout, { + stepName: step.name, + attemptNumber: attempt, + }); + const attemptSignal = mergeSignals(ctx.signal, attemptTimeoutSignal); + + // Create attempt-specific context with timeout signal + const attemptCtx = { ...ctx, signal: attemptSignal }; + + return await deadline( + this.#run(attemptCtx, step, stack), + timeout, + { signal: attemptSignal }, + ); + }, { ...retryConfig, - signal, + signal: ctx.signal, // For canceling retry delays only + shouldRetry: (error) => { + // Don't retry on timeout errors - they indicate the operation is too slow + // for the configured timeout, so retrying with the same timeout will fail again. + if ( + error instanceof StepTimeoutError || + error instanceof ScenarioTimeoutError + ) { + return false; + } + return true; + }, }, ); }); - // Enrich timeout errors with retry context and elapsed time + // Process timeout errors let error = result.status === "failed" ? result.error : undefined; - if (error && isTimeoutError(error)) { - const attemptNumber = - (error as ErrorWithRetryMetadata).__retryAttemptNumber ?? 1; - error = new StepTimeoutError( - step.name, - timeout, - attemptNumber, - result.duration, - { cause: error }, - ); + if (error) { + // Check for ScenarioTimeoutError first (from signal or thrown) + if ( + error instanceof ScenarioTimeoutError || + ctx.signal?.reason instanceof ScenarioTimeoutError + ) { + // Use the error with step info if available, otherwise enrich it + const scenarioError = error instanceof ScenarioTimeoutError + ? error + : ctx.signal!.reason as ScenarioTimeoutError; + + // If not already enriched with step info, add current step name + if (!scenarioError.currentStepName) { + error = new ScenarioTimeoutError( + scenarioError.scenarioName, + scenarioError.timeoutMs, + scenarioError.elapsedMs, + { + cause: scenarioError, + currentStepName: step.name, + }, + ); + } else { + // Already enriched, use as-is + error = scenarioError; + } + } else if (error instanceof StepTimeoutError) { + // Already enriched StepTimeoutError from signal + // Just update duration + error = new StepTimeoutError( + error.stepName, + error.timeoutMs, + error.attemptNumber, + result.duration, + { cause: error }, + ); + } else if ((error as Error).cause instanceof StepTimeoutError) { + // StepTimeoutError wrapped in another error (e.g., from signal abort) + const cause = (error as Error).cause as StepTimeoutError; + error = new StepTimeoutError( + cause.stepName, + cause.timeoutMs, + cause.attemptNumber, + result.duration, + { cause }, + ); + } } const stepResult: StepResult = result.status === "passed" @@ -202,7 +254,3 @@ function isDisposable(x: unknown): x is Disposable | AsyncDisposable { (Symbol.asyncDispose in x || Symbol.dispose in x) ); } - -function isTimeoutError(error: unknown): boolean { - return error instanceof DOMException && error.name === "TimeoutError"; -} diff --git a/packages/probitas-runner/step_runner_test.ts b/packages/probitas-runner/step_runner_test.ts index 18a2c55..b25518b 100644 --- a/packages/probitas-runner/step_runner_test.ts +++ b/packages/probitas-runner/step_runner_test.ts @@ -58,7 +58,7 @@ Deno.test("StepRunner run runs resource step (fail)", async () => { assertSpyCalls(step.fn as SpyLike, 1); expect(stepResult).toMatchObject({ status: "failed", - error: new Error("bar"), + error: "bar", metadata: { kind: "resource", name: "foo", @@ -110,7 +110,7 @@ Deno.test("StepRunner run runs setup step (fail)", async () => { assertSpyCalls(step.fn as SpyLike, 1); expect(stepResult).toMatchObject({ status: "failed", - error: new Error("bar"), + error: "bar", metadata: { kind: "setup", name: "foo", @@ -162,7 +162,7 @@ Deno.test("StepRunner run runs execution step (fail)", async () => { assertSpyCalls(step.fn as SpyLike, 1); expect(stepResult).toMatchObject({ status: "failed", - error: new Error("bar"), + error: "bar", metadata: { kind: "step", name: "foo", diff --git a/packages/probitas-runner/utils/retry.ts b/packages/probitas-runner/utils/retry.ts index e958cf6..d4e281b 100644 --- a/packages/probitas-runner/utils/retry.ts +++ b/packages/probitas-runner/utils/retry.ts @@ -9,14 +9,6 @@ import { delay } from "@std/async/delay"; -/** - * Error with retry metadata attached - * @internal - */ -interface ErrorWithRetryMetadata extends Error { - __retryAttemptNumber?: number; -} - /** * Configuration options for retry behavior */ @@ -34,6 +26,17 @@ export type RetryOptions = { /** AbortSignal to cancel retry delays */ signal?: AbortSignal; + + /** + * Predicate to determine if retry should continue after an error. + * Return false to stop retrying immediately. + * + * @param error - The raw error that occurred (not converted to Error) + * @param attempt - The 1-based attempt number that just failed + * @returns true to continue retrying, false to stop immediately + * @default Always returns true (retry all errors) + */ + shouldRetry?: (error: unknown, attempt: number) => boolean; }; /** @@ -43,7 +46,7 @@ export type RetryOptions = { * specified options. Delays between retries follow either linear or exponential backoff. * * @template T - Return type of the function - * @param fn - Function to execute (can be sync or async) + * @param fn - Function to execute (receives 1-based attempt number, can be sync or async) * @param options - Retry configuration options * @returns Promise resolving to the function's return value * @throws The last error encountered if all retry attempts fail @@ -54,7 +57,7 @@ export type RetryOptions = { * * const mockFetch = async () => ({ ok: true }); * const data = await retry( - * () => mockFetch(), + * (attempt) => mockFetch(), * { maxAttempts: 3, backoff: "exponential" } * ); * console.log(data); @@ -67,28 +70,42 @@ export type RetryOptions = { * const fetchData = async () => ({ status: "ok" }); * const controller = new AbortController(); * const data = await retry( - * () => fetchData(), + * (attempt) => fetchData(), * { maxAttempts: 5, backoff: "linear", signal: controller.signal } * ); * console.log(data); * ``` */ export async function retry( - fn: () => T | Promise, + fn: (attempt: number) => T | Promise, options: Readonly = {}, ): Promise { - const { maxAttempts = 1, backoff = "linear", signal } = options; - let lastError: Error | undefined; + const { + maxAttempts = 1, + backoff = "linear", + signal, + shouldRetry = () => true, + } = options; + let lastError: unknown; for (let attempt = 0; attempt < maxAttempts; attempt++) { + signal?.throwIfAborted(); + try { - return await fn(); + return await fn(attempt + 1); // Pass 1-based attempt number } catch (error) { - lastError = error instanceof Error ? error : new Error(String(error)); + lastError = error; - // Attach retry metadata to the error for context - // This helps StepTimeoutError include retry information - (lastError as ErrorWithRetryMetadata).__retryAttemptNumber = attempt + 1; + // Check if retry should continue based on raw error and attempt number + if (!shouldRetry(error, attempt + 1)) { + throw error; + } + + // Don't retry if the external signal (passed to retry) is aborted. + // This indicates the user cancelled the operation, so continuing is pointless. + if (signal?.aborted) { + throw error; + } if (attempt < maxAttempts - 1) { const t = backoff === "exponential" @@ -99,5 +116,6 @@ export async function retry( } } - throw lastError || new Error("Retry failed"); + // Throw the last error as-is (preserving its original type) + throw lastError; } diff --git a/packages/probitas-runner/utils/retry_test.ts b/packages/probitas-runner/utils/retry_test.ts index b3d303a..d4d0879 100644 --- a/packages/probitas-runner/utils/retry_test.ts +++ b/packages/probitas-runner/utils/retry_test.ts @@ -4,7 +4,7 @@ import { retry } from "./retry.ts"; Deno.test("retry configuration succeeds on first attempt", async () => { let attempts = 0; - const result = await retry(() => { + const result = await retry((_attempt) => { attempts++; return "success"; }); @@ -17,7 +17,7 @@ Deno.test("retry transient failures succeeds on second attempt", async () => { using time = new FakeTime(); let attempts = 0; - const promise = retry(() => { + const promise = retry((_attempt) => { attempts++; if (attempts < 2) { throw new Error("Temporary failure"); @@ -37,7 +37,7 @@ Deno.test("retry permanent failures throws error when all attempts fail", async using time = new FakeTime(); let attempts = 0; - const promise = retry(() => { + const promise = retry((_attempt) => { attempts++; throw new Error("Persistent failure"); }, { maxAttempts: 3 }); @@ -60,7 +60,7 @@ Deno.test("retry delays linear backoff timing", async () => { using time = new FakeTime(); let attempts = 0; - const promise = retry(() => { + const promise = retry((_attempt) => { attempts++; throw new Error("Always fails"); }, { maxAttempts: 3, backoff: "linear" }); @@ -87,7 +87,7 @@ Deno.test("retry delays exponential backoff timing", async () => { using time = new FakeTime(); let attempts = 0; - const promise = retry(() => { + const promise = retry((_attempt) => { attempts++; throw new Error("Always fails"); }, { maxAttempts: 4, backoff: "exponential" }); @@ -119,7 +119,7 @@ Deno.test("retry error handling can be aborted via signal", async () => { const controller = new AbortController(); let attempts = 0; - const promise = retry(() => { + const promise = retry((_attempt) => { attempts++; throw new Error("Always fails"); }, { maxAttempts: 5, backoff: "linear", signal: controller.signal }); @@ -139,3 +139,119 @@ Deno.test("retry error handling can be aborted via signal", async () => { // Should stop early due to abort (only 1 attempt completed) assertEquals(attempts, 1); }); + +Deno.test("retry shouldRetry predicate - stops on specific error", async () => { + let attempts = 0; + + class CustomError extends Error { + constructor(message: string) { + super(message); + this.name = "CustomError"; + } + } + + await assertRejects( + () => + retry( + (_attempt) => { + attempts++; + throw new CustomError("Should not retry"); + }, + { + maxAttempts: 3, + shouldRetry: (error) => !(error instanceof CustomError), + }, + ), + CustomError, + ); + + // Should only attempt once due to shouldRetry returning false + assertEquals(attempts, 1); +}); + +Deno.test("retry shouldRetry predicate - continues on other errors", async () => { + let attempts = 0; + + class CustomError extends Error { + constructor(message: string) { + super(message); + this.name = "CustomError"; + } + } + + await assertRejects( + () => + retry( + (_attempt) => { + attempts++; + throw new Error("Normal error"); + }, + { + maxAttempts: 3, + shouldRetry: (error) => !(error instanceof CustomError), + }, + ), + Error, + "Normal error", + ); + + // Should retry all 3 attempts for normal errors + assertEquals(attempts, 3); +}); + +Deno.test("retry shouldRetry predicate - receives attempt number", async () => { + const receivedAttempts: number[] = []; + + await assertRejects( + () => + retry( + (_attempt) => { + throw new Error("Always fails"); + }, + { + maxAttempts: 3, + shouldRetry: (_error, attempt) => { + receivedAttempts.push(attempt); + return true; + }, + }, + ), + Error, + ); + + // shouldRetry should be called with attempt 1, 2, 3 + assertEquals(receivedAttempts, [1, 2, 3]); +}); + +Deno.test("retry default behavior - retries all error types", async () => { + let attempts = 0; + + await assertRejects( + () => + retry((_attempt) => { + attempts++; + throw new Error("Normal error"); + }, { maxAttempts: 3 }), + Error, + "Normal error", + ); + + // Default shouldRetry returns true, so should retry all attempts + assertEquals(attempts, 3); +}); + +Deno.test("retry passes attempt number to callback", async () => { + const attemptNumbers: number[] = []; + + await assertRejects( + () => + retry((attempt) => { + attemptNumbers.push(attempt); + throw new Error("Fail"); + }, { maxAttempts: 3 }), + Error, + ); + + // Should receive 1, 2, 3 (1-based) + assertEquals(attemptNumbers, [1, 2, 3]); +}); From a767fe9efa1725389d47c78b641d88240b0960cc Mon Sep 17 00:00:00 2001 From: Alisue Date: Thu, 8 Jan 2026 23:08:16 +0900 Subject: [PATCH 2/2] fix(@probitas/runner): abort all scenarios when maxFailures is reached Previously, when maxFailures was reached, the Runner would throw an error, leaving remaining scenarios unrecorded. This caused incomplete test results and prevented proper cleanup. Now, the Runner: - Calls controller.abort() with Skip reason when maxFailures is reached - Aborts in-progress scenarios via signal propagation (status: "skipped") - Creates skip results for unexecuted scenarios (status: "skipped") - Preserves signal.reason from external aborts (e.g., TimeoutError) This ensures all scenarios are properly recorded and reporter events are emitted consistently, providing complete test results even when stopping early due to maxFailures. --- packages/probitas-runner/runner.ts | 56 +++++- packages/probitas-runner/runner_test.ts | 223 +++++++++++++++++++++++- 2 files changed, 265 insertions(+), 14 deletions(-) diff --git a/packages/probitas-runner/runner.ts b/packages/probitas-runner/runner.ts index 666572c..7990b3c 100644 --- a/packages/probitas-runner/runner.ts +++ b/packages/probitas-runner/runner.ts @@ -11,6 +11,7 @@ import { toScenarioMetadata } from "./metadata.ts"; import { timeit } from "./utils/timeit.ts"; import { mergeSignals } from "./utils/signal.ts"; import { ScenarioTimeoutError } from "./errors.ts"; +import { Skip } from "./skip.ts"; /** * Top-level test runner that orchestrates execution of multiple scenarios. @@ -69,7 +70,6 @@ export class Runner { // Create abort controller for outer context const controller = new AbortController(); - const { signal } = controller; options?.signal?.addEventListener("abort", () => { // Pass the reason from external signal to internal controller controller.abort(options.signal?.reason); @@ -88,7 +88,7 @@ export class Runner { maxConcurrency, maxFailures, timeout, - signal, + controller, options?.stepOptions, ) ); @@ -145,20 +145,43 @@ export class Runner { maxConcurrency: number, maxFailures: number, timeout: number, - signal?: AbortSignal, + controller: AbortController, stepOptions?: StepOptions, ): Promise { // Parallel execution with concurrency control // maxConcurrency=1 means sequential execution const concurrency = maxConcurrency || scenarios.length; const scenarioRunner = new ScenarioRunner(this.reporter, stepOptions); + const { signal } = controller; let failureCount = 0; + for (const batch of chunk(scenarios, concurrency)) { - signal?.throwIfAborted(); + // Don't throw - just skip remaining batches if aborted + if (signal.aborted) { + break; + } + await Promise.all( batch.map(async (scenario: ScenarioDefinition) => { - signal?.throwIfAborted(); + // Check if already aborted (by maxFailures or external signal) + if (signal.aborted) { + const skipResult: ScenarioResult = { + status: "skipped", + metadata: toScenarioMetadata(scenario), + duration: 0, + steps: [], + error: signal.reason ?? + new Skip("Skipped due to previous failures"), + }; + scenarioResults.push(skipResult); + await this.reporter.onScenarioStart?.(skipResult.metadata); + await this.reporter.onScenarioEnd?.( + skipResult.metadata, + skipResult, + ); + return; + } // Execute scenario with optional timeout // Priority: scenario timeout > RunOptions timeout @@ -173,14 +196,35 @@ export class Runner { : await scenarioRunner.run(scenario, { signal }); scenarioResults.push(scenarioResult); + + // Check if we've reached maxFailures - abort all remaining scenarios if (scenarioResult.status === "failed") { failureCount++; if (maxFailures !== 0 && failureCount >= maxFailures) { - throw scenarioResult.error; + controller.abort(new Skip("Skipped due to previous failures")); } } }), ); } + + // Add skip results for any remaining scenarios that weren't executed + // This handles the case where we broke out of the batch loop early + const executedCount = scenarioResults.length; + if (executedCount < scenarios.length) { + for (let i = executedCount; i < scenarios.length; i++) { + const scenario = scenarios[i]; + const skipResult: ScenarioResult = { + status: "skipped", + metadata: toScenarioMetadata(scenario), + duration: 0, + steps: [], + error: signal.reason ?? new Skip("Skipped due to previous failures"), + }; + scenarioResults.push(skipResult); + await this.reporter.onScenarioStart?.(skipResult.metadata); + await this.reporter.onScenarioEnd?.(skipResult.metadata, skipResult); + } + } } } diff --git a/packages/probitas-runner/runner_test.ts b/packages/probitas-runner/runner_test.ts index 51e8d38..0be9684 100644 --- a/packages/probitas-runner/runner_test.ts +++ b/packages/probitas-runner/runner_test.ts @@ -141,7 +141,7 @@ Deno.test("Runner run runs multiple scenarios (skip)", async () => { }); }); -Deno.test("Runner run runs multiple scenarios (failed)", async () => { +Deno.test("Runner run runs multiple scenarios (failed) - aborts on maxFailures", async () => { using signal = createScopedSignal(); const runner = new Runner(reporter); const scenarios = [ @@ -150,8 +150,8 @@ Deno.test("Runner run runs multiple scenarios (failed)", async () => { steps: [ createTestStep({ name: "Step 1", - fn: async () => { - await delay(0, { signal }); + fn: async (ctx) => { + await delay(0, { signal: ctx.signal }); return "1"; }, }), @@ -162,8 +162,8 @@ Deno.test("Runner run runs multiple scenarios (failed)", async () => { steps: [ createTestStep({ name: "Step 1", - fn: async () => { - await delay(50, { signal }); + fn: async (ctx) => { + await delay(50, { signal: ctx.signal }); throw "2"; }, }), @@ -174,8 +174,8 @@ Deno.test("Runner run runs multiple scenarios (failed)", async () => { steps: [ createTestStep({ name: "Step 1", - fn: async () => { - await delay(100, { signal }); + fn: async (ctx) => { + await delay(100, { signal: ctx.signal }); return "3"; }, }), @@ -187,12 +187,219 @@ Deno.test("Runner run runs multiple scenarios (failed)", async () => { signal, maxFailures: 1, }); + + // When maxFailures is reached, all scenarios (running or pending) should be aborted + // Scenario 1 passes (fastest, 0ms delay) + // Scenario 2 fails (50ms delay, triggers maxFailures) + // Scenario 3 should be aborted/skipped (100ms delay, still running when maxFailures reached) expect(summary).toMatchObject({ total: 3, passed: 1, - skipped: 1, + skipped: 1, // Scenario 3 aborted + failed: 1, // Scenario 2 + }); +}); + +Deno.test("Runner run with maxFailures skips remaining scenarios (sequential)", async () => { + const runner = new Runner(reporter); + const executionOrder: string[] = []; + const scenarios = [ + createTestScenario({ + name: "Scenario 1", + steps: [ + createTestStep({ + name: "Step 1", + fn: () => { + executionOrder.push("Scenario 1"); + return "1"; + }, + }), + ], + }), + createTestScenario({ + name: "Scenario 2", + steps: [ + createTestStep({ + name: "Step 1", + fn: () => { + executionOrder.push("Scenario 2"); + throw new Error("Scenario 2 failed"); + }, + }), + ], + }), + createTestScenario({ + name: "Scenario 3", + steps: [ + createTestStep({ + name: "Step 1", + fn: () => { + executionOrder.push("Scenario 3"); + return "3"; + }, + }), + ], + }), + createTestScenario({ + name: "Scenario 4", + steps: [ + createTestStep({ + name: "Step 1", + fn: () => { + executionOrder.push("Scenario 4"); + return "4"; + }, + }), + ], + }), + ]; + + const summary = await runner.run(scenarios, { + maxConcurrency: 1, // Sequential execution + maxFailures: 1, + }); + + // Scenario 1 should pass, Scenario 2 should fail, Scenario 3 and 4 should be skipped + expect(summary).toMatchObject({ + total: 4, + passed: 1, failed: 1, + skipped: 2, + }); + + // Only Scenario 1 and 2 should have been executed + expect(executionOrder).toEqual(["Scenario 1", "Scenario 2"]); + + // Check scenario results + const results = summary.scenarios; + expect(results.length).toBe(4); + expect(results[0].status).toBe("passed"); + expect(results[0].metadata.name).toBe("Scenario 1"); + expect(results[1].status).toBe("failed"); + expect(results[1].metadata.name).toBe("Scenario 2"); + expect(results[2].status).toBe("skipped"); + expect(results[2].metadata.name).toBe("Scenario 3"); + expect(results[3].status).toBe("skipped"); + expect(results[3].metadata.name).toBe("Scenario 4"); +}); + +Deno.test("Runner run with maxFailures aborts in-progress scenarios (parallel)", async () => { + using signal = createScopedSignal(); + const runner = new Runner(reporter); + const executionOrder: string[] = []; + const scenarios = [ + createTestScenario({ + name: "Scenario 1", + steps: [ + createTestStep({ + name: "Step 1", + fn: async (ctx) => { + executionOrder.push("Scenario 1 start"); + try { + await delay(100, { signal: ctx.signal }); // Slow - will be aborted + executionOrder.push("Scenario 1 end"); + return "1"; + } catch (err) { + executionOrder.push("Scenario 1 aborted"); + throw err; + } + }, + }), + ], + }), + createTestScenario({ + name: "Scenario 2", + steps: [ + createTestStep({ + name: "Step 1", + fn: async (ctx) => { + executionOrder.push("Scenario 2 start"); + await delay(10, { signal: ctx.signal }); // Fast + executionOrder.push("Scenario 2 end"); + throw new Error("Scenario 2 failed"); + }, + }), + ], + }), + createTestScenario({ + name: "Scenario 3", + steps: [ + createTestStep({ + name: "Step 1", + fn: async (ctx) => { + executionOrder.push("Scenario 3 start"); + try { + await delay(100, { signal: ctx.signal }); // Slow - will be aborted + executionOrder.push("Scenario 3 end"); + return "3"; + } catch (err) { + executionOrder.push("Scenario 3 aborted"); + throw err; + } + }, + }), + ], + }), + createTestScenario({ + name: "Scenario 4", + steps: [ + createTestStep({ + name: "Step 1", + fn: () => { + executionOrder.push("Scenario 4 start"); + return "4"; + }, + }), + ], + }), + ]; + + const summary = await runner.run(scenarios, { + signal, + maxConcurrency: 3, // Scenarios 1-3 run in parallel, 4 is in next batch + maxFailures: 1, }); + + // Scenario 1, 2, 3 all start in parallel + // Scenario 2 finishes first and fails, triggers maxFailures + // Scenarios 1, 3 are aborted immediately (signal.abort called) + // Scenario 4 is in the next batch, never starts + expect(summary).toMatchObject({ + total: 4, + passed: 0, + failed: 1, // Scenario 2 + skipped: 3, // Scenarios 1, 3, 4 all skipped + }); + + // Verify execution order + expect(executionOrder).toContain("Scenario 1 start"); + expect(executionOrder).toContain("Scenario 1 aborted"); // Aborted, not completed + expect(executionOrder).not.toContain("Scenario 1 end"); + + expect(executionOrder).toContain("Scenario 2 start"); + expect(executionOrder).toContain("Scenario 2 end"); + + expect(executionOrder).toContain("Scenario 3 start"); + expect(executionOrder).toContain("Scenario 3 aborted"); // Aborted, not completed + expect(executionOrder).not.toContain("Scenario 3 end"); + + // Scenario 4 should never start + expect(executionOrder).not.toContain("Scenario 4 start"); + + // Check scenario results + const results = summary.scenarios; + expect(results.length).toBe(4); + + // Find scenarios by name since order may vary in parallel execution + const scenario1 = results.find((r) => r.metadata.name === "Scenario 1")!; + const scenario2 = results.find((r) => r.metadata.name === "Scenario 2")!; + const scenario3 = results.find((r) => r.metadata.name === "Scenario 3")!; + const scenario4 = results.find((r) => r.metadata.name === "Scenario 4")!; + + expect(scenario1.status).toBe("skipped"); + expect(scenario2.status).toBe("failed"); + expect(scenario3.status).toBe("skipped"); + expect(scenario4.status).toBe("skipped"); }); Deno.test({