fix(ci_visibility): handle skipif with non-positional arguments [backport 3.11] (#14162)

Backport 13fcc38 from #14150 to 3.11.

The ddtrace pytest plugin currently assumes that the `skipif` marker
will be provided the condition as a positional argument. It breaks if no
condition is provided, or if the condition is passed as a keyword
argument.

This leads to the `_pytest_runtest_protocol_pre_yield()` call raising an
exception, which leaves `coverage_collector` unset, which leads to the
call to `_pytest_runtest_protocol_post_yield()` to fail. If the test was
not finished during `pytest_runtest_protocol` (for example, because the
`pytest-rerunfailures` or `flaky` plugins are in use, which causes
ddtrace to skip its own `pytest_runtest_protocol`), the test span will
be left unfinished and with a `fail` test status.

This PR adds handling for keyword and absent `skipif` conditions, and
also makes sure `coverage_collector` is set even if
`_pytest_runtest_protocol_pre_yield()` fails.

- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- 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)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: vitor-de-araujo

ddtrace/contrib/internal/pytest/_plugin_v2.py

@@ -523,6 +523,7 @@ def pytest_runtest_protocol_wrapper(item, nextitem) -> None:
     try:
         coverage_collector = _pytest_runtest_protocol_pre_yield(item)
     except Exception:  # noqa: E722
+        coverage_collector = None
         log.debug("encountered error during pre-test", exc_info=True)
 
     yield

ddtrace/contrib/internal/pytest/_utils.py

@@ -184,18 +184,29 @@ def _pytest_version_supports_attempt_to_fix():
     return _get_pytest_version_tuple() >= ATTEMPT_TO_FIX_MIN_SUPPORTED_VERSION
 
 
+def _get_skipif_condition(marker):
+    if marker.args:
+        condition = marker.args[0]
+    elif marker.kwargs:
+        condition = marker.kwargs.get("condition")
+    else:
+        condition = True  # `skipif` with no condition is equivalent to plain `skip`.
+
+    return condition
+
+
 def _pytest_marked_to_skip(item: pytest.Item) -> bool:
     """Checks whether Pytest will skip an item"""
     if item.get_closest_marker("skip") is not None:
         return True
 
-    return any(marker.args[0] for marker in item.iter_markers(name="skipif"))
+    return any(_get_skipif_condition(marker) is True for marker in item.iter_markers(name="skipif"))
 
 
 def _is_test_unskippable(item: pytest.Item) -> bool:
     """Returns True if a test has a skipif marker with value false and reason ITR_UNSKIPPABLE_REASON"""
     return any(
-        (marker.args[0] is False and marker.kwargs.get("reason") == ITR_UNSKIPPABLE_REASON)
+        (_get_skipif_condition(marker) is False and marker.kwargs.get("reason") == ITR_UNSKIPPABLE_REASON)
         for marker in item.iter_markers(name="skipif")
     )
 

releasenotes/notes/ci_visibility-fix-skipif-non-positional-arguments-e7b6c593959ba1ee.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    CI Visibility: This fix resolves an issue where using the pytest ``skipif`` marker with the condition passed as a
+    keyword argument (or not provided at all) would cause the test to be reported as failed, in particular when the
+    ``flaky`` or ``pytest-rerunfailures`` were also used.

tests/contrib/pytest_flaky/test_pytest_flaky.py

@@ -70,3 +70,116 @@ def test_pytest_no_flaky(self):
         assert flaky_spans[0].get_tag("test.status") == "fail"
         assert flaky_spans[1].get_tag("test.status") == "pass"
         assert rec.ret == 1
+
+    def test_skipif_without_condition(self):
+        """
+        Test that the plugin does not break if `skipif` is used with no arguments, while advanced features are disabled
+        by an external plugin.
+        """
+        self.testdir.makepyfile(
+            """
+            import pytest
+
+            @pytest.mark.skipif()
+            def test_foo():
+                assert True
+        """
+        )
+        rec = self.inline_run("--ddtrace", "-p", "flaky")
+        rec.assertoutcome(skipped=1)
+        spans = self.pop_spans()
+        [test_span] = _get_spans_from_list(spans, "test", "test_foo")
+        assert test_span.get_tag("test.status") == "skip"
+        assert rec.ret == 0
+
+    def test_skipif_with_keyword_condition(self):
+        """
+        Test that the plugin does not break if `skipif` is used with keyword arguments, while advanced features are
+        disabled by an external plugin.
+        """
+        self.testdir.makepyfile(
+            """
+            import pytest
+
+            @pytest.mark.skipif(condition=1 > 0, reason="because I can")
+            def test_skip():
+                assert True
+
+            @pytest.mark.skipif(condition=1 < 0, reason="because I can't")
+            def test_no_skip():
+                assert True
+        """
+        )
+        rec = self.inline_run("--ddtrace", "-p", "flaky")
+        rec.assertoutcome(skipped=1, passed=1)
+        spans = self.pop_spans()
+        [skip_test_span] = _get_spans_from_list(spans, "test", "test_skip")
+        [no_skip_test_span] = _get_spans_from_list(spans, "test", "test_no_skip")
+        assert skip_test_span.get_tag("test.status") == "skip"
+        assert no_skip_test_span.get_tag("test.status") == "pass"
+        assert rec.ret == 0
+
+    def test_skipif_with_string_condition(self):
+        """
+        Test that the plugin does not break if `skipif` is used with a string condition, while advanced features are
+        disabled by an external plugin.
+        """
+        # TODO(vitor-de-araujo): string conditions are currently not evaluated.
+        self.testdir.makepyfile(
+            """
+            import pytest
+
+            @pytest.mark.skipif("1 > 0")
+            def test_skip():
+                assert True
+
+            @pytest.mark.skipif("1 < 0")
+            def test_no_skip():
+                assert True
+        """
+        )
+        rec = self.inline_run("--ddtrace", "-p", "flaky")
+        rec.assertoutcome(skipped=1, passed=1)
+        spans = self.pop_spans()
+        [skip_test_span] = _get_spans_from_list(spans, "test", "test_skip")
+        [no_skip_test_span] = _get_spans_from_list(spans, "test", "test_no_skip")
+        assert skip_test_span.get_tag("test.status") == "skip"
+        assert no_skip_test_span.get_tag("test.status") == "pass"
+        assert rec.ret == 0
+
+    def test_skipif_with_string_keyword_condition(self):
+        """
+        Test that the plugin does not break if `skipif` is used with a string condition passed as keyword, while
+        advanced features are disabled by an external plugin.
+        """
+        # TODO(vitor-de-araujo): string conditions are currently not evaluated.
+        self.testdir.makepyfile(
+            """
+            import pytest
+
+            @pytest.mark.skipif(condition="1 > 0", reason="")
+            def test_skip():
+                assert True
+        """
+        )
+        self.testdir.makepyfile(
+            """
+            import pytest
+
+            @pytest.mark.skipif(condition="1 > 0", reason="because I can")
+            def test_skip():
+                assert True
+
+            @pytest.mark.skipif(condition="1 < 0", reason="because I can't")
+            def test_no_skip():
+                assert True
+        """
+        )
+        rec = self.inline_run("--ddtrace", "-p", "flaky")
+        rec.assertoutcome(skipped=1, passed=1)
+        spans = self.pop_spans()
+        [skip_test_span] = _get_spans_from_list(spans, "test", "test_skip")
+        [no_skip_test_span] = _get_spans_from_list(spans, "test", "test_no_skip")
+        assert skip_test_span.get_tag("test.status") == "skip"
+        assert no_skip_test_span.get_tag("test.status") == "pass"
+        assert rec.ret == 0