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

Allow using cargo nextest for running tests #13045

Merged
merged 7 commits into from
Oct 25, 2024
29 changes: 25 additions & 4 deletions datafusion/sqllogictest/bin/sqllogictests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,15 @@ async fn run_tests() -> Result<()> {
env_logger::init();

let options: Options = clap::Parser::parse();
if options.list {
// nextest parses stdout, so print messages to stderr
eprintln!("NOTICE: --list option unsupported, quitting");
// return Ok, not error so that tools like nextest which are listing all
// workspace tests (by running `cargo test ... --list --format terse`)
// do not fail when they encounter this binary. Instead, print nothing
// to stdout and return OK so they can continue listing other tests.
Comment on lines +68 to +71
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️
a very good explanation

return Ok(());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why Ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if it returns an error, cargo nextest aborts immediately

What is happening I think is that nextest lists all possible commands first (by running cargo test ... --list --format terse) and then uses that output to determine what is subsequently run.

So if the --list part returns an error then none of the tests run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments in fd15523

}
options.warn_on_ignored();

// Run all tests in parallel, reporting failures at the end
Expand Down Expand Up @@ -276,7 +285,7 @@ fn read_dir_recursive_impl(dst: &mut Vec<PathBuf>, path: &Path) -> Result<()> {

/// Parsed command line options
///
/// This structure attempts to mimic the command line options
/// This structure attempts to mimic the command line options of the built in rust test runner
/// accepted by IDEs such as CLion that pass arguments
///
/// See <https://github.com/apache/datafusion/issues/8287> for more details
Expand Down Expand Up @@ -320,6 +329,18 @@ struct Options {
help = "IGNORED (for compatibility with built in rust test runner)"
)]
show_output: bool,

#[clap(
long,
help = "Quits immediately, not listing anything (for compatibility with built in rust test runner)"
alamb marked this conversation as resolved.
Show resolved Hide resolved
)]
list: bool,

#[clap(
long,
help = "IGNORED (for compatibility with built in rust test runner)"
alamb marked this conversation as resolved.
Show resolved Hide resolved
)]
ignored: bool,
}

impl Options {
Expand Down Expand Up @@ -354,15 +375,15 @@ impl Options {
/// Logs warning messages to stdout if any ignored options are passed
fn warn_on_ignored(&self) {
if self.format.is_some() {
println!("WARNING: Ignoring `--format` compatibility option");
eprintln!("WARNING: Ignoring `--format` compatibility option");
}

if self.z_options.is_some() {
println!("WARNING: Ignoring `-Z` compatibility option");
eprintln!("WARNING: Ignoring `-Z` compatibility option");
}

if self.show_output {
println!("WARNING: Ignoring `--show-output` compatibility option");
eprintln!("WARNING: Ignoring `--show-output` compatibility option");
}
}
}