Improve timeout error handling and maxFailures behavior #19
Conversation
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
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…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.
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.
There was a problem hiding this comment.
Pull request overview
This pull request improves timeout error handling and fixes maxFailures behavior in the probitas-runner package. The changes enhance error tracking with step context, refactor retry logic for better flexibility, and ensure proper scenario abortion when maxFailures is reached.
Key Changes:
- Added static factory methods (
timeoutSignal()) for creating timeout signals with automatic cleanup viausingdeclarations - Enhanced ScenarioTimeoutError to track which step was executing when timeout occurred (step name and index)
- Refactored retry logic to support caller-controlled error classification via
shouldRetrycallback - Fixed maxFailures to properly abort all scenarios (running and pending) using signal propagation instead of throwing
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/probitas-runner/errors.ts | Added static timeoutSignal() factory methods to both error classes with Disposable interface for cleanup; enhanced ScenarioTimeoutError constructor to accept step information |
| packages/probitas-runner/errors_test.ts | Added comprehensive test coverage (19 new tests) for timeout signal creation, cleanup, and error construction with step info |
| packages/probitas-runner/utils/retry.ts | Added shouldRetry callback parameter; changed to preserve raw error types (unknown); pass 1-based attempt numbers to callback function |
| packages/probitas-runner/utils/retry_test.ts | Added 5 new tests for shouldRetry callback behavior and attempt number passing |
| packages/probitas-runner/step_runner.ts | Integrated timeout signal factory with using declarations; implemented shouldRetry to skip timeout errors; enhanced error enrichment logic for timeout errors |
| packages/probitas-runner/step_runner_test.ts | Updated test expectations to match raw error types (strings instead of Error objects) |
| packages/probitas-runner/scenario_runner.ts | Added step index tracking and ScenarioTimeoutError enrichment with current step information when timeout occurs |
| packages/probitas-runner/scenario_runner_test.ts | Updated test expectations to match raw error types |
| packages/probitas-runner/runner.ts | Refactored maxFailures to call controller.abort() instead of throwing; added logic to create skip results for unexecuted scenarios; integrated timeout signal factory |
| packages/probitas-runner/runner_test.ts | Added 3 new tests for maxFailures behavior (sequential and parallel); added 2 new tests for timeout with step info; removed old _internal.isTimeoutError tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+170
to
+183
| 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; |
There was a problem hiding this comment.
This code block for creating skip results is duplicated in two places (lines 170-183 and 217-227). The logic is nearly identical except for the comment. Consider extracting this into a helper method to reduce duplication and improve maintainability.
Comment on lines
+42
to
+73
| 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); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The same cleanup pattern is duplicated in both StepTimeoutError.timeoutSignal and ScenarioTimeoutError.timeoutSignal. Consider extracting this into a shared helper function to reduce code duplication and improve maintainability.
Comment on lines
+128
to
157
| 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); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The same cleanup pattern is duplicated in both StepTimeoutError.timeoutSignal and ScenarioTimeoutError.timeoutSignal. Consider extracting this into a shared helper function to reduce code duplication and improve maintainability.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
usingdeclarationsshouldRetrycallbackWhy
Timeout Error Handling:
Previously, timeout signal creation was scattered and lacked proper cleanup mechanisms. The new static factory methods (
timeoutSignal()) provide a consistent way to create timeout signals with automatic cleanup via the Disposable interface andusingdeclarations.ScenarioTimeoutError now tracks which step was executing when the timeout occurred, providing better debugging context with step name and index information.
Retry Logic Separation:
The retry utility was tightly coupled to specific error types (TimeoutError checks), making it inflexible. By introducing the
shouldRetrycallback, callers can now control error classification without modifying the retry logic. This also allows preserving raw error types (unknown) instead of converting everything to Error, maintaining type information throughout the chain.maxFailures Behavior:
When maxFailures was reached, the Runner would throw an error, leaving remaining scenarios unrecorded and preventing proper cleanup. This caused incomplete test results and inconsistent reporter event emission.
Now, the Runner calls
controller.abort()with a Skip reason when maxFailures is reached. This:Test Plan