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

Hypothesis doesn't recognize unittest.SkipTest exception #514

Closed
coriolinus opened this issue Apr 13, 2017 · 7 comments · Fixed by #731
Closed

Hypothesis doesn't recognize unittest.SkipTest exception #514

coriolinus opened this issue Apr 13, 2017 · 7 comments · Fixed by #731
Labels
bug something is clearly wrong here

Comments

@coriolinus
Copy link

Narrative example of failure

I haven't had time to construct a minimal example, so I'll give a full example of failure. I've recently updated a test in my Django project which conditionally skips a test in cases which no longer make sense.

The test in question is defined in an abstract base class of a couple other test classes which specialize it further. One branch of one test no longer makes sense in the context of one of the specialization classes. I can't solve this with assume() or @unittest.skipIf() because neither has enough contextual data. Instead, I raise unittest.SkipTest() at runtime if the test should be skipped.

The issue is that Hypothesis doesn't know about unittest.SkipTest, so produces failure output, as seen below:

(.venv) coriolinus$ ./manage.py test
Creating test database for alias 'default'...
Installed 39 object(s) from 1 fixture(s)
Installed 21 object(s) from 1 fixture(s)
Installed 9579 object(s) from 1 fixture(s)
Installed 5 object(s) from 1 fixture(s)
Installed 6 object(s) from 1 fixture(s)
.............................s.............................Falsifying example: test_admin_sees_everyone(self=<core.tests.test_user_profile_visibility.ProfileVisibilityTests testMethod=test_admin_sees_everyone>, data=data(...))
Draw 1: <User: alpha>
s..........................................
----------------------------------------------------------------------
Ran 102 tests in 58.985s

OK (skipped=2)
Destroying test database for alias 'default'...
(.venv) coriolinus$ pip list | grep hypothesis
hypothesis (3.6.1)

Expected behavior

Hypothesis does not produce failure output if a test raises unittest.SkipTest.

Observed behavior

Hypothesis produces failure output if a test raises unittest.SkipTest.

@Zac-HD Zac-HD added the bug something is clearly wrong here label Apr 13, 2017
@alexwlchan
Copy link
Contributor

Thanks for the report! I had a quick go at trying to write an example – is this sort of like what you were doing?

import unittest

from hypothesis import given
from hypothesis.strategies import integers


class DemoTest(unittest.TestCase):

    @given(integers())
    def test_to_be_skipped(self, x):
        if x == 0:
            raise unittest.SkipTest()
        else:
            assert x == 0


unittest.main()
$ python issue514.py
Falsifying example: test_to_be_skipped(self=<__main__.DemoTest testMethod=test_to_be_skipped>, x=0)
s
----------------------------------------------------------------------
Ran 1 test in 0.021s

OK (skipped=1)

Sounds like we should just recognise unittest.SkipTest as a special exception – I get the same result whether I raise the exception directly or call self.skipTest(reason). Not sure how hard that is off the top of my head.

@coriolinus
Copy link
Author

coriolinus commented Apr 13, 2017 via email

@alexwlchan
Copy link
Contributor

alexwlchan commented Apr 13, 2017

So at a very quick glance, I think we need to make a change somewhere around line 464 of core.py so that if we catch a unittest.SkipTest exception, we abort the entire test run.

I’m not sure what mechanisms we have for marking a test as skipped (as opposed to just silently dropping the error) – I’ll dig in a little further when I have more time.

@DRMacIver
Copy link
Member

I’m not sure what mechanisms we have for marking a test as skipped (as opposed to just silently dropping the error)

I think just immediately reraising the error should have the desired effect shouldn't it?

@alexwlchan
Copy link
Contributor

I think just immediately reraising the error should have the desired effect shouldn't it?

Oh yeah. PR incoming. :)

@alexwlchan
Copy link
Contributor

alexwlchan commented Jul 20, 2017

@coriolinus This took longer than I was expecting to finish, but the fix is now released as Hypothesis 3.13.

Thanks for your bug report!

@coriolinus
Copy link
Author

coriolinus commented Jul 20, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is clearly wrong here
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants