git-sync-bench: exit non-zero on failed runs, drop -1 sentinels#91
Open
Soph wants to merge 1 commit into
Open
Conversation
Two reporting bugs:
- run() returned nil after printing the report regardless of outcome, so the
process exited 0 even when every benchmark run failed — a failure could
pass for success in CI. Return an error (after printing the report) when
any run failed.
- The aggregate Min* fields start at -1 as an "unset" sentinel updated by the
first qualifying run. With no successful (or no batched) run the sentinel
survived into the report, emitting minWallMillis=-1 etc. Clear the
sentinels to 0 when there were no qualifying runs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: fde63c6432fc
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
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.
Problems
Two reporting bugs in
cmd/git-sync-bench:run()returnednilafter printing the report regardless of outcome, so the process exited 0 even when every benchmark run failed — a failure silently passed for success in CI.-1sentinels in output. The aggregateMin*fields start at-1as an "unset" marker updated by the first qualifying run. With no successful (or no batched) run, the sentinel survived into the report — emitting"minWallMillis": -1,"minSyncElapsedMillis": -1, etc.Fix
Aggregate.FailedRuns > 0, somainexits non-zero. The report stays the useful artifact; the exit code now reflects reality.Min*sentinels to0when there were no qualifying runs (the batch-count fields areomitempty, so0drops them entirely).Tests
TestSummarizeRunsAllFailedHasNoSentinelsruns an all-failed set and asserts the counts, that theMin*fields are0, and that the marshaled JSON contains no-1. The existingTestSummarizeRuns(mixed success) still passes.🤖 Generated with Claude Code
Note
Low Risk
Benchmark CLI reporting and exit behavior only; no sync/runtime logic changes.
Overview
git-sync-benchnow prints the full report (JSON or text) and then returns an error when any benchmark run failed, somainexits 1 instead of always succeeding in CI.summarizeRunsresets aggregateMin*fields from the internal-1“unset” sentinel to0when there are no successful runs (wall/sync mins) or no batched runs (batch mins), so JSON/text reports no longer showminWallMillis: -1and similar.Adds
TestSummarizeRunsAllFailedHasNoSentinelsto lock in all-failed aggregation and JSON without-1.Reviewed by Cursor Bugbot for commit db22241. Configure here.