ci: run integration + acceptance tests, stabilize a flaky perf gate - #56
Merged
Merged
Conversation
CI ran only `tests/unit`, so the whole integration and acceptance layer went unverified on PRs. Two bugs shipped through that gap and were caught by hand during the 0.7.0 release (#55): a resume fixture that predated the version gate, and a perf test still calling the pre-#7 `Dataset.select()`. Add `tests/integration` and `tests/acceptance` to the matrix job, deselecting a new `benchmark` marker. That marker exists because `tests/acceptance` mixes two kinds of test: nine deterministic checks of the regression-detection helper, and one wall-clock throughput gate whose thresholds are calibrated on a dedicated box (long_output holds only 1.10x headroom there, and coverage tracing skews the latency it measures). CI gains the nine; the gate stays a local tool, which a plain `pytest` still runs. Also make `test_dependency_loading_overhead` robust. It compares two ~20ms hydrations as a ratio, so timing each arm once fed any transient stall straight into the metric: a loaded machine measured 876% against its 300% bound, an idle one 27%. Each arm is now hydrated 5 times and compared on its minimum, since contention only ever adds time. Measured under the same concurrent-suite load that produced the 876%, the ratio now stays in an 18-37% band across six runs; the per-arm spread narrows from 7-107% to 2-16%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI deselects `benchmark`, so the release becomes the only place the wall-clock throughput gates are checked. Make that explicit rather than incidental. The gates already ran here, but inside the batch this doc says to "run in parallel" — a wall-clock assertion sharing a box with `ruff`, `ty`, and a 2723-test pytest run measures that load, not the code. That is the same contention that made test_dependency_loading_overhead read 876%. Hold them back with `-m "not benchmark"` and run them alone afterwards. Also write benchmark_summary.json to gitignored outputs/ so the release has a recorded performance baseline, and point at the summary table's actual location in the output (it prints after the engine's log lines). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The same two facts — why CI cannot hold the thresholds, and why a ratio of 20ms timings needs repeats — were restated in the pyproject marker comment, the ci.yml comment, the test docstring, tests/README.md, and the release doc. Keep each where it is closest to the code and drop the echoes: the marker description points at tests/README.md, which owns the CI rationale, and the docstring owns the estimator rationale. Net 39 lines out, 20 in; no behaviour change (same 2723 passed / 1 deselected, same 27.5% idle reading). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Type
Summary
CI ran only
tests/unit, so the entire integration and acceptance layer went unverified on PRs. Two bugs shipped through that gap and had to be caught by hand during the 0.7.0 release (#55): a resume fixture that predated the version gate (#37), and a perf test still calling the pre-#7Dataset.select().tests/integration+tests/acceptanceto the matrix job, deselecting a newbenchmarkmarker.benchmarkmarker exists becausetests/acceptancemixes two kinds of test. Nine are deterministic checks of the regression-detection helper. One (test_benchmark_scenarios) is a wall-clock throughput gate whose thresholds are calibrated on a dedicated box —long_outputholds only 1.10× headroom there (0.771 measured vs 0.70 required), and the coverage tracer skews the very latency it measures. CI gains the nine; the gate stays a local tool that a plainpyteststill runs.test_dependency_loading_overheadrobust. It compares two ~20 ms hydrations as a ratio, so timing each arm once fed any transient stall straight into the metric. Each arm is now hydrated 5× and compared on its minimum — contention only ever adds time, never removes it..claude/commands/sieval-release.md). Since CI deselectsbenchmark, the release becomes the only place those thresholds get checked, so the step is now explicit. It also fixes a latent defect: the gates already ran there, but inside the batch the doc says to run in parallel — a wall-clock assertion sharing a box withruff,ty, and a 2723-test pytest run measures that load, not the code, which is the very contention that produced the 876% reading. They are now held back with-m "not benchmark"and run alone afterwards, writingbenchmark_summary.jsonto gitignoredoutputs/so each release records a performance baseline.tests/README.md, including why abenchmarkfailure means "retry on an idle machine" rather than "regression".Related Issues
Refs #55, #37, #7.
Test Plan
Automated
ruff check && ruff format --check)ty checkon both changed test modules: "All checks passed")tests/performance: 66 passed, 5 deselectedpytest tests/acceptancestill collects all 10 (marker does not hide it locally)pytest -m benchmarkselects exactly the 1 gatetests/README.mdand.claude/commands/sieval-release.md2723 passed, 1 deselected; the serial gate reports1 passed, 2794 deselectedand writes the artifact without dirtying the treeManual
Flakiness fix verified empirically, not just reasoned about.
Measured acceptance headroom on a dedicated box, the basis for excluding the gate from CI:
Checklist
Required (all PRs)
type(scope): description)AI-Generated Code - <model> (<provider>)in module docstring — n/a, no new modulescore/timer_dep/timer_nodeplocals were test-local)Notes for reviewers
tests/unitalone. No new install groups needed — everythingtests/integrationandtests/acceptanceimport (anyio,datasets,orjson,loguruin default;psutil,pytestintest) is already covered byINSTALL_GROUPS.addoptsis deliberately left at-m "not stress", sobenchmarktests keep running on a plain localpytest. Only the CI invocation deselects them.🤖 Generated with Claude Code