Skip to content

pytest.raises(AssertionError) fails with compiled cython modules #645

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

Closed
pytestbot opened this issue Dec 16, 2014 · 27 comments
Closed

pytest.raises(AssertionError) fails with compiled cython modules #645

pytestbot opened this issue Dec 16, 2014 · 27 comments
Labels
good first issue easy issue that is friendly to new contributor type: bug problem that needs to be addressed
Milestone

Comments

@pytestbot
Copy link
Contributor

Originally reported by: Steffen Roecker (BitBucket: sroecker, GitHub: sroecker)


This issue seems similar to issue #176.
Running with unittest works fine, however pytest fails when the imported module assert_helper is compiled to a .so with Cython.

cython assert_helper.py
gcc -c -fPIC -I/usr/include/python2.7/ assert_helper.c
gcc -shared assert_helper.o -o assert_helper.so

platform linux2 -- Python 2.7.3 -- py-1.4.26 -- pytest-2.6.4
plugins: xdist, cov, mock
Cython==0.21


@pytestbot
Copy link
Contributor Author

Original comment by Steffen Roecker (BitBucket: sroecker, GitHub: sroecker):


Fix instructions

@pytestbot pytestbot added the type: bug problem that needs to be addressed label Jun 15, 2015
@RonnyPfannschmidt
Copy link
Member

rauses should take the assertion rewriting into acount and match the BuiltinAssertionError when assertion rewriting is enabled

@RonnyPfannschmidt RonnyPfannschmidt added the good first issue easy issue that is friendly to new contributor label Jul 25, 2015
@RonnyPfannschmidt RonnyPfannschmidt modified the milestones: 2.8, 2.8.dev Sep 13, 2015
@RonnyPfannschmidt
Copy link
Member

pytest-output.txt
test_assertion.py.txt
assert_helper.py.txt

synced the attachements
@sroecker anything you can tell on cython, we already have some code in place to handle it but that seems to miss magic special cases from cython

@RonnyPfannschmidt RonnyPfannschmidt modified the milestones: 2.8.2, 2.8.1 Sep 27, 2015
@RonnyPfannschmidt RonnyPfannschmidt modified the milestones: 2.8.2, 2.8.3 Oct 10, 2015
@nicoddemus nicoddemus modified the milestones: 2.8.3, 2.8.4 Nov 23, 2015
@RonnyPfannschmidt RonnyPfannschmidt modified the milestones: 2.8.4, 2.8.5 Dec 6, 2015
@RonnyPfannschmidt RonnyPfannschmidt modified the milestones: 2.8.5, 2.8.6 Dec 14, 2015
@enkore
Copy link

enkore commented Apr 13, 2016

Yesterday I stumbled over this, and it looks like "assert False" in Cython raises an exception with the name 'AssertionError', but with a different type object than Python's AssertionError

isinstance(exc, AssertionError) => False

try:
except AssertionError:  # didn't caught exception (but Exception does)

@RonnyPfannschmidt
Copy link
Member

seems like we need a cython bug workaround, i wonder where to put it

@enkore
Copy link

enkore commented Apr 13, 2016

Hmm, here's what Cython does for an "assert": https://github.com/cython/cython/blob/master/Cython/Compiler/Nodes.py#L5762 Specifically:

            code.putln(
                "PyErr_SetObject(PyExc_AssertionError, %s);" % self.value.py_result())

            code.putln(
                "PyErr_SetNone(PyExc_AssertionError);")

I'll test whether this behaviour can be reproduced with a simple, plain extension module.

@RonnyPfannschmidt
Copy link
Member

does the same happen if with --assert plain

i recall putting in a fix for a certain assertion error issue, since pytest used a custom assdertionerror subclass for reinterpretation for a while

i wonder if it resurfaced due to the c api usage

@enkore
Copy link

enkore commented Apr 13, 2016

Looks like pytest.raises works, but not assertRaises of unittest.TestCase (at least when run with py.test). The code where I'm seeing this is borgbackup/borg#891, test_nsindex_segment_limit

Works (ugly as hell)

