Repository: pytest-dev/pytest
Pull request: Miscellaneous improvements to approx()
This PR fixes a couple small issues with ``approx()``:

- Change the output of ``repr(approx(np.array(...)))`` to reflect the dimension of the array being compared.  For example, ``repr(approx([[1, 2], [3, 4]]))`` would look something like ``approx([[1±..., 2±...], [3±..., 4±...]])`` rather than ``approx([1±..., 2±..., 3±..., 4±...])``.  This was supposed to fix #3712, but it turns out that #3615 already fixed it.  Still, I think it's nice to have a more accurate repr.
- Check for type errors as soon as the approx object is instantiated, rather than waiting until a comparison is made.  Fixes #3473.  This includes very clear error messages if the user tries to use nested dictionaries or lists, since people seem to assume that this will work.  (And maybe it should, but that's a different discussion.)
- Improve some comments.


Problem statement:
'approx' should error immediately if an invalid parameter is passed
Using pytest.approx with an invalid parameter (eg, a string) should fail immediately rather than waiting until it calls repr(). Example:

```
>>> import pytest
>>> x = pytest.approx("abc")
>>> x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/michael/.virtualenvs/tavern/lib/python3.5/site-packages/_pytest/python_api.py", line 181, in __repr__
    if math.isinf(self.expected):
TypeError: a float is required
```

I would expected it to fail on the second line rather than when it prints it out.

This gets more confusing when you're logging it:

```python
# example.py
import pytest
import logging

x = pytest.approx("abc")

logging.basicConfig(level=logging.DEBUG)

logging.info(x)
```
output:
```
$ python example.py 
--- Logging error ---
Traceback (most recent call last):
  File "/usr/lib/python3.5/logging/__init__.py", line 980, in emit
    msg = self.format(record)
  File "/usr/lib/python3.5/logging/__init__.py", line 830, in format
    return fmt.format(record)
  File "/usr/lib/python3.5/logging/__init__.py", line 567, in format
    record.message = record.getMessage()
  File "/usr/lib/python3.5/logging/__init__.py", line 328, in getMessage
    msg = str(self.msg)
  File "/home/michael/.virtualenvs/tavern/lib/python3.5/site-packages/_pytest/python_api.py", line 181, in __repr__
    if math.isinf(self.expected):
TypeError: a float is required
Call stack:
  File "example.py", line 8, in <module>
    logging.info(x)
Unable to print the message and arguments - possible formatting error.
Use the traceback above to help find the error.
```

This issue in particular would be solved by putting the math.isinf check in the constructor rather than only checking when printing out the value.

Broken output when a multidimensional NumPy array fails to be approximate something
Consider the following example comparing two multidimensional arrays using approx:

```Python
from pytest import approx
import numpy as np

def test_something():
	shape = (5,4)
	X = np.ones(shape)
	Y = np.ones(shape)*2
	assert X==approx(Y)
```

The error thrown by PyTest is:

```
E    assert array([[1., 1., 1., 1.],\n       [1., 1., 1., 1.],\n       [1., 1., 1., 1.],\n       [1., 1., 1., 1.],\n       [1., 1., 1., 1.]]) == <[TypeError("only size-1 arrays can be converted to Python scalars") raised in repr()] ApproxNumpy object at 0x7f63bda42a58>
E     +  where <[TypeError("only size-1 arrays can be converted to Python scalars") raised in repr()] ApproxNumpy object at 0x7f63bda42a58> = approx(array([[2., 2., 2., 2.],\n       [2., 2., 2., 2.],\n       [2., 2., 2., 2.],\n       [2., 2., 2., 2.],\n       [2., 2., 2., 2.]]))
```

This is confusing since at first glance it seems that the problem is that I tried to convert some array in an inappropriate manner, while actually the assertion failed.

My PyTest version is 3.3.2.

Hints from issue discussion:
After some digging I have traced the flow through these files:

## Approx function is fired off ##
 Providing a string argument to this function defaults the cls selection to approx scalar. 
```
if _is_numpy_array(expected):
        cls = ApproxNumpy
    elif isinstance(expected, Mapping):
        cls = ApproxMapping
    elif isinstance(expected, Sequence) and not isinstance(expected, String):
        cls = ApproxSequence
  * else: *
      * cls = ApproxScalar *
    return cls(expected, rel, abs, nan_ok) 
```

## Then we go through the ApproxBase class for instantiation. ## 
```
class ApproxBase(object):
    def __init__(self, expected, rel=None, abs=None, nan_ok=False):
        self.expected = expected
        self.abs = abs
        self.rel = rel
        self.nan_ok = nan_ok
```

