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

Detect misapplication of given to e.g. setup methods #991

Closed
DRMacIver opened this issue Nov 27, 2017 · 1 comment
Closed

Detect misapplication of given to e.g. setup methods #991

DRMacIver opened this issue Nov 27, 2017 · 1 comment
Assignees
Labels
legibility make errors helpful and Hypothesis grokable

Comments

@DRMacIver
Copy link
Member

DRMacIver commented Nov 27, 2017

We have a new entry in the ongoing war between making Hypothesis impossible to misuse and the creativity of determined users. 😉

17:12 <doismellburning> Annotating `setUp` with `given` - is this a thing? Is it ScarySauce(TM)?
17:12 <@DRMacIver> It's not a thing
17:12 <@DRMacIver> I've no idea what would happen, but it would probably be hilariously confusing
17:13 <doismellburning> welp, it's in my codebase!
17:13 — @DRMacIver blinks
17:13 <doismellburning> cheers :)
17:15 <@DRMacIver> Maybe Hypothesis should error (with deprecation path) when @given is applied to methods that don't have "test" in the name
17:16 <doismellburning> that sounds like it might be useful
17:17 <@DRMacIver> Yeah
17:17 <@DRMacIver> Though I'm sure https://xkcd.com/1172/ applies
17:18 <doismellburning> lol
17:23 <@DRMacIver> Maybe I'll just check for setUp for now. I can't think of too many other cases where this would be a problem.

I'm not sure what quite the right level of checking is here. I feel like it's reasonable, especially with toy examples, to use @given on methods that aren't strictly tests, but I can't think of a good use for it being on setUp. Maybe we should just have a small blacklist of method names?

This should go through the usual deprecation -> error later path of course.

@DRMacIver DRMacIver added the enhancement it's not broken, but we want it to be better label Nov 27, 2017
@Zac-HD
Copy link
Member

Zac-HD commented Nov 28, 2017

I think the user error here isn't "@given on things without test in the name" - that would break use of non-English function names, or even just alternative test discovery conventions.

Instead, I'd look for "@given on methods on a subclass of unittest.TestCase, where the name is defined on unittest.TestCase".

It looks like we can do this in run_test_with_generator in given in core.py, by suitably cautious use of the __self__ attribute to get the class it's bound to, ismethod, and issubclass - it'll fit right in with the rest of core.py!

(update: nope, not that easy - the test isn't bound to any class at the point where it's decorated)

@Zac-HD Zac-HD self-assigned this Jan 7, 2018
@Zac-HD Zac-HD added legibility make errors helpful and Hypothesis grokable and removed enhancement it's not broken, but we want it to be better labels Feb 25, 2018
@Zac-HD Zac-HD added this to the 3.x milestone Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legibility make errors helpful and Hypothesis grokable
Projects
None yet
Development

No branches or pull requests

2 participants