def test_nsindex_segment_limit():
    idx = NSIndex()
    # work around known bug in py.test https://github.com/pytest-dev/pytest/issues/645
    try:
        idx[H(1)] = hashindex.MAX_REF + 1, 0
    except Exception as e:
        # it's not Python's AssertionError
        assert e.__class__.__name__ == "AssertionError"
    else:
        assert False
    assert H(1) not in idx
    idx[H(2)] = hashindex.MAX_REF, 0
    assert H(2) in idx

Works:

def test_nsindex_segment_limit():
    idx = NSIndex()
    with pytest.raises(AssertionError):
        idx[H(1)] = hashindex.MAX_REF + 1, 0

(^ just noticed that now, I just assumed that pytest.raises wouldn't work, too, so I'm going to use that instead)

Nope:

class NSSLTestCase(BaseTestCase):
    def test_1234(self):
        idx = NSIndex()
        with self.assert_raises(AssertionError):
            idx[H(1)] = hashindex.MAX_REF + 1, 0

With --assert plain they all pass. Interesting... I wonder why that is. I understand pytest is kinda instrumenting the source under test here, but that naturally won't have any effect on a compiled module. Maybe something is slightly off when the TestCase wrapping thing is instrumented?

Both "reinterp" and "rewrite" fail.

@RonnyPfannschmidt
Copy link
Member

does it work with --assert=plain ?

@enkore
Copy link

enkore commented Apr 13, 2016

Yep, but neither reinterp nor rewrite pass.

@nicoddemus
Copy link
Member

FWIW this is also a valid workaround:

with self.assertRaises(self.failureException):
    ...

I never took the time to look it up, but I suspect that unittest keeps a reference to the builtin AssertionError in unittest.TestCase during import, and pytest replaces the builtin AssertionError when using reinterpret. Then self.assert* methods raise the original AssertionError using self.failureException, that's why it doesn't match during runtime.

@RonnyPfannschmidt
Copy link
Member

since we now do have assertion rewriting we should decide if we want to stop that monkeypatching and close this issue after a decission

@nicoddemus
Copy link
Member

since we now do have assertion rewriting we should decide if we want to stop that monkeypatching and close this issue after a decission

Do we need the monkey patching when using assertion rewriting?

@RonnyPfannschmidt
Copy link
Member

as far as i can tell its mostly unnecessary

@enkore
Copy link

enkore commented Apr 13, 2016

What I don't quite understand is why "assertion rewriting" has any effect here. The asserts in question aren't in Python, and can't be rewritten [easily].
If this is due to "AssertionError" being replaced at run time with a different type, then it shouldn't depend on assertRaises vs. pytest.raises, since I would pass both the wrong exception type?

@RonnyPfannschmidt
Copy link
Member

the problem is the assertion reinterpreter fallback replacing the built-in assertion error with a custom one thats a subclass

c code then will still use the base class, but isinstance will fail on the python side

@The-Compiler
Copy link
Member

What about getting rid of --assert=reinterp (and thus related issues) for pytest 3? Is there still an usecase for it?

If you agree in principle I'll open a separate issue.

@enkore
Copy link

enkore commented Apr 13, 2016

Okay. But I pass the same "wrong" AssertionError to both kind of "raises" check, why does one work and the other doesn't?

Aha. _pytest.python, line 1205ff:

def raises(expected_exception, *args, **kwargs):
    ...
    if expected_exception is AssertionError:
        # we want to catch a AssertionError
        # replace our subclass with the builtin one
        # see https://github.com/pytest-dev/pytest/issues/176
        from _pytest.assertion.util import BuiltinAssertionError \
            as expected_exception

... and this special-casing of AssertionError is then not present in UnitTest.assertRaises. Got it.

@RonnyPfannschmidt
Copy link
Member

the pytest one special cases AssertionError replacing it with the builtin one

@RonnyPfannschmidt
Copy link
Member

@The-Compiler see the pytest-dev ml

@nicoddemus nicoddemus modified the milestones: 3.0, 2.8.6 Jun 19, 2016
@nicoddemus
Copy link
Member

Assertion reinterpret is still used if the assertion code is in a non-test module, right?

@RonnyPfannschmidt
Copy link
Member

@flub @nicoddemus can this be closed for the 3.0 release?

@nicoddemus
Copy link
Member

Yes, should no longer be an issue because we no longer replace the builtin AssertionError.

@TomAugspurger
Copy link

TomAugspurger commented Aug 1, 2016

