-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Integration tests stub #2951
Integration tests stub #2951
Conversation
.travis.yml
Outdated
@@ -11,3 +11,4 @@ script: | |||
# TODO(rhcarvalho): check syntax of other important entrypoint playbooks | |||
- ansible-playbook --syntax-check playbooks/byo/config.yml | |||
- cd utils && make ci | |||
- make test-integration |
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.
We have already some integration with vagrant-openshift to run these tests in our Jenkins, but before we transition it to point to openshift/openshift-ansible, Travis can demo a test run.
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.
If there are tests that we can run under travis-ci, then I think we should. We should only need to run tests on our internal jenkins if they require access to internal content/resources.
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.
make test-integration is running from the utils directory, probably want to do something like the following instead:
- ansible-playbook --syntax-check playbooks/byo/config.yml
- pushd utils
- make ci
- popd
- make test-integration
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.
Problem solved.
utils/setup.cfg
Outdated
tests=../,../roles/openshift_master_facts/test/,test/ | ||
tests = ../test/unit, | ||
., | ||
../roles/openshift_master_facts/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.
Could have been a separate PR... I realized we were missing 2 tests via cd utils; make ci
.
Makefile
Outdated
@@ -0,0 +1,7 @@ | |||
.PHONY: test | |||
test: | |||
nosetests |
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 would probably argue that we should be running unit-tests and flake8 for make test, since both ideally will give quick feedback.
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.
For now we're still using (cd utils; make ...); let's move utils/Makefile -> Makefile in a separate PR and we can have a more focused discussion of what runs when.
I'll take a closer look at this tonight/tomorrow, but I wanted to share this article I came across the other day talking about testing with Ansible: https://medium.com/@sebinjohn/testing-ansible-roles-using-molecule-and-vagrant-a15c22af23ab#.8yu427q47 |
.travis.yml
Outdated
@@ -11,3 +11,4 @@ script: | |||
# TODO(rhcarvalho): check syntax of other important entrypoint playbooks | |||
- ansible-playbook --syntax-check playbooks/byo/config.yml | |||
- cd utils && make ci | |||
- make test-integration |
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.
If there are tests that we can run under travis-ci, then I think we should. We should only need to run tests on our internal jenkins if they require access to internal content/resources.
Makefile
Outdated
@@ -0,0 +1,7 @@ | |||
.PHONY: test | |||
test: | |||
nosetests |
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 would probably argue that we should be running unit-tests and flake8 for make test, since both ideally will give quick feedback.
.travis.yml
Outdated
@@ -11,3 +11,4 @@ script: | |||
# TODO(rhcarvalho): check syntax of other important entrypoint playbooks | |||
- ansible-playbook --syntax-check playbooks/byo/config.yml | |||
- cd utils && make ci | |||
- make test-integration |
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.
make test-integration is running from the utils directory, probably want to do something like the following instead:
- ansible-playbook --syntax-check playbooks/byo/config.yml
- pushd utils
- make ci
- popd
- make test-integration
setup.cfg
Outdated
[nosetests] | ||
tests = test/unit, | ||
utils, | ||
roles/openshift_master_facts/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.
should roles/openshift_master_facts/test/*
be moved to roles/openshift_master_facts/test/unit/
as well?
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.
Possibly not. I moved the Python files under test/
just to have a better organization and reuse the test/
top-level dir.
@detiber, I would even consider moving test/unit/modify_yaml_tests.py
-> library/test_modify_yaml.py
, so that test files live close to what they 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.
consider moving test/unit/modify_yaml_tests.py -> library/test_modify_yaml.py, so that test files live close to what they test
@detiber what do you think about that idea?
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.
@rhcarvalho I'm not sure how well that would work, since then the test would be in the Ansible module lookup path.
@@ -0,0 +1,120 @@ | |||
package preflight |
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.
Can't say I'm a huge fan of using golang for testing here, seems like it is going to be difficult to get external contributors to participate if the tests are written in golang.
I'd much prefer that we move towards a framework like https://molecule.readthedocs.io instead, since it also allows for much more comprehensive testing against multiple machines, etc.
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.
Can't say I'm a huge fan of using golang for testing here, seems like it is going to be difficult to get external contributors to participate if the tests are written in golang.
Why do you think so?
I'd much prefer that we move towards a framework like https://molecule.readthedocs.io
Thanks for the link. I see how it could be useful for us in end-to-end testing of the many playbooks in openshift-ansible (installation, upgrades, one-off actions). IIUC it's a YAML-centric framework for a particular flow of testing playbooks? I see it can do check syntax, convergence, idempotence, verify final state.
For testing the preflight checks, what we need is a way to run openshift-ansible someplaybook.yml
and check the exit code and output. Each test playbook will setup a dynamic inventory composed of one or more containers, and then will include another playbook from the preflight checks, to ensure that the check work correctly under certain environments (e.g. system with missing subscriptions, or incompatible versions of packages).
We wanted something that could give us fast feedback to iterate on the checks. Thus, we use Docker images instead of VMs. Running the tests in parallel seemed desirable, and Go's testing
let us do that without much hassle.
Open to suggestions.
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.
Can't say I'm a huge fan of using golang for testing here, seems like it is going to be difficult to get external contributors to participate if the tests are written in golang.
Why do you think so?
Most external contributors will not be familiar with golang, If they are already advanced Ansible users, then we can generally assume they are familiar with Python.
I'd much prefer that we move towards a framework like https://molecule.readthedocs.io
Thanks for the link. I see how it could be useful for us in end-to-end testing of the many playbooks in openshift-ansible (installation, upgrades, one-off actions). IIUC it's a YAML-centric framework for a particular flow of testing playbooks? I see it can do check syntax, convergence, idempotence, verify final state.
The verify final state is the one that allows for arbitrary validations to be run.
For testing the preflight checks, what we need is a way to run openshift-ansible someplaybook.yml and check the exit code and output. Each test playbook will setup a dynamic inventory composed of one or more containers, and then will include another playbook from the preflight checks, to ensure that the check work correctly under certain environments (e.g. system with missing subscriptions, or incompatible versions of packages).
I'll play around with it some, I'm pretty sure this use case is baked in, even if the docs are lacking in how to do it.
We wanted something that could give us fast feedback to iterate on the checks. Thus, we use Docker images instead of VMs. Running the tests in parallel seemed desirable, and Go's testing let us do that without much hassle.
Molecule is able to use Docker as a backend as well. I'll have to play around with the parallelization, but I think we can accomplish something similar.
4f9fd2f
to
1ac275e
Compare
1ac275e
to
d87361b
Compare
d87361b
to
94596cd
Compare
.travis.yml
Outdated
script: | ||
- tox | ||
# REVIEW(rhcarvalho): if our Travis account doesn't support more than one | ||
# build at a time, there is no point in parallelizing unit/integration tests. |
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.
Our account can do 2 concurrent builds across all openshift/* repos.
To make room for integration tests.
94596cd
to
5d846c2
Compare
Updated to use
|
# Unset GOPATH because tests use relative imports. This should be removed if | ||
# we require openshift-ansible to live in a Go work space and use absolute | ||
# imports in tests (desirable). | ||
integration: env -u GOPATH go test -v ./test/integration/... |
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 doesn't seem like it should be needed: http://tox.readthedocs.io/en/latest/config.html#confval-passenv=SPACE-SEPARATED-GLOBNAMES
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.
Hmm, I failed to see how that would be used?
Did you mean something like
[testenv:integration]
setenv=
GOPATH=
?
But then, that looks like a lot more to unset an envvar than using env -u
?
I'm unsetting GOPATH
just to not require the code to be within GOPATH, and use relative imports. Relative imports are notably a bad idea in Go, I just wanted to experiment and see how it would compare. This basically affects CI in that we need to decide if the checkout path is within a Go work space or not. And we need to decide between Travis and Jenkins...
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.
If I'm reading the tox docs correctly, the GOPATH env var shouldn't be available in the test environment with the default config. Outside of a handful of whitelisted vars, you should have to specify which env cars you want to make available using passenv
or by explicitly defining them with setenv
.
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.
Ah, okay, now I understand.
I'll update it.
OpenShift Ansible Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again |
This was merged through #3816. |
In the past few weeks, @brenton @sosiouxme and I have been working on several pre-flight checks: playbook(s) that verify certain bits of the environment that folks might run before an installation, upgrade or to debug an existing cluster.
We've been also experimenting with an additional approach to integration testing of such playbooks.
I'd like to start bringing our work gradually to make it easier to review, and get better feedback.
I've cherry-picked commits from our work-branch related to the integration tests, for I think this is an easier piece to grasp and part of the foundation of our tests.
More tests (here is just a demo of the concept) and the playbooks will come in separate PRs.