Add better reliability in E2E tests

[hectorhdzg]

Handling exceptions of external libraries to improve reliability of tests

Fixes #589

[codeboten]

Thanks for taking on this work! More reliability is always great with tests :D

I'm likely missing something, can you explain how the assertion that exceptions are raised works in python 3.8? I'm assuming the tests were passing because they were not throwing exception before this change, how would they work with this assert?

[hectorhdzg]

@codeboten sorry I pushed wrong changes of some experiments I was working on, and then I realized there is some weirdness going on when using "with assertRaises" the tests were not actually throwing an exception and passed anyways, my change is basically focused on ignoring if there is any issue when calling the external library, so we don't need to care if something is broken in their side at any point, we just validate OT integrations are actually patching and creating Spans correctly

[ocelotl]

I don't understand this change. The try block is also catching an exception raised in start_as_current_span. This should not be, we want our tests to fail if there is an exception raised there. If the intention of this change is to catch an AssertionError caused by an Exception not being raised while running self._cursor.execute then the except block in line 88 should only be catching AssertionError exceptions.

[hectorhdzg]

Added try inside start_as_current_span, the only problem I can think is if self._cursor.execute patch inside integrations code throws an error this test will not catch it

[hectorhdzg]

The reasoning behind this change is that we don't care about the actual query to be successful, we only care about Span created correctly when the query is executed

[ocelotl]

Same here and in the rest of these changes.

[mauriciovasquezbernal]

I think we should not ignore the exceptions from the queries. The test could be designed taking into consideration that the operation was successful, ignoring the exception would hide that potential problem and tests could fail with a misleading error.

I analyzed more in deep the problem and I think the data that's being passed to the query is wrong, param in cursor.execute() should be a tuple or a dictionary, and in cursor.executemany() it is a sequence of parameters.

Given that, the following diff fixes the problem for me:

--- ext/opentelemetry-ext-docker-tests/tests/mysql/test_mysql_functional.py
+++ ext/opentelemetry-ext-docker-tests/tests/mysql/test_mysql_functional.py
@@ -82,7 +82,7 @@ class TestFunctionalMysql(TestBase):
         """Should create a child span for executemany
         """
         with self._tracer.start_as_current_span("rootSpan"):
-            data = ["1", "2", "3"]
+            data = (("1",), ("2",), ("3",))
             stmt = "INSERT INTO test (id) VALUES (%s)"
             self._cursor.executemany(stmt, data)
         self.validate_spans()

I have no idea why it works in Python3.8.

What do you think about using this solution instead?

[1] https://dev.mysql.com/doc/connector-python/en/connector-python-api-mysqlcursor-execute.html
[2] https://dev.mysql.com/doc/connector-python/en/connector-python-api-mysqlcursor-executemany.html

[hectorhdzg]

Thanks @mauriciovasquezbernal, @ocelotl and @codeboten , I know ignoring the actual exception could be a polemic topic, multiple people already let me know they don't agree with this approach, and is fine for me I was just trying to think a way to not need to worry about any other issue like this one in the future and in fact reduce maintenance of the tests. I will try @mauriciovasquezbernal suggestion just to fix current issue in Python 3.7 in a different PR.