Issue #3473 #3474

Drew-Ack · 2018-05-14T19:35:30Z

A small change to how pytest.approx works.
This is to address issue #3473

…on is a string. If "expected" is a string then raise a typeError and a friendly message.

coveralls · 2018-05-14T20:05:16Z

Coverage increased (+0.05%) to 92.803% when pulling 2c0c161 on Drew-Ack:Issue_#3473 into 36614b0 on pytest-dev:master.

maiksensi · 2018-05-14T20:44:22Z

Would it also make sense to add a small test, checking against the expected type error when passing in a string?

Drew-Ack · 2018-05-14T21:04:20Z

There are two tests I could write.

Test checks to see that a TypeError is correctly raised when trying to use a string with approx.
Test uses the xfail mark. This test would be try to use a string with approx, which is expected to fail, for now.

RonnyPfannschmidt · 2018-05-15T05:47:46Z

xfail is for tests you want to fix, this is a test for a exception condition that is supposed to happen - so use pytest.raises

maiksensi · 2018-05-15T11:11:11Z

Do you consider the first test a unit test and the second one an integration (where you actually try to run a test that uses approx with a string? If so both tests sound useful. As Ronny said, using pytest.raises probably makes more sense for the second test.
I am not too opinionated about how many tests need to be added. In any case I think it would already help to check for correctly raising the TypeError when calling approx with a string.
If you need any help with testing the behavior let me know.

… that a TypeError is raised correctly when a string is passed into approx().

Drew-Ack · 2018-05-15T15:04:22Z

Added a single test case that uses pytest.raises as @RonnyPfannschmidt suggested.
@maiksensi I thought about a second test case, but they're so similar that one is enough for this behaviour.

nicoddemus · 2018-05-15T21:15:32Z

Thanks @Drew-Ack, we appreciate you taking the time to write a PR!

This fixes #3473.

nicoddemus · 2018-05-15T21:20:18Z

_pytest/python_api.py

@@ -443,8 +443,9 @@ def approx(expected, rel=None, abs=None, nan_ok=False):
    # 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.
-
-    if _is_numpy_array(expected):
+    if isinstance(expected, String):


Perhaps it would be better if we change the else: clause here to check for float/int, and then add an else: clause which fails with everything else?

elif isinstance(expected, Decimal): cls = ApproxDecimal elif isinstance(expected, (float,) + six.integer_types): cls = ApproxScalar else: raise TypeError('Unknown type passed to approx: {!r} ({!r})'.format(expected, type(expected))

But isn't string the only thing that cannot currently be approx'd?

Nevermind! I did some tests and noticed it doesnt accept things I thought it could.

if _is_numpy_array(expected): cls = ApproxNumpy elif isinstance(expected, Mapping): cls = ApproxMapping elif isinstance(expected, Sequence) and not isinstance(expected, String): cls = ApproxSequence elif isinstance(expected, Decimal): cls = ApproxDecimal elif isinstance(expected, (float,) + six.integer_types): cls = ApproxScalar else: raise TypeError('Unknown type passed to approx: {!r} ({!r})'.format(expected, type(expected))

Would be the final commit correct?

Im not sure how that gets us from passing in [6, (5,4)] though. Because the above clauses would see it as valid, but math.isinf freaks out about it (rightly so).

Hey @Drew-Ack, thanks for replying so quickly!

Im not sure how that gets us from passing in [6, (5,4)] though. Because the above clauses would see it as valid, but math.isinf freaks out about it (rightly so).

I'm not sure I follow, could you clarify please?

No problem! Love pytest afterall :)

As an example this elif:

elif isinstance(expected, Sequence) and not isinstance(expected, String): cls = ApproxSequence

allows this [5, (6,7)] as valid input into the approx function. This technically shouldnt be, because even though its a sequence it will just blow up when appox runs.

The issue comes when math.isinf runs. math.isinf is evaluated in the ApproxSequence class. I show this in #3473

I assume this clause is used purely to delegate which class to use, but provides no checking to make sure the sequence is valid.
Is this something we should try to do?
Should we throw an exception out when an invalid sequence like [5, (6,7)] is given

allows this [5, (6,7)] as valid input into the approx function.

Oh my bad, it was obvious you meant [5, (6, 7)] the literal. Sorry about that.

This technically shouldnt be, because even though its a sequence it will just blow up when appox runs.

I see. I guess we can iterate over every element of Sequence and Mapping to ensure their elements are float or int objects; there might be a performance problem but I don't think people are using pytest.approx with arrays so large that this would make a difference.

This is the first open source project I've contributed too, so I will defer to you developers on what is best practice.

It seems to me we are just picking what point to throw an exception at.
I put together a small test case to see how long it would take :)

def test_time(): mylist = [] for x in range(1000000): mylist.append(x) start_time = time.time() for x in mylist: if isinstance(x, float) or isinstance(x, int): continue end_time = time.time() print(end_time - start_time)

Its roughly .28 seconds to run through 1 million elements.
Its roughly 2.8 seconds to run through 10 million.
The pattern holds as you increase by x10

This is the first open source project I've contributed too, so I will defer to you developers on what is best practice.

This is great, thanks for contributing to pytest, we definitely appreciate it!

What do you think @RonnyPfannschmidt? I think we can get away by keeping things simple and just going with my suggestion in #3474 (comment), but I don't oppose if others prefer to be more through.

basically massive sequences shouldn't use python lists ^^ so the check should be reasonably fine for normal sequences

OK, @Drew-Ack could go with a more through check for sequences then? Thanks!

Drew-Ack · 2018-05-16T03:01:17Z

I am unsure why the AppVeyor build failed.

maiksensi · 2018-05-17T14:50:18Z

I am unsure why the AppVeyor build failed.

Looks like a version conflict with twisted. What is the pytest-trail 2.7 run actually doing?

…x are valid.

nicoddemus · 2018-05-18T15:26:44Z

_pytest/python_api.py

@@ -450,6 +450,9 @@ def approx(expected, rel=None, abs=None, nan_ok=False):
    elif isinstance(expected, Mapping):
        cls = ApproxMapping
    elif isinstance(expected, Sequence) and not isinstance(expected, String):
+        for value in expected:
+            if isinstance(value, (Sequence, String, Mapping)):


I believe it is better to do check for numbers, and provide a better message to locate the problem:

number_types = six.integer_types + (float,) for index, value in enumerate(expected): if not isinstance(value, number_types): msg = 'Sequence must contain only numbers, but found {!r} ({!r}) at index {}' raise TypeError(msg.format(value, type(value), index))

We also should add a test for this 😉

I cant just check for int, float, and long though. I would also have to check for each numpy type too. I do agree that a better message should be there :)

@nicoddemus How should I approach numpy types?

There's already a elif _is_numpy_array(expected): check near the start of the if/elif chain, so numpy.array are covered already.

There's the situation where the user passes a builtin list containing numpy numbers, but that seems rare to me. So we can either:

Also check every possible number type, including numpy's:

# _get_numpy_types() returns a tuple (numpy.int32, numpy.int64, ...) if numpy is available, # otherwise an empty tuple number_types = six.integer_types + (float,) + self._get_numpy_types()

Consider this overkill and just fail when we encounter a list containing numpy types. In this case
should make sure to show the data type in the error message (like in my previous example).

I think it is worthwhile trying to go with 1), this will make things more convenient for our users at the cost of a little more
code.

What do you folks think?

nicoddemus · 2018-05-18T15:26:57Z

_pytest/python_api.py

@@ -450,6 +450,9 @@ def approx(expected, rel=None, abs=None, nan_ok=False):
    elif isinstance(expected, Mapping):
        cls = ApproxMapping
    elif isinstance(expected, Sequence) and not isinstance(expected, String):
+        for value in expected:
+            if isinstance(value, (Sequence, String, Mapping)):


We also should add a test for this 😉

nicoddemus · 2018-08-25T14:23:24Z

@Drew-Ack gentle ping. 😁

