Adding functional tests for psycopg2 integration by hectorhdzg · Pull Request #528 · open-telemetry/opentelemetry-python

hectorhdzg · 2020-03-25T20:12:28Z

E2E verification for span creation using psycopg2 and dbapi integrations

…zg/opentelemetry-python into hectorhdzg/psycopgTests

codecov-io · 2020-03-27T18:25:17Z

Codecov Report

Merging #528 into master will increase coverage by 0.41%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage   88.97%   89.38%   +0.41%     
==========================================
  Files          43       43              
  Lines        2213     2195      -18     
  Branches      248      250       +2     
==========================================
- Hits         1969     1962       -7     
+ Misses        173      161      -12     
- Partials       71       72       +1

Impacted Files	Coverage Δ
...ntelemetry-api/src/opentelemetry/trace/__init__.py	`83.12% <0.00%> (-2.51%)`	⬇️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`67.56% <0.00%> (-0.58%)`	⬇️
...lemetry-sdk/src/opentelemetry/sdk/util/__init__.py	`85.54% <0.00%> (-0.35%)`	⬇️
...ts/src/opentelemetry/ext/http_requests/__init__.py	`89.18% <0.00%> (-0.29%)`	⬇️
...src/opentelemetry/ext/opentracing_shim/__init__.py	`95.86% <0.00%> (-0.17%)`	⬇️
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py	`86.39% <0.00%> (-0.16%)`	⬇️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`92.46% <0.00%> (-0.06%)`	⬇️
.../src/opentelemetry/sdk/metrics/export/aggregate.py	`100.00% <0.00%> (ø)`
...xt-zipkin/src/opentelemetry/ext/zipkin/__init__.py	`84.41% <0.00%> (ø)`
...opentelemetry/sdk/context/propagation/b3_format.py	`87.27% <0.00%> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b6a52d...0cb2bb4. Read the comment docs.

toumorokoshi

Have a few comments about the error handling patterns and reducing them. Can you look through those? After we discuss I can change to approve: the code overall looks great.

toumorokoshi · 2020-04-01T15:39:12Z

ext/opentelemetry-ext-docker-tests/tests/postgres/test_psycopg_functional.py

+            cls._connection.close()
+
+    @fixture(autouse=True)
+    def connect_to_db(self):


it feels a little weird to me to have a mix of pytest fixtures, and of the standard setup/teardown unittest pattern.

Can we normalize to one or the other? I think given the current precedent on unittest-style, that would be preferable.

since it's autouse, I figure it can be added to the setup/teardown method.

Or, I'm ok with just moving the class level fixtures like cursor to pytest fixtures as well. @c24t for his thoughts, since he's been arguing against pytest fixtures in our codebase.

#526 (comment)

toumorokoshi · 2020-04-01T15:50:46Z

ext/opentelemetry-ext-docker-tests/tests/postgres/test_psycopg_functional.py

+    def connect_to_db(self):
+        if self._connection is None:
+            # Try to connect to DB
+            for i in range(5):


why do we need this retry logic? seems like even if we need it, 5 retries is a lot of times.

DB will take sometime to startup in docker, maybe having less retries and more wait time?, it could take like 15 or 20 seconds to be ready to receive connections

Another option is to fail immediately in the tests and replace docker-compose -up with a script that does the same and blocks until the container can accept connections. If possible I think it's a good idea to avoid sleeping at all in tests, and between this and #526 we're looking at adding up to a minute of idle time to an already-slow test suite.

Added a python script to check DB connections, so tests will not have any sleep but there will be initial script check for all tests, this will improve waiting if something goes wrong with docker setup and test will fail quickly

ext/opentelemetry-ext-docker-tests/tests/postgres/test_psycopg_functional.py

toumorokoshi · 2020-04-01T15:55:12Z

ext/opentelemetry-ext-docker-tests/tests/postgres/test_psycopg_functional.py

+        with self._tracer.start_as_current_span("rootSpan"):
+            try:
+                self._cursor.callproc("test", ())
+            except Exception as err:


does this exception need to be caught? seems like it's fine to let it raise up.

Even if it's unrelated to our code, exceptions are a separate code path that we should exercise explicitly.

Stored procedure doesn't exist so callproc will fail but Span will be created, I want to avoid as much pre setup as possible for this tests

You might want an assertRaises here instead then, the tests generally shouldn't print anything.

c24t

Similar comments to #526. I've got reservations about the autouse fixture and sleeping/retrying in unit tests, but we can address that in another PR. The only blocking comment here is about the printed exception.

c24t · 2020-04-01T22:55:50Z

ext/opentelemetry-ext-docker-tests/tests/postgres/test_psycopg_functional.py

+        with self._tracer.start_as_current_span("rootSpan"):
+            try:
+                self._cursor.callproc("test", ())
+            except Exception as err:


You might want an assertRaises here instead then, the tests generally shouldn't print anything.

c24t · 2020-04-01T22:58:38Z

ext/opentelemetry-ext-docker-tests/tests/postgres/test_psycopg_functional.py

+    def connect_to_db(self):
+        if self._connection is None:
+            # Try to connect to DB
+            for i in range(5):


Another option is to fail immediately in the tests and replace docker-compose -up with a script that does the same and blocks until the container can accept connections. If possible I think it's a good idea to avoid sleeping at all in tests, and between this and #526 we're looking at adding up to a minute of idle time to an already-slow test suite.

toumorokoshi

Thanks for refactoring out the pytest! (Although I personally would love full pytest).

I still think there's some improvement in the connectivity checks that should be addressed, or else it'll provide incomplete information to others if it fails. Once that's fixed up I'll switch to approved.

Thanks for focusing on this!

toumorokoshi · 2020-04-04T04:43:56Z

ext/opentelemetry-ext-docker-tests/tests/check_availability.py

+def check_docker_services_availability():
+    try:
+        print("Checking Postgres availability")
+        check_postgres_connection()


should mongo availability also be checked?

MongoDB have been super fast to be available, will add the logic just to be safe in case this changes.

ext/opentelemetry-ext-docker-tests/tests/check_availability.py

c24t

Thanks for addressing all the feedback @hectorhdzg, no more blocking comments from me.

toumorokoshi

LGTM, thanks for addressing that!

hectorhdzg added 3 commits March 17, 2020 16:05

WIP

f90e503

Adding Postgres functional tests

db8b1f5

Merge branch 'hectorhdzg/psycopgTests' of https://github.com/hectorhd…

8a79875

…zg/opentelemetry-python into hectorhdzg/psycopgTests

hectorhdzg requested a review from a team March 25, 2020 20:12

hectorhdzg added ext needs reviewers PRs with this label are ready for review and needs people to review to move forward. tests labels Mar 25, 2020

hectorhdzg changed the title ~~Adding functional tests for pysopg2 integration~~ Adding functional tests for psycopg2 integration Mar 25, 2020

hectorhdzg added 2 commits March 26, 2020 10:25

Adding exception when db cannot connect

9893d3a

Using else

173a450

hectorhdzg added 2 commits March 27, 2020 11:31

Addressing comments

d56907d

Undo change made by mistake

0cb2bb4

toumorokoshi suggested changes Apr 1, 2020

View reviewed changes

c24t reviewed Apr 1, 2020

View reviewed changes

Addressing comments

9556c18

toumorokoshi suggested changes Apr 4, 2020

View reviewed changes

toumorokoshi reviewed Apr 4, 2020

View reviewed changes

ext/opentelemetry-ext-docker-tests/tests/check_availability.py Outdated Show resolved Hide resolved

Addressing comments

496f739

c24t approved these changes Apr 7, 2020

View reviewed changes

toumorokoshi approved these changes Apr 9, 2020

View reviewed changes

Merge branch 'master' into hectorhdzg/psycopgTests

582ce1c

toumorokoshi merged commit 08b100f into open-telemetry:master Apr 9, 2020

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

chore: add .vscode to gitignore (open-telemetry#528)

6f56c37

Conversation

hectorhdzg commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

toumorokoshi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

toumorokoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hectorhdzg commented Mar 25, 2020 •

edited

Loading

codecov-io commented Mar 27, 2020 •

edited

Loading

toumorokoshi left a comment •

edited

Loading