## Then we go to the ApproxSequence class for evaluation ## 
This class' __repr__ method is fired off. Which is where we get to your issue. 
```
if math.isinf(self.expected):
    return str(self.expected)
```
This function is checking for numbers, and thats when it raises the typeerror for it being a string.

Im unsure where to go from here because i cant even seem to understand pythons math library. I think its a wrapper function for the C math library, but Im still confused on how it works. 


## Conclusion ## 
The developers could check for a string to be passed for the `expected` argument and throw a type error, but since math.isinf seems to do that already I might not be really worth catching it early. 
Perhaps the additional relevancy would be worth it, atleast an error message coming from `approx` would be better than from a method inside. 
Id like for a developer to come to the same conclusion and help guide me through making the pull request for this repo, I would love to contribute the change if possible :)
@nicoddemus ?
the initial call to approx should fail, if its just a string, we should raise a type-error instead of accidentally using a scalar base

an alternative would be to actually define approx for strings, but that needs quite some design discussion at best

so i would favour finally failing early from the mistake
@RonnyPfannschmidt 
Created a potential fix for the issue. 

I do think it would be beneficial to open up discussion on how to approximate strings. 
Should I open an issue for discussion? 
> Should I open an issue for discussion?

Please do, it is better to keep this separate. I think your fix can go in soon and later if we get to a good shape we can add support for strings as well.
[GitMate.io](https://gitmate.io) thinks possibly related issues are https://github.com/pytest-dev/pytest/issues/1164 (test_something fails), https://github.com/pytest-dev/pytest/issues/213 (Issue with numpy arrays and pytest.mark.parametrize), https://github.com/pytest-dev/pytest/issues/2172 (Capturing output causes test to fail.), https://github.com/pytest-dev/pytest/issues/2314 (capsys fails to capture the output by stdout), and https://github.com/pytest-dev/pytest/issues/10 (assert has strange behavior with numpy arrays).
Thanks for catching this.  I'm pretty sure I know what the problem is.  I'll try to make a fix this weekend.
Thanks @kalekundert, that would be great!
Actually, this was fixed by #3615.  Update to the most recent version of pytest and you should be fine.

Commit message: Fix 'at' string for non-numeric messages in approx()

Patch to review:
diff --git a/changelog/3473.bugfix.rst b/changelog/3473.bugfix.rst
new file mode 100644
index 00000000000..ef434b29f16
--- /dev/null
+++ b/changelog/3473.bugfix.rst
@@ -0,0 +1 @@
+Raise immediately if ``approx()`` is given an expected value of a type it doesn't understand (e.g. strings, nested dicts, etc.).
diff --git a/changelog/3712.bugfix.rst b/changelog/3712.bugfix.rst
new file mode 100644
index 00000000000..649ef639487
--- /dev/null
+++ b/changelog/3712.bugfix.rst
@@ -0,0 +1 @@
+Correctly represent the dimensions of an numpy array when calling ``repr()`` on ``approx()``.
diff --git a/src/_pytest/python_api.py b/src/_pytest/python_api.py
index 5331d8a84a3..bbbc144eeed 100644
--- a/src/_pytest/python_api.py
+++ b/src/_pytest/python_api.py
@@ -1,5 +1,8 @@
 import math
+import pprint
 import sys
+from numbers import Number
+from decimal import Decimal
 
 import py
 from six.moves import zip, filterfalse
@@ -30,6 +33,15 @@ def _cmp_raises_type_error(self, other):
     )
 
 
+def _non_numeric_type_error(value, at):
+    at_str = " at {}".format(at) if at else ""
+    return TypeError(
+        "cannot make approximate comparisons to non-numeric values: {!r} {}".format(
+            value, at_str
+        )
+    )
+
+
 # builtin pytest.approx helper
 
 
@@ -39,15 +51,17 @@ class ApproxBase(object):
     or sequences of numbers.
     """
 
-    # Tell numpy to use our `__eq__` operator instead of its
+    # Tell numpy to use our `__eq__` operator instead of its.
     __array_ufunc__ = None
     __array_priority__ = 100
 
     def __init__(self, expected, rel=None, abs=None, nan_ok=False):
+        __tracebackhide__ = True
         self.expected = expected
         self.abs = abs
         self.rel = rel
         self.nan_ok = nan_ok
+        self._check_type()
 
     def __repr__(self):
         raise NotImplementedError
@@ -75,20 +89,31 @@ def _yield_comparisons(self, actual):
         """
         raise NotImplementedError
 
+    def _check_type(self):
+        """
+        Raise a TypeError if the expected value is not a valid type.
+        """
+        # This is only a concern if the expected value is a sequence.  In every
+        # other case, the approx() function ensures that the expected value has
+        # a numeric type.  For this reason, the default is to do nothing.  The
+        # classes that deal with sequences should reimplement this method to
+        # raise if there are any non-numeric elements in the sequence.
+        pass
+
 
 class ApproxNumpy(ApproxBase):
     """
-    Perform approximate comparisons for numpy arrays.
+    Perform approximate comparisons where the expected value is numpy array.
     """
 
     def __repr__(self):
-        # It might be nice to rewrite this function to account for the
-        # shape of the array...
-        import numpy as np
+        def recursive_map(f, x):
+            if isinstance(x, list):
+                return list(recursive_map(f, xi) for xi in x)
+            else:
+                return f(x)
 
-        list_scalars = []
-        for x in np.ndindex(self.expected.shape):
-            list_scalars.append(self._approx_scalar(np.asscalar(self.expected[x])))
+        list_scalars = recursive_map(self._approx_scalar, self.expected.tolist())
 
         return "approx({!r})".format(list_scalars)
 
@@ -128,8 +153,8 @@ def _yield_comparisons(self, actual):
 
 class ApproxMapping(ApproxBase):
     """
-    Perform approximate comparisons for mappings where the values are numbers
-    (the keys can be anything).
+    Perform approximate comparisons where the expected value is a mapping with
+    numeric values (the keys can be anything).
     """
 
     def __repr__(self):
@@ -147,10 +172,20 @@ def _yield_comparisons(self, actual):
         for k in self.expected.keys():
             yield actual[k], self.expected[k]
 
+    def _check_type(self):
+        __tracebackhide__ = True
+        for key, value in self.expected.items():
+            if isinstance(value, type(self.expected)):
+                msg = "pytest.approx() does not support nested dictionaries: key={!r} value={!r}\n  full mapping={}"
+                raise TypeError(msg.format(key, value, pprint.pformat(self.expected)))
+            elif not isinstance(value, Number):
+                raise _non_numeric_type_error(self.expected, at="key={!r}".format(key))
+
 
 class ApproxSequence(ApproxBase):
     """
-    Perform approximate comparisons for sequences of numbers.
+    Perform approximate comparisons where the expected value is a sequence of
+    numbers.
     """
 
     def __repr__(self):
@@ -169,10 +204,21 @@ def __eq__(self, actual):
     def _yield_comparisons(self, actual):
         return zip(actual, self.expected)
 
+    def _check_type(self):
+        __tracebackhide__ = True
+        for index, x in enumerate(self.expected):
+            if isinstance(x, type(self.expected)):
+                msg = "pytest.approx() does not support nested data structures: {!r} at index {}\n  full sequence: {}"
+                raise TypeError(msg.format(x, index, pprint.pformat(self.expected)))
+            elif not isinstance(x, Number):
+                raise _non_numeric_type_error(
+                    self.expected, at="index {}".format(index)
+                )
+
 
 class ApproxScalar(ApproxBase):
     """
-    Perform approximate comparisons for single numbers only.
+    Perform approximate comparisons where the expected value is a single number.
     """
 
     DEFAULT_ABSOLUTE_TOLERANCE = 1e-12
@@ -286,7 +332,9 @@ def set_default(x, default):
 
 
 class ApproxDecimal(ApproxScalar):
-    from decimal import Decimal
+    """
+    Perform approximate comparisons where the expected value is a decimal.
+    """
 
     DEFAULT_ABSOLUTE_TOLERANCE = Decimal("1e-12")
     DEFAULT_RELATIVE_TOLERANCE = Decimal("1e-6")
