Isolate test fixtures instead of serializing (replaces #56)

[perryqh]

Replaces #56, which fixed the same two flakes with #[serial]. Same failures, better mechanism — see the comparison below.

Two tests fail intermittently on main today:

test	failure rate
gitignore_test::test_respect_gitignore_can_be_disabled	2 in 20 idle · 3 in 6 under load
create_test::test_create_already_exists	~1 in 10

Cause

Tests run pks against the shared fixtures in tests/fixtures/, pks writes into whatever project root it is given (tmp/cache/packwerk/...), and the cleanup helpers in tests/common/mod.rs mutate global state:

• teardown() globs tests/fixtures/*/tmp/cache/packwerk and deletes the cache of every fixture, not the one the caller used
• delete_foobar() / delete_foobaz() / delete_foobar_app_with_custom_readme() remove whole pack directories

Tests within a binary run on parallel threads, so those cleanups delete state a sibling test is still using.

The exact window for the gitignore failure: pks writes a cache entry as create_dir_all(parent) then File::create (src/packs/caching/per_file_cache.rs:58-67). Losing the parent between those two calls is why it surfaces as EINVAL rather than the ENOENT you would expect:

Error: Failed to check files: Failed to create cache file
".../app_with_gitignore_disabled/tmp/cache/packwerk/zeitwerk/<hash>":
Invalid argument (os error 22)

Fix

Adds common::Fixture, which copies a fixture into a temp directory and deletes it on drop. Affected tests get their own copy, so there is no shared state to race over and no cleanup calls at all.

let fixture = common::Fixture::new("app_with_gitignore");

Command::new(cargo_bin!("pks"))
    .arg("--project-root").arg(fixture.root())
    .arg("check")

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 — so this consolidates an approach the file already used rather than introducing a new one.

Why this over #[serial] (#56)

	#[serial] (#56)	isolation (this PR)
fixes	the symptom	the cause
tests stay parallel	❌	✅
runtime, these two files	0.83s	0.72s
future test can reintroduce it	✅ by forgetting an attribute	❌ nothing shared to race on
#[serial] attributes needed	10	1

Forgetting an attribute is silent. #[serial] requires every current and future test touching these fixtures to remember it, with no error if it does not — which is how create_test was broken while gitignore_test had one test correctly marked. Isolation removes the hazard instead of documenting it.

Copying is cheap — the largest fixture involved is 64 KB.

One #[serial] remains, and it is correct

test_respects_global_gitignore mutates git config --global. That is machine-wide state which copying files cannot isolate, so serializing is the right tool there specifically. It predates this PR. It now also gets an isolated fixture so it stops writing a scratch file into the repo tree.

Verification

	result
gitignore_test, 25 consecutive runs	25 / 0
create_test, 25 consecutive runs	25 / 0
full suite, 5 consecutive runs	258 passing / 0 failing each
fixtures touched by this PR left modified	none

cargo fmt --all -- --check and cargo clippy --all-targets --all-features clean.

Note

Correction. An earlier version of this description claimed git status tests/fixtures/ was clean after a full run. That was wrong, and thanks to @dduugg for catching it. cargo test --test check_unused_dependencies leaves app_with_unnecessary_dependencies/packs/foo/package.yml modified every time — reproduced 3/3 here.

My own verification had masked it: my loops ran git checkout -- tests/fixtures/ between iterations, so I cleaned up the evidence and then reported the result as clean.

The mechanism is the opposite of what you would guess. set_up_fixtures() writes content byte-identical to what is committed, so it is the restore, not the mutation. The dirt comes from test_auto_correct_unnecessary_dependencies running pks -a, which rewrites the file to its corrected form (drops - packs/baz, reorders keys) with nothing restoring it afterwards — set_up_fixtures() only runs at the start of each case. Whether the tree ends up clean depends on which binary happens to run last.

Pre-existing, in a file this PR does not convert. The row above is now scoped to what this PR actually fixes.

The flakes were 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 — 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.

This narrows the flake surface, it does not close it

Stated plainly, since the numbers make the remaining scope clear. teardown() and its glob survive, and 13 files still call it — check_test.rs alone has 24 call sites:

file	teardown() calls	notes
check_test.rs	24	shared fixtures, unserialized
folder_privacy_test.rs	4
add_dependency_test.rs, update_test.rs, layer_violations_test.rs, validate_test.rs, visibility_test.rs	3 each	some already #[serial]
check_unused_dependencies.rs	0, but calls set_up_fixtures()	the file that dirties the tree

I ran check_test and check_unused_dependencies 12× each and they stayed green, so those read as latent rather than live.

common::Fixture is the migration path: converting those files removes teardown() and the delete_* helpers entirely, and would also fix the dirty-tree problem above. Out of scope here — this PR fixes the two failures that actually reproduce, so CI is trustworthy for the #52/#53/#54 stack.

Latent assumptions, now documented in the code

Both raised in review, neither reachable today:

• The copy relies on cargo test running binaries sequentially. Unconverted files still call the global teardown(); if that ran mid-copy, copy_dir_recursive would panic with NotFound. Cargo finishes each binary before starting the next, so it cannot happen — 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. find tests/fixtures -type l is empty.

🤖 Generated with Claude Code

[dduugg]

Approving. I agree with the approach — isolating the state beats serializing access to it, and the reasoning in the description for preferring this over #56 matches what I found in the code. One factual correction to the Verification table, detailed below.

Verified

• Fixture stores the TempDir itself in _dir, not a .path() snapshot, so the temp dir outlives every use and root() can never point at a deleted path. This is the bug I most expected to find here; it isn't present.
• .gitignore files really are copied. copy_dir_recursive uses plain fs::read_dir, which doesn't skip dotfiles. Worth stating explicitly because the failure mode — gitignore tests passing vacuously against a copy with no .gitignore in it — would have been invisible and would have made the whole suite worthless.
• Moving fixtures to a tmpdir doesn't change what the git tests exercise: build_gitignore_matcher only reads <given_root>/.gitignore and <given_root>/.git/info/exclude directly, with no parent-directory walk to find an enclosing .git, and no fixture ships its own .git. Global core.excludesFile is machine-wide and location-independent. So behavior is identical in-tree vs copied.
• Fixture is Send/Sync, and TempDir::new() yields a unique path per call, so two concurrent copies of the same fixture name can't collide.
• The removed copy_dir_all and the new copy_dir_recursive are logically identical, so folding it into the shared helper is a clean consolidation.
• The create_dir_all / File::create diagnosis is correct — src/packs/caching/per_file_cache.rs:58-67, which explains the EINVAL.
• Both target tests are solid now: test_create_already_exists and test_respect_gitignore_can_be_disabled, 20/20 each.
• Keeping #[serial] on test_respects_global_gitignore is the right call — git config --global is machine-wide state that copying files cannot isolate.

---

Correction: the "git status tests/fixtures/ after 5 full runs: clean" row doesn't hold.

Reproduced from a clean checkout of this branch: running just cargo test --test check_unused_dependencies leaves tests/fixtures/app_with_unnecessary_dependencies/packs/foo/package.yml modified, every time.

The mechanism is worth stating precisely, because it's the opposite of what you might expect: common::set_up_fixtures() writes content byte-identical to what's committed (I diffed it), so set_up_fixtures is the restore, not the mutation. The dirt comes from test_auto_correct_unnecessary_dependencies, which runs pks -a and rewrites that file to its corrected form with nothing restoring it afterward — set_up_fixtures() only runs at the start of each case, inside assert_auto_correct_unused_dependencies. So whether the tree ends clean depends on which binary happens to run last.

This is pre-existing and not caused by this PR — #52's description notes the same file. But it's the exact file named in your "Still available as follow-up" section, and the Verification table asserts the problem is now resolved, which undersells what's left. Suggest adjusting that row to scope the claim to the fixtures this PR actually isolates.

Two inline nits, both latent rather than live. Also, since teardown() and its glob survive this PR, create_test.rs retains one call, and check_test.rs still has 20+ shared-fixture sites, this narrows the flake surface rather than closing it — which your follow-up section already says. Worth landing this first regardless: it makes CI trustworthy for the #52/#53/#54 stack, and I confirmed it merges cleanly with #52 in either order (that PR edits teardown() below your insertion, and both changes survive).

[dduugg]

Latent nit, not live: entry.file_type() does not follow symlinks, so a symlink-to-directory would fail is_dir(), fall into the fs::copy branch, and error on a directory target.

find tests/fixtures -type l returns nothing today, so no fixture exercises this. Only worth handling if a fixture ever needs a symlink — flagging it so the failure is recognizable rather than mysterious if that happens.

[dduugg]

There's an invisible assumption here worth a comment: this copy is only race-free because cargo test runs test binaries sequentially.

check_test.rs and check_unused_dependencies.rs still run pks against the shared tests/fixtures/simple_app and still call the global teardown(), which globs and deletes tests/fixtures/*/tmp/cache/packwerk across all fixtures. If that deletion landed while copy_dir_recursive was mid-walk of the same subtree, read_dir/copy would return NotFound and this unwrap_or_else would panic — a new failure mode introduced by copying.

Not exploitable today: cargo finishes each binary (and all its teardown() calls) before starting the next, and I stress-tested it by forcing all four binaries to run as concurrent OS processes, 15 iterations, with no copy panics. The risk appears only if the repo adopts cargo-nextest, which does run binaries concurrently.

A one-line comment noting the dependency would keep a future nextest migration from rediscovering this the hard way.

[perryqh]

Added in a5a584e — this is the more valuable of the two nits, because it's a failure mode introduced by this PR rather than one it inherits, and it would be invisible until someone migrates.

The doc comment on Fixture now says:

One assumption to be aware of: the copy itself is only race-free because cargo test runs test binaries sequentially. Files not yet converted to this helper (check_test.rs, check_unused_dependencies.rs, and others) still call teardown(), which deletes tests/fixtures/*/tmp/cache/packwerk across every fixture. If that ran while copy_dir_recursive was mid-walk of the same subtree, read_dir/copy would fail with NotFound and the panic below would fire. Cargo finishes each binary, teardowns included, before starting the next, so this cannot happen today — but a move to cargo-nextest, which runs binaries concurrently, would expose it. Converting the remaining callers off teardown() removes the assumption entirely.

I kept your last sentence as the closing line deliberately: the comment should point at the fix, not just describe the hazard, so whoever hits it knows the exit rather than reaching for a retry loop.

Also worth recording that you stress-tested it by forcing all four binaries to run as concurrent OS processes for 15 iterations with no copy panics. That's a stronger negative result than "cargo doesn't do this today" — it says the window is narrow even when you deliberately open it.

[perryqh]

Thanks — the correction is right and I reproduced it 3/3. Description updated.

On the dirty tree. Your mechanism is exactly right: set_up_fixtures() writes content byte-identical to the committed file, so it is the restore, not the mutation. The dirt is test_auto_correct_unnecessary_dependencies running pks -a, which drops - packs/baz and reorders keys with nothing restoring it afterwards.

Worth naming why I got it wrong rather than just fixing the row: my verification loops ran git checkout -- tests/fixtures/ between iterations. I was cleaning up the evidence and then reporting the result as clean. The claim was an artifact of how I tested, not something I observed.

That row is now scoped to the fixtures this PR actually isolates, and the follow-up section carries the real remaining numbers — 13 files still call teardown(), check_test.rs alone has 24 sites — so it no longer reads as more closed than it is.

Both inline nits are now comments in copy_dir_recursive and the Fixture doc. The nextest one is the more valuable of the two: it is a genuine new failure mode introduced by copying, invisible until someone migrates. Confirmed find tests/fixtures -type l is empty for the symlink case.

One small correction back. create_test.rs has no remaining teardown() call — grep counts 1 because my explanatory comment mentions it in backticks. Excluding comment lines, both converted files are at zero:

$ grep -vE '^\s*//' tests/create_test.rs | grep -c 'common::teardown()'
0

Does not change your point that teardown() survives repo-wide, which it does.

Also appreciate you checking the two things that would have made this PR worthless — that .gitignore files are actually copied (read_dir does not skip dotfiles, so the gitignore tests are not passing vacuously), and that build_gitignore_matcher does no parent-directory walk, so a tmpdir root behaves identically to an in-tree one. Those were the failure modes I would least have wanted to miss.