Skip to content
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.

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

Add tidy check for .stderr/.stdout files for non-existent test revisions #121475

Merged
merged 2 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 32 additions & 0 deletions src/tools/tidy/src/iter_header.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
const COMMENT: &str = "//@";

/// A header line, like `//@name: value` consists of the prefix `//@` and the directive
/// `name: value`. It is also possibly revisioned, e.g. `//@[revision] name: value`.
pub(crate) struct HeaderLine<'ln> {
pub(crate) revision: Option<&'ln str>,
pub(crate) directive: &'ln str,
}

/// Iterate through compiletest headers in a test contents.
///
/// Adjusted from compiletest/src/header.rs.
pub(crate) fn iter_header<'ln>(contents: &'ln str, it: &mut dyn FnMut(HeaderLine<'ln>)) {
for ln in contents.lines() {
let ln = ln.trim();

// We're left with potentially `[rev]name: value`.
let Some(remainder) = ln.strip_prefix(COMMENT) else {
continue;
};

if let Some(remainder) = remainder.trim_start().strip_prefix('[') {
let Some((revision, remainder)) = remainder.split_once(']') else {
panic!("malformed revision directive: expected `//@[rev]`, found `{ln}`");
};
// We trimmed off the `[rev]` portion, left with `name: value`.
it(HeaderLine { revision: Some(revision), directive: remainder.trim() });
} else {
it(HeaderLine { revision: None, directive: remainder.trim() });
}
}
}
2 changes: 2 additions & 0 deletions src/tools/tidy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub mod ext_tool_checks;
pub mod extdeps;
pub mod features;
pub mod fluent_alphabetical;
pub(crate) mod iter_header;
pub mod mir_opt_tests;
pub mod pal;
pub mod rustdoc_css_themes;
Expand All @@ -73,6 +74,7 @@ pub mod style;
pub mod target_policy;
pub mod target_specific_tests;
pub mod tests_placement;
pub mod tests_revision_unpaired_stdout_stderr;
pub mod ui_tests;
pub mod unit_tests;
pub mod unstable_book;
Expand Down
1 change: 1 addition & 0 deletions src/tools/tidy/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ fn main() {

// Checks over tests.
check!(tests_placement, &root_path);
check!(tests_revision_unpaired_stdout_stderr, &tests_path);
check!(debug_artifacts, &tests_path);
check!(ui_tests, &tests_path, bless);
check!(mir_opt_tests, &tests_path, bless);
Expand Down
28 changes: 4 additions & 24 deletions src/tools/tidy/src/target_specific_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,12 @@
use std::collections::BTreeMap;
use std::path::Path;

use crate::iter_header::{iter_header, HeaderLine};
use crate::walk::filter_not_rust;

const COMMENT: &str = "//@";
const LLVM_COMPONENTS_HEADER: &str = "needs-llvm-components:";
const COMPILE_FLAGS_HEADER: &str = "compile-flags:";

/// Iterate through compiletest headers in a test contents.
///
/// Adjusted from compiletest/src/header.rs.
fn iter_header<'a>(contents: &'a str, it: &mut dyn FnMut(Option<&'a str>, &'a str)) {
for ln in contents.lines() {
let ln = ln.trim();
if ln.starts_with(COMMENT) && ln[COMMENT.len()..].trim_start().starts_with('[') {
if let Some(close_brace) = ln.find(']') {
let open_brace = ln.find('[').unwrap();
let lncfg = &ln[open_brace + 1..close_brace];
it(Some(lncfg), ln[(close_brace + 1)..].trim_start());
} else {
panic!("malformed condition directive: expected `//[foo]`, found `{ln}`")
}
} else if ln.starts_with(COMMENT) {
it(None, ln[COMMENT.len()..].trim_start());
}
}
}

