fix(ci_visibility): itr skipped counts suites or tests (#14681)

CI Visibility: Fixes some bad behavior in test skipping count,
introduced in #14257

Author: gnufede

ddtrace/contrib/internal/pytest/_plugin_v2.py

@@ -1,6 +1,5 @@
 import os
 from pathlib import Path
-import re
 import typing as t
 
 from _pytest.runner import runtestprotocol
@@ -101,11 +100,12 @@
 log = get_logger(__name__)
 
 
-_NODEID_REGEX = re.compile("^((?P<module>.*)/(?P<suite>[^/]*?))::(?P<name>.*?)$")
 OUTCOME_QUARANTINED = "quarantined"
 DISABLED_BY_TEST_MANAGEMENT_REASON = "Flaky test is disabled by Datadog"
 INCOMPATIBLE_PLUGINS = ("flaky", "rerunfailures")
 
+skipped_suites = set()
+
 skip_pytest_runtest_protocol = False
 
 # Module-level variable to store the current test's coverage collector
@@ -151,13 +151,27 @@ def _handle_itr_should_skip(item, test_id) -> bool:
         if hasattr(item.config, "workeroutput"):
             if "itr_skipped_count" not in item.config.workeroutput:
                 item.config.workeroutput["itr_skipped_count"] = 0
-            item.config.workeroutput["itr_skipped_count"] += 1
+            if not is_suite_skipping_mode:
+                item.config.workeroutput["itr_skipped_count"] += 1
 
         return True
 
     return False
 
 
+def _handle_itr_xdist_skipped_suite(item, suite_id) -> bool:
+    if suite_id in skipped_suites:
+        log.debug("Suite is already skipped")
+        return False
+
+    if hasattr(item.config, "workeroutput"):
+        if "itr_skipped_count" not in item.config.workeroutput:
+            item.config.workeroutput["itr_skipped_count"] = 0
+        item.config.workeroutput["itr_skipped_count"] += 1
+    skipped_suites.add(suite_id)
+    return True
+
+
 def _handle_test_management(item, test_id):
     """Add a user property to identify quarantined tests, and mark them for skipping if quarantine is enabled in
     skipping mode.
@@ -605,6 +619,7 @@ def _pytest_runtest_protocol_post_yield(item, nextitem, coverage_collector):
     if next_test_id is None or next_test_id.parent_id != suite_id:
         if InternalTestSuite.is_itr_skippable(suite_id) and not InternalTestSuite.was_itr_forced_run(suite_id):
             InternalTestSuite.mark_itr_skipped(suite_id)
+            _handle_itr_xdist_skipped_suite(item, suite_id)
         else:
             _handle_coverage_dependencies(suite_id)
         InternalTestSuite.finish(suite_id)

ddtrace/internal/ci_visibility/api/_suite.py

@@ -69,6 +69,9 @@ def finish_itr_skipped(self) -> None:
                 )
                 return
 
+        # Only count for suite-level skipping mode, not test-level
+        if self._session_settings.itr_test_skipping_level == ITR_SKIPPING_LEVEL.SUITE:
+            self.count_itr_skipped()
         self.mark_itr_skipped()
         self.finish()
 

ddtrace/internal/ci_visibility/api/_test.py

@@ -260,7 +260,8 @@ def count_itr_skipped(self) -> None:
 
     def finish_itr_skipped(self) -> None:
         log.debug("Finishing Test Visibility test %s with ITR skipped", self)
-        self.count_itr_skipped()
+        if self._session_settings.itr_test_skipping_level == ITR_SKIPPING_LEVEL.TEST:
+            self.count_itr_skipped()
         self.mark_itr_skipped()
         self.finish_test(TestStatus.SKIP)
 

releasenotes/notes/ci_visibility_fix_itr_count-7f4c5da42df18aea.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    CI Visibility: This fix solves an issue where the ITR skip count metric was aggregating skipped tests even when skipping level was set to suite. It will now count appropriately (skipped suites or skipped tests) depending on ITR skip level.

tests/ci_visibility/api/fake_runner_mix_fail_itr_suite_level.py

@@ -16,6 +16,7 @@
 
 Comment lines in the test start/finish lines are there for visual distinction.
 """
+
 import json
 from multiprocessing import freeze_support
 from pathlib import Path
@@ -427,8 +428,19 @@ def main():
 if __name__ == "__main__":
     freeze_support()
     # NOTE: this is only safe because these tests are run in a subprocess
-    import os
 
-    os.environ["_DD_CIVISIBILITY_ITR_SUITE_MODE"] = "1"
-    with mock.patch("ddtrace.internal.ci_visibility.CIVisibility.is_itr_enabled", return_value=True):
-        main()
+    from ddtrace.internal.ci_visibility._api_client import TestVisibilityAPISettings
+
+    itr_settings = TestVisibilityAPISettings(
+        coverage_enabled=False,
+        skipping_enabled=True,
+        require_git=False,
+        itr_enabled=True,
+        flaky_test_retries_enabled=False,
+    )
+
+    mock.patch(
+        "ddtrace.internal.ci_visibility.recorder.CIVisibility._check_enabled_features", return_value=itr_settings
+    ).start()
+
+    main()

tests/ci_visibility/api/test_api_fake_runners.py

@@ -272,12 +272,8 @@ def test_manual_api_fake_runner_mix_fail_itr_suite_level(self):
                 mock_ci_env=True,
             ),
             replace_os_env=True,
-        ), mock.patch(
-            "ddtrace.internal.ci_visibility._api_client._TestVisibilityAPIClientBase.fetch_settings",
-            return_value=TestVisibilityAPISettings(False, False, False, False, False),
-        ), mock.patch(
-            "ddtrace.internal.ci_visibility.recorder.ddconfig", _get_default_civisibility_ddconfig()
         ):
+            # The fake runner handles its own ITR mocking internally
             subprocess.run(["python", "fake_runner_mix_fail_itr_suite_level.py"])
 
     @snapshot(ignores=SNAPSHOT_IGNORES)

tests/contrib/pytest/test_pytest.py

@@ -2583,15 +2583,15 @@ def test_inner_2():
         for skipped_test_span in skipped_test_spans:
             assert skipped_test_span.get_tag("test.skipped_by_itr") == "true"
 
-    def test_pytest_suite_level_skipping_counts_tests_not_suites(self):
+    def test_pytest_suite_level_skipping_counts_suites(self):
         """
         Regression test for suite level skipping count bug.
 
         When ITR is enabled at suite level and suites are skipped, the `itr.tests_skipping.count` tag
-        should count the number of tests that were skipped (contained within those suites).
+        should count the number of suites that were skipped (instead of the number of tests).
 
         This test creates 2 suites with multiple tests each (4 tests total), expects all suites to be
-        skipped, and verifies that the count reflects the number of tests (4), not suites (2).
+        skipped, and verifies that the count reflects the number of suites (2), not tests (4).
         """
         package_outer_dir = self.testdir.mkpydir("test_outer_package")
         os.chdir(str(package_outer_dir))
@@ -2651,12 +2651,12 @@ def test_inner_2():
         assert session_span.get_tag("_dd.ci.itr.tests_skipped") == "true"
         assert session_span.get_tag("test.itr.tests_skipping.type") == "suite"
 
-        # This is the regression test: should count tests (4), not suites (2)
-        expected_test_count = 4  # 4 individual tests were skipped
+        # This is the regression test: should count suites (2), not tests (4)
+        expected_suite_count = 2  # 4 individual tests were skipped
         actual_count = session_span.get_metric("test.itr.tests_skipping.count")
         assert (
-            actual_count == expected_test_count
-        ), f"Expected {expected_test_count} tests skipped but got {actual_count}"
+            actual_count == expected_suite_count
+        ), f"Expected {expected_suite_count} suites skipped but got {actual_count}"
 
         # Verify all test spans were skipped by ITR
         skipped_test_spans = [x for x in spans if x.get_tag("test.status") == "skip" and x.get_tag("type") == "test"]

tests/contrib/pytest/test_pytest_xdist_itr.py

@@ -118,6 +118,12 @@ def test_pytest_xdist_itr_skips_tests_at_test_level_by_pytest_addopts_env_var(se
     return_value=itr_settings
 ).start()
 
+# Mock fetch_skippable_items to return our test data
+mock.patch(
+    "ddtrace.internal.ci_visibility._api_client._TestVisibilityAPIClientBase.fetch_skippable_items",
+    return_value=itr_data
+).start()
+
 # Set ITR data when CIVisibility is enabled
 import ddtrace.internal.ci_visibility.recorder
 CIVisibility = ddtrace.internal.ci_visibility.recorder.CIVisibility
@@ -173,6 +179,129 @@ def patched_enable(cls, *args, **kwargs):
         # Verify number of skipped tests in session
         assert session_span.get_metric("test.itr.tests_skipping.count") == 2
 
+    def test_xdist_suite_mode_skipped_suites(self):
+        """Test that suite-level ITR skipping works correctly in xdist and counts suites, not individual tests."""
+
+        itr_skipping_sitecustomize = """
+# sitecustomize.py - ITR setup for xdist worker nodes
+from unittest import mock
+
+# Import required modules
+from ddtrace.internal.ci_visibility._api_client import TestVisibilityAPISettings
+from ddtrace.internal.ci_visibility._api_client import EarlyFlakeDetectionSettings
+from ddtrace.internal.ci_visibility._api_client import TestManagementSettings
+from ddtrace.internal.ci_visibility._api_client import ITRData
+from ddtrace.ext.test_visibility._test_visibility_base import TestSuiteId, TestModuleId, TestId
+
+# Create ITR settings and data
+itr_settings = TestVisibilityAPISettings(
+    coverage_enabled=False, skipping_enabled=True, require_git=False, itr_enabled=True,
+    flaky_test_retries_enabled=False, known_tests_enabled=False,
+    early_flake_detection=EarlyFlakeDetectionSettings(), test_management=TestManagementSettings()
+)
+
+# Create skippable suites for suite-level skipping
+skippable_suites = {
+    TestSuiteId(TestModuleId(""), "test_scope1.py"),
+    TestSuiteId(TestModuleId(""), "test_scope2.py")
+}
+itr_data = ITRData(correlation_id="12345678-1234-1234-1234-123456789012", skippable_items=skippable_suites)
+
+# Mock API calls to return our settings
+mock.patch(
+    "ddtrace.internal.ci_visibility._api_client._TestVisibilityAPIClientBase.fetch_settings",
+    return_value=itr_settings
+).start()
+
+mock.patch(
+    "ddtrace.internal.ci_visibility.recorder.CIVisibility._check_enabled_features",
+    return_value=itr_settings
+).start()
+
+# Mock fetch_skippable_items to return our test data
+mock.patch(
+    "ddtrace.internal.ci_visibility._api_client._TestVisibilityAPIClientBase.fetch_skippable_items",
+    return_value=itr_data
+).start()
+
+# Set ITR data when CIVisibility is enabled
+import ddtrace.internal.ci_visibility.recorder
+CIVisibility = ddtrace.internal.ci_visibility.recorder.CIVisibility
+original_enable = CIVisibility.enable
+
+def patched_enable(cls, *args, **kwargs):
+    result = original_enable(*args, **kwargs)
+    if cls._instance:
+        cls._instance._itr_data = itr_data
+    return result
+
+CIVisibility.enable = classmethod(patched_enable)
+"""
+
+        # Create test files
+        self.testdir.makepyfile(sitecustomize=itr_skipping_sitecustomize)
+        self.testdir.makepyfile(
+            test_scope1="""
+import pytest
+
+class TestScope1:
+    def test_scope1_method1(self):
+        assert True
+
+    def test_scope1_method2(self):
+        assert True
+""",
+            test_scope2="""
+import pytest
+
+class TestScope2:
+    def test_scope2_method1(self):
+        assert True
+""",
+        )
+        self.testdir.chdir()
+
+        itr_settings = TestVisibilityAPISettings(
+            coverage_enabled=False,
+            skipping_enabled=True,
+            require_git=False,
+            itr_enabled=True,
+            flaky_test_retries_enabled=False,
+            known_tests_enabled=False,
+            early_flake_detection=EarlyFlakeDetectionSettings(),
+            test_management=TestManagementSettings(),
+        )
+
+        with mock.patch(
+            "ddtrace.internal.ci_visibility.recorder.CIVisibility._check_enabled_features", return_value=itr_settings
+        ), mock.patch(
+            "ddtrace.internal.ci_visibility.recorder.CIVisibility.test_skipping_enabled",
+            return_value=True,
+        ):
+            # Run with xdist using loadscope mode (suite-level skipping)
+            rec = self.inline_run(
+                "--ddtrace",
+                "-n",
+                "2",
+                "--dist=loadscope",
+                "-s",
+                "-vvv",
+                extra_env={
+                    "DD_CIVISIBILITY_AGENTLESS_ENABLED": "1",
+                    "DD_API_KEY": "foobar.baz",
+                    "DD_INSTRUMENTATION_TELEMETRY_ENABLED": "0",
+                },
+            )
+            assert rec.ret == 0  # All tests skipped, so exit code is 0
+
+        # Assert on session span metrics - key assertion for suite-level skipping
+        spans = self.pop_spans()
+        session_span = [span for span in spans if span.get_tag("type") == "test_session_end"][0]
+        assert session_span.get_tag("test.itr.tests_skipping.enabled") == "true"
+        assert session_span.get_tag("test.itr.tests_skipping.type") == "suite"  # loadscope uses suite-level skipping
+        # Verify number of skipped SUITES in session (should be 2 suites, not 3 tests)
+        assert session_span.get_metric("test.itr.tests_skipping.count") == 2
+
     def test_pytest_xdist_itr_skips_tests_at_test_level_without_loadscope(self):
         """Test that ITR tags are correctly aggregated from xdist workers."""
         # Create a simplified sitecustomize with just the essential ITR setup
@@ -210,6 +339,12 @@ def test_pytest_xdist_itr_skips_tests_at_test_level_without_loadscope(self):
     return_value=itr_settings
 ).start()
 
