Repository navigation
fix(pytest): preserve elapsed duration in summaries - #4076
Conversation
📊 Automated PR Analysis
SummaryAdds preservation of pytest's elapsed-duration token in compact summaries, keeping timing signals visible for passed, failed, skipped, xfailed, and xpassed results across wrapped and quiet output formats. Review Checklist
Linked issues: #4071 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.
Tried this locally with real pytest 8.4.2. Short runs look right:
Pytest: 2 passed in 0.01s
Pytest: 1 passed, 1 failed, 1 skipped in 0.04s
The duration is dropped once a run passes 60 seconds, though. pytest appends the wall-clock form after the seconds:
========================= 1 passed in 61.07s (0:01:01) =========================
parse_duration takes everything after the last " in ", i.e. 61.07s (0:01:01). That doesn't end in s, so strip_suffix('s') returns None, and the filtered output is just:
Pytest: 1 passed
That's the case the PR description is about (telling a fast suite apart from a slow regression), so it seems worth covering. Taking the first token after " in " would handle both forms:
let token = duration.split_whitespace().next()?; // "61.07s"
token.strip_suffix('s')?.parse::<f64>().ok()?;
Some(token.to_string())and a test with "=== 1 passed in 61.07s (0:01:01) ===" -> Pytest: 1 passed in 61.07s would pin it.
Overlap: #4192 (collect-only summaries) changes the same PytestCounts / build_pytest_summary / parse_summary_line code, and a trial merge of the two branches conflicts in src/cmds/python/pytest_cmd.rs. Whichever lands second will need a rebase; if this one goes first, the collected-only summary from #4192 could pick up the duration too.
|
Confirming the point above against the current head (28627f3) with pytest 9.1.1. What has to change before this can go in: keep the whole tail after |
|
Thanks @Protocol-zero-0 and @KuSh — fixed in ebb6b72. parse_duration now takes the first whitespace-delimited token after the final I added a regression test for that exact long-run shape. Validation completed successfully:
|
…its it parse_duration rejects a tail with no trailing `s` or no number; the filter keeps the duration on the wrapped long-run form with a warnings part, on the only-skipped path and on the quiet-mode failures line. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — APPROVED, follow-ups filed
Claim: rtk pytest keeps pytest's elapsed-duration token (3.04s, and the seconds half of 65.12s (0:01:05)) on the compact summary line, on the green single-line path and on the multi-line failed/skipped/xfail paths.
Scope: accept (round 1, frozen). #4192 (collect-only summaries) edits the same functions and conflicts on a trial merge; this PR is smaller and CI-green, so it lands first and #4192 rebases. #3577 also touches pytest_cmd.rs, no conflict today.
Ran: pytest 9.1.1 (local, LC_ALL=C) and pytest 7.4.0 (the reporter's version, Docker) × 22 rtk pytest rows (green, failed+skipped+xfail+xpass+warning, only-skipped, xpass-only, fixture error, collection error, no tests, -k no match, -q, -v, --tb=long, --color=yes, --co, --durations, --version, --help, 30 failures over the 10 cap, missing dir) + 6 rtk pipe --filter pytest rows + one real 61 s run, before (merge-base 5e0f92c) / after (ebb6b72); exit codes identical on every row; invariants: under the hook (8 rtk rewrite forms, all return rtk pytest … verbatim) | over-cap (30 failures: 10 shown, +20 more notice and tee hint, duration on the header line) | split streams (stderr empty on every row but the missing-dir one) | every emit path reached; savings 73% green, 60% failing, 74% over-cap vs 20% floor (CONTRIBUTING.md). Gates: fmt, clippy -D warnings, cargo test --all (3615 unit) green on the head and on the head merged onto develop @45b0b4c3.
Matrix: executed this round.
CI @ebb6b720: 10/10 green; @80cee418 (fixup): 10/10 green.
Blocks merge (0)
None. The >60 s form asked for in the review comment is confirmed: real 61 s suite → Pytest: 1 passed in 61.00s (before: Pytest: 1 passed); 1 passed in 61.07s (0:01:01) bare and ===-wrapped → in 61.07s on both entry points; same on pytest 7.4.0.
Filed as follow-ups
- #4312 pytest filter takes a captured-stdout lookalike (
1 passed in 0.01sprinted by a test) as the session summary →Pytest: 1 passed, exit 1 — pre-existing, outside this PR's hunks (the line selection is untouched; the PR only appends that line's duration); filed - #2960
-qqon the CLI hits trigger 1 the same way (rtk pytest -qq→No tests collected, exit 0) — pre-existing; comment added - #3738
rtk pipe --filter pyteston--color=yesoutput →No tests collectedon both binaries;rtk pyteststrips ANSI, the pipe entry does not — pre-existing; comment added
Checked and correct — no need to re-verify
parse_durationon every summary shape real pytest emits: bare-q,===-wrapped,N warning(s)/N errorparts beforein,no tests ran in 0.00s(parsed, then correctly short-circuited by the zero-count return), CRLF,--color=yesunderrtk pytest(stripped first).- Both emit sites carry the suffix: green single line, and the multi-line path for failures, only-skipped and xfail/xpass, with the failure block unchanged after it.
- Passthrough rows (fixture error
1 error in 0.01s, collection error,--version,--help,--co) are byte-identical before/after. - Mutation check on the head: whole-tail (the pre-fix code), dropping either emit site → caught by the PR's tests. The two guards inside
parse_duration(trailings, numeric) had no test; the fixup below pins them.
Your questions
None open.
Next
fixup 80cee41 pushed: test-only — pins the two parse_duration guards and the duration on the wrapped long-run-with-warnings, only-skipped and quiet-mode-failures shapes — approving once its CI is green.
Rounds: 1/3. Threads: 0 open, 0 resolved this round.
|
Thanks @igorzelaya-io and @KuSh for following through on the duration case. I saw this and #4053 were merged. I'd like to take ongoing responsibility for the Python ecosystem's issue triage, regression tests and PR reviews. Is that scope still looking for an Ecosystem Maintainer, and who should I coordinate with? I've read the application requirements of three merged PRs and three helpful reviews; my reviews are linked on #4192, #4076, #4053 and #4196, but my own #4203 and #4204 are still open, so I'm not claiming to meet the merged-PR requirement yet. I'd like to establish the right scope and contact while finishing those. |
|
Hey @Protocol-zero-0 Thanks for the detailed proposal, and thanks @KuSh for sharing it. Yes, we're still looking for ecosystem maintainers, and the Python scope you described sounds like a good fit. I'll take a look at your open PRs (#4203 and #4204) in the meantime. Are you already on our Discord server? If not, feel free to join and message me or @KuSh directly. It'll be easier to go over the scope and next steps there. We're happy to move forward from there if that works for you. |
|
Thanks @aeppling and @KuSh. Both open PRs are now updated: #4203 makes the compound Python rewrite coverage explicit (62becfe); #4204 preserves the shell assignment and resolves the conflict with current develop (7e42000). Both passed fmt, clippy and the full local test suite, and I answered the review findings. For the Python scope, I can take issue reproduction/triage, regression tests for pytest/pip/uv output, and review of Python filtering PRs. I do not have Discord access set up yet; could we use this thread to agree on a first bounded maintenance task and its acceptance criteria? Who would confirm the ongoing scope and maintainer access once the contribution requirements are met? |
Summary
Why
Without the duration, a fast suite and a severe performance regression produce the same filtered output. This keeps pytest's original value (for example,
3.04s) while adding only the small timing signal the agent otherwise cannot recover from the tool result.Fixes #4071.
Verification
cargo fmt --all --checkcargo clippy --all-targets -- -D warningscargo test --all(3,614 unit tests plus all integration tests passed)