From cc57aacd71c3eff8fac4030ca3013546dd4e545a Mon Sep 17 00:00:00 2001 From: Owais Lone Date: Wed, 1 Sep 2021 15:36:13 +0530 Subject: [PATCH] Improve reliability of tests (#643) * Run tests on Windows in Github Actions * core sha update * format code * fix ci yaml * rebase * lint * Try without win+py3.6 fix * Try without win+py3.6 fix * Improve test reliability Update some tests to use more deterministic methods of testing in memory spans. This helps the core repo pass tests after adding Windows to CI matrix. --- .github/workflows/test.yml | 2 +- .../tests/test_datadog_exporter.py | 5 ++ .../tests/test_botocore_instrumentation.py | 36 +++++++++----- .../tests/test_elasticsearch.py | 47 ++++++++----------- .../tests/test_instrumentation.py | 14 ++++-- tox.ini | 3 +- 6 files changed, 61 insertions(+), 46 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7851e2fe5..2926a019a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -6,7 +6,7 @@ on: - 'release/*' pull_request: env: - CORE_REPO_SHA: 9dc17e33bc083e38c1fd55b8ed6787caba8f54fe + CORE_REPO_SHA: c49ad57bfe35cfc69bfa863d74058ca9bec55fc3 jobs: build: diff --git a/exporter/opentelemetry-exporter-datadog/tests/test_datadog_exporter.py b/exporter/opentelemetry-exporter-datadog/tests/test_datadog_exporter.py index c6fe2fc27..7d44f734e 100644 --- a/exporter/opentelemetry-exporter-datadog/tests/test_datadog_exporter.py +++ b/exporter/opentelemetry-exporter-datadog/tests/test_datadog_exporter.py @@ -14,12 +14,14 @@ import itertools import logging +import sys import time import unittest from unittest import mock from ddtrace.internal.writer import AgentWriter from flaky import flaky +from pytest import mark from opentelemetry import trace as trace_api from opentelemetry.context import Context @@ -469,6 +471,9 @@ class TestDatadogSpanExporter(unittest.TestCase): self.assertEqual(len(datadog_spans), 128) tracer_provider.shutdown() + @mark.skipif( + sys.platform == "win32", reason="unreliable test on windows", + ) def test_span_processor_scheduled_delay(self): """Test that spans are exported each schedule_delay_millis""" delay = 300 diff --git a/instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py b/instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py index a9d4eb261..fda6220ca 100644 --- a/instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py @@ -13,6 +13,7 @@ # limitations under the License. import io import json +import sys import zipfile from unittest.mock import Mock, patch @@ -30,6 +31,7 @@ from moto import ( # pylint: disable=import-error mock_sts, mock_xray, ) +from pytest import mark from opentelemetry import trace as trace_api from opentelemetry.context import attach, detach, set_value @@ -128,12 +130,13 @@ class TestBotocoreInstrumentor(TestBase): s3.list_buckets() s3.list_buckets() - spans = self.memory_exporter.get_finished_spans() + spans = self.get_finished_spans() assert spans - span = spans[0] self.assertEqual(len(spans), 2) + + buckets_span = spans.by_attr("aws.operation", "ListBuckets") self.assertSpanHasAttributes( - span, + buckets_span, { "aws.operation": "ListBuckets", "aws.region": "us-west-2", @@ -144,14 +147,13 @@ class TestBotocoreInstrumentor(TestBase): ) # testing for span error - self.memory_exporter.get_finished_spans() with self.assertRaises(ParamValidationError): s3.list_objects(bucket="mybucket") - spans = self.memory_exporter.get_finished_spans() + spans = self.get_finished_spans() assert spans - span = spans[2] + objects_span = spans.by_attr("aws.operation", "ListObjects") self.assertSpanHasAttributes( - span, + objects_span, { "aws.operation": "ListObjects", "aws.region": "us-west-2", @@ -159,7 +161,7 @@ class TestBotocoreInstrumentor(TestBase): }, ) self.assertIs( - span.status.status_code, trace_api.StatusCode.ERROR, + objects_span.status.status_code, trace_api.StatusCode.ERROR, ) # Comment test for issue 1088 @@ -172,10 +174,11 @@ class TestBotocoreInstrumentor(TestBase): s3.put_object(**params) s3.get_object(Bucket="mybucket", Key="foo") - spans = self.memory_exporter.get_finished_spans() + spans = self.get_finished_spans() assert spans self.assertEqual(len(spans), 3) - create_span = spans[0] + + create_span = spans.by_attr("aws.operation", "CreateBucket") self.assertSpanHasAttributes( create_span, { @@ -186,7 +189,8 @@ class TestBotocoreInstrumentor(TestBase): SpanAttributes.HTTP_STATUS_CODE: 200, }, ) - put_span = spans[1] + + put_span = spans.by_attr("aws.operation", "PutObject") self.assertSpanHasAttributes( put_span, { @@ -197,8 +201,10 @@ class TestBotocoreInstrumentor(TestBase): SpanAttributes.HTTP_STATUS_CODE: 200, }, ) - self.assertTrue("params.Body" not in spans[1].attributes.keys()) - get_span = spans[2] + self.assertTrue("params.Body" not in put_span.attributes.keys()) + + get_span = spans.by_attr("aws.operation", "GetObject") + self.assertSpanHasAttributes( get_span, { @@ -359,6 +365,10 @@ class TestBotocoreInstrumentor(TestBase): Path="/my-path/", )["Role"]["Arn"] + @mark.skipif( + sys.platform == "win32", + reason="requires docker and Github CI Windows does not have docker installed by default", + ) @mock_lambda def test_lambda_invoke_propagation(self): diff --git a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py index 3b99238c6..f2eaf509a 100644 --- a/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py +++ b/instrumentation/opentelemetry-instrumentation-elasticsearch/tests/test_elasticsearch.py @@ -60,19 +60,13 @@ class TestElasticsearchIntegration(TestBase): with self.disable_logging(): ElasticsearchInstrumentor().uninstrument() - def get_ordered_finished_spans(self): - return sorted( - self.memory_exporter.get_finished_spans(), - key=lambda s: s.start_time, - ) - def test_instrumentor(self, request_mock): request_mock.return_value = (1, {}, {}) es = Elasticsearch() es.index(index="sw", doc_type="people", id=1, body={"name": "adam"}) - spans_list = self.get_ordered_finished_spans() + spans_list = self.get_finished_spans() self.assertEqual(len(spans_list), 1) span = spans_list[0] @@ -87,7 +81,7 @@ class TestElasticsearchIntegration(TestBase): es.index(index="sw", doc_type="people", id=1, body={"name": "adam"}) - spans_list = self.get_ordered_finished_spans() + spans_list = self.get_finished_spans() self.assertEqual(len(spans_list), 1) def test_span_not_recording(self, request_mock): @@ -127,7 +121,7 @@ class TestElasticsearchIntegration(TestBase): es = Elasticsearch() es.index(index="sw", doc_type="people", id=1, body={"name": "adam"}) - spans_list = self.get_ordered_finished_spans() + spans_list = self.get_finished_spans() self.assertEqual(len(spans_list), 1) span = spans_list[0] self.assertTrue(span.name.startswith(prefix)) @@ -141,7 +135,7 @@ class TestElasticsearchIntegration(TestBase): es = Elasticsearch() es.get(index="test-index", doc_type="tweet", id=1) - spans = self.get_ordered_finished_spans() + spans = self.get_finished_spans() self.assertEqual(1, len(spans)) self.assertEqual("False", spans[0].attributes["elasticsearch.found"]) @@ -169,7 +163,7 @@ class TestElasticsearchIntegration(TestBase): except Exception: # pylint: disable=broad-except pass - spans = self.get_ordered_finished_spans() + spans = self.get_finished_spans() self.assertEqual(1, len(spans)) span = spans[0] self.assertFalse(span.status.is_ok) @@ -186,13 +180,13 @@ class TestElasticsearchIntegration(TestBase): index="sw", doc_type="people", id=1, body={"name": "adam"} ) - spans = self.get_ordered_finished_spans() + spans = self.get_finished_spans() self.assertEqual(len(spans), 2) - self.assertEqual(spans[0].name, "parent") - self.assertEqual(spans[1].name, "Elasticsearch/sw/people/1") - self.assertIsNotNone(spans[1].parent) - self.assertEqual(spans[1].parent.span_id, spans[0].context.span_id) + parent = spans.by_name("parent") + child = spans.by_name("Elasticsearch/sw/people/1") + self.assertIsNotNone(child.parent) + self.assertEqual(child.parent.span_id, parent.context.span_id) def test_multithread(self, request_mock): request_mock.return_value = (1, {}, {}) @@ -222,16 +216,15 @@ class TestElasticsearchIntegration(TestBase): t1.join() t2.join() - spans = self.get_ordered_finished_spans() + spans = self.get_finished_spans() self.assertEqual(3, len(spans)) - s1, s2, s3 = spans - self.assertEqual(s1.name, "parent") + s1 = spans.by_name("parent") + s2 = spans.by_name("Elasticsearch/test-index/tweet/1") + s3 = spans.by_name("Elasticsearch/test-index/tweet/2") - self.assertEqual(s2.name, "Elasticsearch/test-index/tweet/1") self.assertIsNotNone(s2.parent) self.assertEqual(s2.parent.span_id, s1.context.span_id) - self.assertEqual(s3.name, "Elasticsearch/test-index/tweet/2") self.assertIsNone(s3.parent) def test_dsl_search(self, request_mock): @@ -242,7 +235,7 @@ class TestElasticsearchIntegration(TestBase): "term", author="testing" ) search.execute() - spans = self.get_ordered_finished_spans() + spans = self.get_finished_spans() span = spans[0] self.assertEqual(1, len(spans)) self.assertEqual(span.name, "Elasticsearch/test-index/_search") @@ -270,10 +263,11 @@ class TestElasticsearchIntegration(TestBase): client = Elasticsearch() Article.init(using=client) - spans = self.get_ordered_finished_spans() + spans = self.get_finished_spans() self.assertEqual(2, len(spans)) - span1, span2 = spans - self.assertEqual(span1.name, "Elasticsearch/test-index") + span1 = spans.by_attr(key="elasticsearch.method", value="HEAD") + span2 = spans.by_attr(key="elasticsearch.method", value="PUT") + self.assertEqual( span1.attributes, { @@ -283,7 +277,6 @@ class TestElasticsearchIntegration(TestBase): }, ) - self.assertEqual(span2.name, "Elasticsearch/test-index") attributes = { SpanAttributes.DB_SYSTEM: "elasticsearch", "elasticsearch.url": "/test-index", @@ -306,7 +299,7 @@ class TestElasticsearchIntegration(TestBase): ) res = article.save(using=client) self.assertTrue(res) - spans = self.get_ordered_finished_spans() + spans = self.get_finished_spans() self.assertEqual(1, len(spans)) span = spans[0] self.assertEqual(span.name, helpers.dsl_index_span_name) diff --git a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py index 5ddece439..249b730d2 100644 --- a/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py @@ -185,15 +185,19 @@ class TestTornadoInstrumentation(TornadoTest): def _test_async_handler(self, url, handler_name): response = self.fetch(url) self.assertEqual(response.code, 201) - spans = self.memory_exporter.get_finished_spans() + spans = self.get_finished_spans() self.assertEqual(len(spans), 5) - sub2, sub1, sub_wrapper, server, client = self.sorted_spans(spans) + client = spans.by_name("GET") + server = spans.by_name(handler_name + ".get") + sub_wrapper = spans.by_name("sub-task-wrapper") + sub2 = spans.by_name("sub-task-2") self.assertEqual(sub2.name, "sub-task-2") self.assertEqual(sub2.parent, sub_wrapper.context) self.assertEqual(sub2.context.trace_id, client.context.trace_id) + sub1 = spans.by_name("sub-task-1") self.assertEqual(sub1.name, "sub-task-1") self.assertEqual(sub1.parent, sub_wrapper.context) self.assertEqual(sub1.context.trace_id, client.context.trace_id) @@ -238,9 +242,11 @@ class TestTornadoInstrumentation(TornadoTest): response = self.fetch("/error") self.assertEqual(response.code, 500) - spans = self.sorted_spans(self.memory_exporter.get_finished_spans()) + spans = self.get_finished_spans() self.assertEqual(len(spans), 2) - server, client = spans + + client = spans.by_name("GET") + server = spans.by_name("BadHandler.get") self.assertEqual(server.name, "BadHandler.get") self.assertEqual(server.kind, SpanKind.SERVER) diff --git a/tox.ini b/tox.ini index 70a896860..5c20828a3 100644 --- a/tox.ini +++ b/tox.ini @@ -122,7 +122,8 @@ envlist = py3{6,7,8,9}-test-instrumentation-grpc ; opentelemetry-instrumentation-sqlalchemy - py3{6,7,8,9}-test-instrumentation-sqlalchemy{11,14} + py3{6,7}-test-instrumentation-sqlalchemy{11} + py3{6,7,8,9}-test-instrumentation-sqlalchemy{14} pypy3-test-instrumentation-sqlalchemy{11,14} ; opentelemetry-instrumentation-redis