blueyed · 2018-08-25T20:41:42Z

#3473 was closed via #3741 already.
I guess this needs a rebase at least - I have not checked if there is something left here then.

nicoddemus · 2018-08-25T21:02:27Z

Oh right, thanks @blueyed.

It seems indeed that #3741 already implemented the type checking we wanted here, so I'm closing this.

@Drew-Ack feel free to ping/reopen if we are missing something. 👍

Drew-Ack added 4 commits May 14, 2018 13:16

Added a check to see if the "expected" argument for the approx functi…

0700b2b

…on is a string. If "expected" is a string then raise a typeError and a friendly message.

Edit to Authors file.

a6feb0b

Created a report for the bugfix that addresses issue pytest-dev#3473

1ffb6bb

Small change in wording.

9b485d6

Created test case for trying to approximate a string. The test checks…

d29c9eb

… that a TypeError is raised correctly when a string is passed into approx().

Small rewording of the CHANGELOG

25045f3

nicoddemus reviewed May 15, 2018

View reviewed changes

Added a check to make sure all elements of a sequence in pytest.appro…

2c0c161

…x are valid.

nicoddemus reviewed May 18, 2018

View reviewed changes

nicoddemus requested changes May 18, 2018

View reviewed changes

nicoddemus closed this Aug 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3473 #3474

Issue #3473 #3474

Drew-Ack commented May 14, 2018

coveralls commented May 14, 2018 •

edited

Loading

maiksensi commented May 14, 2018

Drew-Ack commented May 14, 2018 •

edited

Loading

RonnyPfannschmidt commented May 15, 2018

maiksensi commented May 15, 2018

Drew-Ack commented May 15, 2018 •

edited

Loading

nicoddemus commented May 15, 2018

nicoddemus May 15, 2018

Drew-Ack May 15, 2018

Drew-Ack May 15, 2018

nicoddemus May 15, 2018

Drew-Ack May 16, 2018 •

edited

Loading

nicoddemus May 16, 2018

Drew-Ack May 16, 2018 •

edited

Loading

nicoddemus May 16, 2018 •

edited

Loading

RonnyPfannschmidt May 17, 2018

nicoddemus May 18, 2018

Drew-Ack commented May 16, 2018

maiksensi commented May 17, 2018

nicoddemus May 18, 2018

nicoddemus May 18, 2018

Drew-Ack May 18, 2018 •

edited

Loading

Drew-Ack May 21, 2018

nicoddemus May 22, 2018

nicoddemus May 18, 2018

nicoddemus commented Aug 25, 2018

blueyed commented Aug 25, 2018

nicoddemus commented Aug 25, 2018

Issue #3473 #3474

Issue #3473 #3474

Conversation

Drew-Ack commented May 14, 2018

coveralls commented May 14, 2018 • edited Loading

maiksensi commented May 14, 2018

Drew-Ack commented May 14, 2018 • edited Loading

RonnyPfannschmidt commented May 15, 2018

maiksensi commented May 15, 2018

Drew-Ack commented May 15, 2018 • edited Loading

nicoddemus commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drew-Ack May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drew-Ack May 16, 2018 • edited Loading

Choose a reason for hiding this comment

nicoddemus May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drew-Ack commented May 16, 2018

maiksensi commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drew-Ack May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Aug 25, 2018

blueyed commented Aug 25, 2018

nicoddemus commented Aug 25, 2018

coveralls commented May 14, 2018 •

edited

Loading

Drew-Ack commented May 14, 2018 •

edited

Loading

Drew-Ack commented May 15, 2018 •

edited

Loading

Drew-Ack May 16, 2018 •

edited

Loading

Drew-Ack May 16, 2018 •

edited

Loading

nicoddemus May 16, 2018 •

edited

Loading

Drew-Ack May 18, 2018 •

edited

Loading