Skip to content

Commit da16e3d

Browse files
github-actions[bot]romainkomorn-exdatadogericlaz
authored
fix(ci_visibility): add safety checks and exception handling around ... [backport 2.0] (#7418)
... _fetch_tests_to_skip Backport 756ba7e from #7369 to 2.0. Prevents a poorly-structured (but still valid JSON) response from causing an exception while fetching tests to skip. An uncaught exception would cause the test runner to crash. The "bare" `except` is probably overly broad (based on the code, I can only see `AttributeError`, `KeyError` or `TypeError` exceptions), but since the goal is to prevent crashing the test runner, I think the broad except makes sense. No release note because ITR is in beta. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) - [x] If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. - [x] This PR doesn't touch any of that. Co-authored-by: Romain Komorn <[email protected]> Co-authored-by: Eric Navarro <[email protected]>
1 parent 6b5730c commit da16e3d

File tree

2 files changed

+273
-10
lines changed

2 files changed

+273
-10
lines changed

‎ddtrace/internal/ci_visibility/recorder.py‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -335,26 +335,35 @@ def _fetch_tests_to_skip(self, skipping_mode):
335335
self._test_suites_to_skip = []
336336

337337
if response.status >= 400:
338-
log.warning("Test skips request responded with status %d", response.status)
338+
log.warning("Skippable tests request responded with status %d", response.status)
339339
return
340340
try:
341341
if isinstance(response.body, bytes):
342342
parsed = json.loads(response.body.decode())
343343
else:
344344
parsed = json.loads(response.body)
345345
except json.JSONDecodeError:
346-
log.warning("Test skips request responded with invalid JSON '%s'", response.body)
346+
log.warning("Skippable tests request responded with invalid JSON '%s'", response.body)
347347
return
348348

349-
for item in parsed["data"]:
350-
if item["type"] == skipping_mode and "suite" in item["attributes"]:
351-
module = item["attributes"].get("configurations", {}).get("test.bundle", "").replace(".", "/")
352-
path = "/".join((module, item["attributes"]["suite"])) if module else item["attributes"]["suite"]
349+
if "data" not in parsed:
350+
log.warning("Skippable tests request missing data, no tests will be skipped")
351+
return
353352

354-
if skipping_mode == SUITE:
355-
self._test_suites_to_skip.append(path)
356-
else:
357-
self._tests_to_skip[path].append(item["attributes"]["name"])
353+
try:
354+
for item in parsed["data"]:
355+
if item["type"] == skipping_mode and "suite" in item["attributes"]:
356+
module = item["attributes"].get("configurations", {}).get("test.bundle", "").replace(".", "/")
357+
path = "/".join((module, item["attributes"]["suite"])) if module else item["attributes"]["suite"]
358+
359+
if skipping_mode == SUITE:
360+
self._test_suites_to_skip.append(path)
361+
else:
362+
self._tests_to_skip[path].append(item["attributes"]["name"])
363+
except Exception:
364+
log.warning("Error processing skippable test data, no tests will be skipped", exc_info=True)
365+
self._test_suites_to_skip = []
366+
self._tests_to_skip = defaultdict(list)
358367

359368
def _should_skip_path(self, path, name, test_skipping_mode=None):
360369
if test_skipping_mode is None:

‎tests/ci_visibility/test_ci_visibility.py‎

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1+
from collections import defaultdict
12
import json
23
import os
4+
import textwrap
35
import time
46

57
import mock
@@ -10,6 +12,8 @@
1012
from ddtrace.ext import ci
1113
from ddtrace.internal.ci_visibility import CIVisibility
1214
from ddtrace.internal.ci_visibility.constants import REQUESTS_MODE
15+
from ddtrace.internal.ci_visibility.constants import SUITE
16+
from ddtrace.internal.ci_visibility.constants import TEST
1317
from ddtrace.internal.ci_visibility.encoder import CIVisibilityEncoderV01
1418
from ddtrace.internal.ci_visibility.filters import TraceCiVisibilityFilter
1519
from ddtrace.internal.ci_visibility.git_client import CIVisibilityGitClient
@@ -1030,6 +1034,256 @@ def test_encoder_py2_payload_force_unicode_strings():
10301034
) == {u"string_key": [1, {u"unicode_key": u"string_value"}, u"unicode_value", {u"string_key": u"unicode_value"}]}
10311035

10321036

