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

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 added 3 commits September 5, 2025 20:29

feat: Added verify_file_layout() to CargoPathExt

1b4a697

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

ea1db98

docs: Added verify_file_layout() to testing docs

b9f9cbb

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Added better filesystem layout testing harness #15874

Added better filesystem layout testing harness #15874

Uh oh!

ranger-ross commented Aug 21, 2025 •

edited

Loading

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!

epage Aug 29, 2025

Uh oh!

ranger-ross Aug 29, 2025

Uh oh!

epage Sep 17, 2025

Uh oh!

epage Sep 17, 2025

Uh oh!

epage Sep 17, 2025

Uh oh!

epage Sep 17, 2025

Uh oh!

epage Aug 29, 2025

Uh oh!

ranger-ross Aug 29, 2025

Uh oh!

This comment has been minimized.

ranger-ross commented Sep 5, 2025

Uh oh!

Uh oh!

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

Uh oh!

ranger-ross commented Aug 21, 2025 •

edited

Loading