add a way to run multiple test suites #67

TomasTomecek · 2018-02-20T15:10:51Z

Related: sclorg/s2i-ruby-container#167

Initial version. I'm open to change the workflow & names. Please, advise.

TODO:

remove docker invocation
add concept of TEST_SUITES := "test1 test2" and then for x in ${TEST_SUITES}; ...
- make sure all tests are being run and exit 1 if one of them failed
TEST env should support: container, local

centos-ci · 2018-02-20T15:10:53Z

Can one of the admins verify this patch?

rhscl-bot · 2018-02-20T15:14:04Z

Can one of the admins verify this patch?

omron93 · 2018-02-21T08:09:49Z

Would it be possible to add Dockerfile.tests also in this repo? To be able to share it across containers...

TomasTomecek · 2018-03-08T15:08:25Z

Found some spare time to work on this. Waiting for tests to pass locally.

praiskup · 2018-03-28T13:50:28Z

@TomasTomecek if you specified Required-by: s2i-ruby-container#167 tag in commit message (see a068a12 e.g.) we could test directly against s2i-ruby-container.

[test]

TomasTomecek · 2018-03-29T07:40:11Z

13:55:52 + false this must fail the whole testsuite

Is it my fault?

(rebased and updated the commit message)

pkubatrh · 2018-03-29T07:51:18Z

That was a red herring. The actual error was this:

13:55:52  # FATAL: Can not be --ff merged into origin/master
13:55:52 make: *** [check] Error 1

So should be fixed by the rebase.

[test]

pkubatrh · 2018-03-29T11:14:57Z

The ruby PR needs to be rebased as well @TomasTomecek

TomasTomecek · 2018-03-29T14:09:48Z

rebased

pkubatrh · 2018-04-03T06:42:58Z

Thanks!

[test]

pkubatrh · 2018-04-03T11:30:06Z

Tests are failing now because conu tests are being run for all the image versions, but the ruby PR only provides run-conu for one version (2.4)

@TomasTomecek Can you add the conu tests for other versions as well?

TomasTomecek · 2018-04-10T11:19:30Z

I'm busy atm, I hope I'll take a look and fix this next week.

TomasTomecek · 2018-04-16T13:28:18Z

Updated.

[test]

pvalena · 2018-04-16T13:35:45Z

test.sh

-  fi
+  for test_script in ${TEST_SUITES}; do
+    if [ "${TEST_MODE}" == "container" ]; then
+      docker run --net=host -e VERSION=$dir -e IMAGE_NAME --rm -v /dev:/dev:ro -v /var/lib/docker:/var/lib/docker:ro --security-opt label=disable --cap-add SYS_ADMIN -ti -v /var/run/docker.sock:/var/run/docker.sock -v ${PWD}:/src -w /src docker.io/modularitycontainers/conu $test_script


Would you mind to split this into multiple lines, so it can be more human-readble?

pvalena · 2018-04-16T13:37:31Z

test.sh


+  # FIXME: deprecate TEST_OPENSHIFT_MODE and use TEST_SUITES instead?


Yes, this seems like the way to go.

I would do that in dedicated PR since I have no idea how much it has to be done in other repos.

There should not be any changes needed in other repositories afaik.

pvalena · 2018-04-16T13:41:36Z

common.mk

@@ -64,7 +66,7 @@ $(VERSIONS): % : %/root/help.1
 .PHONY: test check
 check: test

-test: script_env += TEST_MODE=true
+test: script_env += TEST_MODE=local


The strange thing is, that this variable now has different meaning...

Previously it was used to switch the test suite on, not to define test variant (or environment). I'd say the variable chosen before was somehow unfortunate.

do I invent a new one then?

There is some meaning change behind the variable usage but I do not think it is necessary to introduce a different one just because of that. The variable should only be used in common scripts anyway.

The issue I see with the way it is used now is that there is no way to set this variable to any other value.

pkubatrh · 2018-04-17T07:17:40Z

test.sh

+        -v /var/run/docker.sock:/var/run/docker.sock \
+        -v ${PWD}:/src \
+        -w /src \
+        docker.io/modularitycontainers/conu \


Unable to find image 'docker.io/modularitycontainers/conu:latest' locally Trying to pull repository docker.io/modularitycontainers/conu ... /usr/bin/docker-current: manifest for docker.io/modularitycontainers/conu:latest not found.

Using docker.io/modularitycontainers/conu:0.2.0 instead works.

The image does not seem to have pytest-3 though ...

We moved conu to our github org and I can't figure out how to configure automated build there. Seems github changed how apps/services work.

This docker image is still not working:

