Use streaming in all assertion comparisons consumers#14523
Open
Pierre-Sassoulas wants to merge 12 commits into
Open
Use streaming in all assertion comparisons consumers#14523Pierre-Sassoulas wants to merge 12 commits into
Pierre-Sassoulas wants to merge 12 commits into
Conversation
Following Ronny's review comment on pytest-dev#13762, switch the set comparison helpers in ``_compare_set.py`` to return ``Iterator[str]`` so the composition is direct: ``_set_one_sided_diff`` ``yield``s, and the other helpers ``yield from`` it. This avoids the manual ``explanation = []; .append/.extend`` boilerplate. The "equal sets" branch of ``_compare_gt_set`` / ``_compare_lt_set`` used to peek at the diff for emptiness; replace that with a direct ``left == right`` check so the generator form stays idiomatic. ``SET_COMPARISON_FUNCTIONS`` and ``_compare_eq_set`` now return ``Iterable[str]`` / ``Iterator[str]``; the consumers in ``_compare_eq_any`` materialise with ``list(...)``.
Drop the ``list(...)`` wraps around each per-type comparator call in the match dispatch and ``yield from`` instead. ``_compare_eq_any`` becomes an ``Iterator[str]`` that yields nothing when no specialised explanation applies (replaces the previous ``list[str] | None`` sentinel). The two callers materialise: * ``util.assertrepr_compare`` does ``list(_compare_eq_any(...))`` before its empty/summary check. * ``_compare_eq_cls`` iterates the generator directly via ``for line in _compare_eq_any(...)``. No behavior change yet — this is the stepping stone for letting the truncator upstream consume the iterator lazily so huge diffs don't materialise just to be thrown away.
Turn ``assertrepr_compare`` into a generator. The first line yielded is the summary; subsequent lines are the explanation produced by ``_compare_eq_any``. Yields nothing when no specialised explanation applies — the consumer maps an empty iterator to ``None``. The ``pytest_assertrepr_compare`` hook impl in ``assertion/__init__`` materialises the iterator and returns ``list[str] | None`` so the public hook contract is unchanged. A follow-up commit replaces the ``list(...)`` call with a streaming truncator so an enormous diff doesn't have to be built in full just to be discarded. Behaviour change: previously, if an exception was raised while building the explanation (e.g. a faulty ``__repr__``), the partial output was discarded and only the failure notice was returned. The generator can't unyield lines it has already produced, so the new form preserves the partial output and appends the failure notice after it. This is arguably more useful — the reader sees what was being compared at the point the comparison failed. ``test_list_bad_repr`` is updated to assert that the failure notice appears at the end of the explanation instead of replacing the body.
…ions The existing ``truncate_if_required`` takes a ``list[str]`` — it can only trim *after* the full explanation has been built. Add a streaming counterpart that takes an ``Iterable[str]`` and stops pulling lines as soon as the truncation threshold is reached, so a huge comparison doesn't have to materialise its entire output just to be discarded. The remaining lines are still iterated past the cap (without storing) so the truncation footer can report the exact hidden-line count, and ``_truncate_explanation`` gains an ``extra_hidden`` argument to fold that count into the message. ``_get_truncation_parameters`` is also refactored to take a ``Config`` directly (it never used anything else from ``Item``), so the new streaming helper can be called from places that don't have an item handy. The new helper isn't wired up yet — that's the next commit.
Wire the built-in ``pytest_assertrepr_compare`` hook to return the iterator produced by ``util.assertrepr_compare`` directly, and update ``callbinrepr`` to consume it through ``materialize_with_truncation``. The result: a comparison that would produce millions of explanation lines stops at the truncation threshold (default 8 lines / 640 chars) without materialising the rest, only counting the remaining lines so the truncation footer still reports the exact hidden-line count. The ``callbinrepr`` dispatcher's ``materialize_with_truncation`` call accepts both lists (returned by third-party plugins implementing the hook) and iterators (returned by the built-in impl), so the change is transparent to plugin authors. ``callop`` in ``test_assertion`` now materialises the iterator so tests keep comparing against literal lists.
Pierre-Sassoulas
added
type: performance
* Drop ``truncate.truncate_if_required`` — all callers migrated to
``materialize_with_truncation`` and the function had no remaining
users.
* Add ``TestMaterializeWithTruncation`` covering:
- iterator within limits returns all lines
- iterator past limits is bounded and contains a truncation marker
- sized and unsized inputs produce equivalent shapes
- truncation is skipped at ``-vv``
- the lines that survive truncation start with the original input
Assertions check behaviour (the presence of a "truncated" marker,
the length being bounded, the first lines being preserved), never
the literal footer wording — so the tests survive a future decision
to drop the ``(N lines hidden)`` count from the message.
* Add ``test_plugin_hook_returning_none_is_skipped`` to cover the
``if new_expl is None: continue`` branch in ``callbinrepr``.
* Add ``test_exception_before_first_yield_emits_summary_and_notice``
to cover the ``summary_yielded is False`` arm of
``assertrepr_compare``'s exception handler — when the comparator
raises before yielding anything, the summary is still produced so
the reader sees what was compared.
psf-chronographer
Bot
added
the
bot:chronographer:provided
Member
Author
|
Thank you ! Do you have an opinion about the next step ? (3 options in the PR description) |
bluetech
reviewed
May 27, 2026
Member
bluetech
left a comment
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.
| try: | ||
| if op == "==": | ||
| explanation = _compare_eq_any( | ||
| source: Iterator[str] = _compare_eq_any( |
| elif op == "not in" and istext(left) and istext(right): | ||
| source = _notin_text(left, right, verbose) | ||
| elif op in {"!=", ">=", "<=", ">", "<"} and isset(left) and isset(right): | ||
| source = iter( |
Member
There was a problem hiding this comment.
If we change SetComparisonFunction to return Iterator instead of Iterable, can avoid the iter here. Alternatively, change source to Iterable[str].
Comment on lines
+220
to
+228
| ) -> list[str] | None: | ||
| ) -> Iterator[str]: |
Member
There was a problem hiding this comment.
Unfortunately we can't change the return type here, it is stable API (see hookspec.py).
Member
Author
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 ?
Member
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.
Member
Author
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.
… None`` The hookspec advertises ``list[str] | None`` as the stable return type; the streaming refactor had changed the built-in impl to ``Iterator[str]``. Restore the spec'd shape by materialising inside the hook through ``materialize_with_truncation`` — the iterator from ``util.assertrepr_compare`` is still consumed lazily, so a huge diff short-circuits at the truncation threshold without being fully built. To avoid double-truncation between the hook and ``callbinrepr`` (which still truncates plugin-supplied lists), make ``materialize_with_truncation`` idempotent on inputs that already end in our truncation footer. 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>
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>
Nest the truthiness checks instead of using ``continue`` to skip to the next ``hook_result`` entry. Behaviourally identical, but each ``continue`` was being reported as a partial branch by codecov even when both arcs were hit by tests — pytester-driven in-process tests don't always show the ``continue → loop exit`` arc, so the partials were sticky. With the nested form the for-loop has a single fall- through edge and the partial disappears. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #14521, if we push the concept of generator for comparators to all the consumers of comparator, we can avoid computing big diff when they are going to be truncated anyway, and ultimately make pytest faster (maybe also make the output with
-vvvmore fluid).I didn't do it because the truncation footer could no longer report an exact hidden-line count. It would change from "...Full output truncated (499992 lines hidden), use '-vv' to show" to something like "...Full output truncated, use '-vv' to show", but on 500k element list/dict/set it makes pytest 2x faster.
Ways to claim that speedup:
Fishing for opinions here :)