Repository navigation
test: assert the ledger cache invariants as ratios instead of wall-clock budgets - #54
Merged
Merged
Conversation
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.
Motivation
Two ledger perf tests asserted wall-clock ceilings (
< 200 msfor 9 cached queries,< 500 msfor a 50k-row eviction). A millisecond ceiling can't be calibrated once and stay meaningful:cargo testin a sandbox is permanently red and teaches you to ignore failures — which is how the genuinely brokenmainof fix: unbreak main — six foreign_sessions_from test call sites lost an argument in a merge #49 nearly got waved through;Change
Both now assert the invariant they were written for, measured against a control on the same machine, so the number is portable:
perf_summarize_cached_stays_fast_with_many_rows→perf_summarize_cached_costs_far_less_than_parsing. Times onerefresh_cache_fromof the same 50k rows as the baseline, then requires 9 cached queries to cost less than half of it. The regression it guards (commit0d6e2d3a, where every call linear-scanned the CSV) made each of the nine cost a whole parse, so the two can never be confused.perf_eviction_stays_linear_with_many_rowskeeps its name and now compares 25k rows against 50k on the same machine: linear doubles, quadratic quadruples, so a 3× ceiling separates them with room for scheduler noise.seed_cache_with_synthetic_rowsbecomeswrite_synthetic_ledger(&Path, n), keeping the 30-day row spread so the day/week/month cutoffs still match distinct subsets. It takes the path rather than returning one becauseTestPathsdeletes its tree onDrop— a helper owning the guard and handing back aPathBufwould return a path to a deleted directory, and a timing ratio over a missing ledger passes vacuously. That trap is now called out in the helper's doc comment.Not in this PR
app::tests::perf_do_refresh_io_does_not_explode_with_repeated_callsgets the same treatment, but converting it to a cold-vs-warm ratio fails on real data — warm ticks cost about as much as the cold one (22.8 s vs 32.0 s in a sandbox; the host's 10.37 s for three calls implies the same flat shape). That is a real finding, not a test-calibration problem, so the rewritten test belongs in the same PR as its fix rather than landing red here. Filed separately.Verification
Both pass inside the sandbox, where the versions they replace fail — which is the whole point.