Repository navigation
fix(pytest): preserve collection-only summaries - #4192
Ha1baraA11 wants to merge 2 commits into
Conversation
📊 Automated PR Analysis
SummaryFixes pytest output filtering so that Review Checklist
Linked issues: #4178 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Tested this locally against real pytest 8.4.2 output, comparing develop (b748a5f) with this branch. The plain case is fixed, but two variants of --collect-only still go wrong, and one of them is a regression.
Fixed: pytest --collect-only and --collect-only -q now give Pytest: 15 tests collected instead of No tests collected.
1. -k still reports "No tests collected". With a keyword filter, pytest writes the count as N/M:
================ 2/15 tests collected (13 deselected) in 0.01s =================
words[i - 2] is "2/15", which doesn't parse as usize, so collected stays 0 and both --collect-only -k ... and --collect-only -q -k ... still print Pytest: No tests collected — the #4178 symptom, for a common way of running collection. Taking the part before / ("2/15".split('/').next()) would cover it.
2. A collection error is now hidden (regression). With one test module that fails to import:
$ pytest --collect-only tests broken # exit 2
...
E ModuleNotFoundError: No module named 'nonexistent_module_xyz'
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
15 tests collected, 1 error in 0.22s
On develop, rtk pytest falls back to the raw output here, so the error is visible. On this branch the summary line now matches collected, so the output becomes just:
Pytest: 15 tests collected
[full output: rtk recall 8d248d09eb3f]
The exit code is still 2, but the agent is told "15 tests collected" with nothing about the failed import, so it has to guess why the command failed. It seems safer to skip the collected-only shortcut whenever the summary also reports error/errors, and keep the existing fallback for that case.
Test cases that would pin both:
"================ 2/15 tests collected (13 deselected) in 0.01s ================="->Pytest: 2 tests collected"15 tests collected, 1 error in 0.22s"(plus theInterruptedline) -> must not collapse toPytest: 15 tests collected
Heads-up on overlap: #4076 edits the same PytestCounts / build_pytest_summary / parse_summary_line code. A trial merge of the two branches (git merge-tree) conflicts in src/cmds/python/pytest_cmd.rs, so whichever lands second will need a rebase. If #4076 lands first, the collected-only summary could carry its duration too (15 tests collected in 0.17s) for consistency.
Protocol-zero-0
left a comment
There was a problem hiding this comment.
Re-reviewed c76ebd0 against the previously reviewed 49ba389 with real pytest 9.0.3 on Linux. Both findings from my earlier review are addressed.
I ran 11 collection scenarios through raw pytest, the old head and this head (33 CLI executions): normal and quiet collection; -k with/without -q; zero matches with/without -q; import errors with normal, quiet, filtered and -qq output; and a clean -qq control. With 15 tests and -k test_1, raw pytest selects 6/15: the old head says No tests collected, while this head reports 6 tests collected. With a broken import, the old normal/quiet paths hide the exception behind 15 tests collected; this head keeps the ModuleNotFoundError and module name visible. All tested exit codes match raw pytest (0, 2 and 5). Clean -qq output stays available as pytest's per-file count rather than being claimed as a parsed summary.
All 16 pytest-filter unit tests, fmt and clippy (--all-targets, warnings denied) pass. I did not run the full repository suite or other platforms.
The focused collection fix looks good. #3355 and #4466 also change the pytest summary machinery, so their eventual integration should retain these collection and error-preservation cases.
Summary
Fixes pytest --collect-only renders as "No tests collected" while 15 tests collected #4178
Test plan
cargo fmt --all && cargo clippy --all-targets && cargo testrtk <command>output inspectedAutomated regression coverage and repository validation passed.
The full-suite check reproduces only the recorded pre-existing failures on the pristine base; the affected regression tests pass.