test(sqlite3): add uninstrument, error status, suppress, and no-op tests#4335
test(sqlite3): add uninstrument, error status, suppress, and no-op tests#4335archy-rock3t-cloud wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
Signed-off-by: Artem Muterko <artem@sopho.tech>
JWinermaSplunk
left a comment
JWinermaSplunk
left a comment
There was a problem hiding this comment.
Few comments, also missing a CHANGELOG entry whenever you have a moment.
| spans_list = self.memory_exporter.get_finished_spans() | ||
| self.assertEqual(len(spans_list), 0) | ||
|
|
||
| def test_span_failed(self): |
There was a problem hiding this comment.
Did we want to add something like this for consistency?
| spans_list = self.memory_exporter.get_finished_spans() | ||
| self.assertEqual(len(spans_list), 1) | ||
|
|
||
| def test_uninstrument_connection(self): |
There was a problem hiding this comment.
Seems like this method closely resembles this method from psycopg2. Did we want to use the same query after uninstrumenting like the previously existing test? Also, should we add with_instrument to the method name for consistency?
| self.validate_spans("test") | ||
|
|
||
|
|
||
| class TestSQLite3Integration(TestBase): |
There was a problem hiding this comment.
Are we sure we need a separate integration class? If so, I think we need to update tearDown to close cursor and connections as well?
| cnx = sqlite3.connect(":memory:") | ||
| cursor = cnx.cursor() | ||
|
|
||
| with self.assertRaises(Exception): |
There was a problem hiding this comment.
Do we want with self.assertRaises(sqlite3.OperationalError):?
| self.assertEqual(len(spans_list), 1) | ||
|
|
||
| cnx = SQLite3Instrumentor.uninstrument_connection(cnx) | ||
| cursor = cnx.cursor() |
There was a problem hiding this comment.
Would it be better to self.memory_exporter.clear() here and assert the span count was 0 after uninstrumenting?
|
LGTM, just needs a |
4 participants
Description
The sqlite3 instrumentation had only 3 tests covering basic execute/executemany/callproc. This adds the standard lifecycle tests that other dbapi-based instrumentations (psycopg2, mysql) already have: uninstrument, uninstrument_connection, error status on failed queries, suppress_instrumentation, and NoOpTracerProvider.
Type of change
How Has This Been Tested?
tox -e py314-test-instrumentation-sqlite3— all 8 tests pass (3 existing + 5 new).Does This PR Require a Core Repo Change?
Checklist: