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

Fix filtered test reporting #195

Merged
merged 1 commit into from
Jan 24, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

* report an error instead of panicking when encountering a suggestion that does not belong to the main file.
* number of filtered tests is now > 0 when things actually got filtered.

### Changed

Expand Down
28 changes: 17 additions & 11 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,10 @@ pub fn run_tests(mut config: Config) -> Result<()> {

/// The filter used by `run_tests` to only run on `.rs` files that are
/// specified by [`Config::filter_files`] and [`Config::skip_files`].
pub fn default_file_filter(path: &Path, config: &Config) -> bool {
path.extension().is_some_and(|ext| ext == "rs") && default_any_file_filter(path, config)
/// Returns `None` if there is no extension or the extension is not `.rs`.
pub fn default_file_filter(path: &Path, config: &Config) -> Option<bool> {
path.extension().filter(|&ext| ext == "rs")?;
Some(default_any_file_filter(path, config))
}

/// Run on all files that are specified by [`Config::filter_files`] and
Expand Down Expand Up @@ -203,8 +205,6 @@ pub enum TestOk {
Ok,
/// The test was ignored due to a rule (`//@only-*` or `//@ignore-*`)
Ignored,
/// The test was filtered with the `file_filter` argument.
Filtered,
}

/// The possible results a single test can have.
Expand Down Expand Up @@ -233,9 +233,12 @@ struct TestRun {
/// All `configs` are being run in parallel.
/// If multiple configs are provided, the [`Config::threads`] value of the first one is used;
/// the thread count of all other configs is ignored.
/// The file filter is supposed to return `None` if it was filtered because of file extensions
/// and `Some(false)` if the file was rejected out of other reasons like the file path not matching
/// a user defined filter.
pub fn run_tests_generic(
mut configs: Vec<Config>,
file_filter: impl Fn(&Path, &Config) -> bool + Sync,
file_filter: impl Fn(&Path, &Config) -> Option<bool> + Sync,
per_file_config: impl Fn(&mut Config, &Path, &[u8]) + Sync,
status_emitter: impl StatusEmitter + Send,
) -> Result<()> {
Expand Down Expand Up @@ -275,6 +278,7 @@ pub fn run_tests_generic(
},
};

let mut filtered = 0;
run_and_collect(
num_threads,
|submit| {
Expand All @@ -297,10 +301,14 @@ pub fn run_tests_generic(
for entry in entries {
todo.push_back((entry, config));
}
} else if file_filter(&path, config) {
let status = status_emitter.register_test(path);
// Forward .rs files to the test workers.
submit.send((status, config)).unwrap();
} else if let Some(matched) = file_filter(&path, config) {
if matched {
let status = status_emitter.register_test(path);
// Forward .rs files to the test workers.
submit.send((status, config)).unwrap();
} else {
filtered += 1;
}
}
}
},
Expand Down Expand Up @@ -357,13 +365,11 @@ pub fn run_tests_generic(
let mut failures = vec![];
let mut succeeded = 0;
let mut ignored = 0;
let mut filtered = 0;

for run in results {
match run.result {
Ok(TestOk::Ok) => succeeded += 1,
Ok(TestOk::Ignored) => ignored += 1,
Ok(TestOk::Filtered) => filtered += 1,
Err(errored) => failures.push((run.status, errored)),
}
}
Expand Down
1 change: 0 additions & 1 deletion src/status_emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,6 @@ impl TestStatus for TextTest {
Ok(TestOk::Ok) => "ok".green(),
Err(Errored { .. }) => "FAILED".bright_red().bold(),
Ok(TestOk::Ignored) => "ignored (in-test comment)".yellow(),
Ok(TestOk::Filtered) => return,
};
let old_msg = self.msg();
let msg = format!("... {result}");
Expand Down
40 changes: 22 additions & 18 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,25 +98,29 @@ fn main() -> Result<()> {
.ends_with("-fail");
if cfg!(windows) && path.components().any(|c| c.as_os_str() == "basic-bin") {
// on windows there's also a .pdb file, so we get additional errors that aren't there on other platforms
return false;
return Some(false);
}
path.ends_with("Cargo.toml")
&& path.parent().unwrap().parent().unwrap() == root_dir
&& match config
.comment_defaults
.base_immut()
.mode
.as_deref()
.unwrap()
{
Mode::Pass => !fail,
// This is weird, but `cargo test` returns 101 instead of 1 when
// multiple [[test]]s exist. If there's only one test, it returns
// 1 on failure.
Mode::Panic => fail,
Mode::Run { .. } | Mode::Yolo { .. } | Mode::Fail { .. } => unreachable!(),
}
&& default_any_file_filter(path, config)
if !path.ends_with("Cargo.toml") {
return None;
}
Some(
path.parent().unwrap().parent().unwrap() == root_dir
&& match config
.comment_defaults
.base_immut()
.mode
.as_deref()
.unwrap()
{
Mode::Pass => !fail,
// This is weird, but `cargo test` returns 101 instead of 1 when
// multiple [[test]]s exist. If there's only one test, it returns
// 1 on failure.
Mode::Panic => fail,
Mode::Run { .. } | Mode::Yolo { .. } | Mode::Fail { .. } => unreachable!(),
}
&& default_any_file_filter(path, config),
)
},
|_, _, _| {},
(
Expand Down
Loading