PYTHON-1878 Add mongodb+srv URIs to Atlas Connectivity tests #538
Conversation
| . ./prepare_atlas_connectivity.sh | ||
| rm -f ./prepare_atlas_connectivity.sh | ||
|
|
||
| PYTHON_BINARY=${PYTHON_BINARY} bash ${PROJECT_DIRECTORY}/.evergreen/run-atlas-tests.sh |
There was a problem hiding this comment.
This is a more user friendly way to manage secrets. We load the env variables in secret (with silent:true) and then we're free to use xtrace and view test output as usual. Now we can actually see why a task failed :)
| set -o errexit | ||
| set -o xtrace |
There was a problem hiding this comment.
We can do this safely. We just need to remember not to print or use the secret variables in the shell script.
| echo "Running tests without dnspython" | ||
| python test/atlas/test_connection.py | ||
|
|
||
| # dnspython is incompatible with Jython so don't test that combination. |
There was a problem hiding this comment.
| # DO NOT ECHO WITH XTRACE (which PREPARE_SHELL does) | ||
| PYTHON_BINARY=${PYTHON_BINARY} ATLAS_REPL='${atlas_repl}' ATLAS_SHRD='${atlas_shrd}' ATLAS_FREE='${atlas_free}' ATLAS_TLS11='${atlas_tls11}' ATLAS_TLS12='${atlas_tls12}' sh ${PROJECT_DIRECTORY}/.evergreen/run-atlas-tests.sh | ||
| cat <<EOT > prepare_atlas_connectivity.sh | ||
| export ATLAS_FREE='${atlas_free}' |
There was a problem hiding this comment.
I think we can further optimize some of these invocations by switching to using subprocess.exec instead of shell.exec and then setting either theinclude_expansions_in_env option or the add_expansions_to_env option to automatically set the appropriate environment variables. I suppose we don't need to do it right now, but being able to eventually eliminate the use of $PREPARE_SHELL would be a huge improvement IMO.
There was a problem hiding this comment.
Just to clarify, I am just suggesting this as a future optimization/refactor. Not saying that we need to do it in this PR. Would love to hear your thoughts on it @ShaneHarvey
There was a problem hiding this comment.
Cool idea! Can you open a ticket with this comment?
There was a problem hiding this comment.
| . ./prepare_atlas_connectivity.sh | ||
| rm -f ./prepare_atlas_connectivity.sh | ||
|
|
||
| PYTHON_BINARY=${PYTHON_BINARY} bash ${PROJECT_DIRECTORY}/.evergreen/run-atlas-tests.sh |
test/atlas/test_connection.py
Outdated
| def test_srv_tls_12(self): | ||
| _connect(ATLAS_SRV_TLS12) | ||
|
|
||
| @unittest.skipUnless(PYMONGO_MUST_HAVE_DNS, 'dnspython is optional') |
There was a problem hiding this comment.
Nice! Can you add a comment/docstring detailing what this test achieves? Specifically, we don't end up silently skipping all tests if dnspython is not installed (instead we get a failure due to this test).
There was a problem hiding this comment.
I changed this up a bit. PTAL at the new approach:
@unittest.skipUnless(HAS_DNS or MUST_TEST_SRV, 'SRV requires dnspython')
…, ensure we use srv URIs
There was a problem hiding this comment.
I updated the tests in response to your detailed review and added an extra test to ensure:
- that we don't accidentally duplicate the test URIs in our evergreen project configs (because I'm paranoid).
- that the SRV uris actually contain
mongodb+srv://.
Please take another look.
test/atlas/test_connection.py
Outdated
| def test_srv_tls_12(self): | ||
| _connect(ATLAS_SRV_TLS12) | ||
|
|
||
| @unittest.skipUnless(PYMONGO_MUST_HAVE_DNS, 'dnspython is optional') |
There was a problem hiding this comment.
I changed this up a bit. PTAL at the new approach:
@unittest.skipUnless(HAS_DNS or MUST_TEST_SRV, 'SRV requires dnspython')
Enable xtrace with silent:false to make test failures easier to diagnose. (cherry picked from commit a9d668c)
No description provided.