From fc3eb8d6d85057ad4073198df3e71f7551fa75a0 Mon Sep 17 00:00:00 2001 From: Brett Langdon Date: Mon, 6 May 2024 09:13:34 -0400 Subject: [PATCH 1/5] fix(internal): fix CI Visibility usage of the core API --- ddtrace/ext/ci_visibility/api.py | 10 ++++++---- ddtrace/internal/ci_visibility/recorder.py | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/ddtrace/ext/ci_visibility/api.py b/ddtrace/ext/ci_visibility/api.py index ef8ae2e98a2..09f6d8dba02 100644 --- a/ddtrace/ext/ci_visibility/api.py +++ b/ddtrace/ext/ci_visibility/api.py @@ -239,7 +239,7 @@ def finish( @_catch_and_log_exceptions def get_settings(item_id: Optional[CISessionId] = None): log.debug("Getting settings for session %s", item_id) - core.dispatch_with_results("ci_visibility.session.get_settings", (item_id,)) + core.dispatch("ci_visibility.session.get_settings", (item_id,)) @staticmethod @_catch_and_log_exceptions @@ -309,11 +309,13 @@ def mark_itr_forced_run(item_id: Union[CISuiteId, CITestId]): @staticmethod @_catch_and_log_exceptions - def is_item_itr_skippable(item_id: Union[CISuiteId, CITestId]): + def is_item_itr_skippable(item_id: Union[CISuiteId, CITestId]) -> bool: """Skippable items are not currently tied to a test session, so no session ID is passed""" log.debug("Getting skippable items") - skippable_items = core.dispatch_with_results("ci_visibility.itr.is_item_skippable", (item_id,)) - return skippable_items["skippable_items"] + is_item_skippable: Optional[bool] = core.dispatch_with_results( + "ci_visibility.itr.is_item_skippable", (item_id,) + ).is_item_skippable.value + return bool(is_item_skippable) class AddCoverageArgs(NamedTuple): item_id: Union[_CIVisibilityChildItemIdBase, _CIVisibilityRootItemIdBase] diff --git a/ddtrace/internal/ci_visibility/recorder.py b/ddtrace/internal/ci_visibility/recorder.py index 93725a48404..d2efe5f3e29 100644 --- a/ddtrace/internal/ci_visibility/recorder.py +++ b/ddtrace/internal/ci_visibility/recorder.py @@ -1155,7 +1155,7 @@ def _on_itr_is_item_skippable(item_id: Union[CISuiteId, CITestId]) -> bool: def _register_itr_handlers(): log.debug("Registering ITR-related handlers") core.on("ci_visibility.itr.finish_skipped_by_itr", _on_itr_finish_item_skipped) - core.on("ci_visibility.itr.is_item_skippable", _on_itr_is_item_skippable) + core.on("ci_visibility.itr.is_item_skippable", _on_itr_is_item_skippable, "is_item_skippable") core.on("ci_visibility.itr.mark_unskippable", _on_itr_mark_unskippable) core.on("ci_visibility.itr.mark_forced_run", _on_itr_mark_forced_run) From f43f9a4b4c0c6521a4ae0696360d4850194fecd0 Mon Sep 17 00:00:00 2001 From: Romain Komorn Date: Tue, 14 May 2024 09:41:34 +0100 Subject: [PATCH 2/5] remove unused settings fetching --- ddtrace/ext/ci_visibility/api.py | 6 ------ ddtrace/internal/ci_visibility/recorder.py | 7 ------- 2 files changed, 13 deletions(-) diff --git a/ddtrace/ext/ci_visibility/api.py b/ddtrace/ext/ci_visibility/api.py index 09f6d8dba02..6671237bf22 100644 --- a/ddtrace/ext/ci_visibility/api.py +++ b/ddtrace/ext/ci_visibility/api.py @@ -235,12 +235,6 @@ def finish( "ci_visibility.session.finish", (CISession.FinishArgs(item_id, force_finish_children, override_status),) ) - @staticmethod - @_catch_and_log_exceptions - def get_settings(item_id: Optional[CISessionId] = None): - log.debug("Getting settings for session %s", item_id) - core.dispatch("ci_visibility.session.get_settings", (item_id,)) - @staticmethod @_catch_and_log_exceptions def get_known_tests(item_id: Optional[CISessionId] = None): diff --git a/ddtrace/internal/ci_visibility/recorder.py b/ddtrace/internal/ci_visibility/recorder.py index d2efe5f3e29..eb97b3e1ccb 100644 --- a/ddtrace/internal/ci_visibility/recorder.py +++ b/ddtrace/internal/ci_visibility/recorder.py @@ -920,18 +920,11 @@ def _on_finish_session(finish_args: CISession.FinishArgs): session.finish(finish_args.force_finish_children, finish_args.override_status) -@_requires_civisibility_enabled -def _on_get_session_settings(session_id: CISessionId): - log.debug("Handling get session settings for session id %s", session_id) - return CIVisibility.get_session_settings(session_id) - - def _register_session_handlers(): log.debug("Registering session handlers") core.on("ci_visibility.session.discover", _on_discover_session) core.on("ci_visibility.session.start", _on_start_session) core.on("ci_visibility.session.finish", _on_finish_session) - core.on("ci_visibility.session.get_settings", _on_get_session_settings) @_requires_civisibility_enabled From 26f454dd67daec8064b29d8c7ae49b26ec1d9891 Mon Sep 17 00:00:00 2001 From: Romain Komorn Date: Tue, 14 May 2024 14:00:46 +0100 Subject: [PATCH 3/5] add tests --- ddtrace/internal/ci_visibility/recorder.py | 2 +- .../api/test_ext_ci_visibility_api.py | 126 ++++++++++++++++++ tests/ci_visibility/test_ci_visibility.py | 13 +- tests/ci_visibility/util.py | 82 ++++++++++++ 4 files changed, 218 insertions(+), 5 deletions(-) diff --git a/ddtrace/internal/ci_visibility/recorder.py b/ddtrace/internal/ci_visibility/recorder.py index eb97b3e1ccb..71c2e1df414 100644 --- a/ddtrace/internal/ci_visibility/recorder.py +++ b/ddtrace/internal/ci_visibility/recorder.py @@ -1132,7 +1132,7 @@ def _on_itr_mark_forced_run(item_id: Union[CISuiteId, CITestId]) -> None: @_requires_civisibility_enabled def _on_itr_is_item_skippable(item_id: Union[CISuiteId, CITestId]) -> bool: """Skippable items are fetched as part CIVisibility.enable(), so they are assumed to be available.""" - log.debug("Handling get skippable items") + log.debug("Handling is item skippable for item id %s", item_id) if not isinstance(item_id, (CISuiteId, CITestId)): log.warning("Only suites or tests can be skippable, not %s", type(item_id)) diff --git a/tests/ci_visibility/api/test_ext_ci_visibility_api.py b/tests/ci_visibility/api/test_ext_ci_visibility_api.py index cee2d8b443c..4d29e036997 100644 --- a/tests/ci_visibility/api/test_ext_ci_visibility_api.py +++ b/tests/ci_visibility/api/test_ext_ci_visibility_api.py @@ -3,7 +3,10 @@ import pytest +from ddtrace.ext.ci_visibility import api from ddtrace.ext.ci_visibility.api import CISourceFileInfo +from ddtrace.internal.ci_visibility import CIVisibility +from tests.ci_visibility.util import set_up_mock_civisibility class TestCISourceFileInfo: @@ -53,3 +56,126 @@ def test_source_file_info_enforces_start_line_less_than_end_line(self): with pytest.raises(ValueError): # start_line cannot be None if end_line is provided _ = CISourceFileInfo(Path("/absolute/path/my_file_name"), end_line=1) + + +class TestCIITRMixin: + """Tests whether or not skippable tests and suites are correctly identified + + Note: these tests do not bother discoveirng a session as the ITR + """ + + def test_api_is_item_itr_skippable_test_level(self): + with set_up_mock_civisibility( + itr_enabled=True, + skipping_enabled=True, + suite_skipping_mode=False, + skippable_items={ + "skippable_module/suite.py": ["skippable_test"], + }, + ): + CIVisibility.enable() + + assert CIVisibility.enabled is True + assert CIVisibility._instance._suite_skipping_mode is False + + session_id = api.CISessionId("session_id") + + skippable_module_id = api.CIModuleId(session_id, "skippable_module") + + skippable_suite_id = api.CISuiteId(skippable_module_id, "suite.py") + skippable_test_id = api.CITestId(skippable_suite_id, "skippable_test") + non_skippable_test_id = api.CITestId(skippable_suite_id, "non_skippable_test") + + non_skippable_suite_id = api.CISuiteId(skippable_module_id, "non_skippable_suite.py") + non_skippable_suite_skippable_test_id = api.CITestId(non_skippable_suite_id, "skippable_test") + + assert api.CITest.is_item_itr_skippable(skippable_test_id) is True + assert api.CITest.is_item_itr_skippable(non_skippable_test_id) is False + assert api.CITest.is_item_itr_skippable(non_skippable_suite_skippable_test_id) is False + + CIVisibility.disable() + + def test_api_is_item_itr_skippable_false_when_skipping_disabled_test_level(self): + with set_up_mock_civisibility( + itr_enabled=True, + skipping_enabled=False, + suite_skipping_mode=False, + skippable_items={ + "skippable_module/suite.py": ["skippable_test"], + }, + ): + CIVisibility.enable() + + assert CIVisibility.enabled is True + assert CIVisibility._instance._suite_skipping_mode is False + + session_id = api.CISessionId("session_id") + + skippable_module_id = api.CIModuleId(session_id, "skippable_module") + + skippable_suite_id = api.CISuiteId(skippable_module_id, "suite.py") + skippable_test_id = api.CITestId(skippable_suite_id, "skippable_test") + non_skippable_test_id = api.CITestId(skippable_suite_id, "non_skippable_test") + + non_skippable_suite_id = api.CISuiteId(skippable_module_id, "non_skippable_suite.py") + non_skippable_suite_skippable_test_id = api.CITestId(non_skippable_suite_id, "skippable_test") + + assert api.CITest.is_item_itr_skippable(skippable_test_id) is False + assert api.CITest.is_item_itr_skippable(non_skippable_test_id) is False + assert api.CITest.is_item_itr_skippable(non_skippable_suite_skippable_test_id) is False + + CIVisibility.disable() + + def test_api_is_item_itr_skippable_suite_level(self): + with set_up_mock_civisibility( + itr_enabled=True, + skipping_enabled=True, + suite_skipping_mode=True, + skippable_items=["skippable_module/skippable_suite.py"], + ): + CIVisibility.enable() + + assert CIVisibility.enabled is True + assert CIVisibility._instance._suite_skipping_mode is True + + session_id = api.CISessionId("session_id") + + skippable_module_id = api.CIModuleId(session_id, "skippable_module") + skippable_suite_id = api.CISuiteId(skippable_module_id, "skippable_suite.py") + non_skippable_suite_id = api.CISuiteId(skippable_module_id, "non_skippable_suite.py") + + non_skippable_module_id = api.CIModuleId(session_id, "non_skippable_module") + non_skippable_module_skippable_suite_id = api.CISuiteId(non_skippable_module_id, "skippable_suite.py") + + assert api.CISuite.is_item_itr_skippable(skippable_suite_id) is True + assert api.CISuite.is_item_itr_skippable(non_skippable_suite_id) is False + assert api.CISuite.is_item_itr_skippable(non_skippable_module_skippable_suite_id) is False + + CIVisibility.disable() + + def test_api_is_item_itr_skippable_false_when_skipping_disabled_suite_level(self): + with set_up_mock_civisibility( + itr_enabled=True, + skipping_enabled=False, + suite_skipping_mode=True, + skippable_items=["skippable_module/skippable_suite.py"], + ): + CIVisibility.enable() + + assert CIVisibility.enabled is True + assert CIVisibility._instance._suite_skipping_mode is True + + session_id = api.CISessionId("session_id") + + skippable_module_id = api.CIModuleId(session_id, "skippable_module") + skippable_suite_id = api.CISuiteId(skippable_module_id, "skippable_suite.py") + non_skippable_suite_id = api.CISuiteId(skippable_module_id, "non_skippable_suite.py") + + non_skippable_module_id = api.CIModuleId(session_id, "non_skippable_module") + non_skippable_module_skippable_suite_id = api.CISuiteId(non_skippable_module_id, "skippable_suite.py") + + assert api.CISuite.is_item_itr_skippable(skippable_suite_id) is False + assert api.CISuite.is_item_itr_skippable(non_skippable_suite_id) is False + assert api.CISuite.is_item_itr_skippable(non_skippable_module_skippable_suite_id) is False + + CIVisibility.disable() diff --git a/tests/ci_visibility/test_ci_visibility.py b/tests/ci_visibility/test_ci_visibility.py index 4354c116ab1..0b08980df35 100644 --- a/tests/ci_visibility/test_ci_visibility.py +++ b/tests/ci_visibility/test_ci_visibility.py @@ -1674,10 +1674,10 @@ class TestIsITRSkippable: m3_s2_t7 = api.CITestId(m3_s2, "test_6[param3]") def _get_all_suite_ids(self): - return {getattr(self, suite_id) for suite_id in vars(self) if re.match(r"^m\d_s\d$", suite_id)} + return {getattr(self, suite_id) for suite_id in vars(self.__class__) if re.match(r"^m\d_s\d$", suite_id)} def _get_all_test_ids(self): - return {getattr(self, test_id) for test_id in vars(self) if re.match(r"^m\d_s\d_t\d$", test_id)} + return {getattr(self, test_id) for test_id in vars(self.__class__) if re.match(r"^m\d_s\d_t\d$", test_id)} def test_is_item_itr_skippable_test_level(self): with mock.patch.object(CIVisibility, "enabled", True), mock.patch.object( @@ -1703,6 +1703,8 @@ def test_is_item_itr_skippable_test_level(self): } expected_non_skippable_test_ids = self._get_all_test_ids() - expected_skippable_test_ids + breakpoint() + # Check skippable tests are correct for test_id in expected_skippable_test_ids: assert CIVisibility.is_item_itr_skippable(test_id) is True @@ -1723,12 +1725,15 @@ def test_is_item_itr_skippable_suite_level(self): mock_instance._tests_to_skip = defaultdict(list) mock_instance._suite_skipping_mode = True + expected_skippable_suite_ids = {self.m1_s1, self.m2_s1, self.m2_s2, self.m3_s1} + expected_non_skippable_suite_ids = self._get_all_suite_ids() - set(expected_skippable_suite_ids) + # Check skippable suites are correct - for suite_id in [self.m1_s1, self.m2_s1, self.m2_s2, self.m3_s1]: + for suite_id in expected_skippable_suite_ids: assert CIVisibility.is_item_itr_skippable(suite_id) is True # Check non-skippable suites are correct - for suite_id in [self.m1_s2, self.m3_s2]: + for suite_id in expected_non_skippable_suite_ids: assert CIVisibility.is_item_itr_skippable(suite_id) is False # Check all tests are not skippable diff --git a/tests/ci_visibility/util.py b/tests/ci_visibility/util.py index 05ae67e9de4..58af0f57882 100644 --- a/tests/ci_visibility/util.py +++ b/tests/ci_visibility/util.py @@ -1,8 +1,15 @@ +from collections import defaultdict from contextlib import contextmanager +from unittest import mock import ddtrace from ddtrace.internal.ci_visibility import DEFAULT_CI_VISIBILITY_SERVICE +from ddtrace.internal.ci_visibility.git_client import METADATA_UPLOAD_STATUS +from ddtrace.internal.ci_visibility.git_client import CIVisibilityGitClient +from ddtrace.internal.ci_visibility.recorder import CIVisibility +from ddtrace.internal.ci_visibility.recorder import _CIVisibilitySettings from tests.utils import DummyCIVisibilityWriter +from tests.utils import override_env @contextmanager @@ -22,3 +29,78 @@ def _get_default_civisibility_ddconfig(): }, ) return new_ddconfig + + +@contextmanager +def set_up_mock_civisibility( + use_agentless: bool = True, + coverage_enabled: bool = False, + skipping_enabled: bool = False, + itr_enabled: bool = False, + require_git: bool = False, + suite_skipping_mode: bool = False, + skippable_items=None, +): + """This is a one-stop-shop that patches all parts of CI Visibility for testing. + + Its purpose is to allow testers to call CIVisibility.enable() without side effects and with predictable results + while still exercising most of the internal (eg: non-API, non-subprocess-executing) code. + + It prevents: + * requests to settings and skippable API endpoints + * git client instantiation and use (skipping git metadata upload) + + It additionally raises NotImplementedErrors to try and alert callers if they are trying to do something that should + be mocked, but isn't. + """ + + def _fake_fetch_tests_to_skip(*args, **kwargs): + if skippable_items is None: + if suite_skipping_mode: + CIVisibility._instance._test_suites_to_skip = [] + else: + CIVisibility._instance._tests_to_skip = defaultdict(list) + else: + if suite_skipping_mode: + CIVisibility._instance._test_suites_to_skip = skippable_items + else: + CIVisibility._instance._tests_to_skip = skippable_items + + def _mock_upload_git_metadata(obj, **kwargs): + obj._metadata_upload_status = METADATA_UPLOAD_STATUS.SUCCESS + + env_overrides = { + "DD_CIVISIBILITY_AGENTLESS_ENABLED": "False", + "DD_SERVICE": "civis-test-service", + "DD_ENV": "civis-test-env", + } + if use_agentless: + env_overrides.update({"DD_API_KEY": "civisfakeapikey", "DD_CIVISIBILITY_AGENTLESS_ENABLED": "true"}) + if suite_skipping_mode: + env_overrides.update({"_DD_CIVISIBILITY_ITR_SUITE_MODE": "true"}) + + with override_env(env_overrides), mock.patch( + "ddtrace.internal.ci_visibility.recorder.CIVisibility._check_enabled_features", + return_value=_CIVisibilitySettings( + coverage_enabled=coverage_enabled, + skipping_enabled=skipping_enabled, + require_git=require_git, + itr_enabled=itr_enabled, + ), + ), mock.patch( + "ddtrace.internal.ci_visibility.recorder.CIVisibility._fetch_tests_to_skip", + side_effect=_fake_fetch_tests_to_skip, + ), mock.patch.multiple( + CIVisibilityGitClient, + _get_repository_url=classmethod(lambda *args, **kwargs: "git@github.com:TestDog/dd-test-py.git"), + _is_shallow_repository=classmethod(lambda *args, **kwargs: False), + _get_latest_commits=classmethod(lambda *args, **kwwargs: ["latest1", "latest2"]), + _search_commits=classmethod(lambda *args: ["latest1", "searched1", "searched2"]), + _get_filtered_revisions=classmethod(lambda *args, **kwargs: "revision1\nrevision2"), + _upload_packfiles=classmethod(lambda *args, **kwargs: None), + upload_git_metadata=_mock_upload_git_metadata, + _do_request=NotImplementedError, + ), mock.patch( + "ddtrace.internal.ci_visibility.recorder._do_request", side_effect=NotImplementedError + ): + yield From 70ccbb89ddd63584d23415b03fa1aea279c06f07 Mon Sep 17 00:00:00 2001 From: Romain Komorn Date: Tue, 14 May 2024 15:10:00 +0100 Subject: [PATCH 4/5] fix comment --- tests/ci_visibility/api/test_ext_ci_visibility_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ci_visibility/api/test_ext_ci_visibility_api.py b/tests/ci_visibility/api/test_ext_ci_visibility_api.py index 4d29e036997..5f62b7d0461 100644 --- a/tests/ci_visibility/api/test_ext_ci_visibility_api.py +++ b/tests/ci_visibility/api/test_ext_ci_visibility_api.py @@ -61,7 +61,7 @@ def test_source_file_info_enforces_start_line_less_than_end_line(self): class TestCIITRMixin: """Tests whether or not skippable tests and suites are correctly identified - Note: these tests do not bother discoveirng a session as the ITR + Note: these tests do not bother discovering a session as the ITR functionality currently does not rely on sessions. """ def test_api_is_item_itr_skippable_test_level(self): From 3d50f3c519c2407315e1abe6bc21e93ca094bddd Mon Sep 17 00:00:00 2001 From: Romain Komorn Date: Tue, 14 May 2024 15:58:04 +0100 Subject: [PATCH 5/5] remove breakpoint --- tests/ci_visibility/test_ci_visibility.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/ci_visibility/test_ci_visibility.py b/tests/ci_visibility/test_ci_visibility.py index 0b08980df35..56e89b4bd85 100644 --- a/tests/ci_visibility/test_ci_visibility.py +++ b/tests/ci_visibility/test_ci_visibility.py @@ -1703,8 +1703,6 @@ def test_is_item_itr_skippable_test_level(self): } expected_non_skippable_test_ids = self._get_all_test_ids() - expected_skippable_test_ids - breakpoint() - # Check skippable tests are correct for test_id in expected_skippable_test_ids: assert CIVisibility.is_item_itr_skippable(test_id) is True