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

Unroll calls to any #5062 #5103

Merged
merged 14 commits into from
May 27, 2019
Merged

Unroll calls to any #5062 #5103

merged 14 commits into from
May 27, 2019

Conversation

Tadaboody
Copy link
Contributor

@Tadaboody Tadaboody commented Apr 13, 2019

Closes #5062

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Target the master branch for bug fixes, documentation updates and trivial changes.
  • Target the features branch for new features and removals/deprecations.
  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.

@Tadaboody
Copy link
Contributor Author

Current roadblock - I want to recursively rewrite the assert inside the for loop, but visit_Assert resets the state so I can't call self.visit(assert_node). On the other hand, I want to stay consistent with things like variable names so I don't want to create a new AssertionVisitor

@Tadaboody Tadaboody added the status: help wanted developers would like help from experts on this topic label Apr 13, 2019
@Tadaboody Tadaboody changed the title WIP: Unroll all any 5062 WIP: Unroll calls to any #5062 Apr 13, 2019
@codecov
Copy link

codecov bot commented Apr 13, 2019

Codecov Report

Merging #5103 into features will increase coverage by 1.78%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #5103      +/-   ##
============================================
+ Coverage     91.48%   93.27%   +1.78%     
============================================
  Files           115      115              
  Lines         26179    26194      +15     
  Branches       2582     2582              
============================================
+ Hits          23951    24433     +482     
+ Misses         1904     1445     -459     
+ Partials        324      316       -8
Impacted Files Coverage Δ
src/_pytest/assertion/rewrite.py 95.25% <85.71%> (-0.23%) ⬇️
testing/test_assertrewrite.py 84.28% <93.75%> (+0.22%) ⬆️
testing/python/collect.py 82.94% <0%> (-16.21%) ⬇️
testing/python/metafunc.py 89.15% <0%> (-4.42%) ⬇️
testing/test_tmpdir.py 97.31% <0%> (-1.62%) ⬇️
src/_pytest/compat.py 95.97% <0%> (-1.01%) ⬇️
src/_pytest/helpconfig.py 97.32% <0%> (-0.26%) ⬇️
testing/test_skipping.py 97.47% <0%> (-0.03%) ⬇️
testing/test_assertion.py 97.67% <0%> (-0.01%) ⬇️
testing/test_pdb.py 98.82% <0%> (ø) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10ca84f...22d91a3. Read the comment docs.

@Tadaboody
Copy link
Contributor Author

Tadaboody commented Apr 13, 2019

Ok it's at a point where it shows a better report but not quite detailed enough, would like a proposition on where to take it from here

test_example.py F                                                        [100%]

=================================== FAILURES ===================================
_________________________________ test_rolled __________________________________

    def test_rolled():
>       assert all(x == 1 for x in range(10))
E       assert 0 == 1

test_example.py:2: AssertionError

The rewritten ast is exactly as expected, just as if it was a for loop:

=========================== Rewritten AST as Python ============================
import builtins as @py_builtins
import _pytest.assertion.rewrite as @pytest_ar

def test_rolled():
    for x in range(10):
        import builtins as @py_builtins
        import _pytest.assertion.rewrite as @pytest_ar
        @py_assert2 = 1
        @py_assert1 = x == @py_assert2
        if not @py_assert1:
            @py_format4 = @pytest_ar._call_reprcompare(('==',), (@py_assert1,), ('%(py0)s == %(py3)s',), (x, @py_assert2)) % {'py0': @pytest_ar._saferepr(x) if 'x' in @py_builtins.locals() or @pytest_ar._should_repr_global_name(x) else 'x', 'py3': @pytest_ar._saferepr(@py_assert2)}
            @py_format6 = ('' + 'assert %(py5)s') % {'py5': @py_format4}
            raise AssertionError(@pytest_ar._format_explanation(@py_format6))
        @py_assert1 = @py_assert2 = None
    if 1 is None:
        from _pytest.warning_types import PytestWarning
        from warnings import warn_explicit
        warn_explicit(PytestWarning('asserting the value None, please use "assert is None"'), category=None, filename='/Users/me/Forks/pytest/test_example.py', lineno=2)
    if not 1:
        @py_format0 = ('' + 'assert ') % {}
        raise AssertionError(@pytest_ar._format_explanation(@py_format0))
