fix(bench): stop recording probe errors as tool versions - #116
Merged
Conversation
check_tool never inspected the exit status and fell back to stderr, so a tool that exits non-zero from its version probe had the error text stored as its version. dregsy has no version flag, so 10 of its 13 records in bench/results/ecr.json hold a Go flag error where a version belongs, which defeats the archive's purpose of attributing a metric shift to a version. Tolerating a failed probe is specific to the tool that has no version to report. check_tool is the only gate that runs before the benchmark starts, so for a tool that does report one a failed probe is an error rather than an absent version. Version parsing moves into a pure function so both paths are testable without spawning a process.
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.
check_toolnever inspected the exit status and fell back to stderr, so a tool that exits non-zero from its version probe had the error text stored as its version. dregsy has no version flag at all, so 10 of its 13 records inbench/results/ecr.jsonhold a Go flag error where a version belongs:That defeats what the archive is tracked in git for. A run record that cannot say which dregsy produced a number cannot attribute a shift to a version change.
Tolerating a failed probe is specific to the tool that has no version to report.
check_toolis the only gate that runs before a benchmark starts, so for a tool that does report one, a failed probe is now an error rather than a quietly absent version -- otherwise a broken ocync or regsync would sail through the pre-flight check looking fine.reports_versionmatches exhaustively with no wildcard, so adding a tool forces that decision rather than defaulting to lenient.Version parsing moves into a pure function so both paths are testable without spawning a process. The two guards were kill-tested rather than assumed: removing the exit-status check fails
failed_probe_is_not_recorded_as_a_versionand nothing else, and making every tool exempt failsfailed_probe_errors_for_a_tool_that_reports_a_versionand nothing else. The dregsy fixture is the exact stderr string the archive actually stored, not an invented one.check_toolwas the only site in xtask ignoring exit status --report.rsuses.filter(|o| o.status.success())andremote.rschecks explicitly -- so this makes it consistent with the surrounding code rather than introducing a new pattern.Two related gaps are left alone, both pre-existing and neither caused here.
check_toolspawns the tool by name fromPATHwhilerun_toolruns ocync fromtarget/release/ocync, and it runs beforebuild_ocync, so the recorded ocync version is not necessarily the binary that was benchmarked. Separately, skopeo performs every transfer dregsy is credited with but is not aToolvariant, so its version reaches no record. Both need the bench flow reordered or the record schema extended, which is a larger change than this one.