Add snapshot testing to CLI & set up AWS mock #13672
Conversation
blaginin
changed the title
| @@ -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 | |||
There was a problem hiding this comment.
arguably docker is kind of more standard way.
also, would it be possible not to require manual copy-pasting of the commmands?
There was a problem hiding this 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 🤗
| exit_code: 0 | ||
| ----- stdout ----- | ||
| +----------+ | ||
| | Int64(1) | |
There was a problem hiding this comment.
How are these files generated / updated?
There was a problem hiding this comment.
|
|
||
| glob!("sql/*.sql", |path| { | ||
| let input = fs::read_to_string(path).unwrap(); | ||
| assert_cmd_snapshot!("test_storage_integration", cli().pass_stdin(input)) |
There was a problem hiding this 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
#[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)
There was a problem hiding this comment.
Thank you, fixed in 53c9c51
github-actions
bot
added
documentation
# Conflicts: # datafusion-cli/Cargo.lock
|
This PR is ready ish for the reivew - but i want to merge #13576 first, so this one has a smaller diff |
# Conflicts: # datafusion-cli/Cargo.lock
| - 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 |
There was a problem hiding this comment.
@alamb you sugessting copying object_store approach for testing.
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.
In another part of object_store, Minio is used, and it does validate credentials. So, I think we should switch to using Minio for testing here.
# Conflicts: # datafusion/common/src/config.rs # datafusion/core/tests/config_from_env.rs # datafusion/sqllogictest/test_files/information_schema.slt # docs/source/user-guide/configs.md
github-actions
bot
removed
documentation
Which issue does this PR close?
Related to #13456 (comment)
Rationale for this change
We currently don't test whether our external integrations actually work: as a result #13576 has happened.
What changes are included in this PR?
Integration tests for S3.
Are there any user-facing changes?
No.