help with failing test on mock backport under pypy3

Create issue
Issue #3010 resolved
Chris Withers created an issue

I’ve just finished backporting outstanding patches to the rolling mock backport.

Unfortunately, this has caused a test to start failing on pypy3:

https://travis-ci.org/testing-cabal/mock/jobs/526308383

Would anyone be able to help getting that to pass or give any feedback on what might be going wrong?

Many thanks for any help!

Comments (20)

  1. Carl Friedrich Bolz-Tereick

    So the first source of failures is the tests depending on reference counting. The attached patch fixes 10 of the failures. (in the CPython tests this is done by calling test.support.gc_collect())

  2. Carl Friedrich Bolz-Tereick

    test_assert_has_awaits_not_matching_spec_error is failing for me on CPython3.8.0 as well, so I am going to ignore that.

  3. Carl Friedrich Bolz-Tereick

    (I see now that test_assert_has_awaits_not_matching_spec_error is not in the circleci failures anyway, for some reason I got that locally).

  4. Chris Withers reporter

    Hmm, the gc_collect change is quite invasive. Why does this need to be called 3 times?

  5. mattip

    The docstring of gc_collect in the stdlib’s `test/support/__init__.py reads:

    In non-CPython implementations of Python, this is needed because timely
    deallocation is not guaranteed by the garbage collector. (Even in CPython
    this can be the case in case of reference cycles.) This means that __del__
    methods may be called later than expected and weakrefs may remain alive for
    longer than expected. This function tries its best to force all garbage
    objects to disappear.

    Unfortunately, the stdlib `test/support`facitlites cannot be relied on in projects since it is not always shipped with a python installation, for instance on Debian-based distros, so it must be vendored in to the project’s test code.

  6. Chris Withers reporter

    Thanks for the help, looks like this is the last one that’s problematic:

        def test_spec_has_descriptor_returning_function(self):
    
            class CrazyDescriptor(object):
    
                def __get__(self, obj, type_):
                    if obj is None:
                        return lambda x: None
    
            class MyClass(object):
    
                some_attr = CrazyDescriptor()
    
            mock = create_autospec(MyClass)
            mock.some_attr(1)
    

    On cpython:

    MyClass.some_attr(1)
    mock.some_attr(1)
    <MagicMock name='mock.some_attr()' id='4566378704'>
    

    On pypy:

    (Pdb) MyClass.some_attr(1)
    (Pdb) mock.some_attr(1)
    *** TypeError: too many positional arguments
    

  7. Carl Friedrich Bolz-Tereick

    I can try to look into it a bit today, but it seems harder than the previous ones. Once again it boils down to PyPy not making much of a difference between “builtin” and regular functions, this time in the ``_must_skip`` helper function.

  8. Carl Friedrich Bolz-Tereick

    Ok, this is definitely a bug in the mock code, at least in the context of PyPy. Here’s a way to fix _must_skip mock to work for this test on pypy (see patch). All other tests pass as well. On CPython, this keeps all tests passing too.

    The change is how the checking of whether something is a method or not is done. The current code does this:

    isinstance(getattr(result, '__get__', None), MethodWrapperTypes)

    This is wrong on PyPy, because MethodWrapperTypes are just regular functions on PyPy, so the check returns True for custom descriptors as well. I instead replaced it with the following, which imo is clearer and more correct:

    isinstance(result, FunctionTypes)

    I was trying (and failing) to write a failing test on CPython that shows a problem with the previous version of this line on CPython too. However, it could be definitely be made to fail if you have an extension module that defines a descriptor that does the same thing as CrazyDescriptor in the test.

    I am not sure how best to proceed, I suppose you don't want to deviate from the CPython code of mock too much? Should I try to get the change into CPython?

  9. Chris Withers reporter

    Yep, the idea is to get changes, where at all possible, into CPython. https://github.com/testing-cabal/mock is a pure backport, with only changes needed for prior python versions in it that deviate from upstream.

    I’m a maintainer of cpython’s mock too, so happy to work with you to land a pull request that fixes this…

  10. Carl Friedrich Bolz-Tereick

    I’d be happy to submit a pull request, I suppose I should start with a b.p.o ticket?

  11. Carl Friedrich Bolz-Tereick

    (btw, I think it might make sense to release mock 4.0 in the current state anyway, if you don’t want to wait. this bug has been there since in PyPy’s mock versions 2013 at least, we received no bug reports. so fixing it can wait for another release)

  12. Chris Withers reporter

    Ah, would be nice to clear this up, then we’d have no skips. bpo first is probably safest, makes the PR bots happier.

  13. Log in to comment