Repository: pytest-dev/pytest
Pull request: Fix error assertion handling in approx when None in dict comparison
Closes #10702

Dict comparsion in the ApproxMapping class did not check if values were None before attempting to subtract for max_abs_diff stat, which was throwing an TypeError instead of being handled by pytest error assertion. Check for None has been added before these calculations, so that None will properly show as Obtained/Expected in pytest assert message

<!--
Thanks for submitting a PR, your contribution is really appreciated!

Here is a quick checklist that should be present in PRs.

- [ ] Include documentation when adding new features.
- [ ] Include new tests or update existing tests when applicable.
- [X] Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

If this change fixes an issue, please:

- [ ] Add text like ``closes #XYZW`` to the PR description and/or commits (where ``XYZW`` is the issue number). See the [github docs](https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) for more information.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

- [ ] Create a new changelog file in the `changelog` folder, with a name like `<ISSUE NUMBER>.<TYPE>.rst`. See [changelog/README.rst](https://github.com/pytest-dev/pytest/blob/main/changelog/README.rst) for details.

  Write sentences in the **past or present tense**, examples:

  * *Improved verbose diff output with sequences.*
  * *Terminal summary statistics now use multiple colors.*

  Also make sure to end the sentence with a `.`.

- [ ] Add yourself to `AUTHORS` in alphabetical order.
-->


Problem statement:
`pytest.approx` fails with `TypeError: unsupported operand type(s) for -: 'float' and 'NoneType'`
When using `approx` to test float and one of the objects in the `assert` statement contain `None` I see the following TypeError:

`TypeError: unsupported operand type(s) for -: 'float' and 'NoneType'.
`

## Minimal example
### Test
```
import pytest


# Expecting assertion error with differing item
# Instead I see "TypeError: unsupported operand type(s) for -: 'float' and 'NoneType'."
def test_pytest_none_approx():
    actual_result = {"a": 1.2}
    expected_result = {"a": None}
    approx_expected_result = pytest.approx(expected_result)
    assert approx_expected_result == actual_result
```
### Output
```
E       AssertionError: assert approx({'a': 1.2 ± 1.2e-06}) == {'a': None}
E         (pytest_assertion plugin: representation of details failed: /Users/milanwiedemann/.pyenv/versions/3.10.4/lib/python3.10/site-packages/_pytest/python_api.py:270: TypeError: unsupported operand type(s) for -: 'float' and 'NoneType'.
E          Probably an object has a faulty __repr__.)
```

## `pip list`

```
Package        Version
-------------- -------
attrs          22.2.0
exceptiongroup 1.1.0
iniconfig      2.0.0
packaging      23.0
pip            22.0.4
pluggy         1.0.0
pytest         7.2.1
setuptools     58.1.0
tomli          2.0.1
```

## Cersions of OS and pytest

- macOS 12.6.3
- python 3.10.4
- pytest 7.2.1

Hints from issue discussion:
We should probably error earlier, none is not approx-able

It seems to be a mistake to let approx take dict for convenience 
It appears that `None` is currently functional when passing it directly to approx, but not when passed as part of a dictionary (which gets processed by the `ApproxMapping` class, and a subtraction operation on the dict value is throwing the error).

I do agree that None shouldn't be approx-able based on what the function should be doing, though an early error on `None` may break existing tests, so should `ApproxMapping` instead be adjusted to better handle `None`? The documentation does include dictionaries with `None` values, and keeping the dict functionality may be helpful when dealing with multiple values from the same function run in a way that parameterize wouldn't cover.

I'm open to work on any of the above options, let me know the preferred direction!
> so should ApproxMapping instead be adjusted to better handle None?

I vote for that one; deprecating `None` would be a whole can of worms, but handling `None` seems to be the way to fix this, given the current failure is not acceptable.

Commit message: Merge branch 'main' into add-none-support-approxmapping

Patch to review:
diff --git a/changelog/10702.bugfix.rst b/changelog/10702.bugfix.rst
new file mode 100644
index 00000000000..4008cc882ec
--- /dev/null
+++ b/changelog/10702.bugfix.rst
@@ -0,0 +1 @@
+Fixed error assertion handling in :func:`pytest.approx` when ``None`` is an expected or received value when comparing dictionaries.
diff --git a/src/_pytest/python_api.py b/src/_pytest/python_api.py
index 0967ae8ece6..35560f16860 100644
--- a/src/_pytest/python_api.py
+++ b/src/_pytest/python_api.py
@@ -265,19 +265,20 @@ def _repr_compare(self, other_side: Mapping[object, float]) -> List[str]:
             approx_side_as_map.items(), other_side.values()
         ):
             if approx_value != other_value:
-                max_abs_diff = max(
-                    max_abs_diff, abs(approx_value.expected - other_value)
-                )
-                if approx_value.expected == 0.0:
-                    max_rel_diff = math.inf
-                else:
-                    max_rel_diff = max(
-                        max_rel_diff,
-                        abs(
-                            (approx_value.expected - other_value)
-                            / approx_value.expected
-                        ),
+                if not any((approx_value.expected is None, other_value is None)):
+                    max_abs_diff = max(
+                        max_abs_diff, abs(approx_value.expected - other_value)
                     )
+                    if approx_value.expected == 0.0:
+                        max_rel_diff = math.inf
+                    else:
+                        max_rel_diff = max(
+                            max_rel_diff,
+                            abs(
+                                (approx_value.expected - other_value)
+                                / approx_value.expected
+                            ),
+                        )
                 different_ids.append(approx_key)
 
         message_data = [
diff --git a/testing/python/approx.py b/testing/python/approx.py
index 631e52b56ac..6ad411a3e07 100644
--- a/testing/python/approx.py
+++ b/testing/python/approx.py
@@ -122,6 +122,23 @@ def test_error_messages_native_dtypes(self, assert_approx_raises_regex):
             ],
         )
 
+        assert_approx_raises_regex(
+            {"a": 1.0, "b": None, "c": None},
+            {
+                "a": None,
+                "b": 1000.0,
+                "c": None,
+            },
+            [
+                r"  comparison failed. Mismatched elements: 2 / 3:",
+                r"  Max absolute difference: -inf",
+                r"  Max relative difference: -inf",
+                r"  Index \| Obtained\s+\| Expected\s+",
+                rf"  a     \| {SOME_FLOAT} \| None",
+                rf"  b     \| None\s+\| {SOME_FLOAT} ± {SOME_FLOAT}",
+            ],
+        )
+
         assert_approx_raises_regex(
             [1.0, 2.0, 3.0, 4.0],
             [1.0, 3.0, 3.0, 5.0],


Code review comments:
diff_hunk: @@ -265,19 +265,20 @@ def _repr_compare(self, other_side: Mapping[object, float]) -> List[str]:
             approx_side_as_map.items(), other_side.values()
         ):
             if approx_value != other_value:
-                max_abs_diff = max(
-                    max_abs_diff, abs(approx_value.expected - other_value)
-                )
-                if approx_value.expected == 0.0:
-                    max_rel_diff = math.inf
-                else:
-                    max_rel_diff = max(
-                        max_rel_diff,
-                        abs(
-                            (approx_value.expected - other_value)
-                            / approx_value.expected
-                        ),
+                if not any((approx_value.expected is None, other_value is None)):
path: src/_pytest/python_api.py
text: @user1:
Suggestion: I personally prefer to be explicit, using `any` here makes the code needlessly more complex than it needs to be.

```suggestion
                if approx_value.expected is not None and other_value is not None:
```

@author:
Works for me, seeing both I do agree it's more clear than negating the ``any``. Thanks!