Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

rhcarvalho
Copy link
Contributor

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.

@rhcarvalho
Copy link
Contributor Author

@abutcher @detiber PTAL

@brenton and I would love your 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
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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,
Copy link
Contributor Author

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@detiber @tbielawa when we move utils/Makefile into the top-level, we can have make test be the one to give very quick feedback during development, while keeping the make ci target doing all the things it does today (setup venv, unit tests, flake8, pylint).

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@detiber
Copy link
Contributor

detiber commented Dec 8, 2016

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2017
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2017
.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.
Copy link
Contributor Author

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.

@rhcarvalho
Copy link
Contributor Author

Updated to use tox and have some examples of what tests could look like:

  • automate setting up containers for a dynamic test inventory
  • run a playbook
  • make assertions about exit code and output

# 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/...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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...

Copy link
Contributor

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.

Copy link
Contributor Author

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-bot
Copy link

OpenShift Ansible Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2017
@rhcarvalho
Copy link
Contributor Author

This was merged through #3816.

@rhcarvalho rhcarvalho closed this May 5, 2017
@rhcarvalho rhcarvalho deleted the integration-stub branch May 5, 2017 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants