From aa38f218b5965ad4a53524eb287e3e026f89d1a3 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Thu, 20 Aug 2026 04:46:54 -0500 Subject: [PATCH 1/2] 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 --- tests/common/mod.rs | 63 ++++++++++++++++++++++++++++++++++++++++- tests/create_test.rs | 62 ++++++++++++++++++---------------------- tests/gitignore_test.rs | 62 +++++++++++++--------------------------- 3 files changed, 110 insertions(+), 77 deletions(-) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index eb09c23..6881bb8 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,9 +1,70 @@ -use std::{fs, path::PathBuf}; +use std::{ + fs, + path::{Path, PathBuf}, +}; +use tempfile::TempDir; // // For more information about this file's naming convention, see // https://doc.rust-lang.org/book/ch11-03-test-organization.html // + +/// A private copy of a fixture app, for tests that run `pks` against it. +/// +/// Tests within a binary run on parallel threads, and running `pks` writes into +/// the fixture (`tmp/cache/packwerk/...`), while several helpers in this module +/// delete or rewrite fixture files. Sharing one on-disk copy between threads +/// therefore races: one test removes a directory another is mid-way through +/// writing into. +/// +/// Copying is cheap -- the largest fixture is 64 KB -- and buys real isolation, +/// so tests stay parallel and need no teardown. The copy is removed when the +/// `Fixture` is dropped. +#[allow(dead_code)] +pub struct Fixture { + // Held for its Drop impl: removing it deletes the copy. + _dir: TempDir, + root: PathBuf, +} + +#[allow(dead_code)] +impl Fixture { + /// Copies `tests/fixtures/` into a fresh temporary directory. + pub fn new(name: &str) -> Fixture { + let dir = TempDir::new().expect("could not create temp dir"); + let root = dir.path().join(name); + let source = Path::new("tests/fixtures").join(name); + copy_dir_recursive(&source, &root).unwrap_or_else(|e| { + panic!("could not copy fixture {}: {}", source.display(), e) + }); + Fixture { _dir: dir, root } + } + + /// The fixture root, to pass to `--project-root`. + pub fn root(&self) -> &Path { + &self.root + } + + /// A path inside the fixture. + pub fn path(&self, relative: &str) -> PathBuf { + self.root.join(relative) + } +} + +fn copy_dir_recursive(from: &Path, to: &Path) -> std::io::Result<()> { + fs::create_dir_all(to)?; + for entry in fs::read_dir(from)? { + let entry = entry?; + let target = to.join(entry.file_name()); + if entry.file_type()?.is_dir() { + copy_dir_recursive(&entry.path(), &target)?; + } else { + fs::copy(entry.path(), &target)?; + } + } + Ok(()) +} + #[allow(dead_code)] pub fn teardown() { glob::glob("tests/fixtures/*/tmp/cache/packwerk") diff --git a/tests/create_test.rs b/tests/create_test.rs index 19b4fe6..1516957 100644 --- a/tests/create_test.rs +++ b/tests/create_test.rs @@ -1,17 +1,26 @@ use assert_cmd::cargo::cargo_bin_cmd; use predicates::prelude::*; use pretty_assertions::assert_eq; -use std::{error::Error, fs, path::Path}; +use std::{error::Error, fs}; mod common; +// `pks create` writes into the project root, and these tests previously ran +// against the shared fixtures in `tests/fixtures/`, cleaning up afterwards with +// `common::delete_foobar*()` and `common::teardown()`. Because tests in a binary +// run on parallel threads, those cleanups deleted directories that sibling tests +// were still using -- `test_create_already_exists` failed about one run in ten. +// +// Each test now works on its own copy, so there is nothing shared to clean up and +// no need to serialize. + #[test] fn test_create() -> Result<(), Box> { - common::delete_foobar(); + let fixture = common::Fixture::new("simple_app"); cargo_bin_cmd!("pks") .arg("--project-root") - .arg("tests/fixtures/simple_app") + .arg(fixture.root()) .arg("create") .arg("packs/foobar") .assert() @@ -21,20 +30,14 @@ fn test_create() -> Result<(), Box> { )); let actual = fs::read_to_string( - "tests/fixtures/simple_app/packs/foobar/package.yml", + fixture.path("packs/foobar/package.yml"), ).unwrap_or_else(|_| panic!("Could not read file tests/fixtures/simple_app/packs/foobar/package.yml")); assert!(actual.contains("enforce_dependencies: true")); assert!(actual.contains("enforce_privacy: true")); assert!(actual.contains("enforce_layers: true")); - assert!(Path::new( - "tests/fixtures/simple_app/packs/foobar/app/public/foobar" - ) - .exists()); - assert!(Path::new( - "tests/fixtures/simple_app/packs/foobar/app/services/foobar" - ) - .exists()); - assert!(Path::new("tests/fixtures/simple_app/packs/foobar/spec").exists()); + assert!(fixture.path("packs/foobar/app/public/foobar").exists()); + assert!(fixture.path("packs/foobar/app/services/foobar").exists()); + assert!(fixture.path("packs/foobar/spec").exists()); let expected_readme = String::from("\ Welcome to `packs/foobar`! @@ -54,31 +57,28 @@ README.md should change as your public API changes. See https://github.com/rubyatscale/pks#readme for more info!"); let actual_readme = - fs::read_to_string("tests/fixtures/simple_app/packs/foobar/README.md").unwrap_or_else(|e| { + fs::read_to_string(fixture.path("packs/foobar/README.md")).unwrap_or_else(|e| { panic!("Could not read file tests/fixtures/simple_app/packs/foobar/README.md: {}", e) }); assert_eq!(expected_readme, actual_readme); - common::teardown(); - common::delete_foobar(); - Ok(()) } #[test] fn test_create_with_readme_template_default_path() -> Result<(), Box> { - common::delete_foobaz(); + let fixture = common::Fixture::new("simple_packs_first_app"); fs::write( - "tests/fixtures/simple_packs_first_app/README_TEMPLATE.md", + fixture.path("README_TEMPLATE.md"), "This is a test custom README template", )?; cargo_bin_cmd!("pks") .arg("--project-root") - .arg("tests/fixtures/simple_packs_first_app") + .arg(fixture.root()) .arg("create") .arg("packs/foobaz") .assert() @@ -86,17 +86,13 @@ fn test_create_with_readme_template_default_path() -> Result<(), Box> let expected_readme = String::from("This is a test custom README template"); let actual_readme = - fs::read_to_string("tests/fixtures/simple_packs_first_app/packs/foobaz/README.md").unwrap_or_else(|e| { + fs::read_to_string(fixture.path("packs/foobaz/README.md")).unwrap_or_else(|e| { panic!("Could not read file tests/fixtures/simple_packs_first_app/packs/foobaz/README.md: {}", e) }); assert_eq!(expected_readme, actual_readme); - common::teardown(); - common::delete_foobaz(); - fs::remove_file( - "tests/fixtures/simple_packs_first_app/README_TEMPLATE.md", - )?; + fs::remove_file(fixture.path("README_TEMPLATE.md"))?; Ok(()) } @@ -104,11 +100,11 @@ fn test_create_with_readme_template_default_path() -> Result<(), Box> #[test] fn test_create_with_readme_template_custom_path() -> Result<(), Box> { - common::delete_foobar_app_with_custom_readme(); + let fixture = common::Fixture::new("app_with_custom_readme"); cargo_bin_cmd!("pks") .arg("--project-root") - .arg("tests/fixtures/app_with_custom_readme") + .arg(fixture.root()) .arg("create") .arg("packs/foobar") .assert() @@ -117,29 +113,27 @@ fn test_create_with_readme_template_custom_path() -> Result<(), Box> let expected_readme = String::from("README template\n\nThis is a test\n"); let actual_readme = - fs::read_to_string("tests/fixtures/app_with_custom_readme/packs/foobar/README.md").unwrap_or_else(|e| { + fs::read_to_string(fixture.path("packs/foobar/README.md")).unwrap_or_else(|e| { panic!("Could not read file tests/fixtures/app_with_custom_readme/packs/foobar/README.md: {}", e) }); assert_eq!(expected_readme, actual_readme); - common::teardown(); - common::delete_foobar_app_with_custom_readme(); - Ok(()) } #[test] fn test_create_already_exists() -> Result<(), Box> { + let fixture = common::Fixture::new("simple_packs_first_app"); + cargo_bin_cmd!("pks") .arg("--project-root") - .arg("tests/fixtures/simple_packs_first_app") + .arg(fixture.root()) .arg("create") .arg("packs/foo") .assert() .success() .stdout(predicate::str::contains("`packs/foo` already exists!")); - common::teardown(); Ok(()) } diff --git a/tests/gitignore_test.rs b/tests/gitignore_test.rs index fa0a637..f5adcf4 100644 --- a/tests/gitignore_test.rs +++ b/tests/gitignore_test.rs @@ -5,7 +5,6 @@ use serial_test::serial; use std::fs; use std::path::PathBuf; use std::{error::Error, process::Command}; -use tempfile::TempDir; mod common; @@ -14,13 +13,15 @@ mod common; #[test] fn test_check_ignores_violations_in_gitignored_files( ) -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_gitignore"); + // The fixture has: // - packs/foo/app/services/foo.rb with violation (NOT ignored) // - ignored_folder/violating.rb with violation (IS ignored) let result = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") - .arg("tests/fixtures/app_with_gitignore") + .arg(fixture.root()) .arg("check") .assert() .failure(); // Still fails due to violation in foo.rb @@ -45,7 +46,6 @@ fn test_check_ignores_violations_in_gitignored_files( "Should NOT detect violations in gitignored files.\nstdout: {}\nstderr: {}", stdout, stderr ); - common::teardown(); Ok(()) } @@ -53,9 +53,11 @@ fn test_check_ignores_violations_in_gitignored_files( #[test] fn test_list_included_files_excludes_gitignored() -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_gitignore"); + let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") - .arg("tests/fixtures/app_with_gitignore") + .arg(fixture.root()) .arg("list-included-files") .assert() .success() @@ -89,26 +91,26 @@ fn test_list_included_files_excludes_gitignored() -> Result<(), Box> "Should NOT include files in ignored directories" ); - common::teardown(); Ok(()) } /// Test that the application works correctly even without a .gitignore file. #[test] fn test_check_works_without_gitignore() -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + // simple_app doesn't have a .gitignore file // This should still work (and report violations as usual) Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") - .arg("tests/fixtures/simple_app") + .arg(fixture.root()) .arg("--debug") .arg("check") .assert() .failure() // Has violations .stdout(predicate::str::contains("violation(s) detected")); - common::teardown(); Ok(()) } @@ -187,6 +189,8 @@ fn test_gitignore_matcher_without_gitignore() -> Result<(), Box> { /// CRITICAL: Test that respect_gitignore: false configuration disables gitignore support. #[test] fn test_respect_gitignore_can_be_disabled() -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_gitignore_disabled"); + // The fixture has: // - .gitignore with ignored_folder/ pattern // - respect_gitignore: false in packwerk.yml @@ -196,7 +200,7 @@ fn test_respect_gitignore_can_be_disabled() -> Result<(), Box> { let result = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") - .arg("tests/fixtures/app_with_gitignore_disabled") + .arg(fixture.root()) .arg("check") .assert() .failure(); // Should fail due to violation in ignored_folder/ @@ -212,7 +216,6 @@ fn test_respect_gitignore_can_be_disabled() -> Result<(), Box> { stdout, stderr ); - common::teardown(); Ok(()) } @@ -257,6 +260,8 @@ fn test_gitignore_negation_patterns() -> Result<(), Box> { /// and won't appear in list-included-files regardless of gitignore. #[test] fn test_list_included_files_respects_negation() -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_gitignore"); + // This is already tested by test_gitignore_negation_patterns at the library level. // At the CLI level, .log files aren't included in list-included-files anyway // since they don't match the Ruby file patterns. @@ -264,7 +269,7 @@ fn test_list_included_files_respects_negation() -> Result<(), Box> { // Just verify the basic behavior still works let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") - .arg("tests/fixtures/app_with_gitignore") + .arg(fixture.root()) .arg("list-included-files") .assert() .success() @@ -280,7 +285,6 @@ fn test_list_included_files_respects_negation() -> Result<(), Box> { "Should include non-ignored Ruby files" ); - common::teardown(); Ok(()) } @@ -337,8 +341,8 @@ fn test_respects_global_gitignore() -> Result<(), Box> { fs::write(&global_gitignore, "# Global test\n*.global_ignore\n")?; // Create a test file that should be ignored - let fixture_path = PathBuf::from("tests/fixtures/app_with_gitignore"); - let test_file = fixture_path.join("test.global_ignore"); + let fixture = common::Fixture::new("app_with_gitignore"); + let test_file = fixture.path("test.global_ignore"); fs::write(&test_file, "// Should be ignored by global gitignore\n")?; // Save original core.excludesFile config @@ -379,7 +383,7 @@ fn test_respects_global_gitignore() -> Result<(), Box> { // Test that list-included-files excludes the globally ignored file let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") - .arg("tests/fixtures/app_with_gitignore") + .arg(fixture.root()) .arg("list-included-files") .assert() .success() @@ -396,7 +400,6 @@ fn test_respects_global_gitignore() -> Result<(), Box> { stdout ); - common::teardown(); Ok(()) } @@ -404,18 +407,12 @@ fn test_respects_global_gitignore() -> Result<(), Box> { /// Gitignored files should not cause package_todo.yml updates. #[test] fn test_update_respects_gitignore() -> Result<(), Box> { - // Create a temporary copy of the fixture - let temp_dir = TempDir::new()?; - let temp_fixture = temp_dir.path().join("app"); - - // Copy fixture to temp directory - let fixture_path = "tests/fixtures/app_with_gitignore"; - copy_dir_all(fixture_path, &temp_fixture)?; + let fixture = common::Fixture::new("app_with_gitignore"); // Run update command let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) .arg("--project-root") - .arg(&temp_fixture) + .arg(fixture.root()) .arg("update") .assert() .success() @@ -432,24 +429,5 @@ fn test_update_respects_gitignore() -> Result<(), Box> { stdout ); - common::teardown(); - Ok(()) -} - -// Helper function to copy directories recursively -fn copy_dir_all( - src: impl AsRef, - dst: impl AsRef, -) -> std::io::Result<()> { - fs::create_dir_all(&dst)?; - for entry in fs::read_dir(src)? { - let entry = entry?; - let ty = entry.file_type()?; - if ty.is_dir() { - copy_dir_all(entry.path(), dst.as_ref().join(entry.file_name()))?; - } else { - fs::copy(entry.path(), dst.as_ref().join(entry.file_name()))?; - } - } Ok(()) } From a5a584eab2f65d1a30adf5bef7f02032b0f793a2 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Thu, 20 Aug 2026 16:25:05 -0500 Subject: [PATCH 2/2] 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 --- tests/common/mod.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 6881bb8..f9399c7 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -20,6 +20,17 @@ use tempfile::TempDir; /// Copying is cheap -- the largest fixture is 64 KB -- and buys real isolation, /// so tests stay parallel and need no teardown. The copy is removed when the /// `Fixture` is dropped. +/// +/// 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. #[allow(dead_code)] pub struct Fixture { // Held for its Drop impl: removing it deletes the copy. @@ -56,6 +67,10 @@ fn copy_dir_recursive(from: &Path, to: &Path) -> std::io::Result<()> { for entry in fs::read_dir(from)? { let entry = entry?; let target = to.join(entry.file_name()); + // `file_type()` does not follow symlinks, so a symlink to a directory + // would take the `fs::copy` branch below and fail with a directory + // target. No fixture contains a symlink today (`find tests/fixtures + // -type l` is empty); handle it here if one ever needs to. if entry.file_type()?.is_dir() { copy_dir_recursive(&entry.path(), &target)?; } else {