Skip to content
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.

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

Assertions on nested comprehensions can give NameError #5370

Closed
RacingTadpole opened this issue Jun 3, 2019 · 16 comments

Comments

Projects
None yet
8 participants
@RacingTadpole
Copy link

commented Jun 3, 2019

This test:

def test_nested_comprehension():
    s = [[True, True], [True, True]]
    assert all(e for row in s for e in row)

works fine in pytest 4.4.1 and 4.5.0. In pytest 4.6.1 it produces a NameError:

$ pytest
===================================================== test session starts =====================================================
platform darwin -- Python 3.7.3, pytest-4.6.1, py-1.8.0, pluggy-0.12.0
collected 1 item                                                                                                              

test_it.py F                                                                                                            [100%]

========================================================== FAILURES ===========================================================
__________________________________________________ test_nested_comprehension __________________________________________________

    def test_nested_comprehension():
        s = [[True, True], [True, True]]
>       assert all(e for row in s for e in row)
E       NameError: name 'e' is not defined

test_it.py:3: NameError
================================================== 1 failed in 0.06 seconds ===================================================

Pip list gives:

$ pip list
Package            Version
------------------ -------
atomicwrites       1.3.0  
attrs              19.1.0 
importlib-metadata 0.17   
more-itertools     7.0.0  
packaging          19.0   
pip                19.1.1 
pluggy             0.12.0 
py                 1.8.0  
pyparsing          2.4.0  
pytest             4.6.1  
setuptools         41.0.1 
six                1.12.0 
wcwidth            0.1.7  
wheel              0.33.4 
zipp               0.5.1  
@EnTeQuAk

This comment has been minimized.

Copy link

commented Jun 3, 2019

On addons-server we're running into a very similar issue, https://travis-ci.org/mozilla/addons-server/jobs/540627494#L3621 which does something like

        assert all(
            isinstance(prop['index'], bool)
            for prop in mapping_properties.values()
            if 'index' in prop)

where mapping_properties is a dictionary that sometimes contains the index key.

This produces a KeyError 'index'. It works perfectly fine in pytest 4.5.0

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

CC @nicoddemus
i propose undoing the unrolling as it seems its simply not enough to start with the minimal case and a recurring run of regressions would be a pain

@bootandy

This comment has been minimized.

Copy link

commented Jun 3, 2019

Also seeing something similar, this works in pytest 4.5.0 but fails in 4.6.0 and 4.6.1.
Using Python 3.6.7

def test_basic3():
    l = {1:[1],2:[2],3:[3]}

    for k, data in l.items():
        for x in data:
            assert x == k

    assert all(x == k for k,data in l.items() for x in data)  # Failure

Update:

failure is:

>       assert all(x == k for k,data in l.items() for x in data)
E       assert 3 == 1

common/tests/test_basic.py:31: AssertionError

Despite the list resolving to [True, True, True] if printed

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@bootandy

# Failure

Can you elaborate? (traceback, at least exception)

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

i propose undoing the unrolling as it seems its simply not enough to start with the minimal case and a recurring run of regressions would be a pain

Yeah I'm afraid we might resort to that, unfortunately. Let's see if @Tadaboody wants to comment on this first, otherwise I will revert this later today and make a 4.6.2 release, as this unfortunately breaks the entire test suite during collection and there's no workaround other than disabling assertion rewriting entirely. 😞

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

as this unfortunately breaks the entire test suite during collection

Are the issues only with test execution (not collection)? (although causing failures where it should not)

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Are the issues only with test execution (not collection)? (although causing failures where it should not)

Oh you are right, I'm confusing this with #5358.

Regardless this is pretty annoying. 😬

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Yeah.. also not really clear how many different issues we have here really by now.
I'm ok with reverting it for 4.6.2, and then continue from there - but have not investigated how much it would take to fix it instead.

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@asottile
What do you think?
It seems you've started looking into it? (#5372)

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

My vote is revert and sort this out later, this is much more complicated than meets the eye (I started trying to fix it and found more edge cases): #5373

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Agreed, that's the safest approach for now. 👍

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

this has been released as part of 4.6.2 -- those that were experiencing these issues may need to clean .pyc files after upgrading

@alimcmaster1

This comment has been minimized.

Copy link

commented Jun 4, 2019

Thanks for reverting. Do you think we should add some test cases to the codebase like @RacingTadpole example to prevent such regressions in the future ? I am happy to implement this myself - if someone points me to a suitable home for them.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Hi @alimcmaster1,

Thanks for the offer, but I don't think we should add specific tests for all in the assertion rewriter tests given that we don't have any special handling for it anymore.

But we will definitely revisit all examples reported and include them in the test suite if/when we re-introduce all() unrolling in the assertion rewriter. 👍

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

I've compiled the list of regressions in the original issue (#5062) to make sure we don't miss any in the future. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.