=========================== short test summary info ============================
FAILED test_example.py::test_rolled
=========================== 1 failed in 0.10 seconds ===========================

@Tadaboody
Copy link
Contributor Author

I'm not going to be able to continue this until next week - @danielx123 if you want to fork this until then you're more than welcome!

@@ -987,6 +989,27 @@ def visit_Call_35(self, call):
outer_expl = "%s\n{%s = %s\n}" % (res_expl, res_expl, expl)
return res, outer_expl

def visit_all(self, call):
"""Special rewrite for the builtin all function, see #5602"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it should be #5062 instead of #5602

@danielx123
Copy link
Contributor

@Tadaboody I can work on this during the weekend. How do you want to work on this together?

@Tadaboody Tadaboody changed the title WIP: Unroll calls to any #5062 Unroll calls to any #5062 May 9, 2019
@Tadaboody Tadaboody removed the status: help wanted developers would like help from experts on this topic label May 9, 2019
@Tadaboody
Copy link
Contributor Author

Tadaboody commented May 9, 2019

Alright I think this is pretty stable and does what it's supposed to. @nicoddemus I'll be waiting to start a review process

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Hi @Tadaboody,

Great job!

Here's what we get on master with your samples:

============================= FAILURES ==============================
__________________________ test_generator ___________________________

    def test_generator():
        odd_list = list(range(1,9,2))
>       assert all(check_even(num) for num in odd_list)
E       assert False
E        +  where False = all(<generator object test_generator.<locals>.<genexpr> at 0x000001D7A54FA678>)

.tmp\test-foo.py:8: AssertionError
______________________ test_list_comprehension ______________________

    def test_list_comprehension():
        odd_list = list(range(1,9,2))
>       assert all([check_even(num) for num in odd_list])
E       assert False
E        +  where False = all([False, False, False, False])

.tmp\test-foo.py:13: AssertionError
___________________________ test_for_loop ___________________________

    def test_for_loop():
        odd_list = list(range(1,9,2))
        for num in odd_list:
>           assert check_even(num)
E           assert False
E            +  where False = check_even(1)

.tmp\test-foo.py:19: AssertionError
===================== 3 failed in 0.11 seconds ======================

And with your branch:

============================= FAILURES ==============================
__________________________ test_generator ___________________________

    def test_generator():
        odd_list = list(range(1,9,2))
>       assert all(check_even(num) for num in odd_list)
E       assert False
E        +  where False = check_even(1)

.tmp\test-foo.py:8: AssertionError
______________________ test_list_comprehension ______________________

    def test_list_comprehension():
        odd_list = list(range(1,9,2))
>       assert all([check_even(num) for num in odd_list])
E       assert False
E        +  where False = check_even(1)

.tmp\test-foo.py:13: AssertionError
___________________________ test_for_loop ___________________________

    def test_for_loop():
        odd_list = list(range(1,9,2))
        for num in odd_list:
>           assert check_even(num)
E           assert False
E            +  where False = check_even(1)

.tmp\test-foo.py:19: AssertionError
===================== 3 failed in 0.10 seconds ======================

@nicoddemus
Copy link
Member

Do you mind rebasing on features and pushing again? codecov is providing some funny results.

@Tadaboody
Copy link
Contributor Author

rebased

@nicoddemus
Copy link
Member

Merging as the failure is unrelated: #5317

@nicoddemus nicoddemus merged commit 2b9ca34 into pytest-dev:features May 27, 2019
@nicoddemus
Copy link
Member

Thanks @Tadaboody!

blueyed added a commit to blueyed/pytest that referenced this pull request May 29, 2019
This reverts commit 2b9ca34, reversing
changes made to 0a57124.
@blueyed
Copy link
Contributor

blueyed commented May 29, 2019

@nicoddemus

Merging as the failure is unrelated: #5317

It turns out that it is not really it seems.. ;)

@nicoddemus
Copy link
Member

😮

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants