Features assertion pass hook #3479

Sup3rGeo · 2018-05-16T07:44:54Z

Implements #3457.

Already has documentation and one simple test. Pending questions:

Three pytest tests are not passing:
FAIL testing/acceptance_test.py::test_fixture_values_leak
FAIL testing/python/raises.py::TestRaises::()::test_raises_cyclic_reference[function]
FAIL testing/python/raises.py::TestRaises::()::test_raises_cyclic_reference[with]
Need help to understand why they are failing.
Always generating explanations (which could be expensive): As @nicoddemus mentionned this should be avoided BUT if assertion fails, we will generate them to report the failure. If the assert passes, users of the hook need it anyways, so it needs to be generated.
- The only alternative I see is to not generate it if hook is not implemented by any plugins. Can we detect it? If possible, do you think it is worth it?
Is this test enough?

Thanks!

nicoddemus · 2018-05-25T00:20:35Z

Hey @Sup3rGeo, thanks and sorry for the delay!

The only alternative I see is to not generate it if hook is not implemented by any plugins. Can we detect it? If possible, do you think it is worth it?

As an alternative, we should only activate this function if a certain config option is active, say assertion_pass=True. This way users and/or plugins which make use of this could enable it, while users who don't need it won't pay the price. This probably will solve the memory leaks, which I suspect are not easy to fix because they might be happening because of the extra objects being kept alive in each test (just guessing, just took a quick glance at the code). What do folks think of this idea?

I'm short on time right now, but I intend to take a closer look at the code soon.

nicoddemus · 2018-05-25T00:30:43Z

Quickly used pytest-ast-back-to-python to see the rewritten code for this test:

def test_hook():
    x = 1
    y = {2: 3}
    assert x == 1

Sup3rGeo:features-assertion-pass-hook

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

def test_hook():
    x = 1
    y = {2: 3}
    @py_assert2 = 1
    @py_assert1 = x == @py_assert2
    @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 = '%(py5)s' % {'py5': @py_format4}
    if not @py_assert1:
        raise AssertionError('' + ('assert ' + @pytest_ar._format_explanation(@py_format6)))
    else:
        @pytest_ar._call_assertion_pass(8, 'x == 1', @pytest_ar._format_explanation(@py_format6))
    @py_assert1 = @py_assert2 = None
    @py_format4 = @py_format6 = None

master

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

def test_hook():
    x = 1
    y = {2: 3}
    @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

Sup3rGeo · 2018-05-25T07:20:06Z

@nicoddemus no problem, I am glad you are here to help!

As you can see, the changes in the AST are relatively trivial. I guess the presence of the pytest_ar calls even when test continues normally could be the culprit?

As an alternative, we should only activate this function if a certain config option is active, say assertion_pass=True

This might work as well, but there could be any confusion if a plugin implements the hook and the config option is not active?
In other terms, do you see any drawbacks in making it automatically active if a plugin implements the hook? This would solve this possible confusion of manually setting the config.

nicoddemus · 2018-08-25T14:30:57Z

@Sup3rGeo sorry for the delay on this.

Do you still have interest on this feature?

One of my concerns was the cost of always creating the explanation, but one way of solving that would be to pass a function to the hook that when called produces the explanation; it is weird in the sense that no hooks do that at the moment, but at least that would not incur a cost for normal usage.

Sup3rGeo · 2018-08-27T21:34:49Z

Hi @nicoddemus, no problem at all

Yes I am still interested and I think your idea is good. Could also be an object that would hold a state in case we have many plugins implementing the hook, so it creates the explanation in the first call and stores it to be returned in any following call. And that does not format the explanation at all if no one uses it. I will try to come up with something when I have time.

However the thing I really cannot seem to do is to understand this leak and cyclic reference failures. Any idea how to approach this?

nicoddemus · 2018-08-27T22:09:08Z

However the thing I really cannot seem to do is to understand this leak and cyclic reference failures. Any idea how to approach this?

Let's see if using the lazy approach gets rid of the leaks, as I suspect they might.

Sup3rGeo · 2018-09-12T20:24:53Z

I have investigated a bit more and I think I can pinpoint the exact statement causing the leak: It seems that problem is having @py_builtins.locals() in a test that does not fail.

Does it make sense that calling locals on the test causes a circular reference or something?

Sup3rGeo · 2018-09-13T04:53:25Z

