test: Add tests for CTX manager usage on PEP0249. #448

pvital · 2023-08-30T13:54:11Z

Add the test_connect_ctx_mngr UT to test the span creation when using the psycopg2 connect with context manager, and the test_connect_cursor_ctx_mngr UT when using the psycopg2 connect and cursos context manager.

Ferenc-

Please add a TC where the db is not managed, but the cursor is.
Please cover the rest of the instrumentations where PEP0248 is used, like mysql.

GSVarsha · 2023-08-31T06:27:35Z

Hi @pvital ,
I've also been adding tests to db modules that conform to PEP-249 in this branch.
Do you think we should somehow merge changes from these 2 branches?
I've also covered pymysql and mysqlclient here.

Add the test_connect_ctx_mngr UT to test the span creation when using the psycopg2 connect with context manager, and the test_connect_cursor_ctx_mngr UT when using the psycopg2 connect and cursos context manager. Signed-off-by: Paulo Vital <[email protected]>

pvital · 2023-08-31T07:44:06Z

I am closing this PR once we use #450 to track this fix.

Ferenc- · 2023-08-31T07:44:15Z

tests/clients/test_mysqlclient.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:45:07Z

tests/clients/test_mysqlclient.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:45:16Z

tests/clients/test_mysqlclient.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:45:23Z

tests/clients/test_psycopg2.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:45:36Z

tests/clients/test_psycopg2.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:45:44Z

tests/clients/test_psycopg2.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:45:53Z

tests/clients/test_pymysql.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:46:13Z

tests/clients/test_pymysql.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:46:26Z

tests/clients/test_pymysql.py

+        self.assertEqual(test_span.t, db_span.t)
+        self.assertEqual(db_span.p, test_span.s)
+
+        self.assertEqual(None, db_span.ec)


This should be assertIsNone

Ferenc- · 2023-08-31T07:51:38Z

tests/clients/test_mysqlclient.py

+                    result = cursor.execute("""SELECT * from users""")
+                    cursor.fetchone()
+
+        assert(result >= 0)


This assert is for catching runtime anomalies in the production code, after it is deployed.
For unit testing we should use assertTrue, if nothing else fits better.

Ferenc- · 2023-08-31T07:54:34Z

tests/clients/test_mysqlclient.py

+                result = cursor.execute("""SELECT * from users""")
+                cursor.fetchone()
+
+        assert(result >= 0)


This assert is for catching runtime anomalies in the production code, after it is deployed.
For unit testing we should use assertTrue, if nothing else fits better.

Ferenc- · 2023-08-31T07:54:43Z

tests/clients/test_mysqlclient.py

+                cursor.fetchone()
+            self.db.close()
+
+        assert(result >= 0)


This assert is for catching runtime anomalies in the production code, after it is deployed.
For unit testing we should use assertTrue, if nothing else fits better.

Ferenc- · 2023-08-31T07:54:52Z

tests/clients/test_pymysql.py

+                result = cursor.execute("""SELECT * from users""")
+                cursor.fetchone()
+
+        assert(result >= 0)


This assert is for catching runtime anomalies in the production code, after it is deployed.
For unit testing we should use assertTrue, if nothing else fits better.

Ferenc- · 2023-08-31T07:55:00Z

tests/clients/test_pymysql.py

+                cursor.fetchone()
+            self.db.close()
+
+        assert(result >= 0)


This assert is for catching runtime anomalies in the production code, after it is deployed.
For unit testing we should use assertTrue, if nothing else fits better.

Ferenc- · 2023-08-31T07:55:09Z

tests/clients/test_pymysql.py

+                    result = cursor.execute("""SELECT * from users""")
+                    cursor.fetchone()
+
+        assert(result >= 0)


This assert is for catching runtime anomalies in the production code, after it is deployed.
For unit testing we should use assertTrue, if nothing else fits better.

Ferenc- · 2023-08-31T07:59:01Z

tests/clients/test_mysqlclient.py

