-
Notifications
You must be signed in to change notification settings - Fork 157
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
add test suite based on conu library #167
Conversation
@TomasTomecek wrote:
Explicit test make target seems to be fine to me, if this is going to be shared among containers then it should belong to common.mk.. so @omron93 can run those tests in our jenkins(es). |
It would be nice if the test cases lived inside the global |
Makefile
Outdated
@@ -1,5 +1,7 @@ | |||
# Variables are documented in common/build.sh. | |||
BASE_IMAGE_NAME = ruby | |||
TEST_IMAGE_NAME := $(BASE_IMAGE_NAME)-tests | |||
TEST_TARGET := 2.4/test/test_s2i.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.
Yes, it would be nice to test all in an automated way. And also:
# Variables are documented in common/build.sh.
Run these tests explicitly? (separate make target) IMHO If we want to move logic for conu tests in read-only common dir, then test targets have to be stored in a file in image repo. What's the best name? Or is there any other way how to list all conu tests to run? |
I won't have time to work on this PR in the current week. At the same time, I'm also waiting for you to agree on structure and will follow with the updates afterwards. |
2.4/test/test_s2i.py
Outdated
finally: | ||
c.stop() | ||
c.wait() | ||
c.delete() |
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 workflow of wait
, assert
, stop
, wait
, delete
seems to be to be used pretty often. We might have a method image.check_output(["ruby", "--version"], "ruby 2.4.")
. And the whole test case could be shortened to:
backend.ImageClass(image_name).check_output(command=["ruby", "--version"], matches="*ruby 2.4.*")
...or something along those lines..
Related: sclorg/s2i-ruby-container#167 Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
also in multiple environments Related: sclorg/s2i-ruby-container#167 Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
also in multiple environments Related: sclorg/s2i-ruby-container#167 Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
00b71a4
to
35f64c7
Compare
35f64c7
to
43e0d9d
Compare
Can one of the admins verify this patch? |
2.3/test/test_s2i.py
Outdated
@@ -0,0 +1 @@ | |||
../../2.4/test/test_s2i.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, I'd say, why we have container-common-scripts
repo. I'd rather not symlink between versions.
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.
Or just put it into $ROOT/test
and symlink from the versioned directories.
Makefile
Outdated
@@ -1,5 +1,6 @@ | |||
# Variables are documented in common/build.sh. | |||
BASE_IMAGE_NAME = ruby | |||
TEST_SUITES=test/run-conu |
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 would effectively disable test/run
test suite, right?
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 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 it is ok the way it is. Just change the assignment to append the new test suite instead and move it so that it is appended after common.mk
is included
2.4/test/run-conu
Outdated
@@ -0,0 +1,2 @@ | |||
#!/bin/bash | |||
exec pytest-3 -vv ./test_s2i.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 needs to be: exec pytest-3 -vv ./test/test_s2i.py
The test is being run from $ROOT/$VERSION
2.4/test/test_s2i.py
Outdated
assert c.is_port_open(8080) | ||
response = c.http_request("/", port="8080") | ||
assert response.ok | ||
output = c.execute(["ruby", "--version"]) |
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 needs to be ["bash", "-c", "ruby --version"]
2.4/test/test_s2i.py
Outdated
logs = c.logs().decode("utf-8").strip() | ||
usage = i.usage() | ||
# FIXME: workaround: `docker logs` can't handle logs like these: '\n\n\n' | ||
assert logs.replace("\n", "") == usage.replace("\n", "") |
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.
A failure over here since the usage call does not ignore stderr
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.
Actually the usage call only returns stderr ...
|
I'll be honest: I don't have time to finish this anymore. Is this merge-able if I just rebase or is there anything else what needs to be done? |
There are comments in the code regarding fixes needed- AFAICT just implementation details. |
43e0d9d
to
53bde92
Compare
Finally found some spare time to finish this. Please see my changes. Obviously this requires sclorg/container-common-scripts#67 |
53bde92
to
085e7fd
Compare
sclorg/container-common-scripts#67 is merged, I updated this PR and is ready for review |
Do we have a way to test this in CI now? afaics the conu tests are run only via |
You can probably create a new job and run that command and see if it works. Please let me know what I should do in order to get this merged. |
I've updated CI config for ruby repo [1]. Because elel7 builds are quite outdated, [test-openshift] |
test/run-conu
Outdated
@@ -0,0 +1,2 @@ | |||
#!/bin/bash | |||
exec pytest-3 -vv -k test_s2i |
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.
@TomasTomecek Please use pytest
. If conu
is installed from PyPi, pytest
is also from PyPi and no -3
suffix is there. (to note: although without -3, python3 is used)
EDIT: Ignore the bellow. It was caused by @TomasTomecek Also when I've tried the same (
|
@TomasTomecek CI is ready. So only When trying this locally, I'm getting this test failure:
|
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
085e7fd
to
d09d821
Compare
@omron93 I got an error like this when using older s2i -- newer s2i shouldn't print this debug output: I just updated the command to be (got sick, hence the late response) |
[test][test-openshift] |
running conu tests (sclorg/s2i-ruby-container/pull/167) IMHO, the best way to solve is to do epel build (epel is enabled already)
running conu tests (sclorg/s2i-ruby-container/pull/167) IMHO, the best way to solve is to do epel build (epel is enabled already)
Newer s2i worked. So tests are passing now. This PR got a deep review, so merging. @TomasTomecek Thanks for adding this feature. |
It was almost a year! I wanted to have this merged so it wouldn't have to be my new year resolution. Thank you to everyone who got involved and helped carrying me. |
* Set defaults for lang/locale and I/O encoding. * Fix use of wrong locale. Was AU instead of US.
CC @hhorak @praiskup @pkubatrh
Let's discuss the invocation please. I'm not entirely sure how to integrate it into your makefile hierarchy.
The library: https://github.com/fedora-modularity/conu
Signed-off-by: Tomas Tomecek ttomecek@redhat.com