-
Notifications
You must be signed in to change notification settings - Fork 46
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
Make gpg
tests more resilient
#875
Conversation
tests/test_gpg.py
Outdated
|
||
# Because we imported this key into a temporary directory, | ||
# we should only have one key in the keyring. | ||
if len(fingerprints) == 1: |
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.
Is there a reason you didn't make this an assertion? Otherwise it seems like there needs to be an else case here that throws an exception or pytest.fails
tests/test_gpg.py
Outdated
fingerprints = find_fps_from_gpg_output(p) | ||
|
||
# Especially during development, sd-gpg may contain more than one key | ||
if expected_fp and expected_fp in fingerprints: |
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.
assert or else here as well
tests/test_gpg.py
Outdated
local_fp = get_local_fp() | ||
remote_fp = get_remote_fp(expected_fp=local_fp) | ||
|
||
self.assertIsNotNone(local_fp, "Local key not found") |
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 covers what I mentioned above but I think it would be better for the assertions to be closer to what they are asserting.
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.
(per inline comments)
Thanks Kunal, will pick this back up but need to reprov a 4.1 env first -- doing that part today, so may not get back to the PR til tomorrow. |
From-scratch 4.1 reprovisioning completed without issues or errors, will poke this tomorrow :) |
- Succeed if expected key is present in sd-gpg - Fail if no key is present locally or remotely
66571e1
to
eb5b6fe
Compare
Thanks @legoktm - I've reorganized the file a bit, tell me what you think. I did not see a good reason to have the GPG test setup outside the class so it's integrated now. I've still tried to group the assertions in the test method but avoided the use of conditionals in the test helper. |
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.
LGTM, test plan passes
Status
Ready for review
Description of Changes
Fixes #874
Changes proposed in this pull request:
Testing
Prior to testing,
make clone
from this branch indom0
.Ensure you have a configured workstation and
sd-journalist.sec
andconfig.json
present indom0
.make test-gpg
passes (if not, initial setup may have issues)Replace
dom0
sd-journalist.sec
with a different key not provisioned insd-gpg
make test-gpg
failsImport the new key manually on
sd-gpg
viagpg --import
make test-gpg
passesRemove the new key on
sd-gpg
viagpg --delete-secret-key
andgpg--delete-key
make test-gpg
failsRestore original
dom0
configmake test-gpg
passes