diff --git a/tests/common/mod.rs b/tests/common/mod.rs index eb09c23..f9399c7 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,9 +1,85 @@ -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. +/// +/// 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. + _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()); + // `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 { + 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(()) }