Skip to content

Commit

Permalink
fix(ci_visibility): handle pytest ATR retry failures in unittest clas…
Browse files Browse the repository at this point in the history
…ses (#12030)

Auto Test Retries had a bug where retries of tests defined inside
unittest classes would always succeed, even if the test failed. This was
because for unittest classes, pytest saves the exception status in the
[`pytest_runtest_makereport`
hook](https://github.com/pytest-dev/pytest/blob/8.3.x/src/_pytest/unittest.py#L368),
which was not called during retries. This PR fixes it so that the hook
is called.

## Checklist
- [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))

## Reviewer Checklist
- [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)

(cherry picked from commit dc7037e)
  • Loading branch information
vitor-de-araujo authored and github-actions[bot] committed Jan 23, 2025
1 parent 0e3612f commit 898bf48
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 7 deletions.
2 changes: 1 addition & 1 deletion ddtrace/contrib/pytest/_retry_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def _retry_run_when(item, when, outcomes: RetryOutcomes) -> t.Tuple[CallInfo, _p
)
else:
call = CallInfo.from_call(lambda: hook(item=item), when=when)
report = pytest.TestReport.from_item_and_call(item=item, call=call)
report = item.ihook.pytest_runtest_makereport(item=item, call=call)
if report.outcome == "passed":
report.outcome = outcomes.PASSED
elif report.outcome == "failed" or report.outcome == "error":
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
fixes:
- |
CI Visibility: fixes an issue where Auto Test Retries with pytest would always consider retries of tests defined
inside unittest classes to be successful.
89 changes: 83 additions & 6 deletions tests/contrib/pytest/test_pytest_atr.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,27 +24,50 @@
)

_TEST_PASS_CONTENT = """
import unittest
def test_func_pass():
assert True
class SomeTestCase(unittest.TestCase):
def test_class_func_pass(self):
assert True
"""

_TEST_FAIL_CONTENT = """
import pytest
import unittest
def test_func_fail():
assert False
_test_func_retries_skip_count = 0
def test_func_retries_skip():
global _test_func_retries_skip_count
_test_func_retries_skip_count += 1
if _test_func_retries_skip_count > 1:
pytest.skip()
assert False
_test_class_func_retries_skip_count = 0
class SomeTestCase(unittest.TestCase):
def test_class_func_fail(self):
assert False
def test_class_func_retries_skip(self):
global _test_class_func_retries_skip_count
_test_class_func_retries_skip_count += 1
if _test_class_func_retries_skip_count > 1:
pytest.skip()
assert False
"""


_TEST_PASS_ON_RETRIES_CONTENT = """
import unittest
_test_func_passes_4th_retry_count = 0
def test_func_passes_4th_retry():
global _test_func_passes_4th_retry_count
Expand All @@ -56,10 +79,19 @@ def test_func_passes_1st_retry():
global _test_func_passes_1st_retry_count
_test_func_passes_1st_retry_count += 1
assert _test_func_passes_1st_retry_count == 2
class SomeTestCase(unittest.TestCase):
_test_func_passes_4th_retry_count = 0
def test_func_passes_4th_retry(self):
SomeTestCase._test_func_passes_4th_retry_count += 1
assert SomeTestCase._test_func_passes_4th_retry_count == 5
"""

