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

Missing argument validation for `rule()` #2149

Closed
Zac-HD opened this issue Oct 24, 2019 · 6 comments · Fixed by #2213

Comments

@Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Oct 24, 2019

I was playing around with stateful testing earlier today, and messed up a function call. Here's the problem: this caused an internal error instead of a validation error with a nice message!

hypothesis.stateful.rule(), and initialize(), should use check_type to validate the types of all their arguments. There are examples of this usage elsewhere in the module, though you'll need to loop over kwargs.items() to get the argument names.

Then add tests to hypothesis-python/tests/cover/test_argument_validation.py, a minor release by creating hypothesis-python/RELEASE.rst (check the other open PRs for examples), and you'll be good to go!

You're welcome to check for other cases of missing validation, and we'd love fixes for them, but we would encourage that to happen in follow-up PRs. Better to merge small changes regularly than have them build up and get stuck!

@fragmad

This comment has been minimized.

Copy link

@fragmad fragmad commented Oct 29, 2019

If no one minds, I think I'll take a look at this. :)

@NNRepos

This comment has been minimized.

Copy link

@NNRepos NNRepos commented Nov 1, 2019

@Zac-HD,
Hi, I'm new to hypothesis and type hinting in general, and I have a few questions:

hypothesis.stateful.rule(), and initialize(), should use check_type to validate the types of all their arguments.

How do we know what types to compare against? Do we need to somehow extract the usage of kwargs from f?

There are examples of this usage elsewhere in the module, though you'll need to loop over kwargs.items() to get the argument names.

In both cases (Settings, RuleBasedStateMachine), the check_type is against a locally created object. This is not the case with kwargs.

Then add tests to hypothesis-python/tests/cover/test_argument_validation.py, a minor release by creating hypothesis-python/RELEASE.rst (check the other open PRs for examples), and you'll be good to go!

I couldn't find this in other PRs, do you have an example?

Thanks in advance

@Zac-HD

This comment has been minimized.

Copy link
Member Author

@Zac-HD Zac-HD commented Nov 1, 2019

Hey @NNRepos - I hope you're enjoying Hypothesis 😄 This isn't really type hinting, but explicit runtime checks. We know the types because we only handle certain types - target must be a Bundle , targets must be a tuple of Bundles, and the value of every keyword argument must be a strategy. Passing other types is already an internal error, but we want it to be an explict InvalidArgument error - does that make sense?

Here's a good example of a pull request including contributors entry, release file, change, and tests!

@Zac-HD

This comment has been minimized.

Copy link
Member Author

@Zac-HD Zac-HD commented Nov 5, 2019

Hey @fragmad, any progress? Happy to give you some tips or help out if you're stuck 🙂

@benjpalmer

This comment has been minimized.

Copy link

@benjpalmer benjpalmer commented Nov 18, 2019

Hey @Zac-HD,

I'm also new to hypothesis and would like to help contribute if this issue is still open. I was hoping you could clarify something for me.

It looks like both hypothesis.stateful.rule() and initialize() have the target, and targets argument validation done inside of _convert_targets(). Are you looking to separate some of that logic and perform validation separately from converting while also including validation of **kwargs?

Thanks!

@Zac-HD

This comment has been minimized.

Copy link
Member Author

@Zac-HD Zac-HD commented Nov 19, 2019

Hey @benjpalmer - welcome! We'll reserve this issue for you for a week; after that anyone else can open a PR 😄

It looks like both rule() and initialize() have the target, and targets argument validation done inside of _convert_targets(). Are you looking to separate some of that logic and perform validation separately from converting while also including validation of **kwargs?

The validation for target and targets is just fine - all we need to add is a loop that validates that each value in kwargs is a strategy, and if not also says which. Something like

for k, v in kwargs.items():
    check_type(SearchStrategy, v, k)

plus tests, a changelog, and your name in the authors list

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