Use streaming in all assertion comparisons consumers#14523
Use streaming in all assertion comparisons consumers#14523Pierre-Sassoulas wants to merge 6 commits into
Conversation
|
Thank you ! Do you have an opinion about the next step ? (3 options in the PR description) |
bluetech
left a comment
There was a problem hiding this comment.
See my last comment, I think it might affect the plan, so I'll wait before reviewing the rest.
| ) -> list[str] | None: | ||
| ) -> Iterator[str]: |
There was a problem hiding this comment.
Unfortunately we can't change the return type here, it is stable API (see hookspec.py).
There was a problem hiding this comment.
Maybe we can truncate in util.assertrepr_compare and still get the performance improvements without modifying the return type of the stable API ?
There was a problem hiding this comment.
I haven't looked at the truncation part, but if you can avoid the API break I'd be happy to take a look.
There was a problem hiding this comment.
So I moved the truncation to keep the performance without touching the API, but had to hack truncation a little.
First, we have to truncate twice because plugin maintainer expect the truncation to be handled where it was before I suppose.
Second, if we choose to drop the detail in the truncation message ("x lines hidden") then we can also drop the hacky check to see if there's the truncation header in the string for the second truncation. (I would favor that, small price to pay, imo), but right now I don't see an elegant way to do that as we have multiple way to truncate and it's not idempotent.
There was a problem hiding this comment.
if we choose to drop the detail in the truncation message ("x lines hidden")
I'm OK with dropping that detail; it is a small price to pay comparing to the benefits.
Addresses review feedback on PR pytest-dev#14523: * drop the redundant ``: Iterator[str]`` annotation on ``source`` — every branch already produces an ``Iterator[str]``. * return ``Iterator[str]`` from ``SetComparisonFunction`` instead of ``Iterable[str]`` so the call site no longer needs ``iter(...)``; the ``!=`` branch is promoted from a list-returning lambda to a named generator so the new contract holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds two regression tests to close the patch-coverage gaps in ``callbinrepr`` reported by codecov on PR pytest-dev#14523: * a plugin returning a truthy-but-empty iterator (``iter([])``) to exercise the second ``if not new_expl: continue`` after ``materialize_with_truncation``. * a ``--assert=plain`` run to exercise the false branch of the ``assertmode == "rewrite"`` guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When every ``pytest_assertrepr_compare`` impl returns ``None`` (e.g. ``assert 1 == 2`` — no specialised comparator applies), the dispatcher exhausts ``hook_result``, exits the loop normally, and returns ``None``. The previously-uncovered ``continue → loop exit`` arc on the first ``if not new_expl: continue`` line was the last patch coverage gap on PR pytest-dev#14523. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7c7b16b to
4119073
Compare
Addresses review feedback on PR pytest-dev#14523: * drop the redundant ``: Iterator[str]`` annotation on ``source`` — every branch already produces an ``Iterator[str]``. * return ``Iterator[str]`` from ``SetComparisonFunction`` instead of ``Iterable[str]`` so the call site no longer needs ``iter(...)``; the ``!=`` branch is promoted from a list-returning lambda to a named generator so the new contract holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds two regression tests to close the patch-coverage gaps in ``callbinrepr`` reported by codecov on PR pytest-dev#14523: * a plugin returning a truthy-but-empty iterator (``iter([])``) to exercise the second ``if not new_expl: continue`` after ``materialize_with_truncation``. * a ``--assert=plain`` run to exercise the false branch of the ``assertmode == "rewrite"`` guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When every ``pytest_assertrepr_compare`` impl returns ``None`` (e.g. ``assert 1 == 2`` — no specialised comparator applies), the dispatcher exhausts ``hook_result``, exits the loop normally, and returns ``None``. The previously-uncovered ``continue → loop exit`` arc on the first ``if not new_expl: continue`` line was the last patch coverage gap on PR pytest-dev#14523. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
I removed the number of removed line in the message. Benchmark on pathological case with very big set show big improvement, but I benchmarked on pylint test suite for a realistic perf change: round1-upstream: 118.29s Look like it's within noise. pylint does not compare big data structure (the bigger checks are probably the functional test output ~=80 lines). |
4119073 to
a325f81
Compare
Addresses review feedback on PR pytest-dev#14523: * drop the redundant ``: Iterator[str]`` annotation on ``source`` — every branch already produces an ``Iterator[str]``. * return ``Iterator[str]`` from ``SetComparisonFunction`` instead of ``Iterable[str]`` so the call site no longer needs ``iter(...)``; the ``!=`` branch is promoted from a list-returning lambda to a named generator so the new contract holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds two regression tests to close the patch-coverage gaps in ``callbinrepr`` reported by codecov on PR pytest-dev#14523: * a plugin returning a truthy-but-empty iterator (``iter([])``) to exercise the second ``if not new_expl: continue`` after ``materialize_with_truncation``. * a ``--assert=plain`` run to exercise the false branch of the ``assertmode == "rewrite"`` guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When every ``pytest_assertrepr_compare`` impl returns ``None`` (e.g. ``assert 1 == 2`` — no specialised comparator applies), the dispatcher exhausts ``hook_result``, exits the loop normally, and returns ``None``. The previously-uncovered ``continue → loop exit`` arc on the first ``if not new_expl: continue`` line was the last patch coverage gap on PR pytest-dev#14523. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review feedback on PR pytest-dev#14523: * drop the redundant ``: Iterator[str]`` annotation on ``source`` — every branch already produces an ``Iterator[str]``. * return ``Iterator[str]`` from ``SetComparisonFunction`` instead of ``Iterable[str]`` so the call site no longer needs ``iter(...)``; the ``!=`` branch is promoted from a list-returning lambda to a named generator so the new contract holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…4523) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…4523) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
e1691ca to
15cc828
Compare
c3c0a4a to
04ac28a
Compare
|
@bluetech @nicoddemus this is ready to review ! I could separate the first commit into its own MR, but I don't think it's worth it. |
…ters The assertion-repr hook only has the Config, not the Item, so accept a Config directly; the sole caller (truncate_if_required) passes item.config. Pure refactor, no behaviour change — prepares for deriving the truncation budget in the hook.
2721d1e to
29f151d
Compare
bluetech
left a comment
There was a problem hiding this comment.
Thanks!
The code mostly looks good to me.
Most of my comments are about the comments, which are a bit too verbose in some cases. Sorry for being pedantic on this, but in some cases conciseness is a virtue :)
| # "N more items" subdict is not pretty-printed in full (that work | ||
| # would be thrown away by truncation); only the smallest budget |
There was a problem hiding this comment.
Do we not have tests for truncation already? IIUC this commit does not change behavior really so I'm surprised new tests are needed.
There was a problem hiding this comment.
I'm not sure but we didn't have a test about not materializing the whole list. I tried to cover lines that weren't covered and might have gone too far.
| assert len(uncapped) > 8 | ||
|
|
||
| def test_mapping_extra_items_formatting_work_is_bounded(self) -> None: | ||
| # Non-regression guard: capping the *output* to N lines is not |
There was a problem hiding this comment.
This test is cool but maybe a bit excessive 🤔 We don't usually test for O(N) stuff, although in principle it's nice to have so I'm not against it.
There was a problem hiding this comment.
BTW I maybe mistaken but I think this test is added in a commit, then removed in a commit, then added again, maybe some rebase accident.
There was a problem hiding this comment.
I tried to clean up the history then rebase (merged all the comment simplification with pre-existing commit and resolved the review comment linked to them)
| ).splitlines() | ||
| # Highlight each ndiff line individually so the streaming truncator can | ||
| # stop pulling from ``difflib.ndiff`` once its budget is full; the diff | ||
| # lexer is line-oriented, so per-line highlighting is equivalent. |
There was a problem hiding this comment.
If pygments/pygments#1043 is ever resolved, this won't be correct anymore. But I guess it's fine for now.
| def cap_chars(self, text: str) -> str: | ||
| """Clip ``text`` to ``max_chars`` (``0`` leaves it unbounded).""" | ||
| return text[: self.max_chars] if self.max_chars > 0 else text | ||
|
|
||
| def cap_lines(self, lines: list[str]) -> list[str]: | ||
| """Clip ``lines`` to ``max_lines`` (``0`` leaves it unbounded).""" | ||
| return lines[: self.max_lines] if self.max_lines > 0 else lines |
There was a problem hiding this comment.
I think it's a bit strange for "truncation budget" (which is a piece of data) to do actual truncation. Maybe a standalone function?
A dict comparison with many keys only on one side ("Left/Right contains
N more items") pretty-printed the whole extra subdict even when the
explanation was about to be truncated, spending O(N) formatting lines
that are immediately discarded.
Thread the truncation line budget into ``_compare_eq_mapping``: past the
budget, emit only the smallest ``max_lines`` keys (one per line, via the
same safe sort ``pprint`` uses) instead of formatting the full subdict.
The item count stays in the header and the truncation footer is
unchanged, so no information the user would have seen is lost.
``assert {i: i for i in range(2000)} == {}`` drops from ~18 ms to ~1 ms
for the mapping portion of the explanation.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The assertion-repr hook returns a lazy iterator; feed it through a new
``materialize_with_truncation`` that pulls only until the truncation
budget is reached, instead of building the whole explanation as a list
and truncating it afterwards.
Because the diff is no longer materialised in full, the exact hidden-line
count can no longer be computed: the truncation footer drops it
("...Full output truncated (N lines hidden)..." becomes "...Full output
truncated..."). Docs and the approx test are updated to match.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Widen the mapping ``extra_items_max_lines`` budget into a per-side ``TruncationBudget`` of ``(max_lines, max_chars)`` and thread it through every formatting path: iterable/mapping ``pformat`` is capped before ``ndiff``, the text (``ndiff``) diff caps its inputs, and ``not in`` reuses the same cap. When truncation will clip the output anyway, the helpers no longer spend O(N) formatting lines and chars that are about to be dropped. A truncated diff now reflects a local alignment of the bounded prefix; ``-vv`` (no truncation) still produces the exact full diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add tests for ``materialize_with_truncation`` (lazy stop, sized vs iterator input, idempotence) and the budget caps on the text paths, plus deterministic, timing-free non-regression guards: the explanation stream is not over-consumed, the pulled-line count is independent of input size, and the formatting work behind a 10-line display stays bounded as the input grows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The callequal/callop helpers run pytest_assertrepr_compare with truncation disabled, so the branch that derives a TruncationBudget from the truncation limits and feeds it to the comparison helpers went untested. Extend mock_config with truncation limits and a has_terminalreporter flag, then add TestAssertReprCompareDispatcher covering: both limits (capped formatting work), each partial-budget arm (only lines / only chars), and the no-terminalreporter plaintext fallback (pytest-dev#14377), which now asserts the terminal writer is never reached. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
29f151d to
082efa1
Compare
Follow-up to the streaming-comparison chain: #14521 (comparators → generators), #14546 (base comparisons return an iterator), #14587 (set comparison →
Iterator[str]), #14588 (PrettyPrinterformats lazily, budget-cappable). Those moved the assertion machinery to generators and a lazy pretty-printer; this PR wires the truncation budget through the remaining consumers so a large diff is never fully built when truncation is going to clip it anyway.The truncation footer no longer reports the hidden-line count
...Full output truncated (N lines hidden), use '-vv' to showbecomes...Full output truncated, use '-vv' to show. Counting the hidden lines means materialising the whole diff, which is costly.-vv(no truncation) stays exact, and anything within the budget is unchanged.Benchmark
Small / common case (tiny inputs — the realistic failing assert)
list [1,2,3]vs[1,2,4]tuple3 elemsset3 elemsdict3 keys, 1 differdictextra keystrshortstr5-line multilinelistof 10 dictsCommon asserts cost +3% to +9% (a few µs of per-chunk budget bookkeeping);
setandlist-of-dicts are faster (set-sort fast path + lazy chunked pformat). Absolute cost stays0.06–0.27 ms.
N = 2000
dict == dict(values differ)str == str(lines differ)listof 3 × 100k-char stringstuple == tuplelist == listnested [{N}] == [{}]dictextra keys ({N}=={})set == setN = 20000 (fast shapes;
dict==dictvalues &str==strskipped — O(N²) on main)tuple == tuplelist == listnested [{N}] == [{}]dictextra keys ({N}=={})set == setlist/tuplestay flat (~0.16 ms) — fully bounded.set(sort O(N log N)),dictextra (nsmallestO(N) scan) andnestedstill scale with N — far below main, but not constant.not in(needle not in huge_text) joins the bounded shapes: ~2.8 s → ~3 ms for a 1M-char haystack now that itsndiffinputs are capped.dict == dictis the extreme: near-identical value lines (1999: 1999,vs1999: 2000,) tripdifflib's intraline_fancy_replace, making the full path ~O(N²). This ispre-existing — on pytest 8.4.2,
pytest -vonassert {i: i for i in range(500)} == {i: i + 1 for i in range(500)}takes 83 s; this PR brings it to ~1 ms.Benchmark (branch-adaptive; tiers: small / n2000 / n20000)
Raw benchmark results:
On main:
On branch