Pytest: Assertions on nested comprehensions can give NameError

[RacingTadpole]

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]

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]

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]

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]

/cc @Tadaboody @danielx123

[blueyed]

@bootandy

# Failure

Can you elaborate? (traceback, at least exception)

[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

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]

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]

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]

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]

@asottile
What do you think?
It seems you've started looking into it? (https://github.com/pytest-dev/pytest/issues/5372)

[asottile]

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): https://github.com/pytest-dev/pytest/pull/5373

[nicoddemus]

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

[asottile]

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]

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]

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]

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