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
WalkthroughCentralizes and standardizes script error handling: runtimes return partial results on throw, CLI runner catches and merges those partial results (including skipped pre-request), and Electron IPC normalizes and appends descriptive failure entries for pre-request, post-response, and test script errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI runner
participant IPC as Electron IPC
participant RT as Script Runtime
participant Runner as Folder/Request Runner
CLI->>RT: run pre-request script
alt success
RT-->>CLI: preRequestResult
CLI->>Runner: continue request execution
else error
RT-->>CLI: throw error (includes partialResults, nextRequestName, stopExecution)
CLI->>IPC: send partialResults + error info
IPC->>IPC: appendScriptErrorResult(phase="Pre-Request")
IPC-->>Runner: normalized scriptResult (includes failure entry)
end
Runner->>RT: run post-response/test scripts
alt success
RT-->>Runner: scriptResult
else error
RT-->>Runner: throw error (includes partialResults, nextRequestName, stopExecution)
Runner->>IPC: normalize via appendScriptErrorResult
IPC-->>Runner: normalized scriptResult with failure entry
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
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).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bruno-cli/src/runner/run-single-request.js (1)
734-755: Test script error handling is consistent with other script paths; consider a small helper for DRYnessThe test-script error handler mirrors the post-response handler: it merges
error.partialResults.results, appends a'Test Script Error'failure, and preservesnextRequestNameandstopExecution. This is exactly what we want so that test script syntax/runtime errors are surfaced as failed tests and runner-level control flags are still honored.Given the three handlers (pre-request, post-response, tests) now share the same “partialResults + synthetic failure + nextRequestName/stopExecution” pattern, you might later want to factor that into a tiny local helper (similar in spirit to
appendScriptErrorResulton the Electron side) to keep behavior changes centralized. Not necessary for this PR, but it would reduce future drift.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-cli/src/runner/run-single-request.js(3 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. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-cli/src/runner/run-single-request.js
🧬 Code graph analysis (1)
packages/bruno-cli/src/runner/run-single-request.js (1)
packages/bruno-cli/src/runner/prepare-request.js (2)
decomment(5-5)request(16-16)
🔇 Additional comments (2)
packages/bruno-cli/src/runner/run-single-request.js (2)
665-685: Post-response script error handling now correctly preserves partial results and stopExecutionThis catch block looks solid: it pulls
error.partialResults.resultsintopostResponseTestResults, appends a clear'Post-Response Script Error'failure, and now also preserves bothnextRequestNameandstopExecutionfrompartialResults, which was missing earlier. This aligns the CLI behavior with the Electron IPC path and respectsbru.runner.stopExecution()even when the script later throws.
173-247: Pre-request error handling: avoid duplicate logging and verify behavior changeNice job wiring partialResults, nextRequestName, and stopExecution through the pre-request path. A couple of details to tighten up:
- In the error path you call
logResults(preRequestTestResults, 'Pre-Request Tests')and this may be called again at line 618, so pre-request failures could be printed twice on the CLI when the script throws. Verify that this double logging doesn't occur.- Previously, a thrown pre-request script error would bubble to the outer
tryand short-circuit the request; now it's caught and the HTTP request proceeds with a synthetic failed test entry. Confirm that the higher-level runner treats any non-pass pre-request result as a failed request, so that the run is still marked as failed overall.You can address the potential double logging and optionally log results in the skip path with something like:
- if (result?.skipRequest) { - return { + if (result?.skipRequest) { + logResults(result?.results || [], 'Pre-Request Tests'); + return { @@ - logResults(preRequestTestResults, 'Pre-Request Tests'); + // Let the global pre-request logger handle printing these resultsVerify that the CLI runner marks the overall request as failed when
preRequestTestResultscontains a failed entry.
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.