Skip to content

Add a tidy check to prevent adding UI tests directly under tests/ui/ #144813

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
merged 6 commits into from
Aug 5, 2025
+173 −105
Merged

Add a tidy check to prevent adding UI tests directly under tests/ui/ #144813

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5b03d07
Pull out unexpected extension check into own function
jieyouxu Aug 2, 2025
c10dc99
Pull out stray/empty output snapshot checks into own functions
jieyouxu Aug 2, 2025
a714288
Pull out non-descriptive test name check to own function
jieyouxu Aug 2, 2025
a97d0aa
Make `issues_txt_header` a const
jieyouxu Aug 2, 2025
fa31c7d
Pull out recursive ui test check into its own function
jieyouxu Aug 2, 2025
0b1547e
Reject adding new UI tests directly under `tests/ui/`
jieyouxu Aug 2, 2025
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
278 changes: 173 additions & 105 deletions src/tools/tidy/src/ui_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,55 +7,20 @@ use std::fs;
use std::io::Write;
use std::path::{Path, PathBuf};

const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
"rs", // test source files
"stderr", // expected stderr file, corresponds to a rs file
"svg", // expected svg file, corresponds to a rs file, equivalent to stderr
"stdout", // expected stdout file, corresponds to a rs file
"fixed", // expected source file after applying fixes
"md", // test directory descriptions
"ftl", // translation tests
];

const EXTENSION_EXCEPTION_PATHS: &[&str] = &[
"tests/ui/asm/named-asm-labels.s", // loading an external asm file to test named labels lint
"tests/ui/codegen/mismatched-data-layout.json", // testing mismatched data layout w/ custom targets
"tests/ui/check-cfg/my-awesome-platform.json", // testing custom targets with cfgs
"tests/ui/argfile/commandline-argfile-badutf8.args", // passing args via a file
"tests/ui/argfile/commandline-argfile.args", // passing args via a file
"tests/ui/crate-loading/auxiliary/libfoo.rlib", // testing loading a manually created rlib
"tests/ui/include-macros/data.bin", // testing including data with the include macros
"tests/ui/include-macros/file.txt", // testing including data with the include macros
"tests/ui/macros/macro-expanded-include/file.txt", // testing including data with the include macros
"tests/ui/macros/not-utf8.bin", // testing including data with the include macros
"tests/ui/macros/syntax-extension-source-utils-files/includeme.fragment", // more include
"tests/ui/proc-macro/auxiliary/included-file.txt", // more include
"tests/ui/unpretty/auxiliary/data.txt", // more include
"tests/ui/invalid/foo.natvis.xml", // sample debugger visualizer
"tests/ui/sanitizer/dataflow-abilist.txt", // dataflow sanitizer ABI list file
"tests/ui/shell-argfiles/shell-argfiles.args", // passing args via a file
"tests/ui/shell-argfiles/shell-argfiles-badquotes.args", // passing args via a file
"tests/ui/shell-argfiles/shell-argfiles-via-argfile-shell.args", // passing args via a file
"tests/ui/shell-argfiles/shell-argfiles-via-argfile.args", // passing args via a file
"tests/ui/std/windows-bat-args1.bat", // tests escaping arguments through batch files
"tests/ui/std/windows-bat-args2.bat", // tests escaping arguments through batch files
"tests/ui/std/windows-bat-args3.bat", // tests escaping arguments through batch files
];

