Skip to content

chore(ci_visibility): move retry logic to pytest_runtest_protocol #13448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

vitor-de-araujo
Copy link
Contributor

@vitor-de-araujo vitor-de-araujo commented May 19, 2025

This is essentially #13376, recreated now that the fix in the unittest suite (#13445) has been merged, and with the previous pytest_runtest_protocol logic wrapped in a try/except to avoid breaking the pipeline in case of an internal error in CI Visibility.


Currently we let pytest's builtin pytest_runtest_protocol hook to run the test, and we check whether to retry at the makereport stage. This has a number of consequences:

  • We don't have access to the setup, call, teardown reports all at once; we see one at a time during makereport, and we have to patch them and stash information around to keep state across each time we pass through the reports for a given test.
  • pytest logs each report as it is created, so we have to patch them at the right time so they get printed correctly (to e.g. change the outcome from FAILED to ATR INITIAL ATTEMPT FAILED; this affects not only printing, but also the session exit status).
  • In particular, for EFD when the initial attempt passes, we run too late and the PASSED status was already logged by the time we patch it, so it shows PASSED instead of EFD INITIAL ATTEMPT PASSED. Not only that, but we have to handle this case specially when generating the terminal summary.

This PR moves the retry logic from the pytest_runtest_makereport hook to the pytest_runtest_protocol hook. This means we replace pytest's own pytest_runtest_protocol with our own. We invoke pytest's internal runtestprotocol function directly from our hook, so the behavior of our hook is similar to the pytest's own hook. The difference is that we call this function with log=False, so pytest doesn't log the setup, call, teardown reports as they are created. Instead, we collect all reports, patch them as needed, and then print them out. This means we can write the logic having full knowledge of the final status of a test run, instead of patching things as we see them during setup, call and teardown.

For retries, the responsibility for logging the statuses is moved to the retry handlers themselves, so they can delay printing to after the reports have been patched. In principle, we could even decide to not print the retry results individually and only print the final status (which would make for a cleaner output), but this can come in a future version.

For EFD, the special final outcomes (dd_efd_final_passed, etc.) are replaced with plain passed, failed, skipped states, which xdist can handle, and the final states are only used in efd_get_teststatus (called from the pytest_report_teststatus hook).

Future work:

  • Attempt to Fix has to be modified in similar ways to EFD, but it also has to handle quarantine, so it's a bit more involved.
  • xdist still prints EFD INITIAL ATTEMPT for all atempts (not just the first one).
  • The whole retry logic outside of pytest should be refactored (see chore(ci_visibility): refactor test retry logic #13224), but this PR is a first step to make the rest possible.

Checklist

  • 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
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • 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 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

Copy link
Contributor

CODEOWNERS have been resolved as:

ddtrace/contrib/internal/pytest/_atr_utils.py                           @DataDog/ci-app-libraries
ddtrace/contrib/internal/pytest/_attempt_to_fix.py                      @DataDog/ci-app-libraries
ddtrace/contrib/internal/pytest/_efd_utils.py                           @DataDog/ci-app-libraries
ddtrace/contrib/internal/pytest/_plugin_v2.py                           @DataDog/ci-app-libraries
ddtrace/contrib/internal/pytest/_retry_utils.py                         @DataDog/ci-app-libraries
ddtrace/contrib/internal/pytest/_utils.py                               @DataDog/ci-app-libraries
ddtrace/contrib/internal/pytest/plugin.py                               @DataDog/ci-app-libraries

@vitor-de-araujo vitor-de-araujo added changelog/no-changelog A changelog entry is not required for this PR. CI App labels May 19, 2025
@vitor-de-araujo vitor-de-araujo marked this pull request as ready for review May 19, 2025 14:10
@vitor-de-araujo vitor-de-araujo requested a review from a team as a code owner May 19, 2025 14:10
Copy link
Contributor

github-actions bot commented May 19, 2025

Bootstrap import analysis

Comparison of import times between this PR and base.

Summary

The average import time from this PR is: 248 ± 4 ms.

The average import time from base is: 248 ± 5 ms.

The import time difference between this PR and base is: -0.4 ± 0.2 ms.

The difference is not statistically significant (z = -1.99).

Import time breakdown

The following import paths have shrunk:

ddtrace.auto 1.869 ms (0.75%)
ddtrace.bootstrap.sitecustomize 1.191 ms (0.48%)
ddtrace.bootstrap.preload 1.191 ms (0.48%)
ddtrace.internal.remoteconfig.client 0.582 ms (0.23%)
ddtrace 0.678 ms (0.27%)

@pr-commenter
Copy link

pr-commenter bot commented May 19, 2025

Benchmarks

Benchmark execution time: 2025-05-19 15:47:00

Comparing candidate commit dcc238e in PR branch vitor-de-araujo/SDTEST-1850/refactor-retry-logic-2 with baseline commit 1219e33 in branch main.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 495 metrics, 8 unstable metrics.

scenario:iastdjangostartup-iast

  • 🟩 execution_time [-389.081ms; -173.970ms] or [-16.350%; -7.311%]

@vitor-de-araujo vitor-de-araujo enabled auto-merge (squash) May 19, 2025 15:33
@vitor-de-araujo vitor-de-araujo merged commit 422d025 into main May 19, 2025
323 of 325 checks passed
@vitor-de-araujo vitor-de-araujo deleted the vitor-de-araujo/SDTEST-1850/refactor-retry-logic-2 branch May 19, 2025 17:22
emmettbutler added a commit that referenced this pull request May 20, 2025
vitor-de-araujo added a commit that referenced this pull request May 26, 2025
…#13507)

Remove `RetryTestReport` for good, as part of the effort to support
`pytest-xdist`. Attempt-to-Fix was the only retry mechanism that still
used it; this PR brings it in line with the recent EFD and ATR refactors
(#13448 and #13288).

As a bonus, this fixes the miscounting of active Attempt-to-Fix tests in
the JUnit XML output.

## 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR. CI App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants