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

Improve Integration Test Error Handling #531

Merged
merged 6 commits into from Oct 14, 2019
Merged

Conversation

@llamasoft
Copy link
Contributor

llamasoft commented Oct 10, 2019

This MR fixes issue #530 and partially fixes #207.

It includes the following changes:

  • Throw an exception if a Vault instance is already running.
    • This is mainly to prevent interacting with someone's personal Vault instance or tunnel as if it were a test instance.
  • If any integration test fails, the created Vault instance(s) are terminated.
    • The ServerManager's stop method works correctly regardless if any instances were actually created, so it's always safe to call it.
  • An exception is now thrown if the Vault instance terminates before becoming ready.
    • This handles cases where the port is already in use or some other unspecified error occurs.
  • If a Vault executable isn't found, the integration tests are skipped.
  • If a Java executable isn't found, the LDAP integration tests are skipped.
  • Added a few extra debug statements when starting/killing Vault instances to include process IDs.
  • Minor rewording of variables and docstrings in the ServerManager class.
@llamasoft llamasoft requested a review from hvac/hvac-maintainers as a code owner Oct 10, 2019
@update-docs

This comment has been minimized.

Copy link

update-docs bot commented Oct 10, 2019

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update some of our documentation based on your changes.
See: https://github.com/hvac/hvac/blob/develop/CONTRIBUTING.md#documentation

Copy link
Collaborator

jeffwecan left a comment

Thank you so much for this contribution @llamasoft!!! I have a few small suggested changes around logging and such but this looks like a great set of additions even without those suggestions implemented.

(I'm embarrassed to admit that I eventually just resorted to a personal workflow along the lines of pkill -f vault; make test whenever hitting this scenario this seeks to solve a while back 🙃)

tests/integration_tests/api/auth_methods/test_ldap.py Outdated Show resolved Hide resolved
tests/utils/__init__.py Outdated Show resolved Hide resolved
tests/utils/server_manager.py Show resolved Hide resolved
tests/utils/server_manager.py Show resolved Hide resolved
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 11, 2019

Codecov Report

Merging #531 into develop will increase coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #531     +/-   ##
==========================================
+ Coverage    82.84%   82.95%   +0.1%     
==========================================
  Files           54       54             
  Lines         2949     2968     +19     
==========================================
+ Hits          2443     2462     +19     
  Misses         506      506
Impacted Files Coverage Δ
hvac/api/auth_methods/ldap.py 100% <0%> (ø) ⬆️
hvac/api/system_backend/auth.py 87.17% <0%> (ø) ⬆️
hvac/api/secrets_engines/aws.py 98.07% <0%> (ø) ⬆️
hvac/api/auth_methods/radius.py 28.57% <0%> (ø) ⬆️
hvac/api/system_backend/policy.py 100% <0%> (ø) ⬆️
hvac/api/secrets_engines/azure.py 100% <0%> (ø) ⬆️
hvac/api/secrets_engines/kv_v2.py 100% <0%> (ø) ⬆️
hvac/api/auth_methods/okta.py 100% <0%> (ø) ⬆️
hvac/api/secrets_engines/pki.py 100% <0%> (ø) ⬆️
hvac/api/secrets_engines/identity.py 93.65% <0%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7485853...abc9649. Read the comment docs.

Copy link
Collaborator

jeffwecan left a comment

Thanks again for the effort here! I'll let the actively running tests wrap up and then merge this in soonish (this evening / tomorrow).

@llamasoft

This comment has been minimized.

Copy link
Contributor Author

llamasoft commented Oct 11, 2019

I tried throwing in a few tweaks to get the Travis builds to successfully built Vault but I haven't had much luck yet. It no longer complains about the go version, but now it's complaining about an unknown field that absolutely does exist.
Unfortunately, I'm not at all experienced with go, so my ability to troubleshoot this is severely limited. It's also hampered by the fact that I can't test under Travis directly, I can only push new commits and hope for the best. That said, I managed to build the Vault binary locally in Docker using go 1.12.7, so the issue lies either in the Travis environment or the utility script used to build Vault.

Regardless, I'm moving my Travis tests to a new branch (as I should have done earlier!).

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

jeffwecan commented Oct 12, 2019

Oh those particular build failures are no major concern. Or at least not meant to block merging hah. 1.) the intention in having the source builds in place is sort of a leading indicator of potential upcoming issues 2.) sometimes the build failures are due to unrelated upstream issues. Which is why we have those particular test runs under the "allow failures" travis-ci distinction. I haven't looked into the specifics of the current failures to be clear. Still probably worth calling out those nuances in the contributing documentation w.r.t. upstream builds.

That said, the Travis test matrix is probably out of date at this point and should most likely be updated. However, that need not block this very valuable addition in the meantime.

@jeffwecan

This comment has been minimized.

Copy link
Collaborator

jeffwecan commented Oct 12, 2019

tl;dr: if you care to attempt to get the head vault branch builds passing in this PR, feel free. Otherwise I'm happy to merge things as is.

@llamasoft

This comment has been minimized.

Copy link
Contributor Author

llamasoft commented Oct 12, 2019

I'll work on the head branch build issue in a separate PR. I'll make one PR for testing, then a separate PR once I actually work out a fix. 👍

@jeffwecan jeffwecan merged commit 4b8a872 into hvac:develop Oct 14, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeffwecan jeffwecan added the misc label Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.