pub fn check(root_path: &Path, bless: bool, bad: &mut bool) {
let issues_txt_header = r#"============================================================
const ISSUES_TXT_HEADER: &str = r#"============================================================
⚠️⚠️⚠️NOTHING SHOULD EVER BE ADDED TO THIS LIST⚠️⚠️⚠️
============================================================
"#;

pub fn check(root_path: &Path, bless: bool, bad: &mut bool) {
let path = &root_path.join("tests");

// the list of files in ui tests that are allowed to start with `issue-XXXX`
// BTreeSet because we would like a stable ordering so --bless works
let mut prev_line = "";
let mut is_sorted = true;
let allowed_issue_names: BTreeSet<_> = include_str!("issues.txt")
.strip_prefix(issues_txt_header)
.strip_prefix(ISSUES_TXT_HEADER)
.unwrap()
.lines()
.inspect(|&line| {
Expand All @@ -75,73 +40,9 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) {
);
}

let mut remaining_issue_names: BTreeSet<&str> = allowed_issue_names.clone();

let (ui, ui_fulldeps) = (path.join("ui"), path.join("ui-fulldeps"));
let paths = [ui.as_path(), ui_fulldeps.as_path()];
crate::walk::walk_no_read(&paths, |_, _| false, &mut |entry| {
let file_path = entry.path();
if let Some(ext) = file_path.extension().and_then(OsStr::to_str) {
// files that are neither an expected extension or an exception should not exist
// they're probably typos or not meant to exist
if !(EXPECTED_TEST_FILE_EXTENSIONS.contains(&ext)
|| EXTENSION_EXCEPTION_PATHS.iter().any(|path| file_path.ends_with(path)))
{
tidy_error!(bad, "file {} has unexpected extension {}", file_path.display(), ext);
}

// NB: We do not use file_stem() as some file names have multiple `.`s and we
// must strip all of them.
let testname =
file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0;
if ext == "stderr" || ext == "stdout" || ext == "fixed" {
// Test output filenames have one of the formats:
// ```
// $testname.stderr
// $testname.$mode.stderr
// $testname.$revision.stderr
// $testname.$revision.$mode.stderr
// ```
//
// For now, just make sure that there is a corresponding
// `$testname.rs` file.

if !file_path.with_file_name(testname).with_extension("rs").exists()
&& !testname.contains("ignore-tidy")
{
tidy_error!(bad, "Stray file with UI testing output: {:?}", file_path);
}

if let Ok(metadata) = fs::metadata(file_path)
&& metadata.len() == 0
{
tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path);
}
}
deny_new_top_level_ui_tests(bad, &path.join("ui"));

if ext == "rs"
&& let Some(test_name) = static_regex!(r"^issues?[-_]?(\d{3,})").captures(testname)
{
// these paths are always relative to the passed `path` and always UTF8
let stripped_path = file_path
.strip_prefix(path)
.unwrap()
.to_str()
.unwrap()
.replace(std::path::MAIN_SEPARATOR_STR, "/");

if !remaining_issue_names.remove(stripped_path.as_str())
&& !stripped_path.starts_with("ui/issues/")
{
tidy_error!(
bad,
"file `tests/{stripped_path}` must begin with a descriptive name, consider `{{reason}}-issue-{issue_n}.rs`",
issue_n = &test_name[1],
);
}
}
}
});
let remaining_issue_names = recursively_check_ui_tests(bad, path, &allowed_issue_names);

// if there are any file names remaining, they were moved on the fs.
// our data must remain up to date, so it must be removed from issues.txt
Expand All @@ -152,7 +53,7 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) {
// so we don't bork things on panic or a contributor using Ctrl+C
let blessed_issues_path = tidy_src.join("issues_blessed.txt");
let mut blessed_issues_txt = fs::File::create(&blessed_issues_path).unwrap();
blessed_issues_txt.write_all(issues_txt_header.as_bytes()).unwrap();
blessed_issues_txt.write_all(ISSUES_TXT_HEADER.as_bytes()).unwrap();
// If we changed paths to use the OS separator, reassert Unix chauvinism for blessing.
for filename in allowed_issue_names.difference(&remaining_issue_names) {
writeln!(blessed_issues_txt, "{filename}").unwrap();
Expand All @@ -171,3 +72,170 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) {
}
}
}

fn deny_new_top_level_ui_tests(bad: &mut bool, tests_path: &Path) {
// See <https://github.com/rust-lang/compiler-team/issues/902> where we propose banning adding
// new ui tests *directly* under `tests/ui/`. For more context, see:
//
// - <https://github.com/rust-lang/rust/issues/73494>
// - <https://github.com/rust-lang/rust/issues/133895>

let top_level_ui_tests = walkdir::WalkDir::new(tests_path)
.min_depth(1)
.max_depth(1)
.follow_links(false)
.same_file_system(true)
.into_iter()
.flatten()
.filter(|e| {
let file_name = e.file_name();
file_name != ".gitattributes" && file_name != "README.md"
})
.filter(|e| !e.file_type().is_dir());
for entry in top_level_ui_tests {
tidy_error!(
bad,
"ui tests should be added under meaningful subdirectories: `{}`",
entry.path().display()
)
}
}

fn recursively_check_ui_tests<'issues>(
bad: &mut bool,
path: &Path,
allowed_issue_names: &'issues BTreeSet<&'issues str>,
) -> BTreeSet<&'issues str> {
let mut remaining_issue_names: BTreeSet<&str> = allowed_issue_names.clone();

let (ui, ui_fulldeps) = (path.join("ui"), path.join("ui-fulldeps"));
let paths = [ui.as_path(), ui_fulldeps.as_path()];
crate::walk::walk_no_read(&paths, |_, _| false, &mut |entry| {
let file_path = entry.path();
if let Some(ext) = file_path.extension().and_then(OsStr::to_str) {
check_unexpected_extension(bad, file_path, ext);

// NB: We do not use file_stem() as some file names have multiple `.`s and we
// must strip all of them.
let testname =
file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0;
if ext == "stderr" || ext == "stdout" || ext == "fixed" {
check_stray_output_snapshot(bad, file_path, testname);
check_empty_output_snapshot(bad, file_path);
}

deny_new_nondescriptive_test_names(
bad,
path,
&mut remaining_issue_names,
file_path,
testname,
ext,
);
}
});
remaining_issue_names
}

fn check_unexpected_extension(bad: &mut bool, file_path: &Path, ext: &str) {
const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
"rs", // test source files
"stderr", // expected stderr file, corresponds to a rs file
"svg", // expected svg file, corresponds to a rs file, equivalent to stderr
"stdout", // expected stdout file, corresponds to a rs file
"fixed", // expected source file after applying fixes
"md", // test directory descriptions
"ftl", // translation tests
];

const EXTENSION_EXCEPTION_PATHS: &[&str] = &[
"tests/ui/asm/named-asm-labels.s", // loading an external asm file to test named labels lint
"tests/ui/codegen/mismatched-data-layout.json", // testing mismatched data layout w/ custom targets
"tests/ui/check-cfg/my-awesome-platform.json", // testing custom targets with cfgs
"tests/ui/argfile/commandline-argfile-badutf8.args", // passing args via a file
"tests/ui/argfile/commandline-argfile.args", // passing args via a file
"tests/ui/crate-loading/auxiliary/libfoo.rlib", // testing loading a manually created rlib
"tests/ui/include-macros/data.bin", // testing including data with the include macros
"tests/ui/include-macros/file.txt", // testing including data with the include macros
"tests/ui/macros/macro-expanded-include/file.txt", // testing including data with the include macros
"tests/ui/macros/not-utf8.bin", // testing including data with the include macros
"tests/ui/macros/syntax-extension-source-utils-files/includeme.fragment", // more include
"tests/ui/proc-macro/auxiliary/included-file.txt", // more include
"tests/ui/unpretty/auxiliary/data.txt", // more include
"tests/ui/invalid/foo.natvis.xml", // sample debugger visualizer
"tests/ui/sanitizer/dataflow-abilist.txt", // dataflow sanitizer ABI list file
"tests/ui/shell-argfiles/shell-argfiles.args", // passing args via a file
"tests/ui/shell-argfiles/shell-argfiles-badquotes.args", // passing args via a file
"tests/ui/shell-argfiles/shell-argfiles-via-argfile-shell.args", // passing args via a file
"tests/ui/shell-argfiles/shell-argfiles-via-argfile.args", // passing args via a file
"tests/ui/std/windows-bat-args1.bat", // tests escaping arguments through batch files
"tests/ui/std/windows-bat-args2.bat", // tests escaping arguments through batch files
"tests/ui/std/windows-bat-args3.bat", // tests escaping arguments through batch files
];

// files that are neither an expected extension or an exception should not exist
// they're probably typos or not meant to exist
if !(EXPECTED_TEST_FILE_EXTENSIONS.contains(&ext)
|| EXTENSION_EXCEPTION_PATHS.iter().any(|path| file_path.ends_with(path)))
{
tidy_error!(bad, "file {} has unexpected extension {}", file_path.display(), ext);
}
}

fn check_stray_output_snapshot(bad: &mut bool, file_path: &Path, testname: &str) {
// Test output filenames have one of the formats:
// ```
// $testname.stderr
// $testname.$mode.stderr
// $testname.$revision.stderr
// $testname.$revision.$mode.stderr
// ```
//
// For now, just make sure that there is a corresponding
// `$testname.rs` file.

if !file_path.with_file_name(testname).with_extension("rs").exists()
&& !testname.contains("ignore-tidy")
{
tidy_error!(bad, "Stray file with UI testing output: {:?}", file_path);
}
}

fn check_empty_output_snapshot(bad: &mut bool, file_path: &Path) {
if let Ok(metadata) = fs::metadata(file_path)
&& metadata.len() == 0
{
tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path);
}
}

fn deny_new_nondescriptive_test_names(
bad: &mut bool,
path: &Path,
remaining_issue_names: &mut BTreeSet<&str>,
file_path: &Path,
testname: &str,
ext: &str,
) {
if ext == "rs"
&& let Some(test_name) = static_regex!(r"^issues?[-_]?(\d{3,})").captures(testname)
{
// these paths are always relative to the passed `path` and always UTF8
let stripped_path = file_path
.strip_prefix(path)
.unwrap()
.to_str()
.unwrap()
.replace(std::path::MAIN_SEPARATOR_STR, "/");

if !remaining_issue_names.remove(stripped_path.as_str())
&& !stripped_path.starts_with("ui/issues/")
{
tidy_error!(
bad,
"file `tests/{stripped_path}` must begin with a descriptive name, consider `{{reason}}-issue-{issue_n}.rs`",
issue_n = &test_name[1],
);
}
}
}
Loading