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;

}

epage · 2025-08-29T15:06:31Z

tests/testsuite/build_dir.rs

+    ├── examples
+    │   ├── foo.d                               [target_platform=windows-msvc]
+    │   ├── foo.pdb                             [target_platform=windows-msvc]
+    │   ├── foo[EXE]                            [target_platform=windows-msvc]
+    │   ├── foo-[HASH].d                        [target_platform=macos,linux,windows-gnu]


I feel like some of the benefit of tree output is lost with the conditionals breaking up the output

Yeah, it was not very bad until I realized windows-msvc does not include hashes in many places that linux, macos, and winddows-gnu does. I felt this was still better than duplicating the tree with #[cfg(target_os = "linux")].

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)

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.

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

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

Draft

This comment has been minimized.

Sign in to view

ranger-ross added 3 commits September 20, 2025 22:06

feat: Added assert_file_layout() to CargoPathExt

dbbfd89

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

805d407

docs: Added assert_file_layout() to testing docs

122ee29

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

	// 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

Are you sure you want to change the base?

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

ranger-ross commented Sep 21, 2025

Uh oh!

Uh oh!

ranger-ross commented Aug 21, 2025 •

edited

Loading

weihanglo Sep 21, 2025 •

edited

Loading