mirror of
https://github.com/rust-lang/rust.git
synced 2024-11-26 08:44:35 +00:00
Auto merge of #120628 - workingjubilee:reimpl-meaningful-test-name-lint, r=compiler-errors
Reimpl meaningful test name lint MCP658 This reintroduces the tidy rule originally proposed in https://github.com/rust-lang/rust/pull/113583 that then became an MCP in https://github.com/rust-lang/compiler-team/issues/658 which eventually surfaced a quite-reasonable request for a diagnostic enhancement. I have added that to the rule. It produces output like this: ``` tidy error: file `ui/unsized/issue-115809.rs` must begin with a descriptive name, try `{reason}-issue-115809.rs` tidy error: file `ui/unsized/issue-115203.rs` must begin with a descriptive name, try `{reason}-issue-115203.rs` tidy error: file `ui/privacy/issue-113860-2.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/privacy/issue-117997.rs` must begin with a descriptive name, try `{reason}-issue-117997.rs` tidy error: file `ui/privacy/issue-119463.rs` must begin with a descriptive name, try `{reason}-issue-119463.rs` tidy error: file `ui/privacy/auxiliary/issue-117997.rs` must begin with a descriptive name, try `{reason}-issue-117997.rs` tidy error: file `ui/privacy/auxiliary/issue-119463-extern.rs` must begin with a descriptive name, try `{reason}-issue-119463.rs` tidy error: file `ui/privacy/issue-113860-1.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/privacy/issue-113860.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/const-generics/generic_const_exprs/const_kind_expr/issue_114151.rs` must begin with a descriptive name, try `{reason}-issue-114151.rs` tidy error: file `ui/did_you_mean/issue-114112.rs` must begin with a descriptive name, try `{reason}-issue-114112.rs` tidy error: file `ui/did_you_mean/issue-105225.rs` must begin with a descriptive name, try `{reason}-issue-105225.rs` tidy error: file `ui/did_you_mean/issue-105225-named-args.rs` must begin with a descriptive name, try `{reason}-issue-105225.rs` ``` You get the idea. There are some tests which merely would require reordering of the name according to the rule. I could modify the diagnostic further to identify those, but doing such would make it prone to bad suggestions. I have opted to trust contributors to recognize the diagnostic is robotic, as the pattern we are linting on is easier to match if we do not speculate on what parts of the name are meaningful: sometimes a word is a reason, but sometimes it is a mere "tag", such as with a pair like: - issue-314159265-blue.rs - issue-314159265-red.rs Starting them with `red-` and `blue-` means they do not sort together, despite being related, and the color names are still not very descriptive. Recognizing a good name is an open-ended task, though this pair might be: - colored-circle-gen-blue.rs - colored-circle-gen-red.rs Deciding exactly *how* to solve this is not the business of tidy, only recognizing a what. r? `@compiler-errors`
This commit is contained in:
commit
0b9f6ad994
4387
src/tools/tidy/src/issues.txt
Normal file
4387
src/tools/tidy/src/issues.txt
Normal file
File diff suppressed because it is too large
Load Diff
@ -101,7 +101,7 @@ fn main() {
|
||||
// Checks over tests.
|
||||
check!(tests_placement, &root_path);
|
||||
check!(debug_artifacts, &tests_path);
|
||||
check!(ui_tests, &tests_path);
|
||||
check!(ui_tests, &tests_path, bless);
|
||||
check!(mir_opt_tests, &tests_path, bless);
|
||||
check!(rustdoc_gui_tests, &tests_path);
|
||||
check!(rustdoc_css_themes, &librustdoc_path);
|
||||
|
@ -1,11 +1,13 @@
|
||||
//! Tidy check to ensure below in UI test directories:
|
||||
//! - the number of entries in each directory must be less than `ENTRY_LIMIT`
|
||||
//! - there are no stray `.stderr` files
|
||||
|
||||
use ignore::Walk;
|
||||
use std::collections::HashMap;
|
||||
use lazy_static::lazy_static;
|
||||
use regex::Regex;
|
||||
use std::collections::{BTreeSet, HashMap};
|
||||
use std::ffi::OsStr;
|
||||
use std::fs;
|
||||
use std::io::Write;
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
// FIXME: GitHub's UI truncates file lists that exceed 1000 entries, so these
|
||||
@ -95,8 +97,17 @@ fn check_entries(tests_path: &Path, bad: &mut bool) {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn check(path: &Path, bad: &mut bool) {
|
||||
pub fn check(path: &Path, bless: bool, bad: &mut bool) {
|
||||
check_entries(&path, bad);
|
||||
|
||||
// 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 allowed_issue_names = BTreeSet::from(
|
||||
include!("issues.txt").map(|path| path.replace("/", std::path::MAIN_SEPARATOR_STR)),
|
||||
);
|
||||
|
||||
let mut remaining_issue_names: BTreeSet<String> = 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| {
|
||||
@ -109,6 +120,11 @@ pub fn check(path: &Path, bad: &mut bool) {
|
||||
{
|
||||
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:
|
||||
// ```
|
||||
@ -120,11 +136,7 @@ pub fn check(path: &Path, bad: &mut bool) {
|
||||
//
|
||||
// For now, just make sure that there is a corresponding
|
||||
// `$testname.rs` file.
|
||||
//
|
||||
// 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 !file_path.with_file_name(testname).with_extension("rs").exists()
|
||||
&& !testname.contains("ignore-tidy")
|
||||
{
|
||||
@ -137,6 +149,60 @@ pub fn check(path: &Path, bad: &mut bool) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if ext == "rs" {
|
||||
lazy_static! {
|
||||
static ref ISSUE_NAME_REGEX: Regex =
|
||||
Regex::new(r"^issues?[-_]?(\d{3,})").unwrap();
|
||||
}
|
||||
|
||||
if let Some(test_name) = ISSUE_NAME_REGEX.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();
|
||||
if !remaining_issue_names.remove(stripped_path) {
|
||||
tidy_error!(
|
||||
bad,
|
||||
"file `{stripped_path}` must begin with a descriptive name, consider `{{reason}}-issue-{issue_n}.rs`",
|
||||
issue_n = &test_name[1],
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
// if an excluded file is renamed, it must be removed from this list
|
||||
// do this automatically on bless, otherwise issue a tidy error
|
||||
if bless {
|
||||
let issues_txt_header = r#"
|
||||
/*
|
||||
============================================================
|
||||
⚠️⚠️⚠️NOTHING SHOULD EVER BE ADDED TO THIS LIST⚠️⚠️⚠️
|
||||
============================================================
|
||||
*/
|
||||
[
|
||||
"#;
|
||||
let tidy_src = std::env::current_dir().unwrap().join("src/tools/tidy/src");
|
||||
// instead of overwriting the file, recreate it and use an "atomic rename"
|
||||
// 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(issues_txt_header.as_bytes()).unwrap();
|
||||
for filename in allowed_issue_names.difference(&remaining_issue_names) {
|
||||
write!(blessed_issues_txt, "\"{filename}\",\n").unwrap();
|
||||
}
|
||||
write!(blessed_issues_txt, "]\n").unwrap();
|
||||
let old_issues_path = tidy_src.join("issues.txt");
|
||||
fs::rename(blessed_issues_path, old_issues_path).unwrap();
|
||||
} else {
|
||||
for file_name in remaining_issue_names {
|
||||
let mut p = PathBuf::from(path);
|
||||
p.push(file_name);
|
||||
tidy_error!(
|
||||
bad,
|
||||
"file `{}` no longer exists and should be removed from the exclusions in `src/tools/tidy/src/issues.txt`",
|
||||
p.display()
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user