Add `insta` / snapshot testing to CLI & set up AWS mock #13672

blaginin · 2024-12-05T22:40:32Z

Which issue does this PR close?

Related to [DISCUSSION] Make it easier and faster to query remote files (S3, iceberg, etc) #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.

datafusion-cli/tests/integration_setup.bash

datafusion-cli/tests/snapshots/[email protected]

datafusion-cli/tests/cli_integration.rs

# Conflicts: # datafusion-cli/Cargo.lock

# Conflicts: # .github/workflows/extended.yml

blaginin · 2025-03-05T17:53:12Z

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

blaginin · 2025-03-05T17:55:08Z

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 👊

alamb

Thank you @blaginin -- I think this PR is a nice improvement

In my opinion, switching the Ci tests to run on each PR for a while to make sure they are passing is required prior to merge

I think both inline snapshots and moving the docs are important, but we could do them as a follow on PR

alamb · 2025-03-05T21:15:34Z

.github/workflows/extended.yml

          rustup toolchain install
      - name: Install Protobuf Compiler
        run: sudo apt-get install -y protobuf-compiler
+      - name: Setup Minio - S3-compatible storage


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 🤔

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

.github/workflows/extended.yml

alamb · 2025-03-05T21:17:50Z

datafusion-cli/CONTRIBUTING.md

+
+```shell
+cargo install cargo-insta
+cargo insta review


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 #15007) I think it would be really nice to add insta

alamb · 2025-03-05T21:19:23Z

datafusion-cli/CONTRIBUTING.md

+cargo test
+```
+
+## Snapshot testing


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)

alamb · 2025-03-05T21:21:43Z

datafusion-cli/tests/cli_integration.rs

