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

Permit running sqllogictest as a rust test in IDEs (+ use clap for sqllogicttest parsing, accept (and ignore) rust test harness arguments) #8288

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Nov 20, 2023

Which issue does this PR close?

Closes #8287
Closes #4769

Rationale for this change

Basically I want to be able to run sqllogictest in RustRover more conveniently (and also make it easier for @tustvold to work in DataFusion so he does it more ;) )

In the UI I enter this:
Screenshot 2024-01-01 at 8 23 08 AM

However, what the IDE actually runs is this (notice the new arguments)
Screenshot 2024-01-01 at 8 24 39 AM

What changes are included in this PR?

  1. Rewrite the sqllogictest CLI parsing command to use clap Use Clap for sqllogictest #4769
  2. Add support for the CLI options that RustRover seems to slap on to its invocation, and warn if they are used to avoid confusion

Are these changes tested?

Yes, by existing CI tests

I also tested manually and I can now run these tests just fine in RustRover/ Here is what a session looks like:

/Users/andrewlamb/.cargo/bin/cargo test --color=always --test sqllogictests --no-fail-fast -- --format=json -Z unstable-options --show-output
Testing started at 8:22 AM ...
    Finished test [unoptimized + debuginfo] target(s) in 0.12s
     Running bin/sqllogictests.rs (target/debug/deps/sqllogictests-170b3f54eec6b11f)
WARNING: Ignoring `--format` compatibility option
WARNING: Ignoring `-Z` compatibility option
WARNING: Ignoring `--show-output` compatibility option
Running "binary.slt"
Running "options.slt"
Running "cte.slt"
Running "ddl.slt"
Running "wildcard.slt"
Running "arrow_files.slt"
...

Are there any user-facing changes?

No, it is entirely developer facing

@alamb alamb added the development-process Related to development process of DataFusion label Nov 20, 2023
@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) and removed development-process Related to development process of DataFusion labels Nov 20, 2023
@alamb alamb marked this pull request as ready for review November 20, 2023 20:05
@alamb
Copy link
Contributor Author

alamb commented Nov 20, 2023

FYI @xudong963 (as you originally filed #4769) and @tustvold (as you originally mentioned #8287)

@tustvold
Copy link
Contributor

tustvold commented Nov 20, 2023

I worry this might be fragile if IDEs choose to enable other options. I'm curious why you decided against making an explicit binary and then a test target that just calls the binary entrypoint but with no arguments, as was described in the initial ticket?

@alamb
Copy link
Contributor Author

alamb commented Nov 21, 2023

I worry this might be fragile if IDEs choose to enable other options.

I agree it is fragile (but easy to fix)

I'm curious why you decided against making an explicit binary and then a test target that just calls the binary entrypoint but with no arguments, as was described in the initial ticket?

I tried this approach but then decided on this one because (at least for me in RustRover) I felt it would be easier to use / understand.

making a different binary ended up being something like cargo run -p datafusion-sqllogictest --bin sqllogictest or cargo run -p datafusion-sqllogictest --bin runner (though maybe my Cargo.toml fu is not as good as it could be)

And then when I started trying to add some documents on how to run this command, it got confusing quickly

Thus I figured it would be easier to understand if it just looked like the test binary.

@alamb alamb added the development-process Related to development process of DataFusion label Jan 1, 2024
@alamb alamb changed the title Use clap for sqllogicttest parsing, accept (and ignore) rust test harness arguments Permit running sqllogictest as a rust test in IDEs (+ use clap for sqllogicttest parsing, accept (and ignore) rust test harness arguments) Jan 1, 2024
@github-actions github-actions bot removed the development-process Related to development process of DataFusion label Jan 1, 2024
@alamb
Copy link
Contributor Author

alamb commented Jan 1, 2024

I added some additional warnings in 9506116 to reduce confusion

@tustvold tustvold merged commit e7cc04d into apache:main Jan 9, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easier to run sqllogictest under IDEs with integrated debuggers Use Clap for sqllogictest
2 participants