-
Notifications
You must be signed in to change notification settings - Fork 60
new feature: ch-test #447
new feature: ch-test #447
Conversation
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.
I think the permissions test should be moved to an option by itself. When I frequently am testing/rebuilding I have to redo the setup for the permissions dirs. Taking those tests out of the run segment would allow the permissions tests to be run separately, so users without root who can't run them anyway don't have to take action to skip them.
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.
Needs a tidy but sounds good to me
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.
Nice work. I proposed a revision.
Note you'll need to put it in conf.py
as well to create the man page.
As for Shane's comment, I agree that we don't want to require the user to mess around with this. I propose that ch-test
create the permissions fixtures if appropriate. I think he's right that it could go in a separate test phase, but I'd prefer to keep the three existing phases for this PR.
Aside from a minor travis fix in How this will be integrated with travis is not clear. I don't imagine doubling the test time to run the tests via |
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.
Thanks. Various suggestions in-line.
My overall impression was that it seems kind of complicated for what basically is a cd
followed by make
. However, I became less sure of that as I read on. I do think some of the suggestions could make things simpler.
This will need to be updated to include the builder, which for some has additional configuration (in the case of ch-grow
).
Do we then alter travis to run the test suite exclusively through ch-test?
Yes. I'd say this should become the sole supported way to run the tests. That may require updating the docs.
Also related to #329. |
c9ec1cd
to
b698a4c
Compare
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.
I think overall this looks solid, but I'd like to discuss and walk through it in person. Can we do that next week?
One thing I thought to add: There are currently a few setup/teardown tests, I think mostly to manage the test directories. I think these maybe should be moved to ch-test
?
Sounds good.
Agreed. |
Add check/workaround for #278? |
5fdba3d
to
1ab9dbd
Compare
doc-src/ch-test_desc.rst
Outdated
:code:`clean` | ||
Delete test artifacts on disk and all images in builder storage (whether | ||
or not Charliecloud-related), except for the permissions test fixtures. |
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.
Spicy. Not sure I'd like clean
to wipe my docker/buildah image cache. Am I misunderstanding this?
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.
Your understanding is correct. Why don't we leave it alone for now (i.e., don't wipe builder storage). I would like an easy way to do that though; our current test/docker-clean.sh
isn't very user-facing.
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.
I took this out.
doc-src/ch-test_desc.rst
Outdated
:code:`--pack-dir DIR` | ||
Set packed images directory to :code:`DIR`. Default: | ||
:code:`$CH_TEST_TARDIR` if set, or :code:`/var/tmp/pack`. |
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.
Perhaps we should go ahead and update CH_TEST_TARDIR
to CH_TEST_PACKDIR
as well? I know it is long, but I'm also wondering if CH_TEST_IMGDIR
should be CH_TEST_UNPACKDIR
.
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.
I guess the reasoning to leave the variables along is that it's a pretty pervasive change that alters an existing API, so I wanted to leave it alone for now, in case the new vocabulary is not satisfactory.
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.
Great work. Some tidy comments. Doesn't pass Travis after my changed; haven't looked into that. Perhaps we can wrap it up today and get it merged?
test/travis.bash
Outdated
cd "$PREFIX/libexec/charliecloud-$version" | ||
ch_test="${PREFIX}/bin/ch-test" | ||
else | ||
ch_test=$(readlink -f bin/ch-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.
What is the problem readlink
solves here?
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.
Removing the absolute path here will cause travis to error (https://travis-ci.org/hpc/charliecloud/jobs/604006996?utm_medium=notification&utm_source=email).
147 top_dir=$(readlink -f "${ch_bin%/*}")
[...]
173 # Determine source vs prefix installation.
174 if [[ -d "${top_dir}/test" ]]; then
175 # source install
176 testdir=${top_dir}/test
177 else
178 # prefix installation
179 testdir=${top_dir%/charliecloud*}/libexec/charliecloud-$(version 2>&1)/test
180 fi
The check for ${top_dir}/test
fails (it shouldn't, it is a source install). I feel like this is because base.sh
uses dirname "$0"
instead of dirname "${BASH_SOURCE[0]}
. When we do ch_test=$(readlink -f bin/ch-test
in the travis.bash
, the issue is resolved (https://travis-ci.org/hpc/charliecloud/jobs/604009862?utm_medium=notification&utm_source=github_status).
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.
Very nice work. If you are OK with my tidying and it passes tests, then I think this is ready to merge.
doc-src/ch-test_desc.rst
Outdated
:code:`clean` | ||
Delete test artifacts on disk and all images in builder storage (whether | ||
or not Charliecloud-related), except for the permissions test fixtures. |
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.
I took this out.
Addresses #445.