_TEST_ERRORS_CONTENT = """
import pytest
import unittest
@pytest.fixture
def fixture_fails_setup():
Expand All @@ -79,13 +111,22 @@ def test_func_fails_teardown(fixture_fails_teardown):

_TEST_SKIP_CONTENT = """
import pytest
import unittest
@pytest.mark.skip
def test_func_skip_mark():
assert True
def test_func_skip_inside():
pytest.skip()
class SomeTestCase(unittest.TestCase):
@pytest.mark.skip
def test_class_func_skip_mark(self):
assert True
def test_class_func_skip_inside(self):
pytest.skip()
"""


Expand All @@ -109,7 +150,7 @@ def test_pytest_atr_no_ddtrace_does_not_retry(self):
self.testdir.makepyfile(test_pass_on_retries=_TEST_PASS_ON_RETRIES_CONTENT)
self.testdir.makepyfile(test_skip=_TEST_SKIP_CONTENT)
rec = self.inline_run()
rec.assertoutcome(passed=2, failed=6, skipped=2)
rec.assertoutcome(passed=3, failed=9, skipped=4)
assert len(self.pop_spans()) == 0

def test_pytest_atr_env_var_disables_retrying(self):
Expand All @@ -121,7 +162,7 @@ def test_pytest_atr_env_var_disables_retrying(self):

with mock.patch("ddtrace.internal.ci_visibility.recorder.ddconfig", _get_default_civisibility_ddconfig()):
rec = self.inline_run("--ddtrace", "-s", extra_env={"DD_CIVISIBILITY_FLAKY_RETRY_ENABLED": "0"})
rec.assertoutcome(passed=2, failed=6, skipped=2)
rec.assertoutcome(passed=3, failed=9, skipped=4)
assert len(self.pop_spans()) > 0

def test_pytest_atr_env_var_does_not_override_api(self):
Expand All @@ -137,7 +178,7 @@ def test_pytest_atr_env_var_does_not_override_api(self):
return_value=TestVisibilityAPISettings(flaky_test_retries_enabled=False),
):
rec = self.inline_run("--ddtrace", extra_env={"DD_CIVISIBILITY_FLAKY_RETRY_ENABLED": "1"})
rec.assertoutcome(passed=2, failed=6, skipped=2)
rec.assertoutcome(passed=3, failed=9, skipped=4)
assert len(self.pop_spans()) > 0

def test_pytest_atr_spans(self):
Expand Down Expand Up @@ -178,6 +219,15 @@ def test_pytest_atr_spans(self):
func_fail_retries += 1
assert func_fail_retries == 5

class_func_fail_spans = _get_spans_from_list(spans, "test", "SomeTestCase::test_class_func_fail")
assert len(class_func_fail_spans) == 6
class_func_fail_retries = 0
for class_func_fail_span in class_func_fail_spans:
assert class_func_fail_span.get_tag("test.status") == "fail"
if class_func_fail_span.get_tag("test.is_retry") == "true":
class_func_fail_retries += 1
assert class_func_fail_retries == 5

func_fail_skip_spans = _get_spans_from_list(spans, "test", "test_func_retries_skip")
assert len(func_fail_skip_spans) == 6
func_fail_skip_retries = 0
Expand All @@ -188,6 +238,18 @@ def test_pytest_atr_spans(self):
func_fail_skip_retries += 1
assert func_fail_skip_retries == 5

class_func_fail_skip_spans = _get_spans_from_list(spans, "test", "SomeTestCase::test_class_func_retries_skip")
assert len(class_func_fail_skip_spans) == 6
class_func_fail_skip_retries = 0
for class_func_fail_skip_span in class_func_fail_skip_spans:
class_func_fail_skip_is_retry = class_func_fail_skip_span.get_tag("test.is_retry") == "true"
assert class_func_fail_skip_span.get_tag("test.status") == (
"skip" if class_func_fail_skip_is_retry else "fail"
)
if class_func_fail_skip_is_retry:
class_func_fail_skip_retries += 1
assert class_func_fail_skip_retries == 5

func_pass_spans = _get_spans_from_list(spans, "test", "test_func_pass")
assert len(func_pass_spans) == 1
assert func_pass_spans[0].get_tag("test.status") == "pass"
Expand All @@ -205,7 +267,17 @@ def test_pytest_atr_spans(self):
assert func_skip_inside_spans[0].get_tag("test.status") == "skip"
assert func_skip_inside_spans[0].get_tag("test.is_retry") is None

assert len(spans) == 31
class_func_skip_mark_spans = _get_spans_from_list(spans, "test", "SomeTestCase::test_class_func_skip_mark")
assert len(class_func_skip_mark_spans) == 1
assert class_func_skip_mark_spans[0].get_tag("test.status") == "skip"
assert class_func_skip_mark_spans[0].get_tag("test.is_retry") is None

class_func_skip_inside_spans = _get_spans_from_list(spans, "test", "SomeTestCase::test_class_func_skip_inside")
assert len(class_func_skip_inside_spans) == 1
assert class_func_skip_inside_spans[0].get_tag("test.status") == "skip"
assert class_func_skip_inside_spans[0].get_tag("test.is_retry") is None

assert len(spans) == 51

def test_pytest_atr_fails_session_when_test_fails(self):
self.testdir.makepyfile(test_pass=_TEST_PASS_CONTENT)
Expand All @@ -216,7 +288,7 @@ def test_pytest_atr_fails_session_when_test_fails(self):
rec = self.inline_run("--ddtrace")
spans = self.pop_spans()
assert rec.ret == 1
assert len(spans) == 28
assert len(spans) == 48

def test_pytest_atr_passes_session_when_test_pass(self):
self.testdir.makepyfile(test_pass=_TEST_PASS_CONTENT)
Expand All @@ -226,9 +298,14 @@ def test_pytest_atr_passes_session_when_test_pass(self):
rec = self.inline_run("--ddtrace")
spans = self.pop_spans()
assert rec.ret == 0
assert len(spans) == 15
assert len(spans) == 23

def test_pytest_atr_does_not_retry_failed_setup_or_teardown(self):
# NOTE: This feature only works for regular pytest tests. For tests inside unittest classes, setup and teardown
# happens at the 'call' phase, and we don't have a way to detect that the error happened during setup/teardown,
# so tests will be retried as if they were failing tests.
# See <https://docs.pytest.org/en/8.3.x/how-to/unittest.html#pdb-unittest-note>.

self.testdir.makepyfile(test_errors=_TEST_ERRORS_CONTENT)
rec = self.inline_run("--ddtrace")
spans = self.pop_spans()
Expand Down

0 comments on commit 898bf48

Please sign in to comment.