Add pytest-reportlog integration test and document compatibility - #379
LouisDeconinck wants to merge 2 commits into
Conversation
Rerun attempts are already published through pytest_runtest_logreport, so pytest-reportlog emits a TestReport per attempt with outcome "rerun" and a rerun counter. Pin that with a test, add pytest-reportlog to the tox test deps, and note the compatibility in the README. Closes pytest-dev#134. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
icemac
left a comment
icemac
left a comment
There was a problem hiding this comment.
— Comment created by Claude
Code review findings (high effort) are posted inline.
| commands = pytest tests/ {posargs} | ||
| deps = | ||
| xdist: pytest-xdist | ||
| pytest-reportlog |
There was a problem hiding this comment.
— Comment created by Claude
This only affects local tox runs. The GitHub Actions jobs in .github/workflows/test.yml don't use tox. They run pip install pytest / pip install -e ., so pytest-reportlog is never installed there and test_reportlog_contains_rerun_reports is always skipped by importorskip. The behavior this PR wants to pin is therefore never checked in CI. Please add pytest-reportlog to the pip install steps in test.yml, or at least to the main matrix job.
| @@ -0,0 +1,3 @@ | |||
| Add an integration test covering ``pytest-reportlog`` compatibility: each | |||
There was a problem hiding this comment.
— Comment created by Claude
changes/README.rst says: "Do not hard wrap the text of an entry: towncrier re-wraps each paragraph to fit the change log, and hard wrapping produces a ragged result." This entry is hard wrapped across three lines. It also repeats the README wording. A single short line is enough:
| Add an integration test covering ``pytest-reportlog`` compatibility: each | |
| Add an integration test for ``pytest-reportlog`` compatibility. |
| * This plugin is compatible with | ||
| `pytest-reportlog <https://pypi.org/project/pytest-reportlog/>`_: each | ||
| attempt is emitted as a ``TestReport``, with retried failures using the | ||
| ``"rerun"`` outcome. |
There was a problem hiding this comment.
— Comment created by Claude
The user-facing part of this PR is this new compatibility note. The only change log fragment is 134.misc.rst, and it describes the test setup. If the note should show up under Documentation in the change log, consider adding a 134.doc.rst fragment, e.g. Document compatibility with pytest-reportlog.
| if line.strip() | ||
| ] | ||
| test_reports = [r for r in reports if r.get("$report_type") == "TestReport"] | ||
| call_reports = [r for r in test_reports if r.get("when") == "call"] |
There was a problem hiding this comment.
— Comment created by Claude
The test only checks when == "call" reports. The README now says each attempt is emitted as a TestReport, but these cases are not covered:
- setup/teardown reports of the rerun attempt
- xdist crash reruns, where the rerun report from
pytest_handlecrashitemhaswhen == "???". I checked this manually and it currently works.
A regression in those paths would not be caught. Either check all TestReports for the item, or narrow the README wording.
| result = testdir.runpytest("--reruns", "1", "--report-log=log.jsonl") | ||
| assert_outcomes(result, passed=1, rerun=1) | ||
|
|
||
| import json |
There was a problem hiding this comment.
— Comment created by Claude
Minor: import json is function-local. The rest of the module imports stdlib modules at the top (import random, import re, ...). Please move it up there.
icemac
left a comment
icemac
left a comment
There was a problem hiding this comment.
I like the fix but I'd like to propose a different integration.
| commands = pytest tests/ {posargs} | ||
| deps = | ||
| xdist: pytest-xdist | ||
| pytest-reportlog |
There was a problem hiding this comment.
This makes pytest-reportlog a dependency for all tests, which I do not like.
I'd prefer a conditional dependency like it is done for xdist, but we should combine xdist and reflog into one group "deps" so we do not have a runner for each dependency. Also update the GitHub workflow accordingly.
— 100 % human review, no AI involved: Weight higher than the AI generated comments.
2 participants
Summary
pytest_runtest_logreport, sopytest-reportlogemits aTestReportper attempt withoutcome: "rerun"and areruncounterpytest-reportlogto the tox test deps, and documents the compatibility in the READMECloses #134
Test plan
--report-logand asserts the JSONL contains areruncall report followed by the finalpassedreport with correctrerunindices