One more interesting case: I changed test_raises_cyclic_reference, removing the assert keywords and using checking/raising AssertionError manually:

    def test_raises_cyclic_reference(self, method):
        """
        Ensure pytest.raises does not leave a reference cycle (#1965).
        """
        import gc

        class T(object):
            def __call__(self):
                raise ValueError

        t = T()
        if method == "function":
            pytest.raises(ValueError, t)
        else:
            with pytest.raises(ValueError):
                t()

        # ensure both forms of pytest.raises don't leave exceptions in sys.exc_info()
        if not sys.exc_info() == (None, None, None):
            raise AssertionError("NANA1")
        #locals()
        del t

        # ensure the t instance is not stuck in a cyclic reference
        for o in gc.get_objects():
            if type(o) is T:
                raise AssertionError("NANA2")

This test passes.

However, if I just uncomment the locals() statement (before del t), then it fails. So this confirms that having the locals() at the test body is by itself enough to create a cyclic reference it seems.

src/_pytest/assertion/rewrite.py

Sup3rGeo · 2018-09-14T15:20:00Z

Some update:

It seems then that calling locals() again after deleting t is enough to update the dict and then remove its reference to the variable.

Therefore the tests seem to pass with the test like this:

    def test_raises_cyclic_reference(self, method):
        """
        Ensure pytest.raises does not leave a reference cycle (#1965).
        """
        import gc

        class T(object):
            def __call__(self):
                raise ValueError

        t = T()
        if method == "function":
            pytest.raises(ValueError, t)
        else:
            with pytest.raises(ValueError):
                t()

        # ensure both forms of pytest.raises don't leave exceptions in sys.exc_info()
        assert sys.exc_info() == (None, None, None):

        del t
        locals()

        # ensure the t instance is not stuck in a cyclic reference
        for o in gc.get_objects():
            assert type(o) is not T

I will now work on making the explanation creation lazy.

Sup3rGeo · 2018-09-14T19:02:58Z

One answer seems to confirm that locals() dictionary stays alive until the stack frame dies:
https://stackoverflow.com/questions/52337367/locals-and-object-reference-in-python

Sup3rGeo · 2018-09-15T23:41:40Z

Current errors on CI are mainly:

Error with xdist in the fixture_values_leak_test.py: That's because this depends on one test function to be executed after the other. Can we make xdist ignore or just run it on one thread?
astunparse module uses the async name that became a keyword in python 3.7. I will see if I can rewrite this PR without using astunparse, as it could take a while until the next release and it seems inelegant to use the module just for this matter.

Guys, about avoiding unnecessary explanation calculation, I think that the best way is to check if any plugins implement the hookspec. If none does, then we only explain for failures (as before).
I think pluggy does not have this option but it should not be very hard to add to it, I would be willing to add this feature.
We can also add option switch to permanently turn it off, independently of any plugins/hooks.

I think it is going to make everything simpler than passing a lazy object, which would also make the API more weird to use.

What do you think @nicoddemus and @RonnyPfannschmidt?

Sup3rGeo · 2018-09-16T02:08:13Z

I added this to Pluggy (pytest-dev/pluggy#177) and updated the code in this PR to use it. It seems to work just as expected.

However I was checking this saving the unparsed ast manually with/without hooks and comparing the modified source. I have no idea how to test it to make it part of the unit tests. Ideas?

Sup3rGeo · 2018-09-16T15:19:26Z

Oh well, now I just realized that having the AST code dependent also on the hook implementation by the plugins will create some problems for caching (now not only source file dependent anymore), just like discussed in #3479

nicoddemus · 2018-09-18T00:04:43Z

Oh well, now I just realized that having the AST code dependent also on the hook implementation by the plugins will create some problems for caching

Yeah... no ideas come to mind at the moment to handle that. 😕

codecov · 2019-05-31T14:57:33Z

Codecov Report

Merging #3479 into master will decrease coverage by <.01%.
The diff coverage is 96.26%.

@@            Coverage Diff             @@
##           master    #3479      +/-   ##
==========================================
- Coverage   96.08%   96.07%   -0.01%     
==========================================
  Files         117      117              
  Lines       25564    25642      +78     
  Branches     2475     2484       +9     
==========================================
+ Hits        24562    24636      +74     
- Misses        698      700       +2     
- Partials      304      306       +2

Impacted Files	Coverage Δ
testing/acceptance_test.py	`98.01% <100%> (ø)`	⬆️
src/_pytest/hookspec.py	`100% <100%> (ø)`	⬆️
src/_pytest/assertion/__init__.py	`89.7% <100%> (+0.99%)`	⬆️
src/_pytest/assertion/util.py	`95.03% <100%> (+0.01%)`	⬆️
testing/python/raises.py	`94.24% <100%> (+0.04%)`	⬆️
testing/test_assertrewrite.py	`84.52% <92.59%> (+0.32%)`	⬆️
src/_pytest/assertion/rewrite.py	`94.87% <97.14%> (+0.03%)`	⬆️

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 6a2d844...2ea2221. Read the comment docs.

nicoddemus · 2019-05-31T15:17:11Z

Thanks for bringing this to attention again!

Most of the concerns here are regarding performance. How about if this rewriting is opt-in, controlled by an ini option? This way most users will remain unaffected, and projects that need this can enable it.

If plugins need this enabled, they can initially document so, but potentially in the future we can provide a way to enable this programmatically in a way that those plugins can do it at runtime.

Finally, we should mark the entire feature as experimental so we can revert it in case we discover some serious problem with it once it is in the wild.

Sup3rGeo · 2019-05-31T17:38:32Z

What I am doing now is to check whether any plugins implement the pytest_assertion_pass hook (in the _check_if_assertionpass_impl function), and only format explanation and call the hook if so:

        if not @py_assert6:
            raise AssertionError(@pytest_ar._format_explanation('' + (
                'assert ' + @py_format11)))
        elif @pytest_ar._check_if_assertionpass_impl():
            @pytest_ar._call_assertion_pass(260,
                'len(spy_factory.instances) == 1', @pytest_ar.
                _format_explanation(@py_format11))

The _check_if_assertionpass_impl basically sees if the assertionrewrite plugin has defined or not a callback for this item, and it only does it if the hook call has any implementations by checking get_hookimpls():

pytest/src/_pytest/assertion/__init__.py

Lines 143 to 146 in 1cbafeb

    
           if item.ihook.pytest_assertion_pass.get_hookimpls(): 
        
               def call_assertion_pass_hook(lineno, expl, orig): 
        
                   item.ihook.pytest_assertion_pass(item=item, lineno=lineno, orig=orig, expl=expl) 
        
               util._assertion_pass = call_assertion_pass_hook

Would that be a reasonable solution?

nicoddemus · 2019-05-31T19:13:15Z

Would that be a reasonable solution?

Oh that is a good approach. I would still mark the hook as experimental thought.

@Sup3rGeo do you need this support in Python 2? I ask because we are about to release 4.6, which is supposed to be the last version supporting Python 2.

setup.py

src/_pytest/assertion/rewrite.py

Sup3rGeo · 2019-06-18T12:33:52Z

do you need this support in Python 2? I ask because we are about to release 4.6, which is supposed to be the last version supporting Python 2.

Probably not!

nicoddemus · 2019-06-22T00:11:21Z

As per #5373 (comment), please rebase this on master as we will be releasing 5.0 from there. 👍 🙇

nicoddemus

I think we are almost there, thanks for bearing with me so far!

As I stated, this is a very tricky part of the code so I'm trying to be as comprehensive as possible on my review. 🙇

src/_pytest/assertion/__init__.py

src/_pytest/assertion/rewrite.py

Co-Authored-By: Bruno Oliveira <nicoddemus@gmail.com>

nicoddemus · 2019-06-26T14:11:37Z

I've added this to the 5.0 milestone, I'm confident it is complete enough to go into the 5.0 release. 🎉

@Sup3rGeo let me know if you won't have time to finish this off in the next day or two, I will try to pick it up then. 👍

…mplementing the hook.

…n-pass-hook-master # Conflicts: # src/_pytest/assertion/rewrite.py

…tion_pass_impl function call.

Sup3rGeo · 2019-06-26T17:29:39Z

I guess by this point it should be mostly done.

nicoddemus · 2019-06-26T22:19:07Z

Thanks @Sup3rGeo!

I noticed the default value for the option was "False" instead of False, which means the option was actually enabled by default as "False" is truthy, so I fixed that. That in turn uncovered a bug: the escape for % in _format_assertmsg was removed (probably by accident), so the original assertion rewrite code was left vulnerable to a bug. Also fixed. 👍

I also took sometime to refine the docs and CHANGELOG a bit. 👍

As soon as CI passes we can merge this I believe.

Thanks a lot again, for your perseverance and patience under my barrage of requests! 😁

Small optimization, move the generation of the intermediate formatting variables inside the 'if _check_if_assertion_pass_impl():' block.

nicoddemus · 2019-06-26T22:31:20Z

Pushed another small change: I moved the creation of the formatting variables when the assertion passes under the if _check_if_assertion_pass_impl(): block:

import builtins as @py_builtins
import _pytest.assertion.rewrite as @pytest_ar

def test_foo():
    @py_assert0 = 1
    @py_assert3 = 2
    @py_assert2 = @py_assert0 == @py_assert3
    if @py_assert2 is None:
        from _pytest.warning_types import PytestAssertRewriteWarning
        from warnings import warn_explicit
        warn_explicit(PytestAssertRewriteWarning('asserting the value None, please use "assert is None"'), category=None, filename='c:\\pytest\\.tmp\\test-assert-foo.py', lineno=2)
    if not @py_assert2:
        @py_format5 = @pytest_ar._call_reprcompare(('==',), (@py_assert2,), ('%(py1)s == %(py4)s',), (@py_assert0, @py_assert3)) % {'py1': @pytest_ar._saferepr(@py_assert0), 'py4': @pytest_ar._saferepr(@py_assert3)}
        @py_format7 = '%(py6)s' % {'py6': @py_format5}
        raise AssertionError(@pytest_ar._format_explanation(@pytest_ar._format_assertmsg('To be escaped: %') + ('\n>assert ' + @py_format7)))
    elif @pytest_ar._check_if_assertion_pass_impl():
        @py_format5 = @pytest_ar._call_reprcompare(('==',), (@py_assert2,), ('%(py1)s == %(py4)s',), (@py_assert0, @py_assert3)) % {'py1': @pytest_ar._saferepr(@py_assert0), 'py4': @pytest_ar._saferepr(@py_assert3)}
        @py_format7 = '%(py6)s' % {'py6': @py_format5}
        @pytest_ar._call_assertion_pass(2, '1 == 2', @pytest_ar._format_explanation(@py_format7))
    @py_format5 = @py_format7 = None
    @py_assert0 = @py_assert2 = @py_assert3 = None

This is a small optimization which is important if/when we eventually drop the requirement of using the enable_assertion_pass_hook ini option.

…bled

nicoddemus · 2019-06-27T00:14:25Z

🎉

Sup3rGeo · 2019-06-27T09:44:00Z

Thanks @nicoddemus, I see there were a lot of tweaks!
Glad to have this merged after this lingering for more than a year :)

