Skip to content

Isolate test fixtures instead of serializing (replaces #56) #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
perryqh merged 2 commits into from
Aug 20, 2026
Merged

Isolate test fixtures instead of serializing (replaces #56) #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa38f21
Give tests their own fixture copies instead of sharing one on disk
perryqh Aug 20, 2026
a5a584e
Document two assumptions in the Fixture helper
perryqh Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 77 additions & 1 deletion tests/common/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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/<name>` 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| {

@dduugg dduugg Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 perryqh Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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() {

@dduugg dduugg Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@perryqh perryqh Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in a5a584e:

// `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() {

Confirmed find tests/fixtures -type l is empty independently.

I deliberately documented rather than handled it. Following symlinks would mean choosing between copying the target — which silently changes what the fixture is, since a fixture using a symlink probably does so on purpose — and preserving the link, which would point outside the temp dir and defeat the isolation this PR exists to provide. Neither is obviously right without a fixture that actually needs one, so guessing now would bake in the wrong answer. The comment makes the failure legible when someone has the real requirement in front of them.

Same behavior as the pre-existing copy_dir_all this replaces, so it isn't a regression — just a newly-documented edge.

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")
Expand Down
62 changes: 28 additions & 34 deletions tests/create_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<dyn Error>> {
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()
Expand All @@ -21,20 +30,14 @@ fn test_create() -> Result<(), Box<dyn Error>> {
));

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`!
Expand All @@ -54,61 +57,54 @@ 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<dyn Error>>
{
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()
.success();

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(())
}

#[test]
fn test_create_with_readme_template_custom_path() -> Result<(), Box<dyn Error>>
{
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()
Expand All @@ -117,29 +113,27 @@ fn test_create_with_readme_template_custom_path() -> Result<(), Box<dyn Error>>
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<dyn Error>> {
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(())
}
Loading
Loading