-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add insta
/ snapshot testing to CLI & set up AWS mock
#13672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 30 commits
2887bb9
813d634
c3de620
0e11d36
cb7da8c
7c2b3fe
9146e4b
9d856c3
0574ab8
2045495
06c013d
4870219
a02625e
65809a7
05a562f
36f8550
921f229
107c515
2d430a0
4b56506
1581e7a
ad9734e
0b24daa
5f29c00
a3826d4
9cb1c99
53c9c51
eb87f60
855315f
fd3240d
17e8486
7d4050d
fe2d345
c037577
7293add
680b474
3c2ead2
7922db7
348e881
4238b41
a880c42
5f80368
5cafb77
7e3ed3f
194ce80
631a7bd
fc81ac9
83f81c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 (this is partly so when we start using insta for other tests, we can reuse the same instructions) |
||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we use this in influxdb_iox and I find it super useful There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I love this idea. Thank you! ❤️ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice!! I really don't like 3k-lines files (like in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
``` | ||
|
||
## 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. | ||
|
||
### AWS | ||
|
||
To test the S3 integration against [Minio](https://github.com/minio/minio) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested the instructions locally and they worked great 👍 |
||
|
||
First start up a container with Minio | ||
|
||
``` | ||
$ docker run -d -p 9000:9000 -e MINIO_ROOT_USER=TEST-DataFusionLogin -e MINIO_ROOT_PASSWORD=TEST-DataFusionPassword quay.io/minio/minio server /data | ||
``` | ||
|
||
Setup environment | ||
|
||
``` | ||
export TEST_STORAGE_INTEGRATION=1 | ||
export AWS_ACCESS_KEY_ID=TEST-DataFusionLogin | ||
export AWS_SECRET_ACCESS_KEY=TEST-DataFusionPassword | ||
export AWS_ENDPOINT=http://127.0.0.1:9000 | ||
export AWS_ALLOW_HTTP=true | ||
``` | ||
|
||
Note that `AWS_ENDPOINT` is set without slash at the end. | ||
|
||
Run tests | ||
|
||
``` | ||
$ cargo test | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.