fix: discover Criterion samples at their real depth, under new/ (#95) - #130
Merged
Merged
Conversation
`discover_cells` bounded its walk to exactly depth 4 with the comment "Each leaf is at depth 4 (criterion_dir/row/mode/size/sample.json)". Criterion 0.5 writes `<row>/<mode>/<size>/new/sample.json` — depth 5. The walk therefore matched nothing: on this repo's real Criterion tree, `find target/criterion -maxdepth 4 -name sample.json` returns zero hits and `-maxdepth 5` returns two. The failure was silent rather than loud. `NoCellsFound` never fired because aux_metrics.jsonl still supplied cells through the Step-3 leftover path, so every micro-grid cell reached the renderers with `timing: None` and every p50/p95/p99 in summary.md rendered as an em-dash. The only test covering this walked a fixture tree that omits the `new/` level, so the suite passed against a layout Criterion never produces. Walks depth 5 and requires the leaf's parent directory to be named `new`. The `new` check is the load-bearing half: Criterion retains the previous run under a sibling `base/`, which shares the (row, mode, size) key and would otherwise emit a duplicate Cell. The fixture tree gains the `new/` level and a `base/` sibling holding times 10x larger, so the tests now cover the real layout and the suite fails if `base/` is ever picked up (asserted both by an exactly-one-cell count and by the existing p50 == 3000 assertions). `copy_raw_archive` was always unbounded-depth and already archived the `new/` path correctly; the smoke test's expected archive path is updated to match. Verified end to end: against the real `target/criterion`, summarize now reports `smoke/chisel-strict/256B` with p50 84481021 / p95 89508941, where before it emitted the cell with no timing at all. Closes #95.
This was referenced Jul 29, 2026
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.
Closes #95 (BENCH-1, BUG/correctness).
The defect
discover_cellsbounded its walk to exactly depth 4:Criterion 0.5 writes
<row>/<mode>/<size>/new/sample.json— depth 5. On thisrepo's real tree:
So the walk matched nothing, and it failed silently:
NoCellsFoundneverfires because aux_metrics.jsonl still supplies cells via the Step-3 leftover
path. Every micro-grid cell reached the renderers with
timing: None, andevery p50/p95/p99 in summary.md rendered as an em-dash.
The one test covering this walked a fixture tree that omits the
new/level,so the suite passed against a layout Criterion never produces.
The fix
Walk depth 5, and require the leaf's parent directory to be named
new.The
newcheck is the load-bearing half, not the depth change: Criterionretains the previous run under a sibling
base/, which shares the(row, mode, size)key and would otherwise emit a duplicateCell.Tests
The fixture tree gains the
new/level (so it matches what Criterion actuallywrites) plus a
base/sibling holding times 10x larger. Two things now fail ifbase/leaks in: an explicit exactly-one-cell count, and the pre-existingp50 == 3000assertions (base/ would give 30000).Counterfactual — reverting the walk to depth 4, all three discover tests fail:
copy_raw_archivewas always unbounded-depth and already archived thenew/path correctly — only
summarize_smoke's expected archive path needed updatingto match the fixture move.
End-to-end verification
Against the real
target/criterion,summarizenow reports:Before the fix that cell was emitted with no timing at all.
Verification
cargo testall green,cargo clippy --all-targets -- -D warningsclean,cargo fmt --checkclean.