Added better filesystem layout testing harness #15874

ranger-ross · 2025-08-21T15:02:42Z

What does this PR try to resolve?

The goal is to make filesystem layout tests easier to understand and review.
The build-dir test's have a helper fns that have imperative checks. While this works, its not easy to see what its validating at a glance.

My thought is to reuse the snapbox infrastructure we already have for the cargo ui tests.
The prototype in this PR generates a file tree (like the unix tree command) and uses snapbox to compare against a snapshot. This makes the layout tests much easier to read and failures are much more obvious what what went wrong.

Implementation Details

The LayoutTree struct handles comparison logic (as opposed to using snapbox directly)
- This avoids the issues initial implementation (ordering and platform specific files, see full details below)
- If the comparison fails, 2 human readable tree's are generated and snapbox is used to display the diffs.
  - Note: These diffs are in-memory thus, do not support SNAPSHOTS=overwrite. This is arguably good as often the overwrite would be incorrect as the snapshots are platform specific.
Snapshots would support conditional files based on the target os / platform env
- Files/directories may be suffixed with [target_platform=<target-platform>]
- Multiple platforms may be specified separated by comma

initial implementation issues

However there are some problems with the current implementation that limit the usefulness of it.

Different platforms have different files which cause some tests to fail
- Examples
  - lib prefix/suffixes based on platform
  - dSYM on macos
- One idea I have would be to have a cfg suffix after file name in the tree like so
  - └── foo-[HASH].dSYM [cfg(target_os = "macos")]
  - This would also require rethinking the "tree lines" (└, ├, │) to handle optional files.
When dealing with build scripts there are multiple target/<profile>/build/pkg-[HASH] directories. The hash changes the order of the directories when generating the tree making snapshots inconsistent.
- We have redactions to handle replacing the hash with a placeholder [HASH], but this does not help with the order.

Fun fact: These changes were written and tested at Rust Forge 2025 in New Zealand. 🇳🇿 🥳

weihanglo · 2025-08-21T15:07:19Z

Haven't look at the change, but thanks a lot for improving the test infrastructure!

crates/cargo-test-support/src/compare.rs

rustbot · 2025-08-29T09:29:50Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

ranger-ross · 2025-08-29T09:37:23Z

I updated the implementation to fix the flaws of the initial design.
See the PR description for the details

The build-dir tests now pass on all of the platforms tested in CI.
I think the changes are now in a state worthy of review.
Let me know what you think

epage · 2025-08-29T15:05:24Z

crates/cargo-test-support/src/paths.rs

