Skip to content

Conversation

@gracelombardi
Copy link
Owner

Updated tests to use the revoke and destroy in tearDown and not separately in each test. Added an except to handle "An OpaqueData object has no state and cannot be revoked." error to destroy it without revoking it first.

…ately in each test. Added an except to handle "An OpaqueData object has no state and cannot be revoked." error to destroy it without revoking it first.
@gracelombardi gracelombardi self-assigned this Jul 25, 2022
@gracelombardi gracelombardi requested a review from arp102 July 25, 2022 15:57
@arp102
Copy link
Collaborator

arp102 commented Aug 8, 2022

I think the revocation error you're seeing may be caused by revoking an object that's not a key.
There should be a way to check that before calling revoke, but I'd also be fine with just adding an explanatory comment to the exception handler.

I think you're right that all the try/finally pairs aren't necessary if the teardown function is working correctly, so they can be removed to improve readability.

The old calls to revoke/destroy are also redundant with teardown, but I'm not sure that we really want to remove all of them, even if they seem like duplicated boilerplate code.
Some of the tests use assertRaises to make sure that destroy works as expected, and that would be lost in this pull request (although I didn't check if it was preserved in any other tests).
Using destroy is also part of a complete workflow, which is reflected in the test names, e.g. test_symmetric_key_create_get_destroy.
I think it's also good practice for tests to clean up after themselves when possible (even though teardown is ultimately responsible for ensuring the next test has a pristine environment).

… destroying. Added a comment to tearDown to explain that if a uuid is not for a key that it will not be revoked and will just be destroyed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants