Catch BaseException in safe_getattr

[cybergrind]

Hey, all!

Today I've spent few hours by tracking strange error after update to 3.2.1
git blaming showed that it was caused by 06a4933 from #2490 (that is fix for #580 ) particularly inheritance from BaseException

@nicoddemus @RonnyPfannschmidt
This is quite hard to provide minimal steps to reproduce our case, in general it is caused by https://github.com/pytest-dev/pytest-django/blob/master/pytest_django/plugin.py#L615 throws fail
We're doing some work during class initialization (in descriptors), thus before database is unblocked and it was fine before this commit: upon collecting it wasn't fail and during the tests, fixtures were created without errors.

1. We're creating descriptor that is trying to create object in database when we're accessing it
2. Collecting code is trying to access every attribute with safe_getattr so it triggers creation of object in database
3. safe_getattr isn't catching error, because new fail exception is derived from BaseException

I believe we shouldn't fail during collecting phase, so either:

1. we should change realization of safe_getattr to catch BaseException
2. we should use another safe_getattr realization in FixtureManager.parsefactories

This PR contains 1st as simplest but I can reimplement it to 2nd

[RonnyPfannschmidt]

this change would make it ignore KeyboardInterrupt

[RonnyPfannschmidt]

from what i can tell this is a compound issue and both solutions are incorrect, i'd like to defer that one to @hpk42

[cybergrind]

After writting down the issue I've figured out minimal reproducible script
Works in 3.1.3 and don't work in 3.2.*

import pytest
from unittest import TestCase


class SomeDescriptor:
    def __init__(self, fun):
        self.fun = fun

    def __get__(self, obj, cls=None):
        self.fun(cls)


def exc_fun(cls):
    if cls.fail:
        raise pytest.fail('Hello Exception')
    print('\ncalled exc_fun')


@pytest.fixture(autouse=True, scope='class')
def _runtest_setup(request):
    request.node.cls.fail = False
    request.node.cls.exc_prop


class TestException(TestCase):
    fail = True
    exc_prop = SomeDescriptor(exc_fun)

    def test_something(self):
        pass

@RonnyPfannschmidt yeah, sure we should catch it as TEST_OUTCOMES to prevent other errors from being catched (updated it)

[coveralls]

Coverage remained the same at 91.805% when pulling 12e2cd6 on cybergrind:fix_baseexception into 539523c on pytest-dev:master.

[coveralls]

Coverage increased (+0.0009%) to 91.805% when pulling ad00303 on cybergrind:fix_baseexception into 539523c on pytest-dev:master.

[RonnyPfannschmidt]

well done @cybergrind the new iteration looks correct 👍

a unitest is needed, checking that safe_getattr returns the default on a test outcome should suffice

a simple declared property that causes a test failure and a test getting it should fit to test the behaviour

for completeness sake there should be a extension of the docstring telling it will catch exceptions and OutcomeExceptions. for extra kudos add references to the relevant pr's as well

[cybergrind]

hey, @RonnyPfannschmidt
I've mostly done this PR but now it is failing and it seems like it is caused by hypothesis==3.21.0 (3.20.0 fails too but it works with 3.19.0). Should I add constraint to tox.ini or it should be in separate PR?

[RonnyPfannschmidt]

@cybergrind sorry for the wait
@nicoddemus i just took a look again, i think we best merge this one soonish

[nicoddemus]

Reopening to see how it fares with the latest hypothesis, this problem was fixed in 3.22.0

[nicoddemus]

Also took the liberty of improving the changelog a bit to be more user-friendly about the problem description.

[coveralls]

Coverage increased (+0.0009%) to 91.805% when pulling 12b1bff on cybergrind:fix_baseexception into 539523c on pytest-dev:master.

[coveralls]

Coverage increased (+0.0009%) to 91.805% when pulling f9157b1 on cybergrind:fix_baseexception into 709b8b6 on pytest-dev:master.

[RonnyPfannschmidt]

thanks everyone 👍