Adding pymongo functional tests #340
Conversation
Codecov Report
@@ Coverage Diff @@
## master #340 +/- ##
==========================================
+ Coverage 85.13% 85.32% +0.19%
==========================================
Files 38 38
Lines 1911 1929 +18
Branches 225 227 +2
==========================================
+ Hits 1627 1646 +19
+ Misses 219 218 -1
Partials 65 65
Continue to review full report at Codecov.
|
| check=False, | ||
| ) | ||
| if process.returncode != 0: | ||
| logging.warning("Failed to start MongoDB container") |
There was a problem hiding this comment.
Container could be already running because it was started by the user, the test didn't execute clean up while debugging, etc. I didn't want to fail the test case here, it would fail when creating MongoClient if something goes wrong anyways
hectorhdzg
added
the
needs reviewers
There was a problem hiding this comment.
Overall the tests and the general idea sound great. My one thought is around the startup and shutdown of containers.
Always starting the containers during unit tests
I think that, rather than having a specific CI class, we should just have docker in our CI system, and start up the containers ourselves. Is that possible? My thought here is there's not a lot of value in differentiating the service startup and shutdown process between CI and local testing.
Utility code to start / stop containers.
It looks like there's some libraries that already handle the startup and shutdown of containers.
Now that we're using pytest, we could potentially pick up fixtures and use one of:
- https://pypi.org/project/lovely-pytest-docker/
- https://pypi.org/project/pytest-docker-compose/
Or maybe the more barebones: - https://docker-py.readthedocs.io/en/stable/
Just thinking there's not a lot of value in handling startup and shutdown ourselves, and there's messy cases such as shutting down an already running container (docker run IIRC fails if a container of the same name is already up).
There was a problem hiding this comment.
I don't think it's a good idea to run docker containers from tests, and I think this approach is likely to cause some serious maintenance headaches in the future.
Some problems I see with this, but not an exhaustive list:
- If the user doesn't have the mongo docker image cached, downloading the image can take longer than two seconds. As it's written now, if the image isn't cached, each test just hangs for a couple seconds and then fails with a cryptic
ServerSelectionTimeoutErrorerror. - If the user was running a mongo container before starting the test, this test will stop it. This is pretty surprising behavior.
- if the user is running mongo outside of docker, we shouldn't need to run the container.
My thought here is there's not a lot of value in differentiating the service startup and shutdown process between CI and local testing.
Running mongo as a TravisCI service means that the container starts in parallel with the tests, instead of starting (and potentially downloading the image) when the fixture is loaded.
I'd prefer for these tests to fail fast if mongo isn't available, and for a dedicated tox target (or script, or etc.) to start and stop mongo as needed.
We should also find a better way to quarantine integration tests with dependencies like this so they don't run as part of the normal test suite.
|
@toumorokoshi , @c24t I updated the code to not do any docker related operations in the test classes, created new tox environment just for these to be able to disable easily in case there are any issue and added a dependency on docker-compose to easily add new containers if needed, let me know if you have any feedback |
ext/opentelemetry-ext-docker-tests/tests/pymongo/test_pymongo_functional.py
Outdated
Show resolved
Hide resolved
| self.assertIsInstance(span.start_time, int) | ||
| self.assertIsInstance(span.end_time, int) | ||
| self.assertIsNotNone(pymongo_span.parent) | ||
| self.assertEqual(pymongo_span.parent.name, root_span.name) |
There was a problem hiding this comment.
By this point you require pymongo_span and root_span to be defined. They may not be if spans only includes 2 spans whose name attribute is "rootSpan" or if it includes 2 spans whose name is not. If any of those happen, you'll get a NameError in a test case when you want to get only AssertionErrors. Instead for looping through spans looking for spans whose name attribute matches, it should be asserted that spans actually contains one span whose name attribute is "rootSpan" and another one whose name attribute is not.
There was a problem hiding this comment.
The actual test I wanted to include is that rootSpan is parent of other spans created, that is why I'm saving the reference for it so I can assert later, the check of an extra span being created in pymongo is already taken care in self.assertEqual(len(spans), 2) because all tests start with a rootSpan only, are you suggesting to add more check regarding these spans?
There was a problem hiding this comment.
Yes, what I am pointing out is that your test case requires self._span_exporter.get_finished_spans() to deliver you 2 spans, one named "rootSpan" and other not, and since it requires that, it is probably something that you want to specifically test.
|
Sorry to hold you up @hectorhdzg. This PR LGTM after splitting off the docker tests, and it looks like you addressed @ocelotl's comment in 176cb73. |
|
@ocelotl if this LGTY we can merge it. |
|
Sure, I think this could be addressed but it is not critical. 👍 |
Using similar approach as OpenTelemetry JS to run docker with MongodDB container and run tests against it, currently it will require contributors of this repo to have docker installed and running when executing the tests
Fixes #284