1037+
class TestFetchTestsToSkip:
1038+
@pytest.fixture(scope="function")
1039+
def mock_civisibility(self):
1040+
with mock.patch.object(CIVisibility, "__init__", return_value=None):
1041+
_civisibility = CIVisibility()
1042+
_civisibility._api_key = "notanapikey"
1043+
_civisibility._dd_site = "notdatadog.notcom"
1044+
_civisibility._service = "test-service"
1045+
_civisibility._git_client = None
1046+
_civisibility._requests_mode = REQUESTS_MODE.AGENTLESS_EVENTS
1047+
_civisibility._tags = {
1048+
ci.git.REPOSITORY_URL: "test_repo_url",
1049+
ci.git.COMMIT_SHA: "testcommitsssshhhaaaa1234",
1050+
}
1051+
1052+
yield _civisibility
1053+
CIVisibility._test_suites_to_skip = None
1054+
CIVisibility._tests_to_skip = defaultdict(list)
1055+
1056+
@pytest.fixture(scope="class", autouse=True)
1057+
def _test_context_manager(self):
1058+
with mock.patch("ddtrace.ext.ci._get_runtime_and_os_metadata"), mock.patch("json.dumps", return_value=""):
1059+
yield
1060+
1061+
def test_fetch_tests_to_skip_test_level(self, mock_civisibility):
1062+
with mock.patch(
1063+
"ddtrace.internal.ci_visibility.recorder._do_request",
1064+
return_value=Response(
1065+
status=200,
1066+
body=textwrap.dedent(
1067+
"""{
1068+
"data": [
1069+
{
1070+
"id": "123456789",
1071+
"type": "test",
1072+
"attributes": {
1073+
"configurations": {
1074+
"test.bundle": "testbundle"
1075+
},
1076+
"name": "test_name_1",
1077+
"suite": "test_suite_1.py"
1078+
}
1079+
},
1080+
{
1081+
"id": "987654321",
1082+
"type": "test",
1083+
"attributes": {
1084+
"configurations": {
1085+
"test.bundle": "testpackage/testbundle"
1086+
},
1087+
"name": "test_name_2",
1088+
"suite": "test_suite_2.py"
1089+
}
1090+
}
1091+
]
1092+
}"""
1093+
),
1094+
),
1095+
):
1096+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1097+
mock_civisibility._fetch_tests_to_skip(TEST)
1098+
assert mock_civisibility._test_suites_to_skip == []
1099+
assert mock_civisibility._tests_to_skip == {
1100+
"testbundle/test_suite_1.py": ["test_name_1"],
1101+
"testpackage/testbundle/test_suite_2.py": ["test_name_2"],
1102+
}
1103+
1104+
def test_fetch_tests_to_skip_suite_level(self, mock_civisibility):
1105+
with mock.patch(
1106+
"ddtrace.internal.ci_visibility.recorder._do_request",
1107+
return_value=Response(
1108+
status=200,
1109+
body=textwrap.dedent(
1110+
"""{
1111+
"data": [
1112+
{
1113+
"id": "34640cc7ce80c01e",
1114+
"type": "suite",
1115+
"attributes": {
1116+
"configurations": {
1117+
"test.bundle": "testbundle"
1118+
},
1119+
"suite": "test_module_1.py"
1120+
}
1121+
},
1122+
{
1123+
"id": "239fa7de754db779",
1124+
"type": "suite",
1125+
"attributes": {
1126+
"configurations": {
1127+
"test.bundle": "testpackage/testbundle"
1128+
},
1129+
"suite": "test_suite_2.py"
1130+
}
1131+
}
1132+
]
1133+
}"""
1134+
),
1135+
),
1136+
):
1137+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1138+
mock_civisibility._fetch_tests_to_skip(SUITE)
1139+
assert mock_civisibility._test_suites_to_skip == [
1140+
"testbundle/test_module_1.py",
1141+
"testpackage/testbundle/test_suite_2.py",
1142+
]
1143+
assert mock_civisibility._tests_to_skip == {}
1144+
1145+
def test_fetch_tests_to_skip_no_data_test_level(self, mock_civisibility):
1146+
with mock.patch(
1147+
"ddtrace.internal.ci_visibility.recorder._do_request",
1148+
return_value=Response(
1149+
status=200,
1150+
body="{}",
1151+
),
1152+
):
1153+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1154+
mock_civisibility._fetch_tests_to_skip(TEST)
1155+
assert mock_civisibility._test_suites_to_skip == []
1156+
assert mock_civisibility._tests_to_skip == {}
1157+
1158+
def test_fetch_tests_to_skip_no_data_suite_level(self, mock_civisibility):
1159+
with mock.patch(
1160+
"ddtrace.internal.ci_visibility.recorder._do_request",
1161+
return_value=Response(
1162+
status=200,
1163+
body="{}",
1164+
),
1165+
):
1166+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1167+
mock_civisibility._fetch_tests_to_skip(SUITE)
1168+
assert mock_civisibility._test_suites_to_skip == []
1169+
assert mock_civisibility._tests_to_skip == {}
1170+
1171+
def test_fetch_tests_to_skip_data_is_none_test_level(self, mock_civisibility):
1172+
with mock.patch(
1173+
"ddtrace.internal.ci_visibility.recorder._do_request",
1174+
return_value=Response(
1175+
status=200,
1176+
body='{"data": null}',
1177+
),
1178+
):
1179+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1180+
mock_civisibility._fetch_tests_to_skip(TEST)
1181+
assert mock_civisibility._test_suites_to_skip == []
1182+
assert mock_civisibility._tests_to_skip == {}
1183+
1184+
def test_fetch_tests_to_skip_data_is_none_suite_level(self, mock_civisibility):
1185+
with mock.patch(
1186+
"ddtrace.internal.ci_visibility.recorder._do_request",
1187+
return_value=Response(
1188+
status=200,
1189+
body='{"data": null}',
1190+
),
1191+
):
1192+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1193+
mock_civisibility._fetch_tests_to_skip(SUITE)
1194+
assert mock_civisibility._test_suites_to_skip == []
1195+
assert mock_civisibility._tests_to_skip == {}
1196+
1197+
def test_fetch_tests_to_skip_bad_json(self, mock_civisibility):
1198+
with mock.patch(
1199+
"ddtrace.internal.ci_visibility.recorder._do_request",
1200+
return_value=Response(
1201+
status=200,
1202+
body="{ this is not valid JSON { ",
1203+
),
1204+
):
1205+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1206+
mock_civisibility._fetch_tests_to_skip(SUITE)
1207+
assert mock_civisibility._test_suites_to_skip == []
1208+
assert mock_civisibility._tests_to_skip == {}
1209+
1210+
def test_fetch_tests_to_skip_bad_response(self, mock_civisibility):
1211+
with mock.patch(
1212+
"ddtrace.internal.ci_visibility.recorder._do_request",
1213+
return_value=Response(
1214+
status=500,
1215+
body="Internal server error",
1216+
),
1217+
):
1218+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1219+
mock_civisibility._fetch_tests_to_skip(SUITE)
1220+
assert mock_civisibility._test_suites_to_skip == []
1221+
assert mock_civisibility._tests_to_skip == {}
1222+
1223+
def test_fetch_test_to_skip_invalid_data_missing_key(self, mock_civisibility):
1224+
with mock.patch(
1225+
"ddtrace.internal.ci_visibility.recorder._do_request",
1226+
return_value=Response(
1227+
status=200,
1228+
body='{"data": [{"somekey": "someval"}]}',
1229+
),
1230+
):
1231+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1232+
mock_civisibility._fetch_tests_to_skip(SUITE)
1233+
assert mock_civisibility._test_suites_to_skip == []
1234+
assert mock_civisibility._tests_to_skip == {}
1235+
1236+
def test_fetch_test_to_skip_invalid_data_type_error(self, mock_civisibility):
1237+
with mock.patch(
1238+
"ddtrace.internal.ci_visibility.recorder._do_request",
1239+
return_value=Response(
1240+
status=200,
1241+
body=textwrap.dedent(
1242+
"""{
1243+
"data": [ {
1244+
"id": "12345",
1245+
"type": "suite",
1246+
"attributes": {
1247+
"configurations": {
1248+
"test.bundle": "2"
1249+
},
1250+
"suite": 1
1251+
}
1252+
}]}""",
1253+
),
1254+
),
1255+
):
1256+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1257+
mock_civisibility._fetch_tests_to_skip(SUITE)
1258+
assert mock_civisibility._test_suites_to_skip == []
1259+
assert mock_civisibility._tests_to_skip == {}
1260+
1261+
def test_fetch_test_to_skip_invalid_data_attribute_error(self, mock_civisibility):
1262+
with mock.patch(
1263+
"ddtrace.internal.ci_visibility.recorder._do_request",
1264+
return_value=Response(
1265+
status=200,
1266+
body=textwrap.dedent(
1267+
"""{
1268+
"data": [ {
1269+
"id": "12345",
1270+
"type": "suite",
1271+
"attributes": {
1272+
"configurations": {
1273+
"test.bundle": 2
1274+
},
1275+
"suite": "1"
1276+
}
1277+
}]}""",
1278+
),
1279+
),
1280+
):
1281+
ddtrace.internal.ci_visibility.recorder.ddconfig = ddtrace.settings.Config()
1282+
mock_civisibility._fetch_tests_to_skip(SUITE)
1283+
assert mock_civisibility._test_suites_to_skip == []
1284+
assert mock_civisibility._tests_to_skip == {}
1285+
1286+
10331287
def test_fetch_tests_to_skip_custom_configurations():
10341288
with override_env(
10351289
dict(

0 commit comments

Comments
 (0)