+        let actual_layout = LayoutTree::from_path(&self).unwrap();
+
+        let expected_layout = LayoutTree::parse(expected.as_ref());
+
+        if !actual_layout.matches_snapshot(&expected_layout) {
+            let actual_snapshot = actual_layout.to_string();
+            let expected_snapshot = expected_layout.to_string();
+
+            compare::assert_e2e().eq(actual_snapshot, expected_snapshot);


The LayoutTree struct handles comparison logic (as opposed to using snapbox directly)

This avoids the issues initial implementation (ordering and platform specific files, see full details below)

If the comparison fails, 2 human readable tree's are generated and snapbox is used to display the diffs.

Note: These diffs are in-memory thus, do not support SNAPSHOTS=overwrite. This is arguably good as often the overwrite would be incorrect as the snapshots are platform specific.

Snapshots would support conditional files based on the target os / platform env

Files/directories may be suffixed with [target_platform=<target-platform>]

Multiple platforms may be specified separated by comma

Our general aim should be to support snapshotting wherever possible.

Also, tracking of platform-specific details is rough for contributors

Likely won't discover until the PR is posted

Likely don't have access to a machine, so must do a slow iteration by pushing and seeing CI's result

Unsure f there is something better but felt this was at least worth specifically discussing

Yes I totally agree with you.

One idea I have to only check for files in the snapshot and ignore other files. Meaning if a new file was is created a test would not fail. So tests would be able to ignore many of the platform specific files that are not relevant to the test.
But this comes with the trade off of potentially not including a file that is important.

I think I lean toward the current implementation that explicitly matches the file tree structure

Two alternative ideas:

Have a function that enumerates files in a directory as relative-path-per-line

a snapshot assert can then be run against that with the expected results having a ... inside it and .unordered() called on it

a parameter could take a glob to ignore

Have a low level function that accepts a glob to ignore and enumerates files as a tree. A higher level assert function could assume a specific policy for globs and compare against a snapshot

The problem comes in with

[target_platform=windows-msvc] │ ├── foo[EXE] [target_platform=macos,linux,windows-gnu] │ ├── foo-[HASH][EXE]

The current tests handle this by using globs

The problem with the existing tests doing format!("build-dir/debug/deps/foo*{EXE_SUFFIX}") and if we did foo[..][EXE] is that it over matches. On Unix-like systems, this could match any file and not just the binary.

Maybe we limp along for now with foo[..][EXE] as #15947 would unblock removing -Cextra-filename from more places, see #15947 (comment)

As for why msrv doesn't include the hash, see

cargo/src/cargo/core/compiler/build_runner/compilation_files.rs

Lines 835 to 865 in e0e1a6e

// No metadata in these cases:

//

// - dylibs:

// - if any dylib names are encoded in executables, so they can't be renamed.

// - TODO: Maybe use `-install-name` on macOS or `-soname` on other UNIX systems

// to specify the dylib name to be used by the linker instead of the filename.

// - Windows MSVC executables: The path to the PDB is embedded in the

// executable, and we don't want the PDB path to include the hash in it.

// - wasm32-unknown-emscripten executables: When using emscripten, the path to the

// .wasm file is embedded in the .js file, so we don't want the hash in there.

//

// This is only done for local packages, as we don't expect to export

// dependencies.

//

// The __CARGO_DEFAULT_LIB_METADATA env var is used to override this to

// force metadata in the hash. This is only used for building libstd. For

// example, if libstd is placed in a common location, we don't want a file

// named /usr/lib/libstd.so which could conflict with other rustc

// installs. In addition it prevents accidentally loading a libstd of a

// different compiler at runtime.

// See https://github.com/rust-lang/cargo/issues/3005

let short_name = bcx.target_data.short_name(&unit.kind);

if (unit.target.is_dylib()

|| unit.target.is_cdylib()

|| (unit.target.is_executable() && short_name == "wasm32-unknown-emscripten")

|| (unit.target.is_executable() && short_name.contains("msvc")))

&& unit.pkg.package_id().source_id().is_path()

&& bcx.gctx.get_env("__CARGO_DEFAULT_LIB_METADATA").is_err()

{

return false;

}

tests/testsuite/build_dir.rs

ranger-ross · 2025-09-05T12:21:03Z

I rebased to resolve the conflict with #15910

I'm waiting to see what we decide to do on this PR before writing tests for the build-dir layout changes.

If we feel this is not a meaningful improvement, I am happy to close this and continue with the existing approach in testsuite/build-dir.rs

ranger-ross · 2025-09-20T13:56:22Z

In the latest push I made some changes based on the feedback so far:

Renamed verify_file_layout() to assert_file_layout()
Added an assert_file_layout_with_ignored_paths() fn that accepts a list of paths that will be ignored
assert_file_layout() adds a predefined list of paths (see: default_ignored_paths)
- Currently ignoring platform specific debug symbols on windows and macos that generally get in the way during testing
Used wildcard matches (e.g. foo[..][EXE]) as mentioned in this comment

With these changes the tests are quite a bit cleaner. :)

I left the [target_platform=...] functionality for now as I think it could be potentially useful in the future and is only about 15 lines of code.

epage · 2025-09-20T21:20:32Z

Part of my hope with the rework is we wouldn't need to parse; we generate and then assert. What is left that we still need parsing?

I left the [target_platform=...] functionality for now as I think it could be potentially useful in the future and is only about 15 lines of code.

I'd rather stick to what we need than carry around a burden of unused features that exist for speculative purposes, especially if they over encourage the lack of snapshotting.

epage · 2025-09-20T21:23:58Z

Also, while the tree layout works well for inspecting a test, how well does that work for reading the diff and (in rare cases) updating it by hand?

weihanglo · 2025-09-21T00:39:01Z

src/doc/contrib/src/tests/writing.md

+Tests often to need to verify Cargo created/removed files.
+The `CargoPathExt` trait (implmented by `Path` and `PathBuf`) provides a `assert_file_layout()` to verify a file system tree against a snapshot. The snapshot format is very similar to the unix `tree` command output.
+
+Files vary across operating systems, for example `.pdb` files on Windows. 


I wonder instead this platform specific string we should just have different snapshots for different platforms.

Or maybe we do this kind of tree layout test assertions only on one platform, presumably Linux, which is the most available platforms. For example, I don't have a Windows physical machine. Some don't have macbook. But everyone can access a Linux container :)

Also, some tier 2 and 3 platform maintainers are running Cargo tests in their CI. While they already patched out some tests, we may want to be nice not to fail their CI more.

I wonder instead this platform specific string we should just have different snapshots for different platforms.

I considered that, but it would end up bloating testing and I think it would be quite painful to write tests if you do not have access to all platforms. (and even if you did, it would probably still be tedious)

Yeah that is why another alternative is testing only on Linux. Anything we really want to test platform differences here?

As someone that develops primarily on linux, I am okay with this though not sure about other contributors on Windows.
Though I suppose, its already a problem since they need to think about linux and macos in tests already.

Anything we really want to test platform differences here?

Not really. We are ignoring the most of the platform specific things like debug symbols because they clutter up the tests.
At a high level, the build-dir.rs tests are trying to verify the file layout implementation.
In a perfect world, we would verify all files in the tree, but in practice its difficult to create such a test that is easy to write and maintainable.

ranger-ross · 2025-09-21T04:49:28Z

Part of my hope with the rework is we wouldn't need to parse; we generate and then assert. What is left that we still need parsing?

The issue is the file hashes change the order of files/dirs, resulting tests having diff file orders across platforms.

    ├── foo-[HASH]
    │        ├── dep-test-integration-test-foo
    │        ├── invoked.timestamp
    │        ├── test-integration-test-foo
    │        └── test-integration-test-foo.json
    └── foo-[HASH]
            ├── bin-foo
            ├── bin-foo.json
            ├── dep-bin-foo
            └── invoked.timestamp

Using a tree layout order of lines matters but not the order of the dirs/files.
That is why the LayoutTree is used.

I'd rather stick to what we need than carry around a burden of unused features that exist for speculative purposes, especially if they over encourage the lack of snapshotting.

Sure I can drop this functionality. Its pretty easy to add back if we need it in the future.

Also, while the tree layout works well for inspecting a test, how well does that work for reading the diff and (in rare cases) updating it by hand?

I found the diffs when the test are wrong are pretty good.
The 2 pain points I had were,

[EXE] being removed by mistake in the diff as I was running on linux. Though this is not related to the tree layout.
At times it was a bit awkward to change a ├ to a └ and vise versa. Though this was usually just a matter of copy/pasting from another test.

epage · 2025-09-22T14:27:01Z

The issue is the file hashes change the order of files/dirs, resulting tests having diff file orders across platforms.

At least for myself, this would have me lean towards having a flat list of files and directories. I'm not too much of a fan of the complexity involved in having a bespoke format and then having to parse it. This also loses out on several aspects of our snapshot tooling.

ranger-ross · 2025-09-23T04:32:45Z

At least for myself, this would have me lean towards having a flat list of files and directories. I'm not too much of a fan of the complexity involved in having a bespoke format and then having to parse it. This also loses out on several aspects of our snapshot tooling.

I think this is fair enough.
Sorry, I think I was over complicating this.
I was worried about the ordering of a flat list but learning about .unordered() I think that helps solve the issue.
I updated the implementation to use a flat structure. Seems like this approach resolves many of the small issues with the tree structure and the implementation is significantly simpler.

Let me know what y'all think

ranger-ross · 2025-09-28T16:30:59Z

After some further thought I decided to completely remove assert_build_dir_layout and assert_artifact_dir_layout in favor of the new assert_file_layout. I feel the are largely redundant and file tree snapshots feel like a better approach.

