-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-2545 Test serverless proxy #664
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
PYTHON-2545 Test serverless proxy #664
Conversation
|
|
||
| tasks: | ||
|
|
||
| task_groups: |
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.
Interesting. I haven't since "task_groups" before. I gather it's a way to add arbitrary setup/teardown code to an existing task?
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.
Yes. I too, haven't used them before. I picked this right out of the sample in https://github.com/mongodb-labs/drivers-evergreen-tools/blob/master/.evergreen/config.yml
| elif 'parsed' in self.cmd_line: | ||
| params = self.cmd_line['parsed'].get('setParameter', []) | ||
| if 'enableTestCommands=1' in params: | ||
| self.has_ipv6 = False |
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.
Serverless doesn't support ipv6?
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 am not sure... It seems we only use this in one test (TestClient.test_ipv6) - do we want to run that test class against serverless?
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.
Ah okay, I don't think we need to run that test on serverless. A maybe useful test would be to manually resolve the serverless host to ipv6 and then attempt to connect. Up to you. I don't think it's needed. Should be something that's tested on the cloud team's side.
|
Looks like there are still a large number of failures: Waiting for those to be resolved before reviewing again. |
37d5f0c to
cedc355
Compare
| **self.default_client_options) | ||
|
|
||
| # May not have this if OperationFailure was raised earlier. | ||
| self.cmd_line = self.client.admin.command('getCmdLineOpts') |
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.
Does getCmdLineOpts actually work on serverless?
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.
It works but doesn't give us any useful info:
self.client.admin.command('getCmdLineOpts')
{'argv': [], 'parsed': {}, 'ok': 1}
| # Generate unified tests. | ||
| globals().update(generate_test_classes(TEST_PATH, module=__name__)) | ||
| globals().update(generate_test_classes( | ||
| TEST_PATH, module=__name__, RUN_ON_SERVERLESS=True)) |
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.
Shouldn't RUN_ON_SERVERLESS be true for all unified tests?
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.
Not as per the wording of the spec. It only needs to be enabled for spec tests pertaining to certain driver specifications.
test/test_crud_v1.py
Outdated
| self.skipTest("MongoDB Serverless does not support $out") | ||
|
|
||
| if "collation" in test['operation']['arguments']: | ||
| self.skipTest("MongoDB Serverless does not support collations") |
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 thought they already updated the spec tests themselves. Is this needed anymore? Or did they only update the unified tests and not the v1 ones?
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.
The changes to make tests serverless friendly were only for unified format.
|
|
||
| class TestWriteConcernError(IntegrationTest): | ||
| RUN_ON_LOAD_BALANCER = True | ||
| RUN_ON_SERVERLESS = True |
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 think IgnoreDeprecationsTest also needs RUN_ON_SERVERLESS=True.
eb61ba8 to
82e3b1d
Compare
|
@ShaneHarvey this is ready for final review. |
ShaneHarvey
left a comment
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.
Can you ensure the test suite fails in some way if the driver cannot connect to the serverless instance when TEST_SERVERLESS==True? For example you could add an extra test to TestClientContext. Or is this case already covered?
| "MongoDB Serverless does not support $out") | ||
| if "collation" in operation['arguments']: | ||
| self.skipTest( | ||
| "MongoDB Serverless does not support collations") |
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.
DRIVERS-1836 updated the unified tests and DRIVERS-1851 updated the crud v1 tests. Is there any reason why we did not update the retryable reads/writes tests? Can you file a drivers ticket for it?
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 think this is only a concern for Retrayble Reads. I opened DRIVERS-1869.
test/test_client_context.py
Outdated
| if 'TEST_SERVERLESS' not in os.environ: | ||
| raise SkipTest('TEST_SERVERLESS is not set') | ||
|
|
||
| self.assertTrue(client_context.connected, |
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.
Suggest client_context.connected and client_context.serverless? Or is that redundant?
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.
Somewhat redundant as client_context.serverless is set solely based on the value of TEST_SERVERLESS but it doesn't hurt. Updated.
(cherry picked from commit f07da34)
(cherry picked from commit f07da34)
TO DO:
collationand$out)