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

Limit recursion depth based on size instead of configuration value #37

Open
TysonMN opened this issue Jul 2, 2020 · 10 comments
Open

Limit recursion depth based on size instead of configuration value #37

TysonMN opened this issue Jul 2, 2020 · 10 comments

Comments

@TysonMN
Copy link
Member

TysonMN commented Jul 2, 2020

PR #21 added a configuration value called RecursionDepth to prevent stack overflows when generating a recursive type (c.f. #19 (comment)).

FsCheck solves this problem by relating the recursion depth to the size parameter.

However, a recursive generator like this may fail to terminate with a StackOverflowException, or produce very large results. To avoid this, recursive generators should always use the size control mechanism

This seems like a better solution to me, but I am just getting seriously into property-based testing, so I am probably unaware of all the considerations.

What are the tradeoffs for this configuration value vs utilizing the size parameter?

@TysonMN TysonMN changed the title Limit recursion depth based on size/range instead of configuration value Limit recursion depth based on size instead of configuration value Jul 2, 2020
@cmeeren
Copy link
Collaborator

cmeeren commented Jul 2, 2020

I have no idea; @moodmosaic?

@moodmosaic
Copy link
Member

The sized approach of FsCheck is more elegant since you can use the size parameter via Gen.sized to control the depth (instead of introducing another variable).

@moodmosaic
Copy link
Member

Some thoughts around the size, in general: hedgehogqa/haskell-hedgehog#38 (comment)

@TysonMN
Copy link
Member Author

TysonMN commented Jul 2, 2020

That is what I expected.

@cmeeren, how do you feel about removing RecursionDepth and instead bounding recursive generation with the size parameter?

I am interested to implement this simply because I think it might be a good challenge for me.

@TysonMN
Copy link
Member Author

TysonMN commented Jul 2, 2020

Some thoughts around the size, in general: hedgehogqa/haskell-hedgehog#38 (comment)

Yes, I really like the Range concept. I think it is very clear...after @cmeeren first explained to me in elmish/Elmish.WPF#240 (comment) and then especially after watching the video @moodmosaic linked to in hedgehogqa/fsharp-hedgehog#182 (comment).

@cmeeren
Copy link
Collaborator

cmeeren commented Jul 3, 2020

@cmeeren, how do you feel about removing RecursionDepth and instead bounding recursive generation with the size parameter?

I'm happy to use size to control the recursion depth, but perhaps we should have a MaxRecursionDepth so that users have some control. Too much recursion in auto-generation can substantially increase the test run time. What do you think?

@moodmosaic
Copy link
Member

so that users have some control

They’ll have that control through the size... What happens when size > MaxRecursionDepth?

@cmeeren
Copy link
Collaborator

cmeeren commented Jul 3, 2020

They’ll have that control through the size...

How so? Isn't size automatically incremented from 0 to 99 or something like that based on the test that is run?

@moodmosaic
Copy link
Member

Yes, but you can interpret it (in this case) any way you want, e.g. halve it on each recursion (as FsCheck mentions in the docs).

@cmeeren
Copy link
Collaborator

cmeeren commented Jul 3, 2020

Ok. (For reference, here's the mentioned docs.)

I won't be able to work on this, but I'm more than happy to take a PR that makes sure that auto-generating recursive types aren't unnecessarily slow (i.e., be conservative with the defaults, e.g. exponential with a max depth of, say, 3? Open to suggestions), and gives the user some kind of control over the recursion depth. The readme should have an example of how to control it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants