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

Catch BaseException in safe_getattr #2707

Merged
merged 3 commits into from Aug 31, 2017

Conversation

Projects
None yet
4 participants
@cybergrind
Copy link
Contributor

cybergrind commented Aug 21, 2017

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 comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Aug 21, 2017

this change would make it ignore KeyboardInterrupt

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Aug 21, 2017

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 cybergrind force-pushed the cybergrind:fix_baseexception branch from 12e2cd6 to ad00303 Aug 21, 2017

@cybergrind

This comment has been minimized.

Copy link
Contributor Author

cybergrind commented Aug 21, 2017

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

This comment has been minimized.

Copy link

coveralls commented Aug 21, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 21, 2017

Coverage Status

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

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Aug 22, 2017

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 cybergrind force-pushed the cybergrind:fix_baseexception branch from ad00303 to d9c3d7e Aug 23, 2017

@cybergrind cybergrind force-pushed the cybergrind:fix_baseexception branch from d9c3d7e to 12b1bff Aug 23, 2017

@cybergrind

This comment has been minimized.

Copy link
Contributor Author

cybergrind commented Aug 23, 2017

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

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Aug 30, 2017

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

@nicoddemus nicoddemus closed this Aug 31, 2017

@nicoddemus nicoddemus reopened this Aug 31, 2017

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Aug 31, 2017

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

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Aug 31, 2017

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

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 31, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 31, 2017

Coverage Status

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

@RonnyPfannschmidt RonnyPfannschmidt merged commit b770a32 into pytest-dev:master Aug 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Aug 31, 2017

thanks everyone 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.