+# Mock fetch_skippable_items to return our test data
+mock.patch(
+    "ddtrace.internal.ci_visibility._api_client._TestVisibilityAPIClientBase.fetch_skippable_items",
+    return_value=itr_data
+).start()
+
 # Set ITR data when CIVisibility is enabled
 import ddtrace.internal.ci_visibility.recorder
 CIVisibility = ddtrace.internal.ci_visibility.recorder.CIVisibility
@@ -300,6 +435,12 @@ def test_pytest_xdist_itr_skips_tests_at_suite_level_with_loadscope(self):
     return_value=itr_settings
 ).start()
 
+# Mock fetch_skippable_items to return our test data
+mock.patch(
+    "ddtrace.internal.ci_visibility._api_client._TestVisibilityAPIClientBase.fetch_skippable_items",
+    return_value=itr_data
+).start()
+
 # Set ITR data when CIVisibility is enabled
 import ddtrace.internal.ci_visibility.recorder
 CIVisibility = ddtrace.internal.ci_visibility.recorder.CIVisibility
@@ -460,16 +601,16 @@ def test_handle_itr_should_skip_counts_skipped_tests_in_worker(self):
         test_id = TestId(TestSuiteId(TestModuleId("test_module"), "test_suite"), "test_name")
 
         mock_service = mock.MagicMock()
-        mock_service._suite_skipping_mode = True
+        mock_service._suite_skipping_mode = False  # Use test-level skipping for worker count tests
 
         with mock.patch(
             "ddtrace.internal.test_visibility.api.InternalTestSession.is_test_skipping_enabled", return_value=True
         ), mock.patch(
-            "ddtrace.internal.test_visibility.api.InternalTestSuite.is_itr_unskippable", return_value=False
+            "ddtrace.internal.test_visibility.api.InternalTest.is_itr_unskippable", return_value=False
         ), mock.patch(
             "ddtrace.internal.test_visibility.api.InternalTest.is_attempt_to_fix", return_value=False
         ), mock.patch(
-            "ddtrace.internal.test_visibility.api.InternalTestSuite.is_itr_skippable", return_value=True
+            "ddtrace.internal.test_visibility.api.InternalTest.is_itr_skippable", return_value=True
         ), mock.patch(
             "ddtrace.internal.test_visibility.api.InternalTest.mark_itr_skipped"
         ), mock.patch(
@@ -491,16 +632,16 @@ def test_handle_itr_should_skip_increments_existing_worker_count(self):
         test_id = TestId(TestSuiteId(TestModuleId("test_module"), "test_suite"), "test_name")
 
         mock_service = mock.MagicMock()
-        mock_service._suite_skipping_mode = True
+        mock_service._suite_skipping_mode = False  # Use test-level skipping for worker count tests
 
         with mock.patch(
             "ddtrace.internal.test_visibility.api.InternalTestSession.is_test_skipping_enabled", return_value=True
         ), mock.patch(
-            "ddtrace.internal.test_visibility.api.InternalTestSuite.is_itr_unskippable", return_value=False
+            "ddtrace.internal.test_visibility.api.InternalTest.is_itr_unskippable", return_value=False
         ), mock.patch(
             "ddtrace.internal.test_visibility.api.InternalTest.is_attempt_to_fix", return_value=False
         ), mock.patch(
-            "ddtrace.internal.test_visibility.api.InternalTestSuite.is_itr_skippable", return_value=True
+            "ddtrace.internal.test_visibility.api.InternalTest.is_itr_skippable", return_value=True
         ), mock.patch(
             "ddtrace.internal.test_visibility.api.InternalTest.mark_itr_skipped"
         ), mock.patch(
@@ -1690,6 +1831,11 @@ def test_func2():
 
         # The ITR skipping type should be suite due to explicit env var override
         assert session_span.get_tag("test.itr.tests_skipping.type") == "suite"
+        expected_suite_count = 0  # No suites skipped
+        actual_count = session_span.get_metric("test.itr.tests_skipping.count")
+        assert (
+            actual_count == expected_suite_count
+        ), f"Expected {expected_suite_count} suites skipped but got {actual_count}"
 
     def test_explicit_env_var_overrides_xdist_test_mode(self):
         """Test that explicit _DD_CIVISIBILITY_ITR_SUITE_MODE=False overrides xdist suite-level detection."""
@@ -1742,6 +1888,12 @@ def patched_enable(cls, *args, **kwargs):
     return_value=itr_settings
 ).start()
 
+# Mock fetch_skippable_items to return our test data
+mock.patch(
+    "ddtrace.internal.ci_visibility._api_client._TestVisibilityAPIClientBase.fetch_skippable_items",
+    return_value=itr_data
+).start()
+
 CIVisibility.enable = classmethod(patched_enable)
 
 """