(And removing them means I don't need to update these methods in #15947)

crates/cargo-test-support/src/paths.rs

epage

Thanks!

Update cargo submodule 24 commits in f2932725b045d361ff5f18ba02b1409dd1f44e71..2394ea6cea8b26d717aa67eb1663a2dbf2d26078 2025-09-24 11:31:26 +0000 to 2025-10-03 14:13:01 +0000 - Recommend `package.rust-version` in the Rust version section of `reference/semver.md`. (rust-lang/cargo#15806) - fix(toml): Prevent non-script fields in Cargo scripts (rust-lang/cargo#16026) - chore(ci): unpin libc (rust-lang/cargo#16044) - chore: Update dependencies (rust-lang/cargo#16034) - Fix FileLock path tracking after rename in package operation (rust-lang/cargo#16036) - Lockfile schemas error cleanup (rust-lang/cargo#16039) - Convert a multi-part diagnostic to a report (rust-lang/cargo#16035) - fix(run): Override arg0 for cargo scripts (rust-lang/cargo#16027) - Public in private manifest errors (rust-lang/cargo#16002) - chore(deps): update actions/checkout action to v5 (rust-lang/cargo#16031) - fix: remove FIXME comment that's no longer a problem (rust-lang/cargo#16025) - Add retry for `git fetch` failures in `CARGO_NET_GIT_FETCH_WITH_CLI` path (rust-lang/cargo#16016) - Added better filesystem layout testing harness (rust-lang/cargo#15874) - Small cleanup to normalize_dependencies (rust-lang/cargo#16022) - fix: better error message for rust version incompatibility (rust-lang/cargo#16021) - fix(shell): Use a distinct style for transient status (rust-lang/cargo#16019) - chore(deps): Depend on `serde_core` in `cargo-platform` (rust-lang/cargo#15992) - Remove package-workspace from unstable doc index (rust-lang/cargo#16014) - fix(shell): Switch to annotate snippets for notes (rust-lang/cargo#15945) - docs: update changelog (rust-lang/cargo#15986) - chore(ci): add rustfmt for docs job (rust-lang/cargo#16013) - chore: bump to 0.93.0 (rust-lang/cargo#16009) - fix(config): combine key error context into one (rust-lang/cargo#16004) - test(docker): openssh requires a newer libcrypto3 (rust-lang/cargo#16010) r? ghost

rustbot added the A-testing-cargo-itself Area: cargo's tests label Aug 21, 2025

epage reviewed Aug 21, 2025

View reviewed changes

crates/cargo-test-support/src/compare.rs Show resolved Hide resolved

ranger-ross force-pushed the better-layout-testing branch from 4f31868 to c939a79 Compare August 29, 2025 08:48

rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Aug 29, 2025

ranger-ross force-pushed the better-layout-testing branch 2 times, most recently from 9a42834 to edef1aa Compare August 29, 2025 09:14

ranger-ross marked this pull request as ready for review August 29, 2025 09:29

rustbot assigned weihanglo Aug 29, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2025

epage reviewed Aug 29, 2025

View reviewed changes

tests/testsuite/build_dir.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ranger-ross force-pushed the better-layout-testing branch from edef1aa to b9f9cbb Compare September 5, 2025 11:54

ranger-ross mentioned this pull request Sep 10, 2025

feat(build-dir): Reorganize build-dir layout #15947

Open

1 task

This comment has been minimized.

Sign in to view

ranger-ross force-pushed the better-layout-testing branch from b9f9cbb to 122ee29 Compare September 20, 2025 13:42

weihanglo reviewed Sep 21, 2025

View reviewed changes

ranger-ross force-pushed the better-layout-testing branch from 122ee29 to df753db Compare September 23, 2025 04:26

ranger-ross force-pushed the better-layout-testing branch from df753db to 9a68a7d Compare September 28, 2025 16:23

epage reviewed Sep 29, 2025

View reviewed changes

crates/cargo-test-support/src/paths.rs Show resolved Hide resolved

epage reviewed Sep 29, 2025

View reviewed changes

crates/cargo-test-support/src/paths.rs Outdated Show resolved Hide resolved

epage reviewed Sep 29, 2025

View reviewed changes

crates/cargo-test-support/src/paths.rs Outdated Show resolved Hide resolved

ranger-ross force-pushed the better-layout-testing branch from 9a68a7d to a0a0ab4 Compare September 29, 2025 16:38

epage reviewed Sep 29, 2025

View reviewed changes

crates/cargo-test-support/src/paths.rs Show resolved Hide resolved

ranger-ross added 3 commits September 30, 2025 02:02

feat: Added assert_file_layout() to CargoPathExt

ef1132f

test(build-dir): Migrated build dir tests to snapshot testing

b070db9

docs: Added assert_dir_layout() to testing docs

1e93a2d

ranger-ross force-pushed the better-layout-testing branch from a0a0ab4 to 1e93a2d Compare September 29, 2025 17:03

epage approved these changes Sep 29, 2025

View reviewed changes

epage enabled auto-merge September 29, 2025 17:12

epage added this pull request to the merge queue Sep 29, 2025

Merged via the queue into rust-lang:master with commit ad14280 Sep 29, 2025
27 checks passed

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2025

ranger-ross deleted the better-layout-testing branch September 30, 2025 00:54

weihanglo mentioned this pull request Oct 3, 2025

Update cargo submodule rust-lang/rust#147301

Merged

rustbot added this to the 1.92.0 milestone Oct 3, 2025

ranger-ross mentioned this pull request Oct 6, 2025

Rework Cargo Build Dir Layout rust-lang/rust-project-goals#401

Open

2 tasks

	// No metadata in these cases:
	//
	// - dylibs:
	// - if any dylib names are encoded in executables, so they can't be renamed.
	// - TODO: Maybe use `-install-name` on macOS or `-soname` on other UNIX systems
	// to specify the dylib name to be used by the linker instead of the filename.
	// - Windows MSVC executables: The path to the PDB is embedded in the
	// executable, and we don't want the PDB path to include the hash in it.
	// - wasm32-unknown-emscripten executables: When using emscripten, the path to the
	// .wasm file is embedded in the .js file, so we don't want the hash in there.
	//
	// This is only done for local packages, as we don't expect to export
	// dependencies.
	//
	// The __CARGO_DEFAULT_LIB_METADATA env var is used to override this to
	// force metadata in the hash. This is only used for building libstd. For
	// example, if libstd is placed in a common location, we don't want a file
	// named /usr/lib/libstd.so which could conflict with other rustc
	// installs. In addition it prevents accidentally loading a libstd of a
	// different compiler at runtime.
	// See https://github.com/rust-lang/cargo/issues/3005
	let short_name = bcx.target_data.short_name(&unit.kind);
	if (unit.target.is_dylib()
	\|\| unit.target.is_cdylib()
	\|\| (unit.target.is_executable() && short_name == "wasm32-unknown-emscripten")
	\|\| (unit.target.is_executable() && short_name.contains("msvc")))
	&& unit.pkg.package_id().source_id().is_path()
	&& bcx.gctx.get_env("__CARGO_DEFAULT_LIB_METADATA").is_err()
	{
	return false;
	}

Added better filesystem layout testing harness #15874

Added better filesystem layout testing harness #15874

Uh oh!

Conversation

ranger-ross commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

Implementation Details

Uh oh!

weihanglo commented Aug 21, 2025

Uh oh!

Uh oh!

rustbot commented Aug 29, 2025

Uh oh!

ranger-ross commented Aug 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

ranger-ross commented Sep 5, 2025

Uh oh!

This comment has been minimized.

ranger-ross commented Sep 20, 2025

Uh oh!

epage commented Sep 20, 2025

Uh oh!

epage commented Sep 20, 2025

Uh oh!

weihanglo Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranger-ross commented Sep 21, 2025

Uh oh!

epage commented Sep 22, 2025

Uh oh!

ranger-ross commented Sep 23, 2025

Uh oh!

ranger-ross commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

epage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ranger-ross commented Aug 21, 2025 •

edited

Loading

weihanglo Sep 21, 2025 •

edited

Loading

ranger-ross commented Sep 28, 2025 •

edited

Loading