-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-2940 Fix spec tests that require DNSPython #756
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
test/test_uri_spec.py
Outdated
@@ -181,6 +182,12 @@ def create_tests(test_path): | |||
scenario_def = json.load(scenario_stream) | |||
|
|||
for testcase in scenario_def['tests']: | |||
if (testcase['uri'].startswith("mongodb+srv") and not | |||
_HAVE_DNSPYTHON): |
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 code style is off here
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. At least I think so.
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 indentation is still off.
test/test_uri_spec.py
Outdated
@@ -181,6 +182,12 @@ def create_tests(test_path): | |||
scenario_def = json.load(scenario_stream) | |||
|
|||
for testcase in scenario_def['tests']: | |||
if (testcase['uri'].startswith("mongodb+srv") and not | |||
_HAVE_DNSPYTHON): |
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 indentation is still off.
test/test_uri_spec.py
Outdated
@@ -181,6 +182,12 @@ def create_tests(test_path): | |||
scenario_def = json.load(scenario_stream) | |||
|
|||
for testcase in scenario_def['tests']: | |||
if (testcase['uri'].startswith(SRV_SCHEME) and not |
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.
Please move this skipping logic to the run_scenario
method above and use self.skip(...) instead of print.
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.
Please be mindful about formatting.
test/test_uri_spec.py
Outdated
@@ -92,7 +93,10 @@ def run_scenario(self): | |||
compressors = (test.get('options') or {}).get('compressors', []) | |||
if 'snappy' in compressors and not _HAVE_SNAPPY: | |||
self.skipTest('This test needs the snappy module.') | |||
|
|||
if (test['uri'].startswith(SRV_SCHEME) and not | |||
_HAVE_DNSPYTHON): |
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 should be one line now.
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.
test/test_uri_spec.py
Outdated
if (test['uri'].startswith(SRV_SCHEME) and not | ||
_HAVE_DNSPYTHON): | ||
self.skipTest("Skipping test '%s' because you need DNSPython for " | ||
"mongodb+srv URIs" % dsc) |
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.
Indentation.
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.
LGTM assuming the tests pass. Can you schedule one of the failing tasks?
No description provided.