Repository navigation
Conversation
Two test files reached the HTTP app through state a *previous* file had left on a module global. `tests/test_memory_crud.py` failed four of its own tests and `tests/test_error_shape.py` two when run on their own - and they pass in a single-process run, which is what CI does and what this session had been doing. A test that only passes in the company of another test is not measuring what it claims to. Root cause is the shape, not the two files: five files had each grown their own slightly different installer (`_install_app`, `_client`, `_install_app_state`, an autouse fixture), because nothing shared existed. That is now one function. - `conftest.install_app_state(**overrides)` installs fresh storage, engine, key store and scoring limiter, and returns what it installed; `app_state` and `app_client` fixtures build on it. It takes the knobs the files actually vary - `api_key_store`, `scoring_limit`, `scoring_clock`, `lifecycle_settings`, `retention_days` - so `test_auth`, `test_ratelimit`, `test_scheduler` keep their specifics in a two-line adapter instead of a ten-line copy. - Six files migrated; the duplicated installers are gone. `test_transitions`' three HTTP tests also lost the `created_at` they echoed back, which the previous PR made unnecessary. - **CI runs the server suite one process per file.** The shell already runs with `-e`, so the first failing file stops the build. This is the discipline `scripts/run_tests.sh` enforces in the project this repo borrows its test practice from, and it is what makes the fix stick: without it, someone reintroduces a cross-file dependency and the build stays green because the order happens to work. - `CONTRIBUTING.md` states the rule and shows the fixture pattern, so the next file has an obvious right way to do it rather than a blank page. Verified with the CI loop itself, run locally: all 16 files pass alone, 244 tests, 26 skipped (the Postgres half runs in its own job).
Owner
Author
|
Landed on master in the v0.1.0 chain: the branch was fast-forward merged as part of |
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.
test: one way to install app state, and a CI run that cannot hide a leak
Two test files reached the HTTP app through state a previous file had left on a
module global.
tests/test_memory_crud.pyfailed four of its own tests andtests/test_error_shape.pytwo when run on their own - and they pass in asingle-process run, which is what CI does and what this session had been doing.
A test that only passes in the company of another test is not measuring what it
claims to.
Root cause is the shape, not the two files: five files had each grown their own
slightly different installer (
_install_app,_client,_install_app_state,an autouse fixture), because nothing shared existed. That is now one function.
conftest.install_app_state(**overrides)installs fresh storage, engine, keystore and scoring limiter, and returns what it installed;
app_stateandapp_clientfixtures build on it. It takes the knobs the files actually vary -api_key_store,scoring_limit,scoring_clock,lifecycle_settings,retention_days- sotest_auth,test_ratelimit,test_schedulerkeep theirspecifics in a two-line adapter instead of a ten-line copy.
test_transitions' threeHTTP tests also lost the
created_atthey echoed back, which the previous PRmade unnecessary.
-e, so the first failing file stops the build. This is the disciplinescripts/run_tests.shenforces in the project this repo borrows its testpractice from, and it is what makes the fix stick: without it, someone
reintroduces a cross-file dependency and the build stays green because the order
happens to work.
CONTRIBUTING.mdstates the rule and shows the fixture pattern, so the nextfile has an obvious right way to do it rather than a blank page.
Verified with the CI loop itself, run locally: all 16 files pass alone, 244 tests,
26 skipped (the Postgres half runs in its own job).