$ docker pull docker.io/modularitycontainers/conu:latest Trying to pull repository docker.io/modularitycontainers/conu ... Pulling repository docker.io/modularitycontainers/conu Tag latest not found in repository docker.io/modularitycontainers/conu

Lastest image I've found is dev, which is 5 months old:
https://hub.docker.com/r/modularitycontainers/conu/tags/

TomasTomecek · 2018-09-17T08:18:22Z

Unfortunately, I don't have time to finish this one. What would I need to do to land this one?

pvalena · 2018-09-19T13:39:09Z

I think it just needs to pass the tests- the code looks good.

TomasTomecek · 2018-11-16T16:32:14Z

Finally found some spare time to finish this. Please see my changes.

praiskup · 2018-11-16T21:08:43Z

test.sh

+        -v ${PWD}/../:/src \
+        -w /src/${dir}/ \
+        -ti \
+        docker.io/usercont/conu:0.6.2 \


Don't you want to have some floating docker tag here instead of 0.6.2?

Actually not: in case we accidentally create a breaking change in a new version, we don't want all your conu tests to start failing. I think it's a better approach to systematically update to newer versions once you're sure they work as you expect. Using latest greatest might be pretty adventurous, especially when the container ecosystem is still changing rapidly.

But your call, really.

Using latest greatest might be pretty adventurous

By floating tag I did not mean "latest greatest", but rather "latest stable". Whether we'll update the version here all the time by PR, or you'll tag it into "conu:stable" tag doesn't make much practical difference - API changes will hurt us and PR is only additional work. Btw., conu should simplify our lives so we don't have to take care of ever-changing container API, not that we'll have to take care ever-changing API in conu.

praiskup · 2018-11-16T21:10:57Z

test.sh

+        --security-opt label=disable \
+        --cap-add SYS_ADMIN \
+        -ti \
+        -v /var/run/docker.sock:/var/run/docker.sock \


Hmpfs, this means we give a lot of trust into that container.

what's the difference in running the tests directly on host?

I hope we didn't run privileged container, nor UID=0 container in testsuite so far in our suite -- except for build -- but I don't know, I thought docker build isn't mounting insecure things into the container. That said, I thought it is banned policy to give a random container write access to a docker socket. But I'm probably too old school; /me just dropped the UID from docker group, and I'll run the suite in VM.

praiskup · 2018-11-16T21:17:10Z

The code basically LGTM if that was optional. But ideally, we should give the end-user chance to install conu library on the host. If installing conu isn't possible, user could opt-in running privileged container (e.g. by USE_CONU_CONTAINER=conu:0.6.2?).

TomasTomecek · 2018-11-17T13:45:46Z

The library is packaged in Fedora so it's not a problem installing it. One thing to note is that we are planning to remove python2 support (no one ever used it actually): I'm not sure if you are planning to use it on CentOS 7 hosts.

If installing conu isn't possible, user could opt-in running privileged container

Please advise me the precise steps, maybe something like this?

if `python3 -c "import conu"`; then
  ./test/run-conu
else
  $start_that_container
fi

Btw +1 on being able to pick the conu version with the env var.

praiskup · 2018-11-17T16:25:20Z

I'm not sure if you are planning to use it on CentOS 7 hosts.

Yes, but there is Python 3 from RHSCL, so it shouldn't be a problem.

Please advise me the precise steps, maybe something like this?

I'd much rather see something like:

if $THERE_ARE_NO_CONU_TESTS; then
   exit 0
fi
if test -n "$CONU_IMAGE"; then
   docker run --rm -ti .. "$CONU_IMAGE" ..
else
   ./test/run-conu
fi

In other words, I don't think it is problem to see ./tests/run-conu failing by default (if conu is not installed), as long as we have documented this to be a prerequisite (in README.md, with others). I simply don't like the unannounced execution of privileged container -- e.g. I'm in docker group on my system and I'd feel huge discomfort in running make test.

omron93 · 2018-11-20T10:16:50Z

as long as we have documented this to be a prerequisite (in README.md, with others)

There are other prerequisites, so this shouldn't be a problem as long as rpm is easily installable in Fedora/RHEL/CentOS! Python3 RHSCL is fine, but it's harder to use it as rpm dependency. Using conu in container might have some problems, but user gets running tests out of the box.

How to install conu in RHEL7/Centos7?

TomasTomecek · 2018-11-20T12:46:23Z

It's packaged in epel7. I can see we haven't updated it in a while: https://src.fedoraproject.org/rpms/conu/blob/epel7/f/conu.spec I don't think it's such a big deal to keep packaging it for epel.

On the other hand, I just realized that python-kubernetes is not available in epel:
user-cont/conu#323

I simply don't like the unannounced execution of privileged container

Yes, that's a fair point: the container has more permissions than your unpriv user.