+
+    glob!("sql/*.sql", |path| {
+        let input = fs::read_to_string(path).unwrap();
+        assert_cmd_snapshot!(cli().pass_stdin(input))


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

alamb · 2025-03-07T22:15:30Z

.github/workflows/extended.yml

-          curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
-          source $HOME/.cargo/env
-          rustup toolchain install
+      - name: Setup Rust toolchain


I think this will fail because (as I found out earlier in the week) the ubuntu builder doens't start with rust

alamb · 2025-03-07T22:24:24Z

datafusion-cli/CONTRIBUTING.md

+
+### AWS
+
+To test the S3 integration against [Minio](https://github.com/minio/minio)


I tested the instructions locally and they worked great 👍

alamb

Thank you so much @blaginin -- I think this PR looks really good and will make DataFusion testing better and easier

I expect over time the insta style snapshot tests will become widely used (and loved!)

I also verified that

cargo test -p datafusion-cli

works without running docker / minio and I also followed the instructions locally

I will likely try a follow on PR to try and run the datafusion-cli tests separately from the main test run to make things clearer but this looks 👍

Thanks again for sticking with it

findepi · 2025-03-08T08:01:39Z

datafusion-cli/CONTRIBUTING.md

+First start up a container with Minio and load test files.
+
+```shell
+docker run -d \


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.

I think it’s a nice improvement if tests just handle the setup automatically! Created an issue to discuss: #15092

alamb · 2025-03-09T12:09:56Z

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)

alamb · 2025-03-11T14:09:11Z

I merged up to resolve conflicts and plan to merge this when the tests pass

alamb · 2025-03-11T14:57:47Z

Thanks again @blaginin

blaginin added 7 commits November 26, 2024 22:32

Do not normalize values

2887bb9

Fix tests & update docs

813d634

Prettier

c3de620

Merge branch 'main' into bugfix/do-not-normalize-values

0e11d36

Merge branch 'main' into bugfix/do-not-normalize-values

cb7da8c

Lowercase config params

7c2b3fe

Add snap to CLI & set up AWS mock

9146e4b

blaginin changed the title ~~Add snap to CLI & set up AWS mock~~ Add snapshot testing to CLI & set up AWS mock Dec 6, 2024

Refactor tests

9d856c3

findepi reviewed Dec 7, 2024

View reviewed changes

datafusion-cli/tests/integration_setup.bash Outdated Show resolved Hide resolved

datafusion-cli/tests/snapshots/[email protected] Outdated Show resolved Hide resolved

datafusion-cli/tests/cli_integration.rs Outdated Show resolved Hide resolved

blaginin added 6 commits December 9, 2024 15:29

Unify transform and parse

0574ab8

Merge branch 'main' into bugfix/do-not-normalize-values

2045495

Fix tests

06c013d

Merge branch 'main' into cli-snap

4870219

Merge branch 'bugfix/do-not-normalize-values' into cli-snap

a02625e

Setup CLI

65809a7

github-actions bot added documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Dec 10, 2024

blaginin added 8 commits December 10, 2024 18:05

Show minio output

05a562f

Format Cargo.toml

36f8550

Do not hardcode AWS params

921f229

Test options parsing

107c515

Add allow http

2d430a0

Fix aws build

4b56506

Merge branch 'main' into cli-snap

1581e7a

# Conflicts: # datafusion-cli/Cargo.lock

Fix ip

ad9734e

blaginin added 4 commits March 5, 2025 13:19

Merge branch 'main' into cli-snap

3c2ead2

# Conflicts: # .github/workflows/extended.yml

Fix backslash test

7922db7

Add missing backslash

348e881

put integration scripts in a separate folder

4238b41

alamb reviewed Mar 5, 2025

View reviewed changes

alamb changed the title ~~Add snapshot testing to CLI & set up AWS mock~~ Add insta / snapshot testing to CLI & set up AWS mock Mar 6, 2025

alamb mentioned this pull request Mar 6, 2025

Refactor test suite in EnforceDistribution, to use standard test config. #15010

Merged

wiedld mentioned this pull request Mar 6, 2025

Auto-update mechanism for dataframe test #10373

Closed

blaginin added 2 commits March 7, 2025 12:16

Move s3 tests from extended to rust flow

a880c42

Reorganise the docs

5f80368

github-actions bot added the documentation Improvements or additions to documentation label Mar 7, 2025

blaginin and others added 4 commits March 7, 2025 12:33

Prettier

5cafb77

Do not use rust container to get docker

7e3ed3f

Add missing protobuf

194ce80

Merge remote-tracking branch 'apache/main' into cli-snap

631a7bd

alamb reviewed Mar 7, 2025

View reviewed changes

revert change to extended.yml

fc81ac9

alamb reviewed Mar 7, 2025

View reviewed changes

alamb approved these changes Mar 7, 2025

View reviewed changes

alamb mentioned this pull request Mar 7, 2025

Minor: split datafusion-cli testing into its own CI job #15075

Merged

findepi reviewed Mar 8, 2025

View reviewed changes

blaginin mentioned this pull request Mar 8, 2025

Auto run docker containers needed for tests #15092

Open

Merge remote-tracking branch 'apache/main' into cli-snap

83f81c5

alamb merged commit 51f0833 into apache:main Mar 11, 2025
28 checks passed

blaginin mentioned this pull request Mar 17, 2025

Migrate user_defined tests to insta #15255

Merged


		### AWS

		To test the S3 integration against [Minio](https://github.com/minio/minio)

Add insta / snapshot testing to CLI & set up AWS mock #13672

Add insta / snapshot testing to CLI & set up AWS mock #13672

Uh oh!

Conversation

blaginin commented Dec 5, 2024 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blaginin commented Mar 5, 2025

Uh oh!

blaginin commented Mar 5, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blaginin Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 9, 2025

Uh oh!

alamb commented Mar 11, 2025

Uh oh!

Uh oh!

alamb commented Mar 11, 2025

Uh oh!

Uh oh!

Add `insta` / snapshot testing to CLI & set up AWS mock #13672

Add `insta` / snapshot testing to CLI & set up AWS mock #13672

blaginin commented Dec 5, 2024 •

edited by alamb

Loading

blaginin Mar 7, 2025 •

edited

Loading