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 |
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.