#[derive(Default, Debug)]
struct RevisionInfo<'a> {
target_arch: Option<&'a str>,
Expand All @@ -40,9 +20,9 @@ pub fn check(path: &Path, bad: &mut bool) {
crate::walk::walk(path, |path, _is_dir| filter_not_rust(path), &mut |entry, content| {
let file = entry.path().display();
let mut header_map = BTreeMap::new();
iter_header(content, &mut |cfg, directive| {
iter_header(content, &mut |HeaderLine { revision, directive }| {
if let Some(value) = directive.strip_prefix(LLVM_COMPONENTS_HEADER) {
let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
let info = header_map.entry(revision).or_insert(RevisionInfo::default());
let comp_vec = info.llvm_components.get_or_insert(Vec::new());
for component in value.split(' ') {
let component = component.trim();
Expand All @@ -56,7 +36,7 @@ pub fn check(path: &Path, bad: &mut bool) {
if let Some((arch, _)) =
v.trim_start_matches(|c| c == ' ' || c == '=').split_once("-")
{
let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
let info = header_map.entry(revision).or_insert(RevisionInfo::default());
info.target_arch.replace(arch);
} else {
eprintln!("{file}: seems to have a malformed --target value");
Expand Down
146 changes: 146 additions & 0 deletions src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
//! Checks that there are no unpaired `.stderr` or `.stdout` for a test with and without revisions.

use std::collections::{BTreeMap, BTreeSet};
use std::ffi::OsStr;
use std::path::Path;

use crate::iter_header::*;
use crate::walk::*;

// Should be kept in sync with `CompareMode` in `src/tools/compiletest/src/common.rs`,
// as well as `run`.
const IGNORES: &[&str] = &[
"polonius",
"chalk",
"split-dwarf",
"split-dwarf-single",
"next-solver-coherence",
"next-solver",
"run",
];
const EXTENSIONS: &[&str] = &["stdout", "stderr"];
const SPECIAL_TEST: &str = "tests/ui/command/need-crate-arg-ignore-tidy.x.rs";

pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) {
// Recurse over subdirectories under `tests/`
walk_dir(tests_path.as_ref(), filter, &mut |entry| {
// We are inspecting a folder. Collect the paths to interesting files `.rs`, `.stderr`,
// `.stdout` under the current folder (shallow).
let mut files_under_inspection = BTreeSet::new();
for sibling in std::fs::read_dir(entry.path()).unwrap() {
let Ok(sibling) = sibling else {
continue;
};

if sibling.path().is_dir() {
continue;
}

let sibling_path = sibling.path();

let Some(ext) = sibling_path.extension().map(OsStr::to_str).flatten() else {
continue;
};

if ext == "rs" || EXTENSIONS.contains(&ext) {
files_under_inspection.insert(sibling_path);
}
}

let mut test_info = BTreeMap::new();

for test in
files_under_inspection.iter().filter(|f| f.extension().is_some_and(|ext| ext == "rs"))
{
if test.ends_with(SPECIAL_TEST) {
continue;
}

let mut expected_revisions = BTreeSet::new();

let contents = std::fs::read_to_string(test).unwrap();

// Collect directives.
iter_header(&contents, &mut |HeaderLine { revision, directive }| {
// We're trying to *find* `//@ revision: xxx` directives themselves, not revisioned
// directives.
if revision.is_some() {
return;
}

let directive = directive.trim();

if directive.starts_with("revisions") {
let Some((name, value)) = directive.split_once([':', ' ']) else {
return;
};

if name == "revisions" {
let revs = value.split(' ');
for rev in revs {
expected_revisions.insert(rev.to_owned());
}
}
}
});

let Some((test_name, _)) = test.to_str().map(|s| s.split_once('.')).flatten() else {
continue;
};

test_info.insert(test_name.to_string(), (test, expected_revisions));
}

// Our test file `foo.rs` has specified no revisions. There should not be any
// `foo.rev{.stderr,.stdout}` files. rustc-dev-guide says test output files can have names
// of the form: `test-name.revision.compare_mode.extension`, but our only concern is
// `test-name.revision` and `extension`.
for sibling in files_under_inspection.iter().filter(|f| {
f.extension().map(OsStr::to_str).flatten().is_some_and(|ext| EXTENSIONS.contains(&ext))
}) {
let filename_components = sibling.to_str().unwrap().split('.').collect::<Vec<_>>();
let file_prefix = filename_components[0];

let Some((test_path, expected_revisions)) = test_info.get(file_prefix) else {
continue;
};

match filename_components[..] {
// Cannot have a revision component, skip.
[] | [_] => return,
[_, _] if !expected_revisions.is_empty() => {
// Found unrevisioned output files for a revisioned test.
tidy_error!(
bad,
"found unrevisioned output file `{}` for a revisioned test `{}`",
sibling.display(),
test_path.display(),
);
}
[_, _] => return,
[_, found_revision, .., extension] => {
if !IGNORES.contains(&found_revision)
&& !expected_revisions.contains(found_revision)
// This is from `//@ stderr-per-bitwidth`
&& !(extension == "stderr" && ["32bit", "64bit"].contains(&found_revision))
{
// Found some unexpected revision-esque component that is not a known
// compare-mode or expected revision.
tidy_error!(
bad,
"found output file `{}` for unexpected revision `{}` of test `{}`",
sibling.display(),
found_revision,
test_path.display()
);
}
}
}
}
});
}

fn filter(path: &Path) -> bool {
filter_dirs(path) // ignore certain dirs
|| (path.file_name().is_some_and(|name| name == "auxiliary")) // ignore auxiliary folder
}
17 changes: 17 additions & 0 deletions src/tools/tidy/src/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,20 @@ pub(crate) fn walk_no_read(
}
}
}

// Walk through directories and skip symlinks.
pub(crate) fn walk_dir(
path: &Path,
skip: impl Send + Sync + 'static + Fn(&Path) -> bool,
f: &mut dyn FnMut(&DirEntry),
) {
let mut walker = ignore::WalkBuilder::new(path);
let walker = walker.filter_entry(move |e| !skip(e.path()));
for entry in walker.build() {
if let Ok(entry) = entry {
if entry.path().is_dir() {
f(&entry);
}
}
}
}

This file was deleted.

This file was deleted.

This file was deleted.

Loading
Loading