Fix two flaky tests caused by shared fixture mutation - #56
Closed
perryqh wants to merge 2 commits into
Closed
Conversation
`gitignore_test::test_respect_gitignore_can_be_disabled` failed roughly one run
in three on a busy machine, two in twenty on an idle one:
Error: Failed to check files: Failed to create cache file
".../app_with_gitignore_disabled/tmp/cache/packwerk/zeitwerk/<hash>":
Invalid argument (os error 22)
Cause: `common::teardown()` globs `tests/fixtures/*/tmp/cache/packwerk` and
removes the cache for *every* fixture, not just the one the calling test used.
Seven tests in this file call it, and tests within a binary run on parallel
threads, so one test's teardown deletes a directory another test is mid-way
through writing into. `pks` writes a cache entry as `create_dir_all(parent)`
followed by `File::create`, and losing the parent between those two calls is what
produces the EINVAL above.
The cache is written even though these fixtures set no `cache:` key -- the
constant-resolver cache is not governed by that setting -- so it cannot be
configured away.
Marks the six remaining tests that shell out to `pks` as `#[serial]`. This file
already imported `serial_test` and already marked one test that way, and four
other test files use the same approach, so this follows existing convention
rather than introducing a new mechanism. Scoping `teardown()` to a single fixture
would not have been enough on its own: four of these tests run against the same
fixture and would still race each other over its cache.
Verified: 25 consecutive runs of gitignore_test green (previously 2 failures in
20 on the same machine, and 3 in 6 under load), and 5 consecutive full-suite runs
at 258 passing / 0 failing.
Worth noting the flake was costing more than noise. `cargo test` stops at the
first failing target, so a red gitignore_test truncated the run -- 240 tests
attempted instead of 258, with roughly 18 in later binaries silently never
executing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`create_test::test_create_already_exists` fails roughly one run in ten, from the same shared-fixture race as gitignore_test. Two of this file's tests run against `simple_packs_first_app` concurrently -- one of them creating and deleting `packs/foobaz` inside it -- and all four call `common::teardown()`, which removes the cache for every fixture rather than the one the calling test used. Running in parallel they delete directories out from under each other, so `pks create` fails and the expected "already exists!" never reaches stdout. Verified: 20 consecutive runs of create_test green (previously 1 failure in 8-12 on the same machine), and 6 consecutive full-suite runs at 258 passing / 0 failing. I originally hit this failure while working on something unrelated and wrote it off as generic fixture flakiness. It is the same bug, and it was worth chasing rather than dismissing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Superseded by #57, which fixes the same two flakes by giving each test its own fixture copy rather than serializing access to a shared one. Isolation removes the shared state instead of coordinating access to it, so a future test cannot reintroduce the bug by forgetting an attribute. It also keeps the tests parallel (0.72s vs 0.83s), drops the Closing in favour of that. Diagnosis and evidence carry over unchanged. |
perryqh
added a commit
that referenced
this pull request
Aug 20, 2026
* Give tests their own fixture copies instead of sharing one on disk Two tests fail intermittently on main: `gitignore_test::test_respect_gitignore_can_be_disabled` (2 in 20 idle, 3 in 6 under load) and `create_test::test_create_already_exists` (about 1 in 10). Both come from the same thing: tests run `pks` against the shared fixtures in `tests/fixtures/`, `pks` writes into the project root it is given (`tmp/cache/packwerk/...`), and the cleanup helpers here mutate global state -- `teardown()` deletes the cache of *every* fixture, `delete_foobar*()` removes whole pack directories. Tests in a binary run on parallel threads, so those cleanups delete state a sibling test is still using. `pks` writes a cache entry as `create_dir_all(parent)` then `File::create`, and losing the parent between those two calls is the EINVAL in the gitignore failure. Adds `common::Fixture`, which copies a fixture into a temp directory and removes it on drop. Converting the affected tests to it removes the shared state rather than serializing access to it, so the tests stay parallel and need no cleanup calls at all. Chosen over `#[serial]` because it fixes the cause instead of the symptom: with isolation there is no shared state left to race over, so a future test cannot reintroduce the bug by forgetting an attribute. It is also faster (0.72s vs 0.83s for these two files) since the tests keep running concurrently, and it stops the suite leaving modified fixtures in the working tree -- `git status` after a run is now clean, where before it routinely showed a rewritten package.yml. `test_update_respects_gitignore` already hand-rolled this exact pattern with a local `copy_dir_all`; that is now folded into the shared helper and the duplicate deleted. One `#[serial]` remains, and is correct: `test_respects_global_gitignore` mutates `git config --global`, which is machine-wide and cannot be isolated by copying files. It is now also given an isolated fixture so it stops writing a scratch file into the repo tree. Verified: 25 consecutive runs of each file green, 5 consecutive full-suite runs at 258 passing / 0 failing, and no fixture left dirty afterwards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Document two assumptions in the Fixture helper Both raised in review, both latent rather than live, both worth recording so the failure is recognizable if it ever fires. The copy is only race-free because `cargo test` runs test binaries sequentially. Files not yet converted to `Fixture` still call the global `teardown()`, which deletes `tests/fixtures/*/tmp/cache/packwerk` across every fixture; if that ran during `copy_dir_recursive`, the copy would panic with NotFound. Cargo finishes each binary before starting the next, so it cannot happen today, but `cargo-nextest` runs binaries concurrently and would expose it. `entry.file_type()` does not follow symlinks, so a symlink-to-directory would take the `fs::copy` branch and fail on a directory target. `find tests/fixtures -type l` is empty, so no fixture exercises this. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <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.
Two tests fail intermittently on
maintoday, from the same root cause. Independent of any other work in flight.gitignore_test::test_respect_gitignore_can_be_disabledcreate_test::test_create_already_existsCause
common::teardown()globstests/fixtures/*/tmp/cache/packwerkand removes the cache for every fixture, not just the one the calling test used:Tests within a binary run on parallel threads, so one test's teardown deletes state another test is actively using.
create_testcompounds it:delete_foobar/delete_foobazremove whole pack directories from fixtures a sibling test is running against.The specific window for the gitignore failure:
pkswrites a cache entry ascreate_dir_all(parent)thenFile::create. Losing the parent between those two calls is why it surfaces asEINVAL, not theENOENTyou would expect:Two details worth knowing:
cache:key, yet a cache file is still written — the constant-resolver cache is not governed by that setting.teardown()to a single fixture would not have been enough. Four of the gitignore tests run against the same fixture and would still race each other. That is why this serializes instead of narrowing the glob.Fix
#[serial]on the tests that shell out topks— six ingitignore_test.rs, four increate_test.rs.This follows existing convention rather than adding a mechanism.
gitignore_test.rsalready importedserial_testand already markedtest_respects_global_gitignorethat way;add_dependency_test.rs,add_constant_dependencies.rs, andupdate_test.rsall do the same.serial_testhas been a dev-dependency all along, commented "Run specific tests in serial".Each file gets a comment explaining why, including the observed error, so the attributes do not get stripped later as redundant.
Verification
gitignore_test, 25 consecutive runscreate_test, 20 consecutive runscargo fmt --all -- --checkandcargo clippy --all-targets --all-featuresclean.The flakes were costing more than noise
cargo teststops at the first failing target, so a redgitignore_testtruncated the run: 240 tests attempted instead of 258 — roughly 18 tests in later binaries silently never executed. A flaky test early in the sequence was quietly reducing coverage on exactly the runs where you would most want it.Still latent, not fixed here
The real defect is that
teardown(),delete_foobar*, andset_up_fixtures()all mutate global fixture state while their callers run in parallel. Serializing these two files removes the two failures I could reproduce; it does not remove the wire. Others calling these helpers with no serialization at all:check_test.rsteardowncheck_unused_dependencies.rsset_up_fixturesfolder_privacy_test.rs,visibility_test.rs,layer_violations_test.rs,validate_test.rs, …teardownI ran
check_testandcheck_unused_dependencies12× each and they stayed green, so those look latent rather than live — butcheck_unused_dependenciesrewriting sharedpackage.ymlfiles with four unserialized tests is the same wire, waiting.Fixing it properly means narrowing
teardown()to take the fixture it should clean, which touches every call site across ~15 files. That is a refactor, not a flake fix, so it is out of scope here.🤖 Generated with Claude Code