Skip to content

Conversation

@BD103
Copy link
Member

@BD103 BD103 commented Nov 11, 2025

Sorry for the vague title! This PR is a refactor of our linter UI test runners (ui.rs and ui_cargo.rs) with a number of benefits:

  • Progress towards running UI tests on Windows by building bevy and its dependencies with DependencyBuilder (fixes Fix CI on Windows #568 (comment), which prevented Fix CI on Windows #568 from being merged)
  • New @exit-code: CODE annotation for Cargo UI tests, used to ensure checks pass
  • Support passing arguments to Cargo UI tests (e.g. cargo test --test ui_cargo -- --bless)

Here's a few notes I have on specific changes / design decisions:

Removing test_utils

The test_utils module had two main features:

  • Providing a base Config that both ui.rs and ui_cargo.rs could use, avoiding unnecessary duplication
  • Building and locating the compiled bevy crate so UI tests could import it

When looking over the base_config(), I realized ui.rs used all of the configuration without modification, while ui_cargo.rs essentially overwrote the entire thing. Because of that, I think it is much clearer to define each Config separately, rather than trying to use a shared base when there is little overlap. I think it worked! Compare the before-and-after for ui_cargo.rs:

let mut config = base_config("ui-cargo").unwrap();
let defaults = config.comment_defaults.base();
// The driver returns a '101' on error.
// This allows for any status code to be considered a success.
defaults.exit_status = None.into();
defaults.require_annotations = None.into();
// This sets the '--manifest-path' flag
config.program.input_file_flag = CommandBuilder::cargo().input_file_flag;
config.program.out_dir_flag = CommandBuilder::cargo().out_dir_flag;
// Do not print cargo log messages
config.program.args = vec!["--quiet".into(), "--color".into(), "never".into()];
let current_exe_path = env::current_exe().unwrap();
let deps_path = current_exe_path.parent().unwrap();
let profile_path = deps_path.parent().unwrap();
// Specify the binary to use when executing tests with this `Config`
config.program.program = profile_path.join(if cfg!(windows) {
"bevy_lint_driver.exe"
} else {
"bevy_lint_driver"
});
config.program.program.set_file_name(if cfg!(windows) {
"bevy_lint.exe"
} else {
"bevy_lint"
});
// this clears the default `--edition` flag
config.comment_defaults.base().custom.clear();

let mut config = Config {
// We need to specify the host tuple manually, because if we don't then `ui_test` will try
// running `bevy_lint -vV` to discover the host and promptly error because `bevy_lint`
// doesn't recognize the `-vV` flag.
host: Some(host_tuple()),
program: CommandBuilder {
program: linter_path.into(),
args: vec!["--color=never".into(), "--quiet".into()],
out_dir_flag: Some("--target-dir".into()),
input_file_flag: Some("--manifest-path".into()),
envs: Vec::new(),
cfg_flag: None,
},
out_dir: PathBuf::from("../target/ui"),
..Config::cargo(Path::new("tests/ui-cargo"))
};
// We haven't found a way to get error annotations like `#~v ERROR: msg` to work, so we disable
// the requirement for them.
config.comment_defaults.base().require_annotations = None.into();

The second feature test_utils provided was building and locating libbevy.rlib. Turns out we can replace that with...

Using DependencyBuilder

ui_test provides a DependencyBuilder that makes it easy to build dependencies used by UI tests. DependencyBuilder provides a shift in how we manage dependencies:

test_utils DependencyBuilder
Dependencies specified in linter's [dev-dependencies] Dependencies specified by lint-ui-dependencies crate
Dependencies built by default when cargo test runs Dependencies built only when UI tests run
Dependency .rlibs located by parsing cargo build's JSON output (which was jank enough that I wrote a blog post about an issue I found with it, whose fix never landed because it was in #568) Dependency .rlibs handled by ui_test
Passing dependencies to bevy_lint_driver with --extern and -L Dependency flags handled by ui_test
Does not respect cargo::rustc-link-search in build scripts, causing windows dependency to fail to compile on Windows Handles cargo::rustc-link-search: oli-obk/ui_test#220
Bevy's derive macros successfully detect that bevy::ecs should be imported rather than bevy_ecs Bevy's derive macros cannot detect that bevy::ecs should be imported

Overall, using DependencyBuilder makes it so we can build and run UI tests on Windows while also making maintaining the UI tests easier. The only downside I've found was that BevyManifest isn't able to find these dependencies, so it isn't able to deduce that bevy::ecs and bevy::reflect should be imported rather than bevy_ecs and bevy_reflect. I hacked around it here, but the solution isn't amazing.

Properly query host tuple

One of the oldest issues (it's there in #32!) I ran into when introducing the UI test harness was ui_test trying to auto-detect the host tuple by calling bevy_lint_driver -vV, which the linter driver skipped because it expected bevy_lint_driver rustc -vV instead. I hacked around the issue by setting the host to an empty string Some(String::new()), but DependencyBuilder doesn't like that because it tries passing the host to cargo build and ends up running cargo build --target=. Because there's no value for --target=, Cargo exits with an error.

Because there was no avoiding it anymore, I chose to instead run rustc --print=host-tuple to get the actual host tuple (which for me returns aarch64-apple-darwin). I duplicated this solution to both ui.rs and ui_cargo.rs rather than re-introducing test_utils, but I'm not set on that decision.

#@exit-status in Cargo UI tests

We previously discarded the exit code of Cargo UI tests because fail tests returned exit code 101, which ui_test mistakenly thought was a compiler ICE rather than a proper error. Instead of blanketly allowing all exit codes, I wrote a small annotation that lets us specify what the exit code of a specific test should be. Now all tests intended to fail should have this at the beginning:

#@exit-status: 101

[package]
# ...

And all tests intending to pass should use the normal #@check-pass:

#@check-pass

[package]
# ...

I couldn't figure out a way to reprogram ui_test to accept 101 as the normal error code, so this is the best thing I came up with.

Argument parsing in ui_cargo.rs

ui_cargo.rs uses ui_test::run_tests_generic() because it needs to configure the runner to work on Cargo.toml files, not *.rs files. run_tests_generic() does not do any argument parsing, however, meaning flags like --bless were completely ignored.

Since --bless is super useful, I copied the argument parsing code from run_tests() to ui_cargo.rs. Cool stuff!

BD103 added 10 commits November 11, 2025 12:50
`ui_test` isn't allowed to find the host tuple itself, since it tries running `bevy_lint_driver -vV` and gets very confused when `bevy_lint_driver` silently eats the `-vV` flag because it expected `rustc`'s path instead.

Because of that, we need to specify the host tuple with some default. Before, leaving it as an empty string worked fine, but in a future commit I'm going to introduce `DependencyBuilder`, which needs a valid host tuple. As such, the best solution is to properly query `rustc` for the host tuple and pass it to `ui_test`.
This makes `ui_test` build our dependencies in a separate crate, rather than expecting `bevy` to be built as a dev-dependency. This should hopefully fix #568 (comment), letting us run UI tests on Windows.
I ended up duplicating `host_tuple()`, but I personally am fine with it.
This lets us verify a specific exit code is set by `bevy_lint`, rather than just ignoring it completely.
Since `run_tests_generic()` doesn't parse arguments, I took the contents of `run_tests()` and inlined them here.
@BD103 BD103 added A-Linter Related to the linter and custom lints C-Testing A change to the tests D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review The PR needs to be reviewed before it can be merged labels Nov 11, 2025
Comment on lines -63 to -64
# Used to deserialize `--message-format=json` messages from Cargo.
serde_json = "1.0.145"
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need serde_json anymore, as we're no longer deserializing JSON messages manually. ui_test handles that for us!

Comment on lines 56 to 61
# Used when running UI tests.
bevy = { version = "0.17.2", default-features = false, features = [
"std",
# used for the `camera_modification_in_fixed_update` lint
"bevy_render",
] }
Copy link
Member Author

Choose a reason for hiding this comment

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

This dependency has been moved to bevy_lint/tests/dependencies/Cargo.toml.

name = "lint-ui-dependencies"
description = "A dummy crate that builds dependencies that `bevy_lint`'s UI tests need to import"
edition = "2024"
publish = false
Copy link
Member Author

Choose a reason for hiding this comment

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

This line is important: once we start cargo publish-ing the CLI and linter, this will prevent us from accidentally publishing this crate :)

Comment on lines +1 to +4
#![expect(
clippy::disallowed_macros,
reason = "`bevy_lint`'s macros are intended for lints, not tests"
)]
Copy link
Member Author

Choose a reason for hiding this comment

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

This #[expect(...)] is because we are using the standard library's assert!() macro. It's fine in this situation because we can't and shouldn't use the linter's version.

Comment on lines +6 to +7
# A dummy crate used to build dependencies that the linter needs for its UI tests.
"bevy_lint/tests/dependencies",
Copy link
Member Author

Choose a reason for hiding this comment

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

I figured it was easiest to add ui-lint-dependencies to the workspace; this way it shares the target directory for caching and Cargo.lock for dependency sharing.

On Windows if the first `cargo test` fails, it will not be caught as long as the second `cargo test` passes. It's super jank.
@BD103 BD103 changed the title Improve linter UI test runners Improve linter UI test runners (and fix them on Windows!) Nov 12, 2025
@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review The PR needs to be reviewed before it can be merged labels Nov 16, 2025
@BD103 BD103 marked this pull request as draft November 22, 2025 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Linter Related to the linter and custom lints C-Testing A change to the tests D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants