'approx' should error immediately if an invalid parameter is passed

[michaelboulton]

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:

# 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.

[Drew-Ack]

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 ?

[RonnyPfannschmidt]

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

[Drew-Ack]

@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?

[nicoddemus]

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.