-
-
Notifications
You must be signed in to change notification settings - Fork 123
Introduce test filter flag to nextest archive command #2670
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
base: main
Are you sure you want to change the base?
Conversation
Filtersets can be used to reduce the amount of binaries archived in a test archive. NOTE: Test binaries are not executed to collect a test list. This prevents filtering by test.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2670 +/- ##
==========================================
- Coverage 78.40% 78.39% -0.02%
==========================================
Files 107 108 +1
Lines 24864 25074 +210
==========================================
+ Hits 19495 19656 +161
- Misses 5369 5418 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Looks quite good! Just a couple of comments.
* Return `FiltersetParseErrors` when a `test` predicate is used in the `Archive` command. * Improved integration tests.
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.
Just a few more minor comments, thanks!
fn check_archive_contents(filter: &str, check_archive_contents: impl FnOnce(Vec<PathBuf>)) { | ||
let (_p1, archive_file) = | ||
create_archive_with_args("", false, "", &["-E", filter], false).expect("archive succeeded"); | ||
let file = File::open(archive_file).unwrap(); | ||
let decoder = zstd::stream::read::Decoder::new(file).unwrap(); | ||
let mut archive = tar::Archive::new(decoder); | ||
let paths = archive | ||
.entries() | ||
.unwrap() | ||
.map(|e| e.unwrap().path().unwrap().into_owned()) | ||
.collect::<Vec<_>>(); | ||
check_archive_contents(paths); |
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.
Naming the arg the same as the function is a bit confusing -- I'd just use cb
for "callback" here.
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.
Whoops, didn't realize I did that :)
Updated!
assert!( | ||
!paths | ||
.iter() | ||
.filter(|path| path | ||
.ancestors() | ||
// Test files are in the `deps` folder. | ||
.any(|folder| folder.file_name() == Some(OsStr::new("deps")))) | ||
.any(|path| { | ||
let file_name = path.file_name().unwrap(); | ||
file_name | ||
.to_ascii_lowercase() | ||
.to_str() | ||
.unwrap() | ||
.contains(file) | ||
}) | ||
); |
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.
Include the name of the file you're checking in the assert message -- here, and in the other sections as appropriate.
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.
Added assert messages!
path.file_name() | ||
.unwrap() | ||
.to_str() | ||
.unwrap() | ||
.contains(expected_package_test_file) | ||
})); | ||
assert!(!paths.iter().any(|path| { | ||
path.file_name() | ||
.unwrap() | ||
.to_str() | ||
.unwrap() | ||
.contains(filtered_test) |
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.
Use unwrap_or_else
+ panic!
here with a descriptive error message.
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.
Done!
let all_test_files = [ | ||
"basic", | ||
"cdylib_example", | ||
"cdylib_link", | ||
"dylib_test", | ||
"my_bench", | ||
"nextest_derive", | ||
"nextest_tests", | ||
"nextest_tests", | ||
"other", | ||
"other", | ||
"proc_macro_test", | ||
"segfault", | ||
"with_build_script", | ||
"wrapper", | ||
"nextest_tests", | ||
"other", | ||
]; |
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.
Can you use the list of fixtures as described here? That way when new fixtures get added this list won't have to be updated. https://github.com/nextest-rs/nextest/blob/main/fixture-data/src/nextest_tests.rs
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.
Nice, thank you! Updated
╭──── | ||
1 │ test(sample_test) | ||
· ─────┬───── | ||
· ╰── this predicate causes an unsupported expression |
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 you reword this as "test predicates are not supported while archiving"? You may have to update the other messages as well instead of saying "this predicate causes {}".
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.
Tweaked the wording a bit
Filtersets can be used to reduce the amount of binaries archived in a test archive.
NOTE: Test binaries are not executed to collect a test list. This prevents filtering by test.
See feature request: #2648