-
Notifications
You must be signed in to change notification settings - Fork 117
[feat] Enable running containers inside the test pipeline #853
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
Conversation
|
Hello @rsarm, Thank you for updating! Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide Comment last updated at 2019-10-14 10:57:58 UTC |
vkarak
left a comment
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.
Well done @rsarm! Works fine. I have some minor comments regarding this PR. I will also mark it as WIP, because ideally I would like to have the configuration part and the documentation also part of this before merging. Also, we should find a way to better test the container-specific code inside the pipeline. Perhaps, install Docker in the Travis and have a unit test that will use the example test file you mention in this PR?
|
@rsarm Can you also merge with master to get the latest changes and resolve the conflicts? |
Codecov Report
@@ Coverage Diff @@
## master #853 +/- ##
==========================================
- Coverage 91.88% 91.82% -0.06%
==========================================
Files 80 80
Lines 10481 10674 +193
==========================================
+ Hits 9630 9801 +171
- Misses 851 873 +22
Continue to review full report at Codecov.
|
vkarak
left a comment
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.
The unit tests need to extended:
- You need to a test for the
emit_prepare_cmds(). - You need to test the part of
pipeline.pythat handles the containers.
vkarak
left a comment
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.
There are still a couple of things to fix:
- Unit tests, some comments are not resolved yet.
- I don't like that the
emit_*_cmds()are inconsistent. One returns a string, while all the others return a list. - Other minor stuff.
I will fix these issues.
vkarak
left a comment
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.
@rsarm This PR is now almost ready to go. Only remaining issues: I get a strange error with Singularity unit tests on Daint + the missing Sarus module in Dom config, which I have removed.
…ontainers-pipeline
…ontainers-pipeline
|
This PR is now ready. Thanks @rsarm. I am disabling the Singularity unit test if running on Cray system with CLE6. |
Fixes #644.
This is a simple test to try this feature.
I also added a
--rmto thedocker runcommand after discussing with @teojgo. Without this the container stays on statusstoppedafter the test ends.--rmtakes cares of completely clean the container after the command exits.