Sup3rGeo force-pushed the features-assertion-pass-hook branch from c03f60e to e37c58c Compare September 11, 2018 19:13

RonnyPfannschmidt reviewed Sep 14, 2018

View reviewed changes

src/_pytest/assertion/rewrite.py Outdated Show resolved Hide resolved

Sup3rGeo mentioned this pull request Sep 16, 2018

Feature/get plugins for hook pytest-dev/pluggy#177

Merged

nicoddemus mentioned this pull request Sep 27, 2018

Add pytest_before_assert hook. #4047

Closed

6 tasks

nicoddemus mentioned this pull request Oct 23, 2018

[#3191] Give hints when an assertion value is None instead of a boolean #4146

Merged

6 tasks

Sup3rGeo changed the title ~~Features assertion pass hook~~ WIP: Features assertion pass hook May 31, 2019

nicoddemus reviewed Jun 6, 2019

View reviewed changes

setup.py Show resolved Hide resolved

src/_pytest/assertion/rewrite.py Outdated Show resolved Hide resolved

nicoddemus mentioned this pull request Jun 6, 2019

Better introspection for comparing numpy arrays #5347

Closed

Sup3rGeo changed the title ~~WIP: Features assertion pass hook~~ Features assertion pass hook Jun 17, 2019

Sup3rGeo mentioned this pull request Jun 21, 2019

Revert unrolling of all() #5373

Merged

Added msg to docstring for cleaning pyc.

80ac910

nicoddemus requested changes Jun 25, 2019

View reviewed changes

src/_pytest/assertion/__init__.py Outdated Show resolved Hide resolved

src/_pytest/assertion/rewrite.py Outdated Show resolved Hide resolved

src/_pytest/assertion/rewrite.py Outdated Show resolved Hide resolved

src/_pytest/assertion/rewrite.py Show resolved Hide resolved

Update src/_pytest/assertion/rewrite.py

7efdd50

Co-Authored-By: Bruno Oliveira <nicoddemus@gmail.com>

nicoddemus added this to the 5.0 milestone Jun 26, 2019

Victor Maryama added 8 commits June 26, 2019 18:10

Reverted changes.

0fb5241

Using ini-file option instead of cmd option.

d638da5

Black formatting.

f755ff6

Further reverting changes.

d91a5d3

Explanation variables only defined if failed or passed with plugins i…

9a34d88

…mplementing the hook.

Merge remote-tracking branch 'upstream/master' into features-assertio…

6f851e6

…n-pass-hook-master # Conflicts: # src/_pytest/assertion/rewrite.py

Added config back to AssertionWriter and fixed typo in check_if_asser…

53234bf

…tion_pass_impl function call.

Fixed import order pep8.

6854ff2

nicoddemus added 3 commits June 26, 2019 17:54

Fix default value of 'enable_assertion_pass_hook'

eb90f3d

Restore proper handling of '%' in assertion messages

fcbe66f

Improve docs and reference

3afee36

nicoddemus added 2 commits June 26, 2019 19:21

Add a trivial note about astor

8edf68f

Move formatting variables under the "has impls" if

629eb3e

Small optimization, move the generation of the intermediate formatting variables inside the 'if _check_if_assertion_pass_impl():' block.

Cover assertions with messages when enable_assertion_pass_hook is ena…

2ea2221

…bled

nicoddemus approved these changes Jun 26, 2019

View reviewed changes

nicoddemus merged commit 37fb50a into pytest-dev:master Jun 27, 2019

nicoddemus mentioned this pull request Jun 27, 2019

1.0 plans and general API stability berkerpeksag/astor#149

Closed

Sup3rGeo deleted the features-assertion-pass-hook branch June 27, 2019 09:44

nicoddemus mentioned this pull request Jun 27, 2019

Preparing release version 5.0.0 #5509

Merged

Sup3rGeo mentioned this pull request Jul 1, 2019

Hook for passing assertions #3457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features assertion pass hook #3479

Features assertion pass hook #3479

Sup3rGeo commented May 16, 2018 •

edited

nicoddemus commented May 25, 2018 •

edited

nicoddemus commented May 25, 2018 •

edited by tomviner

Sup3rGeo commented May 25, 2018

nicoddemus commented Aug 25, 2018 •

edited

Sup3rGeo commented Aug 27, 2018

nicoddemus commented Aug 27, 2018

Sup3rGeo commented Sep 12, 2018 •

edited

Sup3rGeo commented Sep 13, 2018 •

edited

Sup3rGeo commented Sep 14, 2018

Sup3rGeo commented Sep 14, 2018

Sup3rGeo commented Sep 15, 2018

Sup3rGeo commented Sep 16, 2018

Sup3rGeo commented Sep 16, 2018

nicoddemus commented Sep 18, 2018

codecov bot commented May 31, 2019 •

edited

nicoddemus commented May 31, 2019

Sup3rGeo commented May 31, 2019 •

edited

nicoddemus commented May 31, 2019

Sup3rGeo commented Jun 18, 2019

nicoddemus commented Jun 22, 2019 •

edited

nicoddemus left a comment

nicoddemus commented Jun 26, 2019

Sup3rGeo commented Jun 26, 2019

nicoddemus commented Jun 26, 2019

nicoddemus commented Jun 26, 2019

nicoddemus commented Jun 27, 2019

Sup3rGeo commented Jun 27, 2019

Features assertion pass hook #3479

Features assertion pass hook #3479

Conversation

Sup3rGeo commented May 16, 2018 • edited

nicoddemus commented May 25, 2018 • edited

nicoddemus commented May 25, 2018 • edited by tomviner

Sup3rGeo commented May 25, 2018

nicoddemus commented Aug 25, 2018 • edited

Sup3rGeo commented Aug 27, 2018

nicoddemus commented Aug 27, 2018

Sup3rGeo commented Sep 12, 2018 • edited

Sup3rGeo commented Sep 13, 2018 • edited

Sup3rGeo commented Sep 14, 2018

Sup3rGeo commented Sep 14, 2018

Sup3rGeo commented Sep 15, 2018

Sup3rGeo commented Sep 16, 2018

Sup3rGeo commented Sep 16, 2018

nicoddemus commented Sep 18, 2018

codecov bot commented May 31, 2019 • edited

Codecov Report

nicoddemus commented May 31, 2019

Sup3rGeo commented May 31, 2019 • edited

nicoddemus commented May 31, 2019

Sup3rGeo commented Jun 18, 2019

nicoddemus commented Jun 22, 2019 • edited

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented Jun 26, 2019

Sup3rGeo commented Jun 26, 2019

nicoddemus commented Jun 26, 2019

nicoddemus commented Jun 26, 2019

nicoddemus commented Jun 27, 2019

Sup3rGeo commented Jun 27, 2019

Sup3rGeo commented May 16, 2018 •

edited

nicoddemus commented May 25, 2018 •

edited

nicoddemus commented May 25, 2018 •

edited by tomviner

nicoddemus commented Aug 25, 2018 •

edited

Sup3rGeo commented Sep 12, 2018 •

edited

Sup3rGeo commented Sep 13, 2018 •

edited

codecov bot commented May 31, 2019 •

edited

Sup3rGeo commented May 31, 2019 •

edited

nicoddemus commented Jun 22, 2019 •

edited