@@ -217,3 +217,93 @@ def test_error_capture(self):
        self.assertEqual(db_span.data["mysql"]["stmt"], 'SELECT * from blah')
        self.assertEqual(db_span.data["mysql"]["host"], testenv['mysql_host'])
        self.assertEqual(db_span.data["mysql"]["port"], testenv['mysql_port'])
+
+    def test_connect_ctx_mngr(self):
+        result = None


The context manager does not create a new scope, the result = cursor.execute( statement is enough. Please remove this line, luckily this is not Pascal or ANSI C, where you have to declare the variables way ahead of assigning them.

Ferenc- · 2023-08-31T07:59:22Z

tests/clients/test_mysqlclient.py

+        self.assertEqual(db_span.data["mysql"]["port"], testenv['mysql_port'])
+
+    def test_cursor_ctx_mngr(self):
+        result = None


Please remove.

Ferenc- · 2023-08-31T07:59:34Z

tests/clients/test_mysqlclient.py

+        self.assertEqual(db_span.data["mysql"]["port"], testenv['mysql_port'])
+
+    def test_connect_cursor_ctx_mngr(self):
+        result = None


Please remove.

Ferenc- · 2023-08-31T07:59:40Z

tests/clients/test_pymysql.py

@@ -243,3 +243,93 @@ def test_error_capture(self):
        self.assertEqual(db_span.data["mysql"]["stmt"], 'SELECT * from blah')
        self.assertEqual(db_span.data["mysql"]["host"], testenv['mysql_host'])
        self.assertEqual(db_span.data["mysql"]["port"], testenv['mysql_port'])
+
+    def test_connect_ctx_mngr(self):
+        result = None


Please remove.

Ferenc- · 2023-08-31T07:59:46Z

tests/clients/test_pymysql.py

+        self.assertEqual(db_span.data["mysql"]["port"], testenv['mysql_port'])
+
+    def test_cursor_ctx_mngr(self):
+        result = None


Please remove.

Ferenc- · 2023-08-31T07:59:53Z

tests/clients/test_pymysql.py

+        self.assertEqual(db_span.data["mysql"]["port"], testenv['mysql_port'])
+
+    def test_connect_cursor_ctx_mngr(self):
+        result = None


Please remove.

Ferenc-

This is just a preliminary review. With some minor formalities commented for now.

pvital · 2023-08-31T08:10:04Z

@Ferenc- as mentioned here, this PR is closed. Thanks anyway.

pvital requested review from Ferenc- and GSVarsha August 30, 2023 14:07

Ferenc- requested changes Aug 30, 2023

View reviewed changes

pvital force-pushed the pep0249_ctx_mngr_ut branch from f67eabc to ccf7ce0 Compare August 31, 2023 06:56

pvital requested a review from Ferenc- August 31, 2023 07:12

pvital closed this Aug 31, 2023

Ferenc- reviewed Aug 31, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add tests for CTX manager usage on PEP0249. #448

test: Add tests for CTX manager usage on PEP0249. #448

pvital commented Aug 30, 2023

Ferenc- left a comment

GSVarsha commented Aug 31, 2023 •

edited

Loading

pvital commented Aug 31, 2023 •

edited by GSVarsha

Loading

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- Aug 31, 2023

Ferenc- left a comment

pvital commented Aug 31, 2023

test: Add tests for CTX manager usage on PEP0249. #448

test: Add tests for CTX manager usage on PEP0249. #448

Conversation

pvital commented Aug 30, 2023

Ferenc- left a comment

Choose a reason for hiding this comment

GSVarsha commented Aug 31, 2023 • edited Loading

pvital commented Aug 31, 2023 • edited by GSVarsha Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ferenc- left a comment

Choose a reason for hiding this comment

pvital commented Aug 31, 2023

GSVarsha commented Aug 31, 2023 •

edited

Loading

pvital commented Aug 31, 2023 •

edited by GSVarsha

Loading