Skip to content
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

test: add integration test with minio #112

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

abyssnlp
Copy link
Contributor

Description

Adds integration tests with s3-compatible MinIO.

  • docker/docker-compose.yaml spins up the minio container
  • docker/copy_tables.sh creates the bucket; unzips, copies and creates the tables in the minio store
  • The environment variables for the integration test are supplied via integration_test/env.toml
  • Adds running integration tests on pull requests via ci.yml

Notes:

  • We can extract the logic to run the integration tests in the Makefile or as a separate script
  • The integration test takes around ~3 minutes to complete
  • The integration test is only for the hudi-rs rust support via datafusion

This closes #81 .

How are the changes test-covered

  • N/A
  • Automated tests (unit and/or integration tests)
  • Manual tests
    • Details are described below

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 86.82%. Comparing base (3359e10) to head (5a747fd).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/tests/src/lib.rs 0.00% 4 Missing ⚠️
integration_test/tests/hudi_test.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   87.42%   86.82%   -0.61%     
==========================================
  Files          14       15       +1     
  Lines         700      736      +36     
==========================================
+ Hits          612      639      +27     
- Misses         88       97       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xushiyan
Copy link
Member

@abyssnlp don't think the ci job is triggered. probably format issue.

@abyssnlp
Copy link
Contributor Author

@xushiyan The CI is failing due to this error:
dtolnay/rust-toolchain@1.75.0 is not allowed to be used in apache/hudi-rs. I used this action to setup the rust toolchain. Is there a way to whitelist this action or should I use an alternative?

@abyssnlp
Copy link
Contributor Author

I've removed the external action since GHA runner has rustup already.

@abyssnlp
Copy link
Contributor Author

abyssnlp commented Aug 15, 2024

@xushiyan All tests (including integration) are green. The labeler is failing due to permissions. I've made the small fix but let me know if this should be done in a different PR.

integration_test/tests/hudi_test.rs Outdated Show resolved Hide resolved
integration_test/tests/hudi_test.rs Outdated Show resolved Hide resolved
integration_test/tests/hudi_test.rs Outdated Show resolved Hide resolved
.github/workflows/pr.yml Outdated Show resolved Hide resolved
@abyssnlp
Copy link
Contributor Author

@xushiyan Addressed all comments in a28debc.

I've reused the TestTable enum but I had to add an extra method to get the s3 path as we unzip and load the tables into minio for the integration tests so we don't need to extract them.

@xushiyan xushiyan added this to the release-0.2.0 milestone Aug 20, 2024
@xushiyan xushiyan added the test label Aug 20, 2024
@xushiyan xushiyan self-assigned this Aug 20, 2024
Copy link
Member

@xushiyan xushiyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for putting this up! Some high-level feedback

  • How we plan for docker/: we can position it as a sandbox where people can easily spin up some e2e setup and run something, we may want to add more query engines later, so integration test can be part of it, serving as a validation to make sure all components are working.
  • Right now the docker setup is just providing minio, and the integ test is running natively and connecting to minio container. To align with the plan above, we should move the test into a container itself, which installs crates from the code repo.
  • The integ test should also cover a python test case by pip install hudi from the repo

find /tmp/data -type d -mindepth 1 -maxdepth 1 -exec mc cp --recursive {} store/$MINIO_TEST_BUCKET \;

echo ">>> Listing uploaded tables"
mc ls store/$MINIO_TEST_BUCKET
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this script be incorporated into Dockerfile? it's just a few commands after all. we want to keep things more consolidated

* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why create a lib ? it's just to run an application so bin works better? we don't expect another crate to depend on integ test

AWS_ACCESS_KEY_ID="minio"
AWS_SECRET_ACCESS_KEY="minio123"
AWS_ENDPOINT="http://localhost:9000"
INTEGRATION_TEST_S3_BUCKET="test-bucket"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still needed given there is s3.env ? can those 2 be consolidated?

@xushiyan
Copy link
Member

@abyssnlp are you planning to resume this work?

@abyssnlp
Copy link
Contributor Author

Hi @xushiyan,

I've been busier than usual so couldn't get around to this earlier. I've addressed your comments in 5a747fd.

  • The integration tests now run in a container
  • I'll add a test for python as well later

Let me know what you think.

@xushiyan
Copy link
Member

Ok, due to time constraint, I'll push this to the next release.

@xushiyan xushiyan modified the milestones: release-0.2.0, release-0.3.0 Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup integration test with minio
2 participants