Was this issue closed because it was fixed? Using a freshly cloned pytest, I'm seeing a difference between pytest's handling of AssertionErrors (via an assert statement) and other exceptions.

With two files

# pytestbug.pyx
def assert_func():
    assert False, "AssertionError"


def type_func():
    raise TypeError("TypeError")

and

test_foo.py
import unittest
import pyximport

pyximport.install()  # avoids need for setup.py

import pytestbug


def assert_func():
    assert False, "AssertionError"


def type_func():
    raise TypeError("TypeError")


class TestFoo(unittest.TestCase):

    def test_assert(self):
        with self.assertRaisesRegexp(AssertionError, "AssertionError"):
            assert_func()

    def test_type(self):
        with self.assertRaisesRegexp(TypeError, "TypeError"):
            type_func()

    def test_assert_cython(self):
        with self.assertRaisesRegexp(AssertionError, "AssertionError"):
            pytestbug.assert_func()

    def test_type_cython(self):
        with self.assertRaisesRegexp(TypeError, "TypeError"):
            pytestbug.type_func()

py.test test_foo.py outputs

================================================ test session starts ================================================
platform darwin -- Python 3.5.2, pytest-2.9.3.dev0, py-1.4.31, pluggy-0.3.1
rootdir: /private/tmp/pybug, inifile:
plugins: cov-2.2.1, xdist-1.14
collected 4 items

test_foo.py .F..

===================================================== FAILURES ======================================================
____________________________________________ TestFoo.test_assert_cython _____________________________________________

self = <test_foo.TestFoo testMethod=test_assert_cython>

    def test_assert_cython(self):
        with self.assertRaisesRegexp(AssertionError, "AssertionError"):
>           pytestbug.assert_func()

test_foo.py:29:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   assert False, "AssertionError"
E   AssertionError: AssertionError

pytestbug.pyx:2: AssertionError
======================================== 1 failed, 3 passed in 0.11 seconds =========================================

EDIT: Forgot to mention that py.test test_foo.py --assert=plain passes.

EDIT2: I see now that the original issue was for pytest.raises, and I've used TestCase.assertRaisesRegexp. Is my example even expected to work?

@nicoddemus
Copy link
Member

It should be fixed in the features branch because we removed assert-reinterpretation and in turn the code which overwritten the builtin AssertionError object, which was the cause for this issue.

@TomAugspurger
Copy link

Yep, user error on my part, sorry :( Checking out the feature-branch fixed it.

Thanks, I'm excited to be porting pandas over to pytest.

@nicoddemus
Copy link
Member

Thanks, I'm excited to be porting pandas over to pytest.

Cool! 😎

OddBloke added a commit to OddBloke/cloud-init that referenced this issue Apr 23, 2020
We're seeing a strange error in xenial builds which appear to be
stemming from the AssertionError that pytest produces being _different_
from the standard AssertionError.  This means that the helpers modified
here weren't behaving correctly, because they weren't catching
AssertionErrors as one would expect.

(I believe this is related, in some way, to
pytest-dev/pytest#645, but the only version of
pytest where we're affected is so far in the past that it's not worth
pursuing it any further as we have a workaround.)
OddBloke added a commit to canonical/cloud-init that referenced this issue Apr 24, 2020
These libraries provide backports of Python 3's stdlib components to Python 2. As we only support Python 3, we can simply use the stdlib now. This pull request does the following:

* removes some unneeded compatibility code for the old spelling of `assertRaisesRegex`
* replaces invocations of the Python 2-only `assertItemsEqual` with its new name, `assertCountEqual`
* replaces all usage of `unittest2` with `unittest`
* replaces all usage of `contextlib2` with `contextlib`
* drops `unittest2` and `contextlib2` from requirements files and tox.ini

It also rewrites some `test_azure` helpers to use bare asserts. We were seeing a strange error in xenial builds of this branch which appear to be stemming from the AssertionError that pytest produces being _different_ from the standard AssertionError.  This means that the modified helpers weren't behaving correctly, because they weren't catching AssertionErrors as one would expect. (I believe this is related, in some way, to pytest-dev/pytest#645, but the only version of pytest where we're affected is so far in the past that it's not worth pursuing it any further as we have a workaround.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor type: bug problem that needs to be addressed
Projects
None yet
Development

No branches or pull requests

6 participants