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

Move integration test into examples #756

Merged
merged 24 commits into from
Jan 11, 2024
Merged

Conversation

mrchtr
Copy link
Contributor

@mrchtr mrchtr commented Jan 3, 2024

  • moved integration test into examples folder
  • adjust sample implementation to new interface and how a user would do it
  • added script to execute the pipeline using fondant cli
  • add execution to ci/cd pipeline

For now I've used a bash script for the execution. If we want to add more integration test we should think about a different approach.

Fixes #727

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @mrchtr!

scripts/run_sample_pipeline.sh Outdated Show resolved Hide resolved
examples/sample_pipeline.py Outdated Show resolved Hide resolved
scripts/run_sample_pipeline.sh Outdated Show resolved Hide resolved
.github/workflows/pipeline.yaml Outdated Show resolved Hide resolved
scripts/run_sample_pipeline.sh Outdated Show resolved Hide resolved
scripts/run_sample_pipeline.sh Outdated Show resolved Hide resolved
@RobbeSneyders
Copy link
Member

As discussed, we need to check each step in the propagation chain of the exit codes:

  • Docker container to docker compose: Check current behavior and --abort-on-container-exit flag
  • Docker compose to Fondant: We should at least use check_call when running docker compose here
  • Fondant to test script: I think we need to reraise the exit code as mentioned in my comment above

@mrchtr
Copy link
Contributor Author

mrchtr commented Jan 10, 2024

I've updated the code.

Docker container to docker compose: Check current behavior and --abort-on-container-exit flag
I've checked it and it is not needed. When we use check_call instead of call in the local runners code, the exception is the same.

I've updated the test script. The sample pipeline is located in examples/sample_pipeline. I've adjusted the code of the pipeline that is behaves as a user would expect it. You can go into the folder and run fondant run local pipeline.py for quickly test the pipeline.

Beside that in the folder is a run.sh now. Which is used to execute the sample pipeline within the ci/cd pipeline. It exits with the exit code of the pipeline execution.

I've adopted the ci/cd script. Now it scans the example folders and looks for run.sh scripts, execute each of them and tracks the exit codes. When one of the scripts failed, it will print the failed ones and exit with 1. Otherwise exit 0 when every test passed.

This allows us to add more integration test samples to the example folder in the future.

@RobbeSneyders
Copy link
Member

  • Docker container to docker compose: Check current behavior and --abort-on-container-exit flag

I think we are still missing this

@mrchtr mrchtr force-pushed the feature/move-integration-test branch from c774827 to 555a7a8 Compare January 11, 2024 14:18
@mrchtr
Copy link
Contributor Author

mrchtr commented Jan 11, 2024

  • Docker container to docker compose: Check current behavior and --abort-on-container-exit flag

I think we are still missing this

A bit of back and forth. In the meanwhile--abort-on-container-exit made it to main. I've merge latest main into this branch. Should be up to date now.

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Yeah I missed your comment about it not being needed before, my mistake. Looks good now.

@RobbeSneyders RobbeSneyders merged commit f961b3d into main Jan 11, 2024
9 checks passed
@RobbeSneyders RobbeSneyders deleted the feature/move-integration-test branch January 11, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move pipeline integration test to example
2 participants