@@ -445,32 +493,35 @@ def approx(expected, rel=None, abs=None, nan_ok=False):
        __ https://docs.python.org/3/reference/datamodel.html#object.__ge__
     """
 
-    from decimal import Decimal
-
     # Delegate the comparison to a class that knows how to deal with the type
     # of the expected value (e.g. int, float, list, dict, numpy.array, etc).
     #
-    # This architecture is really driven by the need to support numpy arrays.
-    # The only way to override `==` for arrays without requiring that approx be
-    # the left operand is to inherit the approx object from `numpy.ndarray`.
-    # But that can't be a general solution, because it requires (1) numpy to be
-    # installed and (2) the expected value to be a numpy array.  So the general
-    # solution is to delegate each type of expected value to a different class.
+    # The primary responsibility of these classes is to implement ``__eq__()``
+    # and ``__repr__()``.  The former is used to actually check if some
+    # "actual" value is equivalent to the given expected value within the
+    # allowed tolerance.  The latter is used to show the user the expected
+    # value and tolerance, in the case that a test failed.
     #
-    # This has the advantage that it made it easy to support mapping types
-    # (i.e. dict).  The old code accepted mapping types, but would only compare
-    # their keys, which is probably not what most people would expect.
+    # The actual logic for making approximate comparisons can be found in
+    # ApproxScalar, which is used to compare individual numbers.  All of the
+    # other Approx classes eventually delegate to this class.  The ApproxBase
+    # class provides some convenient methods and overloads, but isn't really
+    # essential.
 
-    if _is_numpy_array(expected):
-        cls = ApproxNumpy
+    __tracebackhide__ = True
+
+    if isinstance(expected, Decimal):
+        cls = ApproxDecimal
+    elif isinstance(expected, Number):
+        cls = ApproxScalar
     elif isinstance(expected, Mapping):
         cls = ApproxMapping
     elif isinstance(expected, Sequence) and not isinstance(expected, STRING_TYPES):
         cls = ApproxSequence
-    elif isinstance(expected, Decimal):
-        cls = ApproxDecimal
+    elif _is_numpy_array(expected):
+        cls = ApproxNumpy
     else:
-        cls = ApproxScalar
+        raise _non_numeric_type_error(expected, at=None)
 
     return cls(expected, rel, abs, nan_ok)
 
diff --git a/testing/python/approx.py b/testing/python/approx.py
index 0509fa67238..c31b8bc5a67 100644
--- a/testing/python/approx.py
+++ b/testing/python/approx.py
@@ -59,17 +59,21 @@ def test_repr_string(self, plus_minus):
             ),
         )
 
-    def test_repr_0d_array(self, plus_minus):
+    @pytest.mark.parametrize(
+        "value, repr_string",
+        [
+            (5., "approx(5.0 {pm} 5.0e-06)"),
+            ([5.], "approx([5.0 {pm} 5.0e-06])"),
+            ([[5.]], "approx([[5.0 {pm} 5.0e-06]])"),
+            ([[5., 6.]], "approx([[5.0 {pm} 5.0e-06, 6.0 {pm} 6.0e-06]])"),
+            ([[5.], [6.]], "approx([[5.0 {pm} 5.0e-06], [6.0 {pm} 6.0e-06]])"),
+        ],
+    )
+    def test_repr_nd_array(self, plus_minus, value, repr_string):
+        """Make sure that arrays of all different dimensions are repr'd correctly."""
         np = pytest.importorskip("numpy")
-        np_array = np.array(5.)
-        assert approx(np_array) == 5.0
-        string_expected = "approx([5.0 {} 5.0e-06])".format(plus_minus)
-
-        assert repr(approx(np_array)) == string_expected
-
-        np_array = np.array([5.])
-        assert approx(np_array) == 5.0
-        assert repr(approx(np_array)) == string_expected
+        np_array = np.array(value)
+        assert repr(approx(np_array)) == repr_string.format(pm=plus_minus)
 
     def test_operator_overloading(self):
         assert 1 == approx(1, rel=1e-6, abs=1e-12)
@@ -439,6 +443,13 @@ def test_foo():
             ["*At index 0 diff: 3 != 4 * {}".format(expected), "=* 1 failed in *="]
         )
 
+    @pytest.mark.parametrize(
+        "x", [None, "string", ["string"], [[1]], {"key": "string"}, {"key": {"key": 1}}]
+    )
+    def test_expected_value_type_error(self, x):
+        with pytest.raises(TypeError):
+            approx(x)
+
     @pytest.mark.parametrize(
         "op",
         [


Code review comments:
diff_hunk: @@ -75,20 +89,31 @@ def _yield_comparisons(self, actual):
         """
         raise NotImplementedError
 
+    def _check_type(self):
+        """
+        Raise a TypeError if the expected value is not a valid type.
+        """
+        # This is only a concern if the expected value is a sequence.  In every
+        # other case, the approx() function ensures that the expected value has
+        # a numeric type.  For this reason, the default is to do nothing.  The
+        # classes that deal with sequences should reimplement this method to
+        # raise if there are any non-numeric elements in the sequence.
+        pass
+
 
 class ApproxNumpy(ApproxBase):
     """
-    Perform approximate comparisons for numpy arrays.
+    Perform approximate comparisons where the expected value is numpy array.
     """
 
     def __repr__(self):
-        # It might be nice to rewrite this function to account for the
-        # shape of the array...
-        import numpy as np
+        def recursive_map(f, x):
path: src/_pytest/python_api.py
text: @user1:
this helper is independent, lets move it out

@user2:
Done