-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-3297 Test auto decryption occurs after CommandSucceeded events #980
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
Conversation
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/test_encryption.py
Outdated
# /README.rst#14-decryption-events | ||
class TestDecryptProse(EncryptionIntegrationTest): | ||
def setUp(self): | ||
self.setup_client = MongoClient() |
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.
We should use self.client (same as client_context.client) instead of creating a new client here. Otherwise the test will fail on auth or tls.
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.
Done.
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 is not done. Please re-read my comment.
test/test_encryption.py
Outdated
self.addCleanup(self.encrypted_client.close) | ||
|
||
def test_command_error(self): | ||
self.setup_client.admin.command( |
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.
We should use the with self.fail_point(...)
helper since it always disables the failpoint.
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.
Done.
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.
FYI you can put multiple context managers in one with-statement. So this:
with self.fail_point(...):
with self.assertRaises(...):
pass
Can be written as:
with self.fail_point(...), self.assertRaises(...):
pass
No need to change anything though.
i = self.listener.results["succeeded"][0] | ||
self.assertEqual( | ||
i.reply["cursor"]["firstBatch"][0]["encrypted"], self.malformed_cipher_text | ||
) |
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.
I know the spec doesn't say to but it would be good to assert there are 0 failed events.
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.
Done.
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.
I don't see this added.
i = self.listener.results["succeeded"][0] | ||
self.assertEqual( | ||
i.reply["cursor"]["firstBatch"][0]["encrypted"], self.malformed_cipher_text | ||
) |
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.
I don't see this added.
test/test_encryption.py
Outdated
# /README.rst#14-decryption-events | ||
class TestDecryptProse(EncryptionIntegrationTest): | ||
def setUp(self): | ||
self.setup_client = MongoClient() |
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 is not done. Please re-read my comment.
test/test_encryption.py
Outdated
self.addCleanup(self.encrypted_client.close) | ||
|
||
def test_command_error(self): | ||
self.setup_client.admin.command( |
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.
FYI you can put multiple context managers in one with-statement. So this:
with self.fail_point(...):
with self.assertRaises(...):
pass
Can be written as:
with self.fail_point(...), self.assertRaises(...):
pass
No need to change anything though.
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!
No description provided.