-
-
Notifications
You must be signed in to change notification settings - Fork 31
adding test for entrypoint+cmd and adding proper testing requirements #129
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
… (will need to be in conda too) Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
|
|
||
| TESTS_REQUIRES = ( | ||
| ('pytest', {'min_version': '4.6.2'}), | ||
| ('pytest-cov', {'min_version': '2.5.1'}), |
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.
Should not be required
But will be needed on CI if coverage is added
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.
So thinking ahead! Without realizing it :)
| TESTS_REQUIRES = ( | ||
| ('pytest', {'min_version': '4.6.2'}), | ||
| ('pytest-cov', {'min_version': '2.5.1'}), | ||
| ('pytest-fixtures', {'min_version': '0.1.0'}), |
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.
Not required
| ) | ||
|
|
||
| TESTS_REQUIRES = ( | ||
| ('pytest', {'min_version': '4.6.2'}), |
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.
Something like 4.0.0 should already be enough
| Bootstrap: docker | ||
| From: busybox:latest | ||
| %runscript | ||
| exec python /code/script.py "$@" |
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.
This is an inexact translation of the Dockerfile. See #111 (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.
Yes but this is what works and is expected for Singularity. You don't see users writing /bin/bash -c "..." as this is handled internally in Singularity.
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.
Completely missing the point. docker run entrypoint-cmd --foo does a python --foo not a python /code/script.py --foo
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.
It will need to be fixed then.
The testing requirements were not fully represented in the setup.py. I'm worried now that there are too many additional dependencies to run basic testing, and perhaps this will come back to bite me. For the meantime the damage is already done, and I'll need to update conda deps too. Right now as is, a user only thinks that pytest is required and won't understand why many of the tests fail. This will also add a test for the entrypoint+cmd conversion in a Dockerfile to Singularity recipe.
I will need to update conda too.
Signed-off-by: Vanessa Sochat vsochat@stanford.edu