I updated the code with your suggestions.

The ruby test suite keeps passing:

test/test_s2i.py::TestSuite::test_s2i_apps[/home/tt/g/sclorg/s2i-ruby-container/2.5/test/puma-test-app] PASSED                                                                                              [ 25%]
test/test_s2i.py::TestSuite::test_s2i_apps[/home/tt/g/sclorg/s2i-ruby-container/2.5/test/rack-test-app] PASSED                                                                                              [ 50%]
test/test_s2i.py::TestSuite::test_invoking_container PASSED                                                                                                                                                 [ 75%]
test/test_s2i.py::TestSuite::test_usage PASSED                                                                                                                                                              [100%]

praiskup · 2018-11-20T12:55:13Z

Thanks!

test.sh

Required-by: s2i-ruby-container#167 Signed-off-by: Tomas Tomecek <[email protected]>

TomasTomecek · 2018-11-20T13:52:09Z

I just realized that the container had too many privileges for no reason, I stripped that down. PTAL

praiskup · 2018-11-20T14:35:09Z

[test] let's test the Required-by

praiskup · 2018-11-20T14:35:43Z

Pushed as f1539e9 (fixed $PWD escape).

TomasTomecek · 2018-11-20T15:05:33Z

Thank you!

I'll have a beer to celebrate this.

TomasTomecek force-pushed the add-conu branch from a4948c1 to a7e8a3a Compare March 8, 2018 15:07

TomasTomecek force-pushed the add-conu branch from a7e8a3a to be5c88a Compare March 8, 2018 15:11

TomasTomecek changed the title ~~add a way to run conu tests in a container~~ add a way to run multiple test suites Mar 8, 2018

TomasTomecek force-pushed the add-conu branch 2 times, most recently from b95ff2b to a7a1d72 Compare March 29, 2018 07:39

pvalena reviewed Apr 16, 2018

View reviewed changes

TomasTomecek force-pushed the add-conu branch from a7a1d72 to 29e2c27 Compare April 16, 2018 13:48

pkubatrh reviewed Apr 17, 2018

View reviewed changes

TomasTomecek force-pushed the add-conu branch 4 times, most recently from 7e2ddb3 to 7d05015 Compare November 16, 2018 16:29

TomasTomecek mentioned this pull request Nov 16, 2018

add test suite based on conu library sclorg/s2i-ruby-container#167

Merged

praiskup reviewed Nov 16, 2018

View reviewed changes

TomasTomecek force-pushed the add-conu branch from 7d05015 to f760d23 Compare November 20, 2018 12:42

praiskup reviewed Nov 20, 2018

View reviewed changes

test.sh Outdated Show resolved Hide resolved

add a new target to invoke tests using conu library

1244033

Required-by: s2i-ruby-container#167 Signed-off-by: Tomas Tomecek <[email protected]>

TomasTomecek force-pushed the add-conu branch from f760d23 to 1244033 Compare November 20, 2018 13:51

praiskup closed this Nov 20, 2018

TomasTomecek deleted the add-conu branch November 20, 2018 15:04


		# FIXME: deprecate TEST_OPENSHIFT_MODE and use TEST_SUITES instead?

add a way to run multiple test suites #67

add a way to run multiple test suites #67

Conversation

TomasTomecek commented Feb 20, 2018 • edited Loading

centos-ci commented Feb 20, 2018

rhscl-bot commented Feb 20, 2018

omron93 commented Feb 21, 2018

TomasTomecek commented Mar 8, 2018

praiskup commented Mar 28, 2018

TomasTomecek commented Mar 29, 2018

pkubatrh commented Mar 29, 2018

pkubatrh commented Mar 29, 2018

TomasTomecek commented Mar 29, 2018

pkubatrh commented Apr 3, 2018

pkubatrh commented Apr 3, 2018

TomasTomecek commented Apr 10, 2018

TomasTomecek commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasTomecek commented Sep 17, 2018

pvalena commented Sep 19, 2018

TomasTomecek commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup Nov 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup commented Nov 16, 2018

TomasTomecek commented Nov 17, 2018 • edited Loading

praiskup commented Nov 17, 2018

omron93 commented Nov 20, 2018

TomasTomecek commented Nov 20, 2018 • edited Loading

praiskup commented Nov 20, 2018

TomasTomecek commented Nov 20, 2018

praiskup commented Nov 20, 2018

praiskup commented Nov 20, 2018

TomasTomecek commented Nov 20, 2018

TomasTomecek commented Feb 20, 2018 •

edited

Loading

praiskup Nov 17, 2018 •

edited

Loading

TomasTomecek commented Nov 17, 2018 •

edited

Loading

TomasTomecek commented Nov 20, 2018 •

edited

Loading