You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "blaginin (via GitHub)" <gi...@apache.org> on 2024/12/05 22:40:35 UTC
[PR] Add snap to CLI & set up AWS mock [datafusion]
blaginin opened a new pull request, #13672:
URL: https://github.com/apache/datafusion/pull/13672
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Related to https://github.com/apache/datafusion/issues/13456#issuecomment-2515916144
## Rationale for this change
We currently don't test whether our external integrations actually work: as a result https://github.com/apache/datafusion/pull/13576 has happened.
## What changes are included in this PR?
Integration tests for S3.
## Are there any user-facing changes?
No.
**To-Do:**
- [ ] Add documentation on testing.
- [ ] Set up continuous integration (CI).
- [ ] Consider better snapshot names.
- [ ] Test to ensure case preservation (the reason this all started π
).
- [ ] Create an issue to add test cases for other services.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#issuecomment-2714438025
I merged up to resolve conflicts and plan to merge this when the tests pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#issuecomment-2708825239
I plan to merge this on Tuesday unless anyone needs additional time to review.
I may then organize some tickets to port existing tests to use the new framework (especially as we seem to be a little short on good first issue tickets)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1894533684
##########
datafusion-cli/tests/cli_integration.rs:
##########
@@ -17,42 +17,223 @@
use std::process::Command;
-use assert_cmd::prelude::{CommandCargoExt, OutputAssertExt};
-use predicates::prelude::predicate;
use rstest::rstest;
+use aws_config::Region;
+use aws_credential_types::Credentials;
+use aws_sdk_s3::error::SdkError;
+use aws_sdk_ssooidc::config::BehaviorVersion;
+use insta::{glob, Settings};
+use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
+use std::{env, fs};
+
+fn cli() -> Command {
+ Command::new(get_cargo_bin("datafusion-cli"))
+}
+
+fn make_settings() -> Settings {
+ let mut settings = Settings::clone_current();
+ settings.set_prepend_module_to_snapshot(false);
+ settings.add_filter(r"Elapsed .* seconds\.", "[ELAPSED]");
+ settings.add_filter(r"DataFusion CLI v.*", "[CLI_VERSION]");
+ settings
+}
+
#[cfg(test)]
#[ctor::ctor]
fn init() {
// Enable RUST_LOG logging configuration for tests
let _ = env_logger::try_init();
}
-// Disabled due to https://github.com/apache/datafusion/issues/10793
-#[cfg(not(target_family = "windows"))]
#[rstest]
-#[case::exec_from_commands(
- ["--command", "select 1", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
-)]
#[case::exec_multiple_statements(
- ["--command", "select 1; select 2;", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n"
+ "statements",
+ ["--command", "select 1; select 2;", "-q"],
)]
#[case::exec_from_files(
- ["--file", "tests/data/sql.txt", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
+ "files",
+ ["--file", "tests/sql/select.sql", "-q"],
)]
#[case::set_batch_size(
- ["--command", "show datafusion.execution.batch_size", "--format", "json", "-q", "-b", "1"],
- "[{\"name\":\"datafusion.execution.batch_size\",\"value\":\"1\"}]\n"
+ "batch_size",
+ ["--command", "show datafusion.execution.batch_size", "-q", "-b", "1"],
)]
#[test]
fn cli_quick_test<'a>(
+ #[case] snapshot_name: &'a str,
#[case] args: impl IntoIterator<Item = &'a str>,
- #[case] expected: &str,
) {
- let mut cmd = Command::cargo_bin("datafusion-cli").unwrap();
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(snapshot_name);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
cmd.args(args);
- cmd.assert().stdout(predicate::eq(expected));
+
+ assert_cmd_snapshot!(cmd);
+}
+
+#[rstest]
+#[case("csv")]
+#[case("tsv")]
+#[case("table")]
+#[case("json")]
+#[case("nd-json")]
+#[case("automatic")]
+#[test]
+fn test_cli_format<'a>(#[case] format: &'a str) {
Review Comment:
I was wondering why we need all the new aws-s3-sdk dependencies and it looks like it is needed in order to programmatically setup the bucket.
I think minio also supports just serving the contents of directories as s3 files
Did you consider looking at just configuring minio directly rather than using the AWS S3 ASK just to create. a bucket?
I know it doesn't seem like a big deal, but this PR adds many dependencies (that is why Cargo.lock is so big) and we try to keep the dependency chain down as much as possible (it is already large)
I realize it is only for datafusion-cli dev, but that dev directly impacts maintenance (
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,72 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Snapshot testing
+
+To test CLI output, [Insta](https://github.com/mitsuhiko/insta) is used for snapshot testing. Snapshots are generated
+and compared on each test run. If the output changes, tests will fail.
+To review the changes, you can use Insta CLI:
+
+```shell
+cargo install cargo-insta
+cargo insta review
Review Comment:
we use this in influxdb_iox and I find it super useful
##########
datafusion-cli/tests/cli_integration.rs:
##########
@@ -17,42 +17,223 @@
use std::process::Command;
-use assert_cmd::prelude::{CommandCargoExt, OutputAssertExt};
-use predicates::prelude::predicate;
use rstest::rstest;
+use aws_config::Region;
+use aws_credential_types::Credentials;
+use aws_sdk_s3::error::SdkError;
+use aws_sdk_ssooidc::config::BehaviorVersion;
+use insta::{glob, Settings};
+use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
+use std::{env, fs};
+
+fn cli() -> Command {
+ Command::new(get_cargo_bin("datafusion-cli"))
+}
+
+fn make_settings() -> Settings {
+ let mut settings = Settings::clone_current();
+ settings.set_prepend_module_to_snapshot(false);
+ settings.add_filter(r"Elapsed .* seconds\.", "[ELAPSED]");
+ settings.add_filter(r"DataFusion CLI v.*", "[CLI_VERSION]");
+ settings
+}
+
#[cfg(test)]
#[ctor::ctor]
fn init() {
// Enable RUST_LOG logging configuration for tests
let _ = env_logger::try_init();
}
-// Disabled due to https://github.com/apache/datafusion/issues/10793
-#[cfg(not(target_family = "windows"))]
#[rstest]
-#[case::exec_from_commands(
- ["--command", "select 1", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
-)]
#[case::exec_multiple_statements(
- ["--command", "select 1; select 2;", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n"
+ "statements",
+ ["--command", "select 1; select 2;", "-q"],
)]
#[case::exec_from_files(
- ["--file", "tests/data/sql.txt", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
+ "files",
+ ["--file", "tests/sql/select.sql", "-q"],
)]
#[case::set_batch_size(
- ["--command", "show datafusion.execution.batch_size", "--format", "json", "-q", "-b", "1"],
- "[{\"name\":\"datafusion.execution.batch_size\",\"value\":\"1\"}]\n"
+ "batch_size",
+ ["--command", "show datafusion.execution.batch_size", "-q", "-b", "1"],
)]
#[test]
fn cli_quick_test<'a>(
+ #[case] snapshot_name: &'a str,
#[case] args: impl IntoIterator<Item = &'a str>,
- #[case] expected: &str,
) {
- let mut cmd = Command::cargo_bin("datafusion-cli").unwrap();
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(snapshot_name);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
cmd.args(args);
- cmd.assert().stdout(predicate::eq(expected));
+
+ assert_cmd_snapshot!(cmd);
+}
+
+#[rstest]
+#[case("csv")]
+#[case("tsv")]
+#[case("table")]
+#[case("json")]
+#[case("nd-json")]
+#[case("automatic")]
+#[test]
+fn test_cli_format<'a>(#[case] format: &'a str) {
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(format);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
+ cmd.args(["--command", "select 1", "-q", "--format", format]);
+
+ assert_cmd_snapshot!(cmd);
+}
+
+async fn s3_client() -> aws_sdk_s3::Client {
+ let access_key_id =
+ env::var("AWS_ACCESS_KEY_ID").expect("AWS_ACCESS_KEY_ID is not set");
+ let secret_access_key =
+ env::var("AWS_SECRET_ACCESS_KEY").expect("AWS_SECRET_ACCESS_KEY is not set");
+
+ let endpoint_url = env::var("AWS_ENDPOINT").expect("AWS_ENDPOINT is not set");
+ let region = Region::new(env::var("AWS_REGION").unwrap_or("df-test".to_string()));
+
+ let allow_non_test_credentials = env::var("ALLOW_NON_TEST_CREDENTIALS")
+ .map(|v| v == "1")
+ .unwrap_or(false);
+
+ if allow_non_test_credentials
+ || !access_key_id.starts_with("TEST-")
+ || !secret_access_key.starts_with("TEST-")
+ {
+ panic!("Refusing with non-test credentials. Either set ALLOW_NON_TEST_CREDENTIALS=1 or add TEST- for AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY");
+ }
+
+ let creds = Credentials::new(access_key_id, secret_access_key, None, None, "test");
+ let config = aws_sdk_s3::Config::builder()
+ .credentials_provider(creds)
+ .endpoint_url(endpoint_url)
+ .region(region)
+ .behavior_version(BehaviorVersion::v2024_03_28())
+ .build();
+
+ aws_sdk_s3::Client::from_conf(config)
+}
+
+async fn create_bucket(bucket_name: &str, client: &aws_sdk_s3::Client) {
+ match client.head_bucket().bucket(bucket_name).send().await {
+ Ok(_) => {}
+ Err(SdkError::ServiceError(err))
+ if matches!(
+ err.err(),
+ aws_sdk_s3::operation::head_bucket::HeadBucketError::NotFound(_)
+ ) =>
+ {
+ client
+ .create_bucket()
+ .bucket(bucket_name)
+ .send()
+ .await
+ .expect("Failed to create bucket");
+ }
+ Err(e) => panic!("Failed to head bucket: {:?}", e),
+ }
+}
+
+async fn move_file_to_bucket(
+ from_path: &str,
+ to_path: &str,
+ bucket_name: &str,
+ client: &aws_sdk_s3::Client,
+) {
+ let body =
+ aws_sdk_s3::primitives::ByteStream::from_path(std::path::Path::new(from_path))
+ .await
+ .expect("Failed to read file");
+
+ client
+ .put_object()
+ .bucket(bucket_name)
+ .key(to_path)
+ .body(body)
+ .send()
+ .await
+ .expect("Failed to put object");
+}
+
+#[tokio::test]
+async fn test_cli() {
+ if env::var("TEST_STORAGE_INTEGRATION").is_err() {
+ eprintln!("Skipping external storages integration tests");
+ return;
+ }
+
+ let client = s3_client().await;
+ create_bucket("cli", &client).await;
+ move_file_to_bucket(
+ "../datafusion/core/tests/data/cars.csv",
+ "cars.csv",
+ "cli",
+ &client,
+ )
+ .await;
+
+ let settings = make_settings();
+ let _bound = settings.bind_to_scope();
+
+ glob!("sql/*.sql", |path| {
+ let input = fs::read_to_string(path).unwrap();
+ assert_cmd_snapshot!(cli().pass_stdin(input))
Review Comment:
FWIW we have had really nice luck in influxdb_iox using insta, and instead of using the external snap shot testing we used inline snapshots (so the expected results are inline with the test code)
It looks like this:
```rust
fn test_union_not_nested() {
let plan = Arc::new(UnionExec::new(vec![other_node()]));
let opt = NestedUnion;
insta::assert_yaml_snapshot!(
OptimizationTest::new(plan, opt),
@r#"
input:
- " UnionExec"
- " EmptyExec"
output:
Ok:
- " UnionExec"
- " EmptyExec"
"#
);
}
```
Is that possible with assert_cmt_snapshot?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1896793013
##########
datafusion-cli/tests/cli_integration.rs:
##########
@@ -17,42 +17,223 @@
use std::process::Command;
-use assert_cmd::prelude::{CommandCargoExt, OutputAssertExt};
-use predicates::prelude::predicate;
use rstest::rstest;
+use aws_config::Region;
+use aws_credential_types::Credentials;
+use aws_sdk_s3::error::SdkError;
+use aws_sdk_ssooidc::config::BehaviorVersion;
+use insta::{glob, Settings};
+use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
+use std::{env, fs};
+
+fn cli() -> Command {
+ Command::new(get_cargo_bin("datafusion-cli"))
+}
+
+fn make_settings() -> Settings {
+ let mut settings = Settings::clone_current();
+ settings.set_prepend_module_to_snapshot(false);
+ settings.add_filter(r"Elapsed .* seconds\.", "[ELAPSED]");
+ settings.add_filter(r"DataFusion CLI v.*", "[CLI_VERSION]");
+ settings
+}
+
#[cfg(test)]
#[ctor::ctor]
fn init() {
// Enable RUST_LOG logging configuration for tests
let _ = env_logger::try_init();
}
-// Disabled due to https://github.com/apache/datafusion/issues/10793
-#[cfg(not(target_family = "windows"))]
#[rstest]
-#[case::exec_from_commands(
- ["--command", "select 1", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
-)]
#[case::exec_multiple_statements(
- ["--command", "select 1; select 2;", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n"
+ "statements",
+ ["--command", "select 1; select 2;", "-q"],
)]
#[case::exec_from_files(
- ["--file", "tests/data/sql.txt", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
+ "files",
+ ["--file", "tests/sql/select.sql", "-q"],
)]
#[case::set_batch_size(
- ["--command", "show datafusion.execution.batch_size", "--format", "json", "-q", "-b", "1"],
- "[{\"name\":\"datafusion.execution.batch_size\",\"value\":\"1\"}]\n"
+ "batch_size",
+ ["--command", "show datafusion.execution.batch_size", "-q", "-b", "1"],
)]
#[test]
fn cli_quick_test<'a>(
+ #[case] snapshot_name: &'a str,
#[case] args: impl IntoIterator<Item = &'a str>,
- #[case] expected: &str,
) {
- let mut cmd = Command::cargo_bin("datafusion-cli").unwrap();
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(snapshot_name);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
cmd.args(args);
- cmd.assert().stdout(predicate::eq(expected));
+
+ assert_cmd_snapshot!(cmd);
+}
+
+#[rstest]
+#[case("csv")]
+#[case("tsv")]
+#[case("table")]
+#[case("json")]
+#[case("nd-json")]
+#[case("automatic")]
+#[test]
+fn test_cli_format<'a>(#[case] format: &'a str) {
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(format);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
+ cmd.args(["--command", "select 1", "-q", "--format", format]);
+
+ assert_cmd_snapshot!(cmd);
+}
+
+async fn s3_client() -> aws_sdk_s3::Client {
+ let access_key_id =
+ env::var("AWS_ACCESS_KEY_ID").expect("AWS_ACCESS_KEY_ID is not set");
+ let secret_access_key =
+ env::var("AWS_SECRET_ACCESS_KEY").expect("AWS_SECRET_ACCESS_KEY is not set");
+
+ let endpoint_url = env::var("AWS_ENDPOINT").expect("AWS_ENDPOINT is not set");
+ let region = Region::new(env::var("AWS_REGION").unwrap_or("df-test".to_string()));
+
+ let allow_non_test_credentials = env::var("ALLOW_NON_TEST_CREDENTIALS")
+ .map(|v| v == "1")
+ .unwrap_or(false);
+
+ if allow_non_test_credentials
+ || !access_key_id.starts_with("TEST-")
+ || !secret_access_key.starts_with("TEST-")
+ {
+ panic!("Refusing with non-test credentials. Either set ALLOW_NON_TEST_CREDENTIALS=1 or add TEST- for AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY");
+ }
+
+ let creds = Credentials::new(access_key_id, secret_access_key, None, None, "test");
+ let config = aws_sdk_s3::Config::builder()
+ .credentials_provider(creds)
+ .endpoint_url(endpoint_url)
+ .region(region)
+ .behavior_version(BehaviorVersion::v2024_03_28())
+ .build();
+
+ aws_sdk_s3::Client::from_conf(config)
+}
+
+async fn create_bucket(bucket_name: &str, client: &aws_sdk_s3::Client) {
+ match client.head_bucket().bucket(bucket_name).send().await {
+ Ok(_) => {}
+ Err(SdkError::ServiceError(err))
+ if matches!(
+ err.err(),
+ aws_sdk_s3::operation::head_bucket::HeadBucketError::NotFound(_)
+ ) =>
+ {
+ client
+ .create_bucket()
+ .bucket(bucket_name)
+ .send()
+ .await
+ .expect("Failed to create bucket");
+ }
+ Err(e) => panic!("Failed to head bucket: {:?}", e),
+ }
+}
+
+async fn move_file_to_bucket(
+ from_path: &str,
+ to_path: &str,
+ bucket_name: &str,
+ client: &aws_sdk_s3::Client,
+) {
+ let body =
+ aws_sdk_s3::primitives::ByteStream::from_path(std::path::Path::new(from_path))
+ .await
+ .expect("Failed to read file");
+
+ client
+ .put_object()
+ .bucket(bucket_name)
+ .key(to_path)
+ .body(body)
+ .send()
+ .await
+ .expect("Failed to put object");
+}
+
+#[tokio::test]
+async fn test_cli() {
+ if env::var("TEST_STORAGE_INTEGRATION").is_err() {
+ eprintln!("Skipping external storages integration tests");
+ return;
+ }
+
+ let client = s3_client().await;
+ create_bucket("cli", &client).await;
+ move_file_to_bucket(
+ "../datafusion/core/tests/data/cars.csv",
+ "cars.csv",
+ "cli",
+ &client,
+ )
+ .await;
+
+ let settings = make_settings();
+ let _bound = settings.bind_to_scope();
+
+ glob!("sql/*.sql", |path| {
+ let input = fs::read_to_string(path).unwrap();
+ assert_cmd_snapshot!(cli().pass_stdin(input))
Review Comment:
I personally prefer it to be inline, but perhaps we can do that as a follow on PR
(the way we do this in influxdb_iox is that we have a test function that returns a string and then compare the string with the `assert_yaml_snapshot` -- that way the setup code isn't replicated and we can still have the tests inline
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1985782592
##########
.github/workflows/extended.yml:
##########
@@ -67,11 +67,8 @@ jobs:
fetch-depth: 1
- name: Free Disk Space (Ubuntu)
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be
- - name: Install Rust
- run: |
- curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
- source $HOME/.cargo/env
- rustup toolchain install
+ - name: Setup Rust toolchain
Review Comment:
I think this will fail because (as I found out earlier in the week) the ubuntu builder doens't start with rust
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#issuecomment-2701668145
Because CLI integration tests don't break very often, I moved them to the extended tests
https://github.com/blaginin/datafusion/actions/runs/13679702615/job/38249126001#step:8:2585
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#issuecomment-2579427590
Sorry, was on holidays. Will remove `aws-sdk-s3` now, that's a very fair point
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#issuecomment-2541858344
This PR is ready ish for the reivew - but i want to merge https://github.com/apache/datafusion/pull/13576 first, so this one has a smaller diff
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1986051734
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,75 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Running Storage Integration Tests
+
+By default, storage integration tests are not run. To run them you will need to set `TEST_STORAGE_INTEGRATION=1` and
+then provide the necessary configuration for that object store.
+
+For some of the tests, [snapshots](https://datafusion.apache.org/contributor-guide/testing.html#snapshot-testing) are used.
+
+### AWS
+
+To test the S3 integration against [Minio](https://github.com/minio/minio)
+
+First start up a container with Minio and load test files.
+
+```shell
+docker run -d \
Review Comment:
I think itβs a nice improvement if tests just handle the setup automatically! Created an issue to discuss: https://github.com/apache/datafusion/issues/15092
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1984960392
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,72 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Snapshot testing
+
+To test CLI output, [Insta](https://github.com/mitsuhiko/insta) is used for snapshot testing. Snapshots are generated
+and compared on each test run. If the output changes, tests will fail.
+To review the changes, you can use Insta CLI:
+
+```shell
+cargo install cargo-insta
+cargo insta review
Review Comment:
sure! there's also an input data in the snap file, i can work on making them more readable in the future if you want
<img width="1103" alt="RustRover-EAP 2025-03-07 12 13 58" src="https://github.com/user-attachments/assets/b5d8204d-3321-460b-89e5-e75719d872bc" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1984956677
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,72 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Snapshot testing
+
+To test CLI output, [Insta](https://github.com/mitsuhiko/insta) is used for snapshot testing. Snapshots are generated
+and compared on each test run. If the output changes, tests will fail.
+To review the changes, you can use Insta CLI:
+
+```shell
+cargo install cargo-insta
+cargo insta review
Review Comment:
I think we will agree to disagree -- I find the inline snapshots easier to reason about as I can see the test and expected output in the same place. But I can agree it is a matter of preference
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1982195950
##########
.github/workflows/extended.yml:
##########
@@ -67,25 +67,45 @@ jobs:
fetch-depth: 1
- name: Free Disk Space (Ubuntu)
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be
- - name: Install Rust
+ - name: Setup Rust toolchain
run: |
- curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
- source $HOME/.cargo/env
+ rustup toolchain install stable
Review Comment:
I don't think it needs to install stable toolchain (maybe this is a merge mistake π€ )
##########
.github/workflows/extended.yml:
##########
@@ -67,25 +67,45 @@ jobs:
fetch-depth: 1
- name: Free Disk Space (Ubuntu)
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be
- - name: Install Rust
+ - name: Setup Rust toolchain
run: |
- curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
- source $HOME/.cargo/env
+ rustup toolchain install stable
rustup toolchain install
- name: Install Protobuf Compiler
run: sudo apt-get install -y protobuf-compiler
+ - name: Setup Minio - S3-compatible storage
Review Comment:
I am worried that an extended test may fail (as it is only run on commits to main). And I have spent a bunch of time recently keeping the extended tests gree.
Maybe we can put this test in the normal rust.yaml that runs on each PR for a while until we are sure it is stable π€
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,84 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Snapshot testing
Review Comment:
Could you please put these instructions in the existing https://datafusion.apache.org/contributor-guide/testing.html instead?
And then perhaps you can add a link to that page in `datafusion-cli/CONTRIBUTING.md`
(this is partly so when we start using insta for other tests, we can reuse the same instructions)
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,72 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Snapshot testing
+
+To test CLI output, [Insta](https://github.com/mitsuhiko/insta) is used for snapshot testing. Snapshots are generated
+and compared on each test run. If the output changes, tests will fail.
+To review the changes, you can use Insta CLI:
+
+```shell
+cargo install cargo-insta
+cargo insta review
Review Comment:
In general I worry about adding "yet another dependency / test tool" to DataFusion
However, given how painful it is to make plan output changes (e.g. see @wiedld 's PR here https://github.com/apache/datafusion/pull/15007) I think it would be really nice to add insta
##########
datafusion-cli/tests/cli_integration.rs:
##########
@@ -17,42 +17,223 @@
use std::process::Command;
-use assert_cmd::prelude::{CommandCargoExt, OutputAssertExt};
-use predicates::prelude::predicate;
use rstest::rstest;
+use aws_config::Region;
+use aws_credential_types::Credentials;
+use aws_sdk_s3::error::SdkError;
+use aws_sdk_ssooidc::config::BehaviorVersion;
+use insta::{glob, Settings};
+use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
+use std::{env, fs};
+
+fn cli() -> Command {
+ Command::new(get_cargo_bin("datafusion-cli"))
+}
+
+fn make_settings() -> Settings {
+ let mut settings = Settings::clone_current();
+ settings.set_prepend_module_to_snapshot(false);
+ settings.add_filter(r"Elapsed .* seconds\.", "[ELAPSED]");
+ settings.add_filter(r"DataFusion CLI v.*", "[CLI_VERSION]");
+ settings
+}
+
#[cfg(test)]
#[ctor::ctor]
fn init() {
// Enable RUST_LOG logging configuration for tests
let _ = env_logger::try_init();
}
-// Disabled due to https://github.com/apache/datafusion/issues/10793
-#[cfg(not(target_family = "windows"))]
#[rstest]
-#[case::exec_from_commands(
- ["--command", "select 1", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
-)]
#[case::exec_multiple_statements(
- ["--command", "select 1; select 2;", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n"
+ "statements",
+ ["--command", "select 1; select 2;", "-q"],
)]
#[case::exec_from_files(
- ["--file", "tests/data/sql.txt", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
+ "files",
+ ["--file", "tests/sql/select.sql", "-q"],
)]
#[case::set_batch_size(
- ["--command", "show datafusion.execution.batch_size", "--format", "json", "-q", "-b", "1"],
- "[{\"name\":\"datafusion.execution.batch_size\",\"value\":\"1\"}]\n"
+ "batch_size",
+ ["--command", "show datafusion.execution.batch_size", "-q", "-b", "1"],
)]
#[test]
fn cli_quick_test<'a>(
+ #[case] snapshot_name: &'a str,
#[case] args: impl IntoIterator<Item = &'a str>,
- #[case] expected: &str,
) {
- let mut cmd = Command::cargo_bin("datafusion-cli").unwrap();
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(snapshot_name);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
cmd.args(args);
- cmd.assert().stdout(predicate::eq(expected));
+
+ assert_cmd_snapshot!(cmd);
+}
+
+#[rstest]
+#[case("csv")]
+#[case("tsv")]
+#[case("table")]
+#[case("json")]
+#[case("nd-json")]
+#[case("automatic")]
+#[test]
+fn test_cli_format<'a>(#[case] format: &'a str) {
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(format);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
+ cmd.args(["--command", "select 1", "-q", "--format", format]);
+
+ assert_cmd_snapshot!(cmd);
+}
+
+async fn s3_client() -> aws_sdk_s3::Client {
+ let access_key_id =
+ env::var("AWS_ACCESS_KEY_ID").expect("AWS_ACCESS_KEY_ID is not set");
+ let secret_access_key =
+ env::var("AWS_SECRET_ACCESS_KEY").expect("AWS_SECRET_ACCESS_KEY is not set");
+
+ let endpoint_url = env::var("AWS_ENDPOINT").expect("AWS_ENDPOINT is not set");
+ let region = Region::new(env::var("AWS_REGION").unwrap_or("df-test".to_string()));
+
+ let allow_non_test_credentials = env::var("ALLOW_NON_TEST_CREDENTIALS")
+ .map(|v| v == "1")
+ .unwrap_or(false);
+
+ if allow_non_test_credentials
+ || !access_key_id.starts_with("TEST-")
+ || !secret_access_key.starts_with("TEST-")
+ {
+ panic!("Refusing with non-test credentials. Either set ALLOW_NON_TEST_CREDENTIALS=1 or add TEST- for AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY");
+ }
+
+ let creds = Credentials::new(access_key_id, secret_access_key, None, None, "test");
+ let config = aws_sdk_s3::Config::builder()
+ .credentials_provider(creds)
+ .endpoint_url(endpoint_url)
+ .region(region)
+ .behavior_version(BehaviorVersion::v2024_03_28())
+ .build();
+
+ aws_sdk_s3::Client::from_conf(config)
+}
+
+async fn create_bucket(bucket_name: &str, client: &aws_sdk_s3::Client) {
+ match client.head_bucket().bucket(bucket_name).send().await {
+ Ok(_) => {}
+ Err(SdkError::ServiceError(err))
+ if matches!(
+ err.err(),
+ aws_sdk_s3::operation::head_bucket::HeadBucketError::NotFound(_)
+ ) =>
+ {
+ client
+ .create_bucket()
+ .bucket(bucket_name)
+ .send()
+ .await
+ .expect("Failed to create bucket");
+ }
+ Err(e) => panic!("Failed to head bucket: {:?}", e),
+ }
+}
+
+async fn move_file_to_bucket(
+ from_path: &str,
+ to_path: &str,
+ bucket_name: &str,
+ client: &aws_sdk_s3::Client,
+) {
+ let body =
+ aws_sdk_s3::primitives::ByteStream::from_path(std::path::Path::new(from_path))
+ .await
+ .expect("Failed to read file");
+
+ client
+ .put_object()
+ .bucket(bucket_name)
+ .key(to_path)
+ .body(body)
+ .send()
+ .await
+ .expect("Failed to put object");
+}
+
+#[tokio::test]
+async fn test_cli() {
+ if env::var("TEST_STORAGE_INTEGRATION").is_err() {
+ eprintln!("Skipping external storages integration tests");
+ return;
+ }
+
+ let client = s3_client().await;
+ create_bucket("cli", &client).await;
+ move_file_to_bucket(
+ "../datafusion/core/tests/data/cars.csv",
+ "cars.csv",
+ "cli",
+ &client,
+ )
+ .await;
+
+ let settings = make_settings();
+ let _bound = settings.bind_to_scope();
+
+ glob!("sql/*.sql", |path| {
+ let input = fs::read_to_string(path).unwrap();
+ assert_cmd_snapshot!(cli().pass_stdin(input))
Review Comment:
> It is absolutely possible! My idea was to make it closer to slt and separate the code from the data.
FWIW I think .slt keeps the tests setup and expected output together (the slt driver is separate for sure, but most of the tests are self contained in the .slt file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#issuecomment-2701672473
Also resolved conflicts with main. I remember @alamb highlighted external data sources as a priority for this year, so I think adding tests is important π
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1986025313
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,75 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Running Storage Integration Tests
+
+By default, storage integration tests are not run. To run them you will need to set `TEST_STORAGE_INTEGRATION=1` and
+then provide the necessary configuration for that object store.
+
+For some of the tests, [snapshots](https://datafusion.apache.org/contributor-guide/testing.html#snapshot-testing) are used.
+
+### AWS
+
+To test the S3 integration against [Minio](https://github.com/minio/minio)
+
+First start up a container with Minio and load test files.
+
+```shell
+docker run -d \
Review Comment:
Did we consider having the tests themselves start the containers they need?
In Trino land this is done with testcontainers-java and it works awesomely well from developer experience perspective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1884196468
##########
datafusion-cli/tests/cli_integration.rs:
##########
@@ -17,42 +17,90 @@
use std::process::Command;
-use assert_cmd::prelude::{CommandCargoExt, OutputAssertExt};
-use predicates::prelude::predicate;
use rstest::rstest;
+use insta::{glob, Settings};
+use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
+use std::{env, fs};
+
+fn cli() -> Command {
+ Command::new(get_cargo_bin("datafusion-cli"))
+}
+
+fn make_settings() -> Settings {
+ let mut settings = Settings::clone_current();
+ settings.set_prepend_module_to_snapshot(false);
+ settings
+}
+
#[cfg(test)]
#[ctor::ctor]
fn init() {
// Enable RUST_LOG logging configuration for tests
let _ = env_logger::try_init();
}
-// Disabled due to https://github.com/apache/datafusion/issues/10793
-#[cfg(not(target_family = "windows"))]
#[rstest]
-#[case::exec_from_commands(
- ["--command", "select 1", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
-)]
#[case::exec_multiple_statements(
- ["--command", "select 1; select 2;", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n"
+ "statements",
+ ["--command", "select 1; select 2;", "-q"],
)]
#[case::exec_from_files(
- ["--file", "tests/data/sql.txt", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
+ "files",
+ ["--file", "tests/sql/select.sql", "-q"],
)]
#[case::set_batch_size(
- ["--command", "show datafusion.execution.batch_size", "--format", "json", "-q", "-b", "1"],
- "[{\"name\":\"datafusion.execution.batch_size\",\"value\":\"1\"}]\n"
+ "batch_size",
+ ["--command", "show datafusion.execution.batch_size", "-q", "-b", "1"],
)]
#[test]
fn cli_quick_test<'a>(
+ #[case] snapshot_name: &'a str,
#[case] args: impl IntoIterator<Item = &'a str>,
- #[case] expected: &str,
) {
- let mut cmd = Command::cargo_bin("datafusion-cli").unwrap();
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(snapshot_name);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
cmd.args(args);
- cmd.assert().stdout(predicate::eq(expected));
+
+ assert_cmd_snapshot!("cli_quick_test", cmd);
+}
+
+#[rstest]
+#[case("csv")]
+#[case("tsv")]
+#[case("table")]
+#[case("json")]
+#[case("nd-json")]
+#[case("automatic")]
+#[test]
+fn cli_format_test<'a>(#[case] format: &'a str) {
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(format);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
+ cmd.args(["--command", "select 1", "-q", "--format", format]);
+
+ assert_cmd_snapshot!("cli_format_test", cmd);
+}
+
+#[test]
+fn test_storage_integration() {
+ if env::var("TEST_STORAGE_INTEGRATION").is_err() {
+ eprintln!("Skipping external storages integration tests");
+ return;
+ }
+
+ let mut settings = make_settings();
+ settings.add_filter(r"Elapsed .* seconds\.", "[ELAPSED]");
+ settings.add_filter(r"DataFusion CLI v.*", "[CLI_VERSION]");
+ let _bound = settings.bind_to_scope();
+
+ glob!("sql/*.sql", |path| {
+ let input = fs::read_to_string(path).unwrap();
+ assert_cmd_snapshot!("test_storage_integration", cli().pass_stdin(input))
Review Comment:
Thank you, fixed in https://github.com/apache/datafusion/pull/13672/commits/53c9c51a1f4349bfccdae2a603f164010c6680e0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1909239704
##########
datafusion-cli/tests/cli_integration.rs:
##########
@@ -17,42 +17,223 @@
use std::process::Command;
-use assert_cmd::prelude::{CommandCargoExt, OutputAssertExt};
-use predicates::prelude::predicate;
use rstest::rstest;
+use aws_config::Region;
+use aws_credential_types::Credentials;
+use aws_sdk_s3::error::SdkError;
+use aws_sdk_ssooidc::config::BehaviorVersion;
+use insta::{glob, Settings};
+use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
+use std::{env, fs};
+
+fn cli() -> Command {
+ Command::new(get_cargo_bin("datafusion-cli"))
+}
+
+fn make_settings() -> Settings {
+ let mut settings = Settings::clone_current();
+ settings.set_prepend_module_to_snapshot(false);
+ settings.add_filter(r"Elapsed .* seconds\.", "[ELAPSED]");
+ settings.add_filter(r"DataFusion CLI v.*", "[CLI_VERSION]");
+ settings
+}
+
#[cfg(test)]
#[ctor::ctor]
fn init() {
// Enable RUST_LOG logging configuration for tests
let _ = env_logger::try_init();
}
-// Disabled due to https://github.com/apache/datafusion/issues/10793
-#[cfg(not(target_family = "windows"))]
#[rstest]
-#[case::exec_from_commands(
- ["--command", "select 1", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
-)]
#[case::exec_multiple_statements(
- ["--command", "select 1; select 2;", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n"
+ "statements",
+ ["--command", "select 1; select 2;", "-q"],
)]
#[case::exec_from_files(
- ["--file", "tests/data/sql.txt", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
+ "files",
+ ["--file", "tests/sql/select.sql", "-q"],
)]
#[case::set_batch_size(
- ["--command", "show datafusion.execution.batch_size", "--format", "json", "-q", "-b", "1"],
- "[{\"name\":\"datafusion.execution.batch_size\",\"value\":\"1\"}]\n"
+ "batch_size",
+ ["--command", "show datafusion.execution.batch_size", "-q", "-b", "1"],
)]
#[test]
fn cli_quick_test<'a>(
+ #[case] snapshot_name: &'a str,
#[case] args: impl IntoIterator<Item = &'a str>,
- #[case] expected: &str,
) {
- let mut cmd = Command::cargo_bin("datafusion-cli").unwrap();
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(snapshot_name);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
cmd.args(args);
- cmd.assert().stdout(predicate::eq(expected));
+
+ assert_cmd_snapshot!(cmd);
+}
+
+#[rstest]
+#[case("csv")]
+#[case("tsv")]
+#[case("table")]
+#[case("json")]
+#[case("nd-json")]
+#[case("automatic")]
+#[test]
+fn test_cli_format<'a>(#[case] format: &'a str) {
Review Comment:
> I think minio also supports just serving the contents of directories as s3 files
I believe this feature got removed - https://github.com/minio/minio/discussions/15496#discussioncomment-3350469
However, I did something similar and removed aws sdk - would this work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1891645256
##########
.github/workflows/rust.yml:
##########
@@ -165,22 +165,33 @@ jobs:
name: cargo test datafusion-cli (amd64)
needs: [ linux-build-lib ]
runs-on: ubuntu-latest
- container:
- image: amd64/rust
+ env:
+ AWS_ENDPOINT: http://127.0.0.1:9000
+ AWS_ACCESS_KEY_ID: TEST-DataFusionLogin
+ AWS_SECRET_ACCESS_KEY: TEST-DataFusionPassword
+ TEST_STORAGE_INTEGRATION: 1
+ AWS_ALLOW_HTTP: true
+
steps:
- uses: actions/checkout@v4
with:
submodules: true
- name: Setup Rust toolchain
- uses: ./.github/actions/setup-builder
- with:
- rust-version: stable
- - name: Run tests (excluding doctests)
run: |
- cd datafusion-cli
- cargo test --lib --tests --bins --all-features
+ rustup toolchain install stable
+ rustup default stable
+ - name: Setup Minio - S3-compatible storage
+ working-directory: datafusion-cli
+ run:
+ echo "MINIO_CONTAINER=$(docker run -d -p 9000:9000 -e MINIO_ROOT_USER=TEST-DataFusionLogin -e MINIO_ROOT_PASSWORD=TEST-DataFusionPassword quay.io/minio/minio server /data)" >> $GITHUB_ENV
+ - name: Run tests (excluding doctests, but with integration tests)
+ working-directory: datafusion-cli
+ run: cargo test --lib --tests --bins --all-features
Review Comment:
@alamb you sugessting copying `object_store` [approach for testing](https://github.com/apache/arrow-rs/blob/9047d99f6bf87582532ee6ed0acb3f2d5f889f11/.github/workflows/object_store.yml#L91-L184).
In `object_store`, they use Localstack for S3 simulation. It works fine for testing, but the problem is that [it doesn't actually validate the credentials](https://github.com/localstack/localstack/blob/master/localstack-core/localstack/services/s3/provider.py#L840-L842).
In another part of `object_store`, [Minio is used](https://github.com/apache/arrow-rs/blob/fc814bca6b743010bcde972e611c8ff8ea68c9f5/object_store/CONTRIBUTING.md?plain=1#L108), and it *does* validate credentials. So, I think we should switch to using Minio for testing here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1982759763
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,72 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Snapshot testing
+
+To test CLI output, [Insta](https://github.com/mitsuhiko/insta) is used for snapshot testing. Snapshots are generated
+and compared on each test run. If the output changes, tests will fail.
+To review the changes, you can use Insta CLI:
+
+```shell
+cargo install cargo-insta
+cargo insta review
Review Comment:
I love this idea. Thank you! β€οΈ
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1884200136
##########
datafusion-cli/tests/integration_setup.bash:
##########
@@ -0,0 +1,16 @@
+# you should have localstack up, e.g by
+#$ LOCALSTACK_VERSION=sha256:a0b79cb2430f1818de2c66ce89d41bba40f5a1823410f5a7eaf3494b692eed97
+#$ podman run -d -p 4566:4566 localstack/localstack@$LOCALSTACK_VERSION
Review Comment:
I initially stole it from https://github.com/apache/arrow-rs/blob/9047d99f6bf87582532ee6ed0acb3f2d5f889f11/object_store/CONTRIBUTING.md?plain=1#L43 - but yes, made consistent now π€
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#issuecomment-2558027702
> The test is not run automatically in CI and thus someone has to remember to run it. I predict this means it sill slowly bit rot (aka not get run and break and no one will notice)
I believe it does work in the CI (hence so many commits in this PR to make it work π)
https://github.com/apache/datafusion/actions/runs/12435578988/job/34722247404?pr=13672
<img width="876" alt="Firefox 2024-12-21 09 59 34" src="https://github.com/user-attachments/assets/60abdb4e-4bbd-4aa9-8e2c-cd495196a67e" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1884176053
##########
datafusion-cli/tests/snapshots/cli_format_test@automatic.snap:
##########
@@ -0,0 +1,21 @@
+---
+source: tests/cli_integration.rs
+info:
+ program: datafusion-cli
+ args:
+ - "--command"
+ - select 1
+ - "-q"
+ - "--format"
+ - automatic
+---
+success: true
+exit_code: 0
+----- stdout -----
++----------+
+| Int64(1) |
Review Comment:
Added docs in https://github.com/apache/datafusion/blob/9cb1c99e9cef22d7b8868820cde22f46f33a243d/datafusion-cli/CONTRIBUTING.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1874533180
##########
datafusion-cli/tests/integration_setup.bash:
##########
@@ -0,0 +1,16 @@
+# you should have localstack up, e.g by
+#$ LOCALSTACK_VERSION=sha256:a0b79cb2430f1818de2c66ce89d41bba40f5a1823410f5a7eaf3494b692eed97
+#$ podman run -d -p 4566:4566 localstack/localstack@$LOCALSTACK_VERSION
Review Comment:
arguably docker is kind of more standard way.
also, would it be possible not to require manual copy-pasting of the commmands?
##########
datafusion-cli/tests/snapshots/cli_format_test@automatic.snap:
##########
@@ -0,0 +1,21 @@
+---
+source: tests/cli_integration.rs
+info:
+ program: datafusion-cli
+ args:
+ - "--command"
+ - select 1
+ - "-q"
+ - "--format"
+ - automatic
+---
+success: true
+exit_code: 0
+----- stdout -----
++----------+
+| Int64(1) |
Review Comment:
How are these files generated / updated?
##########
datafusion-cli/tests/cli_integration.rs:
##########
@@ -17,42 +17,90 @@
use std::process::Command;
-use assert_cmd::prelude::{CommandCargoExt, OutputAssertExt};
-use predicates::prelude::predicate;
use rstest::rstest;
+use insta::{glob, Settings};
+use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
+use std::{env, fs};
+
+fn cli() -> Command {
+ Command::new(get_cargo_bin("datafusion-cli"))
+}
+
+fn make_settings() -> Settings {
+ let mut settings = Settings::clone_current();
+ settings.set_prepend_module_to_snapshot(false);
+ settings
+}
+
#[cfg(test)]
#[ctor::ctor]
fn init() {
// Enable RUST_LOG logging configuration for tests
let _ = env_logger::try_init();
}
-// Disabled due to https://github.com/apache/datafusion/issues/10793
-#[cfg(not(target_family = "windows"))]
#[rstest]
-#[case::exec_from_commands(
- ["--command", "select 1", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
-)]
#[case::exec_multiple_statements(
- ["--command", "select 1; select 2;", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n"
+ "statements",
+ ["--command", "select 1; select 2;", "-q"],
)]
#[case::exec_from_files(
- ["--file", "tests/data/sql.txt", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
+ "files",
+ ["--file", "tests/sql/select.sql", "-q"],
)]
#[case::set_batch_size(
- ["--command", "show datafusion.execution.batch_size", "--format", "json", "-q", "-b", "1"],
- "[{\"name\":\"datafusion.execution.batch_size\",\"value\":\"1\"}]\n"
+ "batch_size",
+ ["--command", "show datafusion.execution.batch_size", "-q", "-b", "1"],
)]
#[test]
fn cli_quick_test<'a>(
+ #[case] snapshot_name: &'a str,
#[case] args: impl IntoIterator<Item = &'a str>,
- #[case] expected: &str,
) {
- let mut cmd = Command::cargo_bin("datafusion-cli").unwrap();
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(snapshot_name);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
cmd.args(args);
- cmd.assert().stdout(predicate::eq(expected));
+
+ assert_cmd_snapshot!("cli_quick_test", cmd);
+}
+
+#[rstest]
+#[case("csv")]
+#[case("tsv")]
+#[case("table")]
+#[case("json")]
+#[case("nd-json")]
+#[case("automatic")]
+#[test]
+fn cli_format_test<'a>(#[case] format: &'a str) {
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(format);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
+ cmd.args(["--command", "select 1", "-q", "--format", format]);
+
+ assert_cmd_snapshot!("cli_format_test", cmd);
+}
+
+#[test]
+fn test_storage_integration() {
+ if env::var("TEST_STORAGE_INTEGRATION").is_err() {
+ eprintln!("Skipping external storages integration tests");
+ return;
+ }
+
+ let mut settings = make_settings();
+ settings.add_filter(r"Elapsed .* seconds\.", "[ELAPSED]");
+ settings.add_filter(r"DataFusion CLI v.*", "[CLI_VERSION]");
+ let _bound = settings.bind_to_scope();
+
+ glob!("sql/*.sql", |path| {
+ let input = fs::read_to_string(path).unwrap();
+ assert_cmd_snapshot!("test_storage_integration", cli().pass_stdin(input))
Review Comment:
would be nice to pull "test_storage_integration" out of the test name automatically, to avoid copy-paste mistakes.
fwiw such functionality exists in the goldie crate (that's also for output testing), and is as simple as
```rust
#[macro_export]
macro_rules! current_function_plain_name {
() => {{
fn f() {}
fn type_name_of_val<T>(_: T) -> &'static str {
::std::any::type_name::<T>()
}
let mut name = type_name_of_val(f).strip_suffix("::f").unwrap_or("");
while let Some(rest) = name.strip_suffix("::{{closure}}") {
name = rest;
}
name.split("::").last().unwrap_or(name)
}};
}
```
(admittedly hacky, but quite helpful)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13672:
URL: https://github.com/apache/datafusion/pull/13672
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1894575135
##########
datafusion-cli/tests/cli_integration.rs:
##########
@@ -17,42 +17,223 @@
use std::process::Command;
-use assert_cmd::prelude::{CommandCargoExt, OutputAssertExt};
-use predicates::prelude::predicate;
use rstest::rstest;
+use aws_config::Region;
+use aws_credential_types::Credentials;
+use aws_sdk_s3::error::SdkError;
+use aws_sdk_ssooidc::config::BehaviorVersion;
+use insta::{glob, Settings};
+use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
+use std::{env, fs};
+
+fn cli() -> Command {
+ Command::new(get_cargo_bin("datafusion-cli"))
+}
+
+fn make_settings() -> Settings {
+ let mut settings = Settings::clone_current();
+ settings.set_prepend_module_to_snapshot(false);
+ settings.add_filter(r"Elapsed .* seconds\.", "[ELAPSED]");
+ settings.add_filter(r"DataFusion CLI v.*", "[CLI_VERSION]");
+ settings
+}
+
#[cfg(test)]
#[ctor::ctor]
fn init() {
// Enable RUST_LOG logging configuration for tests
let _ = env_logger::try_init();
}
-// Disabled due to https://github.com/apache/datafusion/issues/10793
-#[cfg(not(target_family = "windows"))]
#[rstest]
-#[case::exec_from_commands(
- ["--command", "select 1", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
-)]
#[case::exec_multiple_statements(
- ["--command", "select 1; select 2;", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n"
+ "statements",
+ ["--command", "select 1; select 2;", "-q"],
)]
#[case::exec_from_files(
- ["--file", "tests/data/sql.txt", "--format", "json", "-q"],
- "[{\"Int64(1)\":1}]\n"
+ "files",
+ ["--file", "tests/sql/select.sql", "-q"],
)]
#[case::set_batch_size(
- ["--command", "show datafusion.execution.batch_size", "--format", "json", "-q", "-b", "1"],
- "[{\"name\":\"datafusion.execution.batch_size\",\"value\":\"1\"}]\n"
+ "batch_size",
+ ["--command", "show datafusion.execution.batch_size", "-q", "-b", "1"],
)]
#[test]
fn cli_quick_test<'a>(
+ #[case] snapshot_name: &'a str,
#[case] args: impl IntoIterator<Item = &'a str>,
- #[case] expected: &str,
) {
- let mut cmd = Command::cargo_bin("datafusion-cli").unwrap();
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(snapshot_name);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
cmd.args(args);
- cmd.assert().stdout(predicate::eq(expected));
+
+ assert_cmd_snapshot!(cmd);
+}
+
+#[rstest]
+#[case("csv")]
+#[case("tsv")]
+#[case("table")]
+#[case("json")]
+#[case("nd-json")]
+#[case("automatic")]
+#[test]
+fn test_cli_format<'a>(#[case] format: &'a str) {
+ let mut settings = make_settings();
+ settings.set_snapshot_suffix(format);
+ let _bound = settings.bind_to_scope();
+
+ let mut cmd = cli();
+ cmd.args(["--command", "select 1", "-q", "--format", format]);
+
+ assert_cmd_snapshot!(cmd);
+}
+
+async fn s3_client() -> aws_sdk_s3::Client {
+ let access_key_id =
+ env::var("AWS_ACCESS_KEY_ID").expect("AWS_ACCESS_KEY_ID is not set");
+ let secret_access_key =
+ env::var("AWS_SECRET_ACCESS_KEY").expect("AWS_SECRET_ACCESS_KEY is not set");
+
+ let endpoint_url = env::var("AWS_ENDPOINT").expect("AWS_ENDPOINT is not set");
+ let region = Region::new(env::var("AWS_REGION").unwrap_or("df-test".to_string()));
+
+ let allow_non_test_credentials = env::var("ALLOW_NON_TEST_CREDENTIALS")
+ .map(|v| v == "1")
+ .unwrap_or(false);
+
+ if allow_non_test_credentials
+ || !access_key_id.starts_with("TEST-")
+ || !secret_access_key.starts_with("TEST-")
+ {
+ panic!("Refusing with non-test credentials. Either set ALLOW_NON_TEST_CREDENTIALS=1 or add TEST- for AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY");
+ }
+
+ let creds = Credentials::new(access_key_id, secret_access_key, None, None, "test");
+ let config = aws_sdk_s3::Config::builder()
+ .credentials_provider(creds)
+ .endpoint_url(endpoint_url)
+ .region(region)
+ .behavior_version(BehaviorVersion::v2024_03_28())
+ .build();
+
+ aws_sdk_s3::Client::from_conf(config)
+}
+
+async fn create_bucket(bucket_name: &str, client: &aws_sdk_s3::Client) {
+ match client.head_bucket().bucket(bucket_name).send().await {
+ Ok(_) => {}
+ Err(SdkError::ServiceError(err))
+ if matches!(
+ err.err(),
+ aws_sdk_s3::operation::head_bucket::HeadBucketError::NotFound(_)
+ ) =>
+ {
+ client
+ .create_bucket()
+ .bucket(bucket_name)
+ .send()
+ .await
+ .expect("Failed to create bucket");
+ }
+ Err(e) => panic!("Failed to head bucket: {:?}", e),
+ }
+}
+
+async fn move_file_to_bucket(
+ from_path: &str,
+ to_path: &str,
+ bucket_name: &str,
+ client: &aws_sdk_s3::Client,
+) {
+ let body =
+ aws_sdk_s3::primitives::ByteStream::from_path(std::path::Path::new(from_path))
+ .await
+ .expect("Failed to read file");
+
+ client
+ .put_object()
+ .bucket(bucket_name)
+ .key(to_path)
+ .body(body)
+ .send()
+ .await
+ .expect("Failed to put object");
+}
+
+#[tokio::test]
+async fn test_cli() {
+ if env::var("TEST_STORAGE_INTEGRATION").is_err() {
+ eprintln!("Skipping external storages integration tests");
+ return;
+ }
+
+ let client = s3_client().await;
+ create_bucket("cli", &client).await;
+ move_file_to_bucket(
+ "../datafusion/core/tests/data/cars.csv",
+ "cars.csv",
+ "cli",
+ &client,
+ )
+ .await;
+
+ let settings = make_settings();
+ let _bound = settings.bind_to_scope();
+
+ glob!("sql/*.sql", |path| {
+ let input = fs::read_to_string(path).unwrap();
+ assert_cmd_snapshot!(cli().pass_stdin(input))
Review Comment:
It is absolutely possible! My idea was to make it closer to `slt` and separate the code from the data. Setting up the suite is quite hard (we need to create a new user, upload a test file, etc.), but with the current approach, we only need to do it once rather than for every single test.
However, I'm happy to make changes if you'd prefer it to be inline?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1984964451
##########
.github/workflows/extended.yml:
##########
@@ -67,25 +67,45 @@ jobs:
fetch-depth: 1
- name: Free Disk Space (Ubuntu)
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be
- - name: Install Rust
+ - name: Setup Rust toolchain
run: |
- curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
- source $HOME/.cargo/env
+ rustup toolchain install stable
rustup toolchain install
- name: Install Protobuf Compiler
run: sudo apt-get install -y protobuf-compiler
+ - name: Setup Minio - S3-compatible storage
Review Comment:
Fair! I've added them back to the rust flow. Fells like testing on the existing linux check should be enough (to save time) but happy to add to mac os / move to a new check if preferred
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1984937074
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,72 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Snapshot testing
+
+To test CLI output, [Insta](https://github.com/mitsuhiko/insta) is used for snapshot testing. Snapshots are generated
+and compared on each test run. If the output changes, tests will fail.
+To review the changes, you can use Insta CLI:
+
+```shell
+cargo install cargo-insta
+cargo insta review
Review Comment:
Nice!! I really don't like 3k-lines files (like in `enforce_distribution.rs`) so IMO would be really good to split those separate snapshots... Will create an issue to discuss after this PR is merged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1984964451
##########
.github/workflows/extended.yml:
##########
@@ -67,25 +67,45 @@ jobs:
fetch-depth: 1
- name: Free Disk Space (Ubuntu)
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be
- - name: Install Rust
+ - name: Setup Rust toolchain
run: |
- curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
- source $HOME/.cargo/env
+ rustup toolchain install stable
rustup toolchain install
- name: Install Protobuf Compiler
run: sudo apt-get install -y protobuf-compiler
+ - name: Setup Minio - S3-compatible storage
Review Comment:
Fair! I've added them back to the rust flow. Fells like testing on linux should be enough (to save time) but happy to add to mac os too if you prefer having it tested everywhere
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#discussion_r1985792488
##########
datafusion-cli/CONTRIBUTING.md:
##########
@@ -0,0 +1,75 @@
+<!---
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# Development instructions
+
+## Running Tests
+
+Tests can be run using `cargo`
+
+```shell
+cargo test
+```
+
+## Running Storage Integration Tests
+
+By default, storage integration tests are not run. To run them you will need to set `TEST_STORAGE_INTEGRATION=1` and
+then provide the necessary configuration for that object store.
+
+For some of the tests, [snapshots](https://datafusion.apache.org/contributor-guide/testing.html#snapshot-testing) are used.
+
+### AWS
+
+To test the S3 integration against [Minio](https://github.com/minio/minio)
Review Comment:
I tested the instructions locally and they worked great π
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `insta` / snapshot testing to CLI & set up AWS mock [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13672:
URL: https://github.com/apache/datafusion/pull/13672#issuecomment-2714655356
Thanks again @blaginin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org