Skip to content

Issue #3473 #3474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed

Issue #3473 #3474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0700b2b
Added a check to see if the "expected" argument for the approx functi…
Drew-Ack May 14, 2018
a6feb0b
Edit to Authors file.
Drew-Ack May 14, 2018
1ffb6bb
Created a report for the bugfix that addresses issue #3473
Drew-Ack May 14, 2018
9b485d6
Small change in wording.
Drew-Ack May 14, 2018
d29c9eb
Created test case for trying to approximate a string. The test checks…
Drew-Ack May 15, 2018
25045f3
Small rewording of the CHANGELOG
nicoddemus May 15, 2018
2c0c161
Added a check to make sure all elements of a sequence in pytest.appro…
Drew-Ack May 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ Denis Kirisov
Diego Russo
Dmitry Dygalo
Dmitry Pribysh
Drew Ackerman
Duncan Betts
Edison Gustavo Muenz
Edoardo Batini
Expand Down
8 changes: 6 additions & 2 deletions _pytest/python_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,12 +443,16 @@ 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):
Copy link
Member

Choose a reason for hiding this comment

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

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))

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@Drew-Ack Drew-Ack May 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Drew-Ack Drew-Ack May 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@nicoddemus nicoddemus May 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

raise TypeError("Strings can not be approximated")
elif _is_numpy_array(expected):
cls = ApproxNumpy
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)):
Copy link
Member

Choose a reason for hiding this comment

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

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))

Copy link
Member

Choose a reason for hiding this comment

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

We also should add a test for this 😉

Copy link
Author

@Drew-Ack Drew-Ack May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Author

Choose a reason for hiding this comment

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

@nicoddemus How should I approach numpy types?

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. 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()

  2. 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?

raise TypeError("Sequence contained a value that was not float or int")
cls = ApproxSequence
elif isinstance(expected, Decimal):
cls = ApproxDecimal
Expand Down
2 changes: 2 additions & 0 deletions changelog/3473.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
``pytest.approx`` does not handle strings, but this was not easily apparent so it now raises a ``TypeError`` when a string is passed in as the expected argument.
``pytest.approx`` did not check all elements in a sequence to make sure they were valid type, a ``TypeError`` is now raised when an invalid element is in a sequence.
4 changes: 4 additions & 0 deletions testing/python/approx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,10 @@ def test_int(self):
assert approx(x, rel=5e-6, abs=0) == a
assert approx(x, rel=5e-7, abs=0) != a

def test_string(self):
with pytest.raises(TypeError):
approx('abc')

def test_decimal(self):
within_1e6 = [
(Decimal('1.000001'), Decimal('1.0')),
Expand Down