Integrates testinfra tests with Molecule "verify" action #3703
Conversation
Using a shared test directory, outside of a single scenario, so that the same staging config tests can be referenced by multiple scenarios (we already have virtualbox, libvirt, and AWS staging scenarios). This is a straightforward move operation, with two exceptions: * updates some of the hardcoded relative paths in the OSSEC tests * removes unused (always-skipped) ansible vars tests Other than that, couldn't be simpler.
Uses a shared directory that's accessible by all other Molecule scenarios. Using the 'converge' playbook override, rather than the symlink, because the "ossec" role was not running otherwise. I cannot explain why; timeboxed the solution, and this is more straightforward anyway. Parallelizes the testinfra test invocation. Made possible since we're running against both hosts concurrently now, so the combined test run goes from 10m to 3.5m.
Using the `testinfra_hosts` pragma for multihost testing, taken straight out of the testinfra docs [0]. Rather than run the test suites sequentially against app and mon, we can run them at the same time, and trust pytest to run correct per-hosts tests, based on the targeting. Accordingly, removes occurrences of the SECUREDROP_TESTINFRA_TARGET_HOST env var throughout the tests, since we're no longer depending on that env var for the tests to work. [0] http://testinfra.readthedocs.io/en/latest/invocation.html#test-multiples-hosts
We're eliminating use of the TESTINFRA_TARGET_HOST env var for importing
vars. The vast majority of staging vars were intended as overrides
vis-a-vis prod; not between app and mon. We're not currently testing
prod, so that's an unnecessary optimization.
The staging.yml file is concatenated from the {app,mon}-staging.yml
files, with a few exceptions:
* removed unused iptables ruleset (Jinja2 templates are used)
* removed mention of SSH tor service (untested in mon-staging)
Pytest expects the collected test files to be uniquely named, in terms
of basename. We didn't encounter this before because we were running the
config tests sequentially, against each host, so the collection was not
aggregated. When trying to run against a combined hostgroup, the
following error is shown:
==================================== ERRORS ====================================
____________ ERROR collecting testinfra/staging/mon/test_network.py ____________
import file mismatch:
imported module 'test_network' has this __file__ attribute:
/home/conor/freedomofpress/securedrop/molecule/testinfra/staging/app/test_network.py
which is not the same as the test file we want to collect:
/home/conor/freedomofpress/securedrop/molecule/testinfra/staging/mon/test_network.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules
=============================== warnings summary ===============================
Renaming the test files for unique basenames resolves, and is also more explicit.
These tests were failing rather regularly during local development on the testinfra reorg. After investigation, turns out they were being inadvertently skipped in CI, so I was debugging tests that never worked reliably. Opened a separate issue to fix those tests, skipping for now to keep moving.
Codecov Report
@@ Coverage Diff @@
## develop #3703 +/- ##
========================================
Coverage 85.34% 85.34%
========================================
Files 42 42
Lines 2682 2682
Branches 290 290
========================================
Hits 2289 2289
Misses 328 328
Partials 65 65Continue to review full report at Codecov.
|
Begins the process of overriding the env vars for staging. Includes a small change to the tor-apt test, for Python2 compatibility (otherwise the test can't be run locally). More work required on balancing the side-effect logic. We'll still need side-effect to run the app tests, we can run the verify step after side-effect. Includes some flake8 linting, since via Molecule, we're now linting the entire testinfra directory. Updates JUnit test file handling The paths to the helper scripts changed a bit, so updating them here. We had some skipped logic in the side-effect which has no been removed, so we don't have to maintain it any longer. Moved the junit logic out of the side-effect since the testinfra tests are run after the side-effect (and therefore the XML file wouldn't have existed yet).
These scenarios aren't exactly DRY, so there's a fair bit of duplication at play here. The Molecule project has open issues discussing removing duplication between scenarios. By culling the symlink to the staging playbook, we reduce a bit of redundancy, but there's still lots of copypasta. It's *not* worth generating these configs via YAML at present, since that would introduce more complexity for future maintainers. Would rather lean on upstream and wait for Molecule to support direct reuse of scenario configs.
The config test suite passes locally against the AWS scenario reliably; in CI, however, there are sporadic connectivity issues. Let's instruct testinfra to retry failed connections, to make sure the tests are running well.
Specifically added to ignore pytestdebug.log files but I can't think of any other .log files we want to hold onto. Did a preliminary search to ensure I wasnt negatively affecting existing files.
msheiny
force-pushed
the
3205-molecule-verify-step
branch
from
041d7b1 to
d280b11
Compare
There was a problem hiding this comment.
Parallelizes the testinfra test invocation.
❤️ this ... way to go buddehhh.
Looking solid. Just a big heads up 🚨 . I found a lingering molecule/libvirt-staging/pytestdebug.log which obviously was unintentionally committed. I know ol' @conorsch is a stictly for tight git commit history so to save you some time i rebased, took that file out, and added a commit ontop to ignore future log files. If this was infact an intentionally left log file then please get loud ASAP before pulling the latest changes.
| env: | ||
| ANSIBLE_ROLES_PATH: ../../install_files/ansible-base/roles | ||
| ANSIBLE_CONFIG: ../../install_files/ansible-base/ansible.cfg |
|
|
||
| # Combine tests for export as artifacts. There are at least two XML files, | ||
| # one from the application tests, and another from the config tests. | ||
| ./molecule/testinfra/combine-junit.py ./*results.xml > ./junit/junit.xml |
There was a problem hiding this comment.
I'm not sure about moving all the tests into a shared molecule/testinfra folder.. its not really a scenario (like the other folders) so this could be confusing later on. 🤷♂️ i guess its okay. just talking out loud here
There was a problem hiding this comment.
@msheiny Totally agree, perhaps devops/scripts/ would be the best location?
There was a problem hiding this comment.
devops/scripts/
yeah i could get down with that :)
| @@ -2,6 +2,7 @@ | |||
| import re | |||
|
|
|||
|
|
|||
| testinfra_hosts = ["app-staging"] | |||
There was a problem hiding this comment.
I would ideally love this to be an environment variable. I foresee the host names changing in the future but this isnt a deal-breaker for me. Just noting it for future implementations.
There was a problem hiding this comment.
That's true, for example if/when we go back to testing e.g. app-prod regularly, we'll want to override here. Appending to the list seems sane. Willing to defer, since we don't have a clear picture of the future testing scenarios just yet.
| # Make SSH calls more resilient, as we're operating against remote hosts, | ||
| # and running from CI. We've observed flakey connections in CI at times. | ||
| os.environ['ANSIBLE_SSH_RETRIES'] = '5' | ||
| ssh_args = [ |
There was a problem hiding this comment.
Love this approach to making the connection more stable ❤️
There was a problem hiding this comment.
It took me far too long to come up with that simple solution—been working reliably since. 👌
It should have been here all along. Moving it, however, reveals a bandit violation. Rather than patch it, let's permit the violation, since the impact is low (script runs in CI).
|
Made the requested changes, @msheiny; ready for re-review! |
Status
Ready for review.
Description of Changes
Completes and therefore closes #3205. Follows up on #3644.
Integrates the testinfra config test suite with Molecule, so that it runs as a
verifyaction. Deleted the customtestinfra/test.pywrapper script that managed the connection info for remote hosts.Mostly this is a move operation, shoving the testinfra tests into the
molecule/dir. Some minor tweaks were necessary inside the tests given the path change, but the tests still validate the same functionality they previously did.Testing
CI
Local env
molecule test -s libvirt-staging(requires Linux) and confirm all tests passing.Deployment
Dev-env only. Reorganizes tests, but preserves the content of what they're testing.
Checklist
If you made changes to the server application code:
make ci-lint) and tests (make -C securedrop test) pass in the development containerIf you made changes to
securedrop-admin:make -C admin test) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint) passed locally