From 5b03d0711ad6a2af1199fd28488a5c3ed813b130 Mon Sep 17 00:00:00 2001 From: Jieyou Xu Date: Sat, 2 Aug 2025 15:55:56 +0800 Subject: [PATCH 1/6] Pull out unexpected extension check into own function --- src/tools/tidy/src/ui_tests.rs | 88 ++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 42 deletions(-) diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 4d195b3952e27..91b89d4114972 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -7,41 +7,6 @@ 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#"============================================================ ⚠️⚠️⚠️NOTHING SHOULD EVER BE ADDED TO THIS LIST⚠️⚠️⚠️ @@ -82,13 +47,7 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { 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); - } + 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. @@ -171,3 +130,48 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { } } } + +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); + } +} From c10dc999f004aa04d29652f6fa9dc9535cb10899 Mon Sep 17 00:00:00 2001 From: Jieyou Xu Date: Sat, 2 Aug 2025 15:58:53 +0800 Subject: [PATCH 2/6] Pull out stray/empty output snapshot checks into own functions --- src/tools/tidy/src/ui_tests.rs | 51 +++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 91b89d4114972..ee26e81c97f64 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -54,28 +54,8 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { 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); - } + check_stray_output_snapshot(bad, file_path, testname); + check_empty_output_snapshot(bad, file_path); } if ext == "rs" @@ -175,3 +155,30 @@ fn check_unexpected_extension(bad: &mut bool, file_path: &Path, ext: &str) { 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); + } +} From a71428825a3322d2662efdc4299f9cfac3e3f5e5 Mon Sep 17 00:00:00 2001 From: Jieyou Xu Date: Sat, 2 Aug 2025 16:09:10 +0800 Subject: [PATCH 3/6] Pull out non-descriptive test name check to own function --- src/tools/tidy/src/ui_tests.rs | 60 ++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 21 deletions(-) diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index ee26e81c97f64..98a6b466ae914 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -58,27 +58,14 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { check_empty_output_snapshot(bad, file_path); } - 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], - ); - } - } + deny_new_nondescriptive_test_names( + bad, + path, + &mut remaining_issue_names, + file_path, + testname, + ext, + ); } }); @@ -182,3 +169,34 @@ fn check_empty_output_snapshot(bad: &mut bool, file_path: &Path) { 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], + ); + } + } +} From a97d0aabc8bbaeff1aff88df67bc99e8c778ba06 Mon Sep 17 00:00:00 2001 From: Jieyou Xu Date: Sat, 2 Aug 2025 16:12:56 +0800 Subject: [PATCH 4/6] Make `issues_txt_header` a const --- src/tools/tidy/src/ui_tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 98a6b466ae914..6c83764cc1943 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -7,12 +7,12 @@ use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; -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` @@ -20,7 +20,7 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { 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| { @@ -78,7 +78,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(); From fa31c7d49e7456b57bc32de118e72e0d7045ef6e Mon Sep 17 00:00:00 2001 From: Jieyou Xu Date: Sat, 2 Aug 2025 16:16:41 +0800 Subject: [PATCH 5/6] Pull out recursive ui test check into its own function --- src/tools/tidy/src/ui_tests.rs | 65 +++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 6c83764cc1943..b968ea5f2d89a 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -40,34 +40,7 @@ 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) { - 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, - ); - } - }); + 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 @@ -98,6 +71,42 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { } } +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 From 0b1547e9c052b29d246a8e5cb3d6408407e88dab Mon Sep 17 00:00:00 2001 From: Jieyou Xu Date: Sat, 2 Aug 2025 16:54:26 +0800 Subject: [PATCH 6/6] Reject adding new UI tests directly under `tests/ui/` As we want future UI tests to be added under a more meaningful subdirectory instead. --- src/tools/tidy/src/ui_tests.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index b968ea5f2d89a..5bf966b658c63 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -40,6 +40,8 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { ); } + deny_new_top_level_ui_tests(bad, &path.join("ui")); + 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. @@ -71,6 +73,34 @@ 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 where we propose banning adding + // new ui tests *directly* under `tests/ui/`. For more context, see: + // + // - + // - + + 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,