From dfa0c81f21b64f77a48ff84b48234b908d9b28fc Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Wed, 19 Aug 2026 21:08:21 -0500 Subject: [PATCH 1/2] Fix flaky gitignore_test by serializing the tests that shell out to pks `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/": 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 --- tests/gitignore_test.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/gitignore_test.rs b/tests/gitignore_test.rs index fa0a637..790ba8a 100644 --- a/tests/gitignore_test.rs +++ b/tests/gitignore_test.rs @@ -9,9 +9,26 @@ use tempfile::TempDir; mod common; +// Every test below that shells out to `pks` is marked `#[serial]`. +// +// `common::teardown()` globs `tests/fixtures/*/tmp/cache/packwerk` and 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 could delete a +// directory another test was mid-way through writing into. `pks` creates a cache +// entry with `create_dir_all(parent)` followed by `File::create`, and losing the +// parent between those two calls surfaced as: +// +// Failed to create cache file ".../tmp/cache/packwerk/zeitwerk/": +// Invalid argument (os error 22) +// +// which failed the run about one time in three. Note the cache is written even +// though these fixtures do not set `cache:` -- the constant-resolver cache is +// not governed by that setting, so it cannot be configured away. + /// Test that gitignored files are completely excluded from violation checking. /// The fixture has a violation in ignored_folder/violating.rb which should NOT be reported. #[test] +#[serial] fn test_check_ignores_violations_in_gitignored_files( ) -> Result<(), Box> { // The fixture has: @@ -51,6 +68,7 @@ fn test_check_ignores_violations_in_gitignored_files( /// Test that list-included-files respects gitignore patterns. #[test] +#[serial] fn test_list_included_files_excludes_gitignored() -> Result<(), Box> { let output = Command::new(assert_cmd::cargo::cargo_bin!("pks")) @@ -95,6 +113,7 @@ fn test_list_included_files_excludes_gitignored() -> Result<(), Box> /// Test that the application works correctly even without a .gitignore file. #[test] +#[serial] fn test_check_works_without_gitignore() -> Result<(), Box> { // simple_app doesn't have a .gitignore file // This should still work (and report violations as usual) @@ -186,6 +205,7 @@ fn test_gitignore_matcher_without_gitignore() -> Result<(), Box> { /// CRITICAL: Test that respect_gitignore: false configuration disables gitignore support. #[test] +#[serial] fn test_respect_gitignore_can_be_disabled() -> Result<(), Box> { // The fixture has: // - .gitignore with ignored_folder/ pattern @@ -256,6 +276,7 @@ fn test_gitignore_negation_patterns() -> Result<(), Box> { /// Note: We test this at the library level since .log files aren't Ruby files /// and won't appear in list-included-files regardless of gitignore. #[test] +#[serial] fn test_list_included_files_respects_negation() -> Result<(), Box> { // 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 @@ -403,6 +424,7 @@ fn test_respects_global_gitignore() -> Result<(), Box> { /// Test that gitignore works with the update command. /// Gitignored files should not cause package_todo.yml updates. #[test] +#[serial] fn test_update_respects_gitignore() -> Result<(), Box> { // Create a temporary copy of the fixture let temp_dir = TempDir::new()?; From 4a61bee0ad41dada44f1dd3bfb2a17ad279e28c7 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Wed, 19 Aug 2026 21:13:53 -0500 Subject: [PATCH 2/2] Also serialize create_test, same root cause `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 --- tests/create_test.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/create_test.rs b/tests/create_test.rs index 19b4fe6..d1016fb 100644 --- a/tests/create_test.rs +++ b/tests/create_test.rs @@ -4,8 +4,17 @@ use pretty_assertions::assert_eq; use std::{error::Error, fs, path::Path}; mod common; +use serial_test::serial; + +// `#[serial]` because these tests mutate shared fixture state. Two of them run +// against `simple_packs_first_app` -- one creating and deleting `packs/foobaz` +// inside it -- and all four call `common::teardown()`, which removes the cache +// for every fixture, not just the one the calling test used. Run in parallel, +// they delete directories out from under each other, which failed +// `test_create_already_exists` roughly one run in ten. #[test] +#[serial] fn test_create() -> Result<(), Box> { common::delete_foobar(); @@ -67,6 +76,7 @@ See https://github.com/rubyatscale/pks#readme for more info!"); } #[test] +#[serial] fn test_create_with_readme_template_default_path() -> Result<(), Box> { common::delete_foobaz(); @@ -102,6 +112,7 @@ fn test_create_with_readme_template_default_path() -> Result<(), Box> } #[test] +#[serial] fn test_create_with_readme_template_custom_path() -> Result<(), Box> { common::delete_foobar_app_with_custom_readme(); @@ -130,6 +141,7 @@ fn test_create_with_readme_template_custom_path() -> Result<(), Box> } #[test] +#[serial] fn test_create_already_exists() -> Result<(), Box> { cargo_bin_cmd!("pks") .arg("--project-root")