-
Notifications
You must be signed in to change notification settings - Fork 13.3k
tidy: split dots in filename not the entire path when checking for stray stdout/stderr files #121992
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
tidy: split dots in filename not the entire path when checking for stray stdout/stderr files #121992
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,10 +84,16 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { | |
} | ||
}); | ||
|
||
let Some((test_name, _)) = test.to_str().map(|s| s.split_once('.')).flatten() else { | ||
let Some(test_name) = test.file_stem().map(OsStr::to_str).flatten() else { | ||
continue; | ||
}; | ||
|
||
assert!( | ||
!test_name.contains('.'), | ||
"test name cannot contain dots '.': `{}`", | ||
test.display() | ||
); | ||
|
||
test_info.insert(test_name.to_string(), (test, expected_revisions)); | ||
} | ||
|
||
|
@@ -98,14 +104,20 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { | |
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(filename) = sibling.file_name().map(OsStr::to_str).flatten() else { | ||
continue; | ||
}; | ||
|
||
let filename_components = filename.split('.').collect::<Vec<_>>(); | ||
let [file_prefix, ..] = &filename_components[..] else { | ||
continue; | ||
}; | ||
Comment on lines
+111
to
+114
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this logic here isn't robust either. If a test name is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually not sure if we can properly check here, if we don't assume test name itself to not have any dots. According to rustc-dev-guide, a test output can take the form
Maybe it's just better to just say "we assume a test's name don't contain dots or else this tidy check won't catch stray stdout/stderr files for that test"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that's necessary to assume for file names. We can assume that of a file name even if we can't assume that for the full path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We may need to add an assertion with a message to ensure that it doesn't happen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add an aasert for if the test name contains dots, and update the dev-guide to describe this behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably a good idea. A lot of the test infra assumes that things like e.g. |
||
|
||
let Some((test_path, expected_revisions)) = test_info.get(file_prefix) else { | ||
let Some((test_path, expected_revisions)) = test_info.get(*file_prefix) else { | ||
continue; | ||
}; | ||
|
||
match filename_components[..] { | ||
match &filename_components[..] { | ||
// Cannot have a revision component, skip. | ||
[] | [_] => return, | ||
[_, _] if !expected_revisions.is_empty() => { | ||
|
@@ -120,9 +132,9 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { | |
[_, _] => return, | ||
[_, found_revision, .., extension] => { | ||
if !IGNORES.contains(&found_revision) | ||
&& !expected_revisions.contains(found_revision) | ||
&& !expected_revisions.contains(*found_revision) | ||
// This is from `//@ stderr-per-bitwidth` | ||
&& !(extension == "stderr" && ["32bit", "64bit"].contains(&found_revision)) | ||
&& !(*extension == "stderr" && ["32bit", "64bit"].contains(&found_revision)) | ||
{ | ||
// Found some unexpected revision-esque component that is not a known | ||
// compare-mode or expected revision. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
// Regression test for <https://github.com/rust-lang/rust/issues/121986>. | ||
// Check that `tests_revision_unpaired_stdout_stderr` don't accidentally get confused by | ||
// paths containing periods. | ||
|
||
//@ check-pass | ||
|
||
fn main() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.file_prefix which is sadly unstable
https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.with_extension should work too, and be nicer than... split 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we not use some unstable features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I actually want
file_stem
semantics here, because I want to keep the test name which is everything but the "rs" extension. Thissplit_once
doesn't seem right. E.g.a.b.rs
, the test name is should bea.b
nota
. Revisioned output gets additional.[rev].std{out,err}
after the test name to the best of my knowledge.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tidy requires stable no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh 😢