Skip to content

Fix 13010 - Ensure pytest.approx in sequence/collections only process instances of Number - #13012

Merged
nicoddemus merged 12 commits into
pytest-dev:mainfrom
marcelotrevisani:fix-13010-iterable-non-numeric-skip
Nov 30, 2024
Merged

nicoddemus merged 12 commits into
pytest-dev:mainfrom
marcelotrevisani:fix-13010-iterable-non-numeric-skip

Conversation

@marcelotrevisani

@marcelotrevisani marcelotrevisani commented Nov 29, 2024

Copy link
Copy Markdown
Contributor

#13010
Add a check to ensure that approx calculations only process instances of Number, preventing errors with incompatible types.

Add a check to ensure that `approx` calculations only process instances of `Number`, preventing errors with incompatible types.
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Nov 29, 2024

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @marcelotrevisani for the PR, appreciated!

Left some small comments, other than that, LGTM.

Comment thread testing/python/approx.py
Comment thread testing/python/approx.py Outdated
Comment thread changelog/13010.bugfix.rst Outdated
@marcelotrevisani

Copy link
Copy Markdown
Contributor Author

all done @nicoddemus !
thanks for taking the time to review this, appreciated! :)

Comment thread src/_pytest/python_api.py
@marcelotrevisani

Copy link
Copy Markdown
Contributor Author

I sorted all comments and problems. If you have new suggestions please let me know. Thanks!

@nicoddemus

Copy link
Copy Markdown
Member

FYI changed to a try/except instead of an instance check because mypy was correctly complaining about Decimal and float -- this is safer backward-compatibility wise too.

@nicoddemus
nicoddemus enabled auto-merge (squash) November 30, 2024 12:18
@nicoddemus
nicoddemus merged commit d65d991 into pytest-dev:main Nov 30, 2024
@marcelotrevisani

Copy link
Copy Markdown
Contributor Author

FYI changed to a try/except instead of an instance check because mypy was correctly complaining about Decimal and float -- this is safer backward-compatibility wise too.

Indeed, I believe your modification is better, thanks!

RonnyPfannschmidt added a commit to RonnyPfannschmidt/pytest that referenced this pull request Sep 13, 2026
All three _repr_compare implementations seeded their running maxima with
-math.inf and tested `== 0.0`, so building a failure message for Decimal
collections compared a float against a Decimal. That comparison is exact,
but it signals decimal.FloatOperation when that trap is set, and the two
container variants handled the signal differently:

- ApproxSequenceLike caught it. decimal.FloatOperation is a TypeError
  subclass, so the `except TypeError` added for pytest-dev#13012 to skip
  non-numbers swallowed it and the message reported
  `Max absolute difference: -inf`.
- ApproxMapping did not, so it escaped out of the assertion formatting
  as a bare decimal.FloatOperation.

Accumulate from None instead and only substitute -math.inf at the end,
where it still means "no difference could be computed" as pytest-dev#13012 needs.
Divide-by-zero is tracked as a flag rather than by assigning math.inf
mid-loop, which would reintroduce the mixing on the next element. The
sequence variant now re-raises decimal.FloatOperation explicitly, so a
future float leak fails loudly instead of reporting -inf.

Co-Authored-By: Claude Opus 5 (1M context) via Claude Code <noreply@anthropic.com>
RonnyPfannschmidt added a commit to RonnyPfannschmidt/pytest that referenced this pull request Sep 13, 2026
All three _repr_compare implementations seeded their running maxima with
-math.inf and tested `== 0.0`, so building a failure message for Decimal
collections compared a float against a Decimal. That comparison is exact,
but it signals decimal.FloatOperation when that trap is set, and the two
container variants handled the signal differently:

- ApproxSequenceLike caught it. decimal.FloatOperation is a TypeError
  subclass, so the `except TypeError` added for pytest-dev#13012 to skip
  non-numbers swallowed it and the message reported
  `Max absolute difference: -inf`.
- ApproxMapping did not, so it escaped out of the assertion formatting
  as a bare decimal.FloatOperation.

Accumulate from None instead and only substitute -math.inf at the end,
where it still means "no difference could be computed" as pytest-dev#13012 needs.
Divide-by-zero is tracked as a flag rather than by assigning math.inf
mid-loop, which would reintroduce the mixing on the next element. The
sequence variant now re-raises decimal.FloatOperation explicitly, so a
future float leak fails loudly instead of reporting -inf.

Co-Authored-By: Claude Opus 5 (1M context) via Claude Code <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants