fix/902 --bail flag not stopping execution when a test fails#8103
Conversation
sharan-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDisplays a table-based execution summary, adds an ORANGE color constant and the cli-table3 dependency, and synthesizes skipped results when --bail triggers so the summary shows "Skipped (Bail)" counts. ChangesExecution Summary with Bail Reporting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-cli/package.json`:
- Line 62: The repo's lockfile is out of sync after adding "cli-table3" to
packages/bruno-cli/package.json; run npm install from the repository root to
regenerate package-lock.json (ensuring the new dependency is resolved into the
lock), verify the updated package-lock.json includes the "cli-table3" entry, and
commit and push the updated lockfile so CI can pass.
In `@packages/bruno-cli/src/commands/run.js`:
- Around line 729-744: The bail placeholder objects pushed in the remainingItems
loop are missing required fields (test, request, iterationIndex) causing
reporters like JUnit to crash; update the object created in that loop inside
run.js so it matches the skipped shape used by createSkippedFileResults: include
iterationIndex (e.g., 0 or same default used elsewhere), a request object with
the same empty/default structure as createSkippedFileResults, and a test object
containing filename set from ri.pathname (or ri.name as appropriate) so
reporters can safely access result.test.filename and result.request.url.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8fa99dcf-5ac2-41ff-8fa4-0c30d22dbb50
📒 Files selected for processing (3)
packages/bruno-cli/package.jsonpackages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/constants.js
bijin-bruno
left a comment
bijin-bruno
left a comment
There was a problem hiding this comment.
Please update package-lock file
|
|
||
| const CLI_EPILOGUE = `Documentation: https://docs.usebruno.com (v${version})`; | ||
| const CLI_VERSION = version; | ||
| const ORANGE_COLOR = '#FFA500'; |
There was a problem hiding this comment.
I think it is better to export a COLORS object rather than exporting individual colors.
const COLORS = {
ORANGE: '#FFA500'
};
There was a problem hiding this comment.
Thank you for the suggestion. I removed ORANGE_COLOR and am now accessing it through the color object
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-cli/src/commands/run.js (1)
693-699:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDouble
hrtimecall introduces timing drift.
process.hrtime(start)is invoked twice, returning slightly different values each time. Store the result once for accurate duration.Proposed fix
+ const elapsed = process.hrtime(start); results.push({ ...result, - runDuration: process.hrtime(start)[0] + process.hrtime(start)[1] / 1e9, + runDuration: elapsed[0] + elapsed[1] / 1e9, suitename: pathname.replace('.bru', ''), name, path: result.test?.filename || path.relative(collectionPath, pathname) });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-cli/src/commands/run.js` around lines 693 - 699, The runDuration calculation in the results.push block calls process.hrtime(start) twice (in runDuration: process.hrtime(start)[0] + process.hrtime(start)[1] / 1e9) which can cause small timing drift; capture the hrtime result once into a local variable (e.g., hr = process.hrtime(start)) and compute runDuration from hr[0] and hr[1] before pushing the object in results.push so runDuration uses the single consistent measurement; update the block that constructs the pushed object (referencing results.push, runDuration, and start) to use that stored hr value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/bruno-cli/src/commands/run.js`:
- Around line 693-699: The runDuration calculation in the results.push block
calls process.hrtime(start) twice (in runDuration: process.hrtime(start)[0] +
process.hrtime(start)[1] / 1e9) which can cause small timing drift; capture the
hrtime result once into a local variable (e.g., hr = process.hrtime(start)) and
compute runDuration from hr[0] and hr[1] before pushing the object in
results.push so runDuration uses the single consistent measurement; update the
block that constructs the pushed object (referencing results.push, runDuration,
and start) to use that stored hr value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af89a3d3-a9c1-4956-9d8a-49c31798d0b6
📒 Files selected for processing (2)
packages/bruno-cli/src/commands/run.jspackages/bruno-cli/src/constants.js
done, updated |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-cli/src/commands/run.js (1)
761-766: 💤 Low value
bailInfois assigned but never used.This variable is populated with bail metadata but isn't read, returned, or passed anywhere afterward. Consider removing it or utilizing it if intended for future features.
♻️ Proposed removal if not needed
- bailInfo = { - bailed: true, - bailReason, - bailedAt: name, - skippedByBail: remainingItems.length - };Also remove the declaration at line 662:
let bailInfo = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-cli/src/commands/run.js` around lines 761 - 766, The variable bailInfo is being assigned but never used; either remove the unused declaration and assignment or wire it into the function's output/callback so the metadata is consumed. Locate the bailInfo declaration (let bailInfo = null) and the later assignment where bailInfo = { bailed: true, bailReason, bailedAt: name, skippedByBail: remainingItems.length } and either delete both lines (and any dead-code dependent on it) or modify the surrounding function to return or emit this bailInfo (e.g., include it in the resolved object or pass it to the existing result/emit path) so the metadata is actually consumed. Ensure no other references to bailInfo remain after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/bruno-cli/src/commands/run.js`:
- Around line 761-766: The variable bailInfo is being assigned but never used;
either remove the unused declaration and assignment or wire it into the
function's output/callback so the metadata is consumed. Locate the bailInfo
declaration (let bailInfo = null) and the later assignment where bailInfo = {
bailed: true, bailReason, bailedAt: name, skippedByBail: remainingItems.length }
and either delete both lines (and any dead-code dependent on it) or modify the
surrounding function to return or emit this bailInfo (e.g., include it in the
resolved object or pass it to the existing result/emit path) so the metadata is
actually consumed. Ensure no other references to bailInfo remain after removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2256a15f-2dd7-44b4-ad09-332fd68a892f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
packages/bruno-cli/src/commands/run.js
| if (totalPreRequestTests > 0) { | ||
| preRequestTestSummary = formatTestSummary('Pre-Request Tests:', maxLength, passedPreRequestTests, failedPreRequestTests, totalPreRequestTests); | ||
| } | ||
| const summary = getRunnerSummary(results); |
There was a problem hiding this comment.
I think the summary should already get bail information extracted using getRunnerSummary method.
There was a problem hiding this comment.
addressed now its coming from getRunnerSummary(results)
| request: T_EmptyRequest | T_Request; | ||
| response: T_EmptyResponse | T_Response | T_SkippedResponse; | ||
| status: null | undefined | string; | ||
| skipReason?: string; |
There was a problem hiding this comment.
I think we are missing skipped: boolean here. skipReason should coexist with skipped property.
| preRequestTestResults: [], | ||
| postResponseTestResults: [], | ||
| runDuration: 0, | ||
| suitename: ri.pathname.replace('.bru', ''), |
There was a problem hiding this comment.
Is this handing yml collections?
3 participants
Description
CLI run command now prints a cli-table3 execution summary table and handles --bail cleanly:
New 📊 Execution Summary table with Status / Requests / Tests / Assertions / Duration columns.
Requests cell breaks down counts inline, e.g. 4 (1 Passed, 1 Failed, 2 Skipped (Bail)).
On --bail, the run synthesizes skipReason: 'bail' placeholder results for remaining requests so they're counted as skipped, and prints an inline orange notice:
Bail: Stopping run, test failure in "2nd API". Remaining 2 request(s) skipped.
Before:
After
Adds cli-table3 dependency to print the table similar to golden edition.
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
New Features
Chores