From dee01548dcd5e4b88913eb353e726046f8096ff0 Mon Sep 17 00:00:00 2001 From: Romain Komorn Date: Thu, 26 Oct 2023 13:33:04 +0100 Subject: [PATCH 1/4] Add safety checks and exception handling around _fetch_tests_to_skip --- ddtrace/internal/ci_visibility/recorder.py | 30 +-- tests/ci_visibility/test_ci_visibility.py | 214 +++++++++++++++++++++ 2 files changed, 233 insertions(+), 11 deletions(-) diff --git a/ddtrace/internal/ci_visibility/recorder.py b/ddtrace/internal/ci_visibility/recorder.py index 8ffd8f860cd..9b53442abbc 100644 --- a/ddtrace/internal/ci_visibility/recorder.py +++ b/ddtrace/internal/ci_visibility/recorder.py @@ -313,13 +313,12 @@ def _fetch_tests_to_skip(self, skipping_mode): response = _do_request("POST", url, json.dumps(payload), _headers) except TimeoutError: log.warning("Request timeout while fetching skippable tests") - self._test_suites_to_skip = [] return self._test_suites_to_skip = [] if response.status >= 400: - log.warning("Test skips request responded with status %d", response.status) + log.warning("Skippable tests request responded with status %d", response.status) return try: if isinstance(response.body, bytes): @@ -327,18 +326,27 @@ def _fetch_tests_to_skip(self, skipping_mode): else: parsed = json.loads(response.body) except json.JSONDecodeError: - log.warning("Test skips request responded with invalid JSON '%s'", response.body) + log.warning("Skippable tests request responded with invalid JSON '%s'", response.body) return - for item in parsed["data"]: - if item["type"] == skipping_mode and "suite" in item["attributes"]: - module = item["attributes"].get("configurations", {}).get("test.bundle", "").replace(".", "/") - path = "/".join((module, item["attributes"]["suite"])) if module else item["attributes"]["suite"] + if "data" not in parsed: + log.warning("Skippable tests request missing data, no tests will be skipped") + return - if skipping_mode == SUITE: - self._test_suites_to_skip.append(path) - else: - self._tests_to_skip[path].append(item["attributes"]["name"]) + try: + for item in parsed["data"]: + if item["type"] == skipping_mode and "suite" in item["attributes"]: + module = item["attributes"].get("configurations", {}).get("test.bundle", "").replace(".", "/") + path = "/".join((module, item["attributes"]["suite"])) if module else item["attributes"]["suite"] + + if skipping_mode == SUITE: + self._test_suites_to_skip.append(path) + else: + self._tests_to_skip[path].append(item["attributes"]["name"]) + except Exception: + log.warning("Error processing skippable test data, no tests will be skipped", exc_info=True) + self._test_suites_to_skip = [] + self._tests_to_skip = {} def _should_skip_path(self, path, name, test_skipping_mode=None): if test_skipping_mode is None: diff --git a/tests/ci_visibility/test_ci_visibility.py b/tests/ci_visibility/test_ci_visibility.py index feeaaccce5c..a624027e5ef 100644 --- a/tests/ci_visibility/test_ci_visibility.py +++ b/tests/ci_visibility/test_ci_visibility.py @@ -1,5 +1,7 @@ +from collections import defaultdict import json import os +import textwrap import time import mock @@ -10,6 +12,8 @@ from ddtrace.ext import ci from ddtrace.internal.ci_visibility import CIVisibility from ddtrace.internal.ci_visibility.constants import REQUESTS_MODE +from ddtrace.internal.ci_visibility.constants import SUITE +from ddtrace.internal.ci_visibility.constants import TEST from ddtrace.internal.ci_visibility.encoder import CIVisibilityEncoderV01 from ddtrace.internal.ci_visibility.filters import TraceCiVisibilityFilter from ddtrace.internal.ci_visibility.git_client import CIVisibilityGitClient @@ -1028,3 +1032,213 @@ def test_encoder_py2_payload_force_unicode_strings(): assert CIVisibilityEncoderV01._py2_payload_force_unicode_strings( {"string_key": [1, {u"unicode_key": "string_value"}, u"unicode_value", {"string_key": u"unicode_value"}]} ) == {u"string_key": [1, {u"unicode_key": u"string_value"}, u"unicode_value", {u"string_key": u"unicode_value"}]} + + +class TestFetchTestsToSkip: + @pytest.fixture(scope="function") + def mock_civisibility(self): + with mock.patch.object(CIVisibility, "__init__", return_value=None): + _civisibility = CIVisibility() + _civisibility._api_key = "notanapikey" + _civisibility._dd_site = "notdatadog.notcom" + _civisibility._service = "test-service" + _civisibility._git_client = None + _civisibility._requests_mode = REQUESTS_MODE.AGENTLESS_EVENTS + _civisibility._tags = { + ci.git.REPOSITORY_URL: "test_repo_url", + ci.git.COMMIT_SHA: "testcommitsssshhhaaaa1234", + } + + yield _civisibility + CIVisibility._test_suites_to_skip = None + CIVisibility._tests_to_skip = defaultdict(list) + + @pytest.fixture(scope="class", autouse=True) + def _test_context_manager(self): + with mock.patch("ddtrace.ext.ci._get_runtime_and_os_metadata"), mock.patch("json.dumps", return_value=""): + yield + + def test_fetch_tests_to_skip_test_level(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body=textwrap.dedent( + """{ + "data": [ + { + "id": "123456789", + "type": "test", + "attributes": { + "configurations": { + "test.bundle": "testbundle" + }, + "name": "test_name_1", + "suite": "test_suite_1.py" + } + }, + { + "id": "987654321", + "type": "test", + "attributes": { + "configurations": { + "test.bundle": "testpackage/testbundle" + }, + "name": "test_name_2", + "suite": "test_suite_2.py" + } + } + ] + }""" + ), + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(TEST) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == { + "testbundle/test_suite_1.py": ["test_name_1"], + "testpackage/testbundle/test_suite_2.py": ["test_name_2"], + } + + def test_fetch_tests_to_skip_suite_level(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body=textwrap.dedent( + """{ + "data": [ + { + "id": "34640cc7ce80c01e", + "type": "suite", + "attributes": { + "configurations": { + "test.bundle": "testbundle" + }, + "suite": "test_module_1.py" + } + }, + { + "id": "239fa7de754db779", + "type": "suite", + "attributes": { + "configurations": { + "test.bundle": "testpackage/testbundle" + }, + "suite": "test_suite_2.py" + } + } + ] + }""" + ), + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip == [ + "testbundle/test_module_1.py", + "testpackage/testbundle/test_suite_2.py", + ] + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_tests_to_skip_no_data_test_level(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body="{}", + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(TEST) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_tests_to_skip_no_data_suite_level(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body="{}", + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_tests_to_skip_data_is_none_test_level(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body='{"data": null}', + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(TEST) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_tests_to_skip_data_is_none_suite_level(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body='{"data": null}', + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_tests_to_skip_bad_json(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body="{ this is not valid JSON { ", + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_tests_to_skip_bad_response(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=500, + body="Internal server error", + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_tests_to_skip_timeout_error(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + side_effect=TimeoutError, + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip is None + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_test_to_skip_invalid_data(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body='{"data": [{"somekey": "someval"}]}', + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} From 52f323f6f74a2148621a91de7afdc512450d199a Mon Sep 17 00:00:00 2001 From: Romain Komorn Date: Thu, 26 Oct 2023 13:46:40 +0100 Subject: [PATCH 2/4] add some tests --- ddtrace/internal/ci_visibility/recorder.py | 2 +- tests/ci_visibility/test_ci_visibility.py | 52 +++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/ddtrace/internal/ci_visibility/recorder.py b/ddtrace/internal/ci_visibility/recorder.py index 9b53442abbc..f6554e8ac96 100644 --- a/ddtrace/internal/ci_visibility/recorder.py +++ b/ddtrace/internal/ci_visibility/recorder.py @@ -346,7 +346,7 @@ def _fetch_tests_to_skip(self, skipping_mode): except Exception: log.warning("Error processing skippable test data, no tests will be skipped", exc_info=True) self._test_suites_to_skip = [] - self._tests_to_skip = {} + self._tests_to_skip = defaultdict(list) def _should_skip_path(self, path, name, test_skipping_mode=None): if test_skipping_mode is None: diff --git a/tests/ci_visibility/test_ci_visibility.py b/tests/ci_visibility/test_ci_visibility.py index a624027e5ef..d9f0cd4913d 100644 --- a/tests/ci_visibility/test_ci_visibility.py +++ b/tests/ci_visibility/test_ci_visibility.py @@ -1230,7 +1230,7 @@ def test_fetch_tests_to_skip_timeout_error(self, mock_civisibility): assert mock_civisibility._test_suites_to_skip is None assert mock_civisibility._tests_to_skip == {} - def test_fetch_test_to_skip_invalid_data(self, mock_civisibility): + def test_fetch_test_to_skip_invalid_data_missing_key(self, mock_civisibility): with mock.patch( "ddtrace.internal.ci_visibility.recorder._do_request", return_value=Response( @@ -1242,3 +1242,53 @@ def test_fetch_test_to_skip_invalid_data(self, mock_civisibility): mock_civisibility._fetch_tests_to_skip(SUITE) assert mock_civisibility._test_suites_to_skip == [] assert mock_civisibility._tests_to_skip == {} + + def test_fetch_test_to_skip_invalid_data_type_error(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body=textwrap.dedent( + """{ + "data": [ { + "id": "12345", + "type": "suite", + "attributes": { + "configurations": { + "test.bundle": "2" + }, + "suite": 1 + } + }]}""", + ), + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} + + def test_fetch_test_to_skip_invalid_data_attribute_error(self, mock_civisibility): + with mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", + return_value=Response( + status=200, + body=textwrap.dedent( + """{ + "data": [ { + "id": "12345", + "type": "suite", + "attributes": { + "configurations": { + "test.bundle": 2 + }, + "suite": "1" + } + }]}""", + ), + ), + ): + ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() + mock_civisibility._fetch_tests_to_skip(SUITE) + assert mock_civisibility._test_suites_to_skip == [] + assert mock_civisibility._tests_to_skip == {} From 51d7e3101e915d2d922a04c13c8ae5b0458fa52a Mon Sep 17 00:00:00 2001 From: Romain Komorn <136473744+romainkomorndatadog@users.noreply.github.com> Date: Fri, 27 Oct 2023 15:15:44 +0100 Subject: [PATCH 3/4] Update tests/ci_visibility/test_ci_visibility.py --- tests/ci_visibility/test_ci_visibility.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/ci_visibility/test_ci_visibility.py b/tests/ci_visibility/test_ci_visibility.py index 7a64da21dff..bbfa8bd4f10 100644 --- a/tests/ci_visibility/test_ci_visibility.py +++ b/tests/ci_visibility/test_ci_visibility.py @@ -1293,6 +1293,7 @@ def test_fetch_test_to_skip_invalid_data_attribute_error(self, mock_civisibility assert mock_civisibility._test_suites_to_skip == [] assert mock_civisibility._tests_to_skip == {} + def test_fetch_tests_to_skip_custom_configurations(): with override_env( dict( From f2fe6d40cce36ae2113f6498b930a2fab30ea36b Mon Sep 17 00:00:00 2001 From: Romain Komorn Date: Mon, 30 Oct 2023 10:08:31 +0000 Subject: [PATCH 4/4] Unbreak test, and remove other unnecessary test --- ddtrace/internal/ci_visibility/recorder.py | 1 + tests/ci_visibility/test_ci_visibility.py | 10 ---------- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/ddtrace/internal/ci_visibility/recorder.py b/ddtrace/internal/ci_visibility/recorder.py index a77aba9fbe2..c72600a8e7b 100644 --- a/ddtrace/internal/ci_visibility/recorder.py +++ b/ddtrace/internal/ci_visibility/recorder.py @@ -329,6 +329,7 @@ def _fetch_tests_to_skip(self, skipping_mode): response = _do_request("POST", url, json.dumps(payload), _headers) except TimeoutError: log.warning("Request timeout while fetching skippable tests") + self._test_suites_to_skip = [] return self._test_suites_to_skip = [] diff --git a/tests/ci_visibility/test_ci_visibility.py b/tests/ci_visibility/test_ci_visibility.py index bbfa8bd4f10..ec21a3605c1 100644 --- a/tests/ci_visibility/test_ci_visibility.py +++ b/tests/ci_visibility/test_ci_visibility.py @@ -1220,16 +1220,6 @@ def test_fetch_tests_to_skip_bad_response(self, mock_civisibility): assert mock_civisibility._test_suites_to_skip == [] assert mock_civisibility._tests_to_skip == {} - def test_fetch_tests_to_skip_timeout_error(self, mock_civisibility): - with mock.patch( - "ddtrace.internal.ci_visibility.recorder._do_request", - side_effect=TimeoutError, - ): - ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config() - mock_civisibility._fetch_tests_to_skip(SUITE) - assert mock_civisibility._test_suites_to_skip is None - assert mock_civisibility._tests_to_skip == {} - def test_fetch_test_to_skip_invalid_data_missing_key(self, mock_civisibility): with mock.patch( "ddtrace.internal.ci_visibility.recorder._do_request",