Mark test script errors as failed in runner #6261
Conversation
KaranPradhan266
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughUnified script error handling across runtimes and IPC: script runtimes now return partialResults on throw, CLI runner preserves and logs partial results (including skipped pre-request responses), and Electron IPC centralizes normalization of script error results while propagating nextRequestName and stopExecution when provided. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bruno-cli/src/runner/run-single-request.js (1)
687-705: CLI test error handling correctly surfaces script errorsThe new catch block correctly:
- Preserves any
error.partialResults.results.- Appends a synthetic
"Test Script Error"failure.- Propagates
nextRequestNamefromerror.partialResults.- Reuses
logResultsso the CLI output reflects the failure.This aligns well with the goal of marking script errors as failing tests, while keeping chaining semantics intact.
If you find this pattern spreading further, consider extracting the “append test-script error entry” logic into a small shared helper (similar to the Electron helper) to keep behavior consistent across environments, but that’s optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-cli/src/runner/run-single-request.js(1 hunks)packages/bruno-electron/src/ipc/network/index.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/bruno-cli/src/runner/run-single-request.js (1)
packages/bruno-cli/src/commands/run.js (1)
nextRequestName(627-627)
🔇 Additional comments (3)
packages/bruno-electron/src/ipc/network/index.js (3)
403-421: Helper cleanly normalizes test results on script errors
appendTestScriptErrorResultis a good abstraction here: it keeps existingtestResultsshape (including env andnextRequestName) and guarantees a synthetic"Test Script Error"failure entry whenever an error is present, while staying a no‑op whenerroris falsy.This gives you a single, consistent place to evolve test‑error behavior in the Electron path.
883-921: Standalone request path now reliably emits failing results on test script errorsUsing
appendTestScriptErrorResulthere ensures that:
- The renderer always receives at least one failing test entry when the test script throws.
- Any partial results and environment/global-environment updates from
partialResultsare preserved.This should fix the “tests appear passed despite script error” symptom for the single-request Electron path.
1534-1573: Folder/runner path correctly integrates test script errors into results and flow controlApplying
appendTestScriptErrorResultin the folder runner:
- Ensures
testResults.resultsalways contains a failing"Test Script Error"entry when a test script crashes.- Preserves
nextRequestNamefrom partial results, so jump-based control flow still works.- Keeps env/global env fields intact for downstream updates.
This aligns the runner’s behavior with the standalone request path and should prevent “Passed” statuses when the test script itself fails.
|
@KaranPradhan266 Thanks for the PR contribution. However, if the tests are failing, it marks the request as failed. But if the post-response (which contains the tests) has syntactical issues, it shows the request as passed. We should definitely address this as well, right? 
|
|
@Pragadesh44-Bruno Thanks for pointing this out! I tested the scenario you described — adding a syntactically invalid After updating the PR, both syntax errors and runtime exceptions in Also, just to clarify: the unit test failures reported in CI are not I’ve attached screenshots below for your reference. For Pre-request: For Post-request: |
|
@Pragadesh44-Bruno |
… CLI and Electron
|
@Pragadesh44-Bruno Added a single appendScriptErrorResult helper and used it for pre-request, post-response, and test script errors. Do let me know if this works! |
|
@KaranPradhan266 Thanks for your changes to the PR. I'll be reviewing it today and will approve it |
…l results for pre-request and post-response scripts across CLI and Electron. This ensures that tests passing before an error are still reported.
|
@KaranPradhan266 I have created a PR in your branch that fixes several bugs in the error logging implementation.
Before: After: |
feat: Enhance error handling in script execution
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/bruno-cli/src/runner/run-single-request.js (1)
221-246: Consider extracting a helper for error result handling.The error handling pattern is repeated three times (pre-request, post-response, test). The Electron package uses
appendScriptErrorResultfor this. A similar helper here would reduce duplication:const appendScriptErrorResult = (scriptType, partialResults, error) => { return [ ...(partialResults || []), { status: 'fail', description: `${scriptType} Script Error`, error: error.message || `An error occurred while executing the ${scriptType.toLowerCase()} script.` } ]; };This is optional but would improve maintainability if more script types are added.
Also applies to: 664-682, 731-747
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bruno-cli/src/runner/run-single-request.js(3 hunks)packages/bruno-electron/src/ipc/network/index.js(7 hunks)packages/bruno-js/src/runtime/script-runtime.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation, never tabs
Use single quotes for strings instead of double quotes
Always add semicolons at the end of statements
No trailing commas in code
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line with a minimum of 2 elements for multiline formatting
No newlines inside function parentheses
Space before and after the arrow in arrow functions:() => {}
No space between function name and parentheses:func()notfunc ()
Semicolons should go at the end of the line, not on a new line
Function names should be concise and descriptive
Add JSDoc comments to provide details to abstractions
Avoid single-line abstractions where all that is being done is increasing the call stack with one additional function
Add meaningful comments to explain complex code flow instead of obvious comments
Files:
packages/bruno-js/src/runtime/script-runtime.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-cli/src/runner/run-single-request.js
🧠 Learnings (1)
📚 Learning: 2025-12-03T06:32:11.721Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: packages/bruno-js/src/runtime/hooks-runtime.js:8-135
Timestamp: 2025-12-03T06:32:11.721Z
Learning: vm2 runtime is being removed from the Bruno application and QuickJS will become the default runtime for script execution.
Applied to files:
packages/bruno-js/src/runtime/script-runtime.js
🧬 Code graph analysis (3)
packages/bruno-js/src/runtime/script-runtime.js (4)
packages/bruno-js/src/sandbox/quickjs/utils/index.js (1)
error(50-50)packages/bruno-electron/src/utils/oauth2.js (1)
error(77-77)packages/bruno-js/src/sandbox/quickjs/index.js (2)
executeQuickJsVmAsync(92-190)script(160-176)packages/bruno-app/src/utils/codemirror/autocomplete.js (1)
context(365-365)
packages/bruno-electron/src/ipc/network/index.js (2)
packages/bruno-app/src/components/ResponsePane/ScriptError/index.js (2)
preRequestError(7-7)postResponseError(8-8)packages/bruno-cli/src/runner/run-single-request.js (1)
testResults(700-700)
packages/bruno-cli/src/runner/run-single-request.js (2)
packages/bruno-cli/src/commands/run.js (6)
result(552-563)runtimeVariables(334-334)collectionPath(295-295)processEnvVars(466-468)runSingleRequestByPathname(519-543)nextRequestName(627-627)packages/bruno-cli/src/runner/prepare-request.js (3)
decomment(5-5)request(16-16)collectionPath(18-18)
🔇 Additional comments (12)
packages/bruno-electron/src/ipc/network/index.js (7)
403-433: LGTM - Clean centralization of script error handling.Good use of maps for script-type-specific descriptions and sensible fallbacks for unknown script types. The null-safe spreading ensures robustness when
scriptResultis undefined.
693-697: LGTM - Proper partial results preservation.Correctly extracts partial results before normalizing the error, ensuring any passing tests before the script failure are retained.
864-871: LGTM - Consistent error handling with helpful inline comment.The comment at lines 864-866 clearly explains the rationale for preserving partial results.
946-947: LGTM - Consistent with existing test script error handling.Correctly applies the normalized error result appending after the existing partial results extraction logic.
1291-1295: LGTM - Folder-run pre-request error handling mirrors single-request flow.
1519-1525: LGTM - Folder-run post-response error handling is consistent.
1613-1614: LGTM - Folder-run test script error handling is consistent.packages/bruno-js/src/runtime/script-runtime.js (4)
70-82: LGTM - Clean extraction of result builder.Consolidates the return object construction and avoids duplication across runtime branches.
84-127: LGTM - Consistent error handling across both runtimes.The pattern of capturing the error, attaching partial results, and re-throwing ensures callers can display tests that passed before the script failed.
200-243: LGTM - Response script error handling mirrors request script pattern.Both nodevm and quickjs paths consistently capture errors, attach partial results, and re-throw.
186-198: VerifypersistentEnvVariablesserialization consistency between builders.
buildRequestScriptResult(line 76) andbuildResponseScriptResult(line 191) may handlepersistentEnvVariablesdifferently—one directly and one wrapped incleanJson(). Confirm whether this difference is intentional or if both should use the same serialization approach for consistency.packages/bruno-cli/src/runner/run-single-request.js (1)
173-246: LGTM - Comprehensive pre-request script error handling.Properly handles the skip case with early return, and the error case correctly extracts partial results, appends a failure entry, and preserves flow control properties (
nextRequestName,stopExecution).
| } catch (error) { | ||
| console.error('Post-response script execution error:', error); | ||
|
|
||
| const partialResults = error?.partialResults?.results || []; | ||
| postResponseTestResults = [ | ||
| ...partialResults, | ||
| { | ||
| status: 'fail', | ||
| description: 'Post-Response Script Error', | ||
| error: error.message || 'An error occurred while executing the post-response script.' | ||
| } | ||
| ]; | ||
|
|
||
| if (error?.partialResults?.nextRequestName !== undefined) { | ||
| nextRequestName = error.partialResults.nextRequestName; | ||
| } | ||
|
|
||
| logResults(postResponseTestResults, 'Post-Response Tests'); | ||
| } |
There was a problem hiding this comment.
Missing stopExecution preservation in post-response error handler.
The pre-request error handler (lines 240-243) preserves stopExecution from error.partialResults, but this block does not. The Electron counterpart (network/index.js lines 1541-1543) checks postResponseScriptResult?.stopExecution after the error is handled. Consider adding:
if (error?.partialResults?.nextRequestName !== undefined) {
nextRequestName = error.partialResults.nextRequestName;
}
+
+ if (error?.partialResults?.stopExecution) {
+ shouldStopRunnerExecution = true;
+ }
logResults(postResponseTestResults, 'Post-Response Tests');📝 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.
| } catch (error) { | |
| console.error('Post-response script execution error:', error); | |
| const partialResults = error?.partialResults?.results || []; | |
| postResponseTestResults = [ | |
| ...partialResults, | |
| { | |
| status: 'fail', | |
| description: 'Post-Response Script Error', | |
| error: error.message || 'An error occurred while executing the post-response script.' | |
| } | |
| ]; | |
| if (error?.partialResults?.nextRequestName !== undefined) { | |
| nextRequestName = error.partialResults.nextRequestName; | |
| } | |
| logResults(postResponseTestResults, 'Post-Response Tests'); | |
| } | |
| } catch (error) { | |
| console.error('Post-response script execution error:', error); | |
| const partialResults = error?.partialResults?.results || []; | |
| postResponseTestResults = [ | |
| ...partialResults, | |
| { | |
| status: 'fail', | |
| description: 'Post-Response Script Error', | |
| error: error.message || 'An error occurred while executing the post-response script.' | |
| } | |
| ]; | |
| if (error?.partialResults?.nextRequestName !== undefined) { | |
| nextRequestName = error.partialResults.nextRequestName; | |
| } | |
| if (error?.partialResults?.stopExecution) { | |
| shouldStopRunnerExecution = true; | |
| } | |
| logResults(postResponseTestResults, 'Post-Response Tests'); | |
| } |
🤖 Prompt for AI Agents
In packages/bruno-cli/src/runner/run-single-request.js around lines 664 to 682,
the catch block for post-response script errors builds test results and sets
nextRequestName but does not preserve stopExecution from error.partialResults;
update the handler to check for error?.partialResults?.stopExecution and assign
it to the existing stopExecution variable (same pattern used in the pre-request
handler), ensuring you only overwrite stopExecution when that property is
defined so downstream logic that relies on stopExecution (e.g., network/electron
checks) respects the post-response script intent.
There was a problem hiding this comment.
@Pragadesh44-Bruno should we preserve stopExecution in the post-response error path so a stop request survives script errors?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@Pragadesh44-Bruno I'm noticing that the CLI continues executing subsequent requests even when I call bru.runner.stopExecution() inside a post-response script.
In my test, a collection contains multiple requests. In the first request’s post-response script, I call bru.runner.stopExecution() and then throw an error. However, the run still proceeds to the next request. This doesn’t seem like the expected behavior, I would expect execution to stop.
Let me know your thoughts on this approach. I’ll wait for your response before proceeding further.
|
|
||
| const partialResults = error?.partialResults?.results || []; | ||
| testResults = [ | ||
| ...partialResults, | ||
| { | ||
| status: 'fail', | ||
| description: 'Test Script Error', | ||
| error: error.message || 'An error occurred while executing the test script.' | ||
| } | ||
| ]; | ||
|
|
||
| if (error?.partialResults?.nextRequestName !== undefined) { | ||
| nextRequestName = error.partialResults.nextRequestName; | ||
| } | ||
|
|
||
| logResults(testResults, 'Tests'); | ||
| } |
There was a problem hiding this comment.
Missing stopExecution preservation in test script error handler.
Same issue as the post-response handler. If bru.stopExecution() was called before the script errored, it won't be honored. Consider adding:
if (error?.partialResults?.nextRequestName !== undefined) {
nextRequestName = error.partialResults.nextRequestName;
}
+
+ if (error?.partialResults?.stopExecution) {
+ shouldStopRunnerExecution = true;
+ }
logResults(testResults, 'Tests');📝 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.
| const partialResults = error?.partialResults?.results || []; | |
| testResults = [ | |
| ...partialResults, | |
| { | |
| status: 'fail', | |
| description: 'Test Script Error', | |
| error: error.message || 'An error occurred while executing the test script.' | |
| } | |
| ]; | |
| if (error?.partialResults?.nextRequestName !== undefined) { | |
| nextRequestName = error.partialResults.nextRequestName; | |
| } | |
| logResults(testResults, 'Tests'); | |
| } | |
| const partialResults = error?.partialResults?.results || []; | |
| testResults = [ | |
| ...partialResults, | |
| { | |
| status: 'fail', | |
| description: 'Test Script Error', | |
| error: error.message || 'An error occurred while executing the test script.' | |
| } | |
| ]; | |
| if (error?.partialResults?.nextRequestName !== undefined) { | |
| nextRequestName = error.partialResults.nextRequestName; | |
| } | |
| if (error?.partialResults?.stopExecution) { | |
| shouldStopRunnerExecution = true; | |
| } | |
| logResults(testResults, 'Tests'); |
🤖 Prompt for AI Agents
In packages/bruno-cli/src/runner/run-single-request.js around lines 731 to 747,
the test-script error block builds testResults but fails to preserve the
stopExecution flag from partialResults (so bru.stopExecution() calls are
ignored); update the handler to read stopExecution from
error?.partialResults?.stopExecution (or from the existing stopExecution
variable if present) and include it on the appended error result and/or assign
it back to the outer stopExecution variable, e.g. propagate
error.partialResults.stopExecution to nextRequestName/stopExecution handling so
the runner honors stopExecution when a test script throws.
3 participants
Description
fixes #6258
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.