Skip to content

Commit ebd865e

Browse files
samueltardieusamueltardieu
authored
Rollup merge of #144813 - jieyouxu:no-top-level-tests, r=Kobzol
Add a tidy check to prevent adding UI tests directly under `tests/ui/` This PR implements rust-lang/compiler-team#902. Only the last commit (adding the new check) is functional; earlier commits are just small drive-by changes to make the other ui/ui-fulldeps checks more logically contained. r? ```@Kobzol``` (or compiler)
2 parents 3ec8b67 + 0b1547e commit ebd865e

File tree

1 file changed

+173
-105
lines changed

1 file changed

+173
-105
lines changed

‎src/tools/tidy/src/ui_tests.rs

Lines changed: 173 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -7,55 +7,20 @@ use std::fs;
77
use std::io::Write;
88
use std::path::{Path, PathBuf};
99

10-
const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
11-
"rs", // test source files
12-
"stderr", // expected stderr file, corresponds to a rs file
13-
"svg", // expected svg file, corresponds to a rs file, equivalent to stderr
14-
"stdout", // expected stdout file, corresponds to a rs file
15-
"fixed", // expected source file after applying fixes
16-
"md", // test directory descriptions
17-
"ftl", // translation tests
18-
];
19-
20-
const EXTENSION_EXCEPTION_PATHS: &[&str] = &[
21-
"tests/ui/asm/named-asm-labels.s", // loading an external asm file to test named labels lint
22-
"tests/ui/codegen/mismatched-data-layout.json", // testing mismatched data layout w/ custom targets
23-
"tests/ui/check-cfg/my-awesome-platform.json", // testing custom targets with cfgs
24-
"tests/ui/argfile/commandline-argfile-badutf8.args", // passing args via a file
25-
"tests/ui/argfile/commandline-argfile.args", // passing args via a file
26-
"tests/ui/crate-loading/auxiliary/libfoo.rlib", // testing loading a manually created rlib
27-
"tests/ui/include-macros/data.bin", // testing including data with the include macros
28-
"tests/ui/include-macros/file.txt", // testing including data with the include macros
29-
"tests/ui/macros/macro-expanded-include/file.txt", // testing including data with the include macros
30-
"tests/ui/macros/not-utf8.bin", // testing including data with the include macros
31-
"tests/ui/macros/syntax-extension-source-utils-files/includeme.fragment", // more include
32-
"tests/ui/proc-macro/auxiliary/included-file.txt", // more include
33-
"tests/ui/unpretty/auxiliary/data.txt", // more include
34-
"tests/ui/invalid/foo.natvis.xml", // sample debugger visualizer
35-
"tests/ui/sanitizer/dataflow-abilist.txt", // dataflow sanitizer ABI list file
36-
"tests/ui/shell-argfiles/shell-argfiles.args", // passing args via a file
37-
"tests/ui/shell-argfiles/shell-argfiles-badquotes.args", // passing args via a file
38-
"tests/ui/shell-argfiles/shell-argfiles-via-argfile-shell.args", // passing args via a file
39-
"tests/ui/shell-argfiles/shell-argfiles-via-argfile.args", // passing args via a file
40-
"tests/ui/std/windows-bat-args1.bat", // tests escaping arguments through batch files
41-
"tests/ui/std/windows-bat-args2.bat", // tests escaping arguments through batch files
42-
"tests/ui/std/windows-bat-args3.bat", // tests escaping arguments through batch files
43-
];
44-
45-
pub fn check(root_path: &Path, bless: bool, bad: &mut bool) {
46-
let issues_txt_header = r#"============================================================
10+
const ISSUES_TXT_HEADER: &str = r#"============================================================
4711
⚠️⚠️⚠️NOTHING SHOULD EVER BE ADDED TO THIS LIST⚠️⚠️⚠️
4812
============================================================
4913
"#;
5014

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

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

78-
let mut remaining_issue_names: BTreeSet<&str> = allowed_issue_names.clone();
79-
80-
let (ui, ui_fulldeps) = (path.join("ui"), path.join("ui-fulldeps"));
81-
let paths = [ui.as_path(), ui_fulldeps.as_path()];
82-
crate::walk::walk_no_read(&paths, |_, _| false, &mut |entry| {
83-
let file_path = entry.path();
84-
if let Some(ext) = file_path.extension().and_then(OsStr::to_str) {
85-
// files that are neither an expected extension or an exception should not exist
86-
// they're probably typos or not meant to exist
87-
if !(EXPECTED_TEST_FILE_EXTENSIONS.contains(&ext)
88-
|| EXTENSION_EXCEPTION_PATHS.iter().any(|path| file_path.ends_with(path)))
89-
{
90-
tidy_error!(bad, "file {} has unexpected extension {}", file_path.display(), ext);
91-
}
92-
93-
// NB: We do not use file_stem() as some file names have multiple `.`s and we
94-
// must strip all of them.
95-
let testname =
96-
file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0;
97-
if ext == "stderr" || ext == "stdout" || ext == "fixed" {
98-
// Test output filenames have one of the formats:
99-
// ```
100-
// $testname.stderr
101-
// $testname.$mode.stderr
102-
// $testname.$revision.stderr
103-
// $testname.$revision.$mode.stderr
104-
// ```
105-
//
106-
// For now, just make sure that there is a corresponding
107-
// `$testname.rs` file.
108-
109-
if !file_path.with_file_name(testname).with_extension("rs").exists()
110-
&& !testname.contains("ignore-tidy")
111-
{
112-
tidy_error!(bad, "Stray file with UI testing output: {:?}", file_path);
113-
}
114-
115-
if let Ok(metadata) = fs::metadata(file_path)
116-
&& metadata.len() == 0
117-
{
118-
tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path);
119-
}
120-
}
43+
deny_new_top_level_ui_tests(bad, &path.join("ui"));
12144

122-
if ext == "rs"
123-
&& let Some(test_name) = static_regex!(r"^issues?[-_]?(\d{3,})").captures(testname)
124-
{
125-
// these paths are always relative to the passed `path` and always UTF8
126-
let stripped_path = file_path
127-
.strip_prefix(path)
128-
.unwrap()
129-
.to_str()
130-
.unwrap()
131-
.replace(std::path::MAIN_SEPARATOR_STR, "/");
132-
133-
if !remaining_issue_names.remove(stripped_path.as_str())
134-
&& !stripped_path.starts_with("ui/issues/")
135-
{
136-
tidy_error!(
137-
bad,
138-
"file `tests/{stripped_path}` must begin with a descriptive name, consider `{{reason}}-issue-{issue_n}.rs`",
139-
issue_n = &test_name[1],
140-
);
141-
}
142-
}
143-
}
144-
});
45+
let remaining_issue_names = recursively_check_ui_tests(bad, path, &allowed_issue_names);
14546

14647
// if there are any file names remaining, they were moved on the fs.
14748
// our data must remain up to date, so it must be removed from issues.txt
@@ -152,7 +53,7 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) {
15253
// so we don't bork things on panic or a contributor using Ctrl+C
15354
let blessed_issues_path = tidy_src.join("issues_blessed.txt");
15455
let mut blessed_issues_txt = fs::File::create(&blessed_issues_path).unwrap();
155-
blessed_issues_txt.write_all(issues_txt_header.as_bytes()).unwrap();
56+
blessed_issues_txt.write_all(ISSUES_TXT_HEADER.as_bytes()).unwrap();
15657
// If we changed paths to use the OS separator, reassert Unix chauvinism for blessing.
15758
for filename in allowed_issue_names.difference(&remaining_issue_names) {
15859
writeln!(blessed_issues_txt, "{filename}").unwrap();
@@ -171,3 +72,170 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) {
17172
}
17273
}
17374
}
75+
76+
fn deny_new_top_level_ui_tests(bad: &mut bool, tests_path: &Path) {
77+
// See <https://github.com/rust-lang/compiler-team/issues/902> where we propose banning adding
78+
// new ui tests *directly* under `tests/ui/`. For more context, see:
79+
//
80+
// - <https://github.com/rust-lang/rust/issues/73494>
81+
// - <https://github.com/rust-lang/rust/issues/133895>
82+
83+
let top_level_ui_tests = walkdir::WalkDir::new(tests_path)
84+
.min_depth(1)
85+
.max_depth(1)
86+
.follow_links(false)
87+
.same_file_system(true)
88+
.into_iter()
89+
.flatten()
90+
.filter(|e| {
91+
let file_name = e.file_name();
92+
file_name != ".gitattributes" && file_name != "README.md"
93+
})
94+
.filter(|e| !e.file_type().is_dir());
95+
for entry in top_level_ui_tests {
96+
tidy_error!(
97+
bad,
98+
"ui tests should be added under meaningful subdirectories: `{}`",
99+
entry.path().display()
100+
)
101+
}
102+
}
103+
104+
fn recursively_check_ui_tests<'issues>(
105+
bad: &mut bool,
106+
path: &Path,
107+
allowed_issue_names: &'issues BTreeSet<&'issues str>,
108+
) -> BTreeSet<&'issues str> {
109+
let mut remaining_issue_names: BTreeSet<&str> = allowed_issue_names.clone();
110+
111+
let (ui, ui_fulldeps) = (path.join("ui"), path.join("ui-fulldeps"));
112+
let paths = [ui.as_path(), ui_fulldeps.as_path()];
113+
crate::walk::walk_no_read(&paths, |_, _| false, &mut |entry| {
114+
let file_path = entry.path();
115+
if let Some(ext) = file_path.extension().and_then(OsStr::to_str) {
116+
check_unexpected_extension(bad, file_path, ext);
117+
118+
// NB: We do not use file_stem() as some file names have multiple `.`s and we
119+
// must strip all of them.
120+
let testname =
121+
file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0;
122+
if ext == "stderr" || ext == "stdout" || ext == "fixed" {
123+
check_stray_output_snapshot(bad, file_path, testname);
124+
check_empty_output_snapshot(bad, file_path);
125+
}
126+
127+
deny_new_nondescriptive_test_names(
128+
bad,
129+
path,
130+
&mut remaining_issue_names,
131+
file_path,
132+
testname,
133+
ext,
134+
);
135+
}
136+
});
137+
remaining_issue_names
138+
}
139+
140+
fn check_unexpected_extension(bad: &mut bool, file_path: &Path, ext: &str) {
141+
const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
142+
"rs", // test source files
143+
"stderr", // expected stderr file, corresponds to a rs file
144+
"svg", // expected svg file, corresponds to a rs file, equivalent to stderr
145+
"stdout", // expected stdout file, corresponds to a rs file
146+
"fixed", // expected source file after applying fixes
147+
"md", // test directory descriptions
148+
"ftl", // translation tests
149+
];
150+
151+
const EXTENSION_EXCEPTION_PATHS: &[&str] = &[
152+
"tests/ui/asm/named-asm-labels.s", // loading an external asm file to test named labels lint
153+
"tests/ui/codegen/mismatched-data-layout.json", // testing mismatched data layout w/ custom targets
154+
"tests/ui/check-cfg/my-awesome-platform.json", // testing custom targets with cfgs
155+
"tests/ui/argfile/commandline-argfile-badutf8.args", // passing args via a file
156+
"tests/ui/argfile/commandline-argfile.args", // passing args via a file
157+
"tests/ui/crate-loading/auxiliary/libfoo.rlib", // testing loading a manually created rlib
158+
"tests/ui/include-macros/data.bin", // testing including data with the include macros
159+
"tests/ui/include-macros/file.txt", // testing including data with the include macros
160+
"tests/ui/macros/macro-expanded-include/file.txt", // testing including data with the include macros
161+
"tests/ui/macros/not-utf8.bin", // testing including data with the include macros
162+
"tests/ui/macros/syntax-extension-source-utils-files/includeme.fragment", // more include
163+
"tests/ui/proc-macro/auxiliary/included-file.txt", // more include
164+
"tests/ui/unpretty/auxiliary/data.txt", // more include
165+
"tests/ui/invalid/foo.natvis.xml", // sample debugger visualizer
166+
"tests/ui/sanitizer/dataflow-abilist.txt", // dataflow sanitizer ABI list file
167+
"tests/ui/shell-argfiles/shell-argfiles.args", // passing args via a file
168+
"tests/ui/shell-argfiles/shell-argfiles-badquotes.args", // passing args via a file
169+
"tests/ui/shell-argfiles/shell-argfiles-via-argfile-shell.args", // passing args via a file
170+
"tests/ui/shell-argfiles/shell-argfiles-via-argfile.args", // passing args via a file
171+
"tests/ui/std/windows-bat-args1.bat", // tests escaping arguments through batch files
172+
"tests/ui/std/windows-bat-args2.bat", // tests escaping arguments through batch files
173+
"tests/ui/std/windows-bat-args3.bat", // tests escaping arguments through batch files
174+
];
175+
176+
// files that are neither an expected extension or an exception should not exist
177+
// they're probably typos or not meant to exist
178+
if !(EXPECTED_TEST_FILE_EXTENSIONS.contains(&ext)
179+
|| EXTENSION_EXCEPTION_PATHS.iter().any(|path| file_path.ends_with(path)))
180+
{
181+
tidy_error!(bad, "file {} has unexpected extension {}", file_path.display(), ext);
182+
}
183+
}
184+
185+
fn check_stray_output_snapshot(bad: &mut bool, file_path: &Path, testname: &str) {
186+
// Test output filenames have one of the formats:
187+
// ```
188+
// $testname.stderr
189+
// $testname.$mode.stderr
190+
// $testname.$revision.stderr
191+
// $testname.$revision.$mode.stderr
192+
// ```
193+
//
194+
// For now, just make sure that there is a corresponding
195+
// `$testname.rs` file.
196+
197+
if !file_path.with_file_name(testname).with_extension("rs").exists()
198+
&& !testname.contains("ignore-tidy")
199+
{
200+
tidy_error!(bad, "Stray file with UI testing output: {:?}", file_path);
201+
}
202+
}
203+
204+
fn check_empty_output_snapshot(bad: &mut bool, file_path: &Path) {
205+
if let Ok(metadata) = fs::metadata(file_path)
206+
&& metadata.len() == 0
207+
{
208+
tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path);
209+
}
210+
}
211+
212+
fn deny_new_nondescriptive_test_names(
213+
bad: &mut bool,
214+
path: &Path,
215+
remaining_issue_names: &mut BTreeSet<&str>,
216+
file_path: &Path,
217+
testname: &str,
218+
ext: &str,
219+
) {
220+
if ext == "rs"
221+
&& let Some(test_name) = static_regex!(r"^issues?[-_]?(\d{3,})").captures(testname)
222+
{
223+
// these paths are always relative to the passed `path` and always UTF8
224+
let stripped_path = file_path
225+
.strip_prefix(path)
226+
.unwrap()
227+
.to_str()
228+
.unwrap()
229+
.replace(std::path::MAIN_SEPARATOR_STR, "/");
230+
231+
if !remaining_issue_names.remove(stripped_path.as_str())
232+
&& !stripped_path.starts_with("ui/issues/")
233+
{
234+
tidy_error!(
235+
bad,
236+
"file `tests/{stripped_path}` must begin with a descriptive name, consider `{{reason}}-issue-{issue_n}.rs`",
237+
issue_n = &test_name[1],
238+
);
239+
}
240+
}
241+
}

0 commit comments

Comments
 (0)