Skip to content

Do not run avro sqllogictests tests unless the avro feature is enabled #6429

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.

Sign up for GitHub

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

Jump to bottom
Merged
xudong963 merged 1 commit into from
May 23, 2023
Merged

Do not run avro sqllogictests tests unless the avro feature is enabled #6429

xudong963 merged 1 commit into from
May 23, 2023

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented May 23, 2023 •
edited
Loading

Which issue does this PR close?

Closes #6399

Rationale for this change

If a feature is not enabled, we should not fail tests . This test was newly added in #6362 by @e1ijah1 👋 ❤️ but has the unfortunate property that running cargo test -p datafusion now fails because avro.slt will only pass if the avro feature is enabled

What changes are included in this PR?

Change sqllogictest so that it does not run avro.sql if the avro feature is not available

Are these changes tested?

Yes

Are there any user-facing changes?

You can now do:

RUST_LOG=debug cargo test -p datafusion --test sqllogictests -- avro

And it will pass

    Finished test [unoptimized + debuginfo] target(s) in 0.28s
     Running tests/sqllogictests/src/main.rs (/Users/alamb/Software/target-df/debug/deps/sqllogictests-ef50afea7db0aec6)
[2023-05-23T10:59:09Z INFO  sqllogictests] Running with DataFusion runner: tests/sqllogictests/test_files/avro.slt
[2023-05-23T10:59:09Z INFO  sqllogictests] Skipping avro.slt because avro feature is not enabled
[2023-05-23T10:59:09Z INFO  sqllogictests] Skipping: tests/sqllogictests/test_files/avro.slt

@alamb alamb marked this pull request as ready for review May 23, 2023 11:03
@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels May 23, 2023
@alamb
Copy link
Contributor Author

alamb commented May 23, 2023

@xudong963 or @melgenek I wonder if you have time to review this PR?

@alamb alamb mentioned this pull request May 23, 2023
Closed
tustvold
tustvold reviewed May 23, 2023
View reviewed changes
datafusion/core/tests/sqllogictests/src/main.rs
info!("Running with DataFusion runner: {}", path.display());
let test_ctx = context_for_test_file(&relative_path).await;
let Some(test_ctx) = context_for_test_file(&relative_path).await else {
info!("Skipping: {}", path.display());
Copy link
Contributor

@tustvold tustvold May 23, 2023

Choose a reason for hiding this comment

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

I think this will log twice, once in context_for_test_file and once here. I feel at least one of these is redundant...

Copy link
Contributor Author

@alamb alamb May 23, 2023

Choose a reason for hiding this comment

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

Yes, it will log twice. I was thinking that I wanted to log the reason the test was skipped (in context_for_test_file ) as well as make double sure that the logs showed any test that was skipped (if/when we add more tests that are skipped)

Given that the logs aren't printed by default (you need to do RUST_LOG=... to do so ) I chose the "mo logs mo better" approach, but I can see how others might have a different preference

I don't feel strongly and will change it if others prefer less logging

Copy link
Contributor

@tustvold tustvold May 23, 2023

Choose a reason for hiding this comment

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

Don't feel strongly, was just an observation

melgenek
melgenek approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@melgenek melgenek left a comment

Choose a reason for hiding this comment

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

👍

xudong963
xudong963 approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

LGTM

@xudong963 xudong963 merged commit 5a696dd into apache:main May 23, 2023
@alamb
Copy link
Contributor Author

alamb commented May 23, 2023

Thanks for the quick turnaround everyone!@

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tustvold tustvold tustvold approved these changes

@xudong963 xudong963 xudong963 approved these changes

+1 more reviewer

@melgenek melgenek melgenek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tests fail if avro feature is not enabled

4 participants

@alamb @tustvold @melgenek @xudong963