Added PropertyConfig

[dharmaturtle]

Sometimes I don't want Hedgehog to shrink. For example, if I'm writing an integration test with a temporary database, it takes several seconds to spin up a new DB. When I'm in the initial stages of RedGreenRefactor, there's a lot of red. Often the first test will immediately fail, but it will incur ~30 shrinks because my datatypes aren't simple. Those shrinks become quite time-expensive for no real benefit.

This PR adds an (maxShrinks : int<shrinks> option) parameter to takeSmallest which controls how many times shrinking will occur. I also added Property.check'' and friends. I'm not fond of the name... but I'm out of ideas.

[moodmosaic]

Thank you for the pull request, @dharmaturtle 👍

A couple of things to consider:

• maxShrinks is QC terminology, shrinkLimit is HH terminology, so perhaps we'd want this as shrinkLimit
• Instead of Property.check'' perhaps it's time to extract all those arguments into a config data type as in HH (PropertyConfig, RunnerConfig) and QC (Args, etc) and then we'd probably also get rid of Property.check', Property.check'', and friends.

Until those are discussed, this pull request may be converted to draft.

[ghost]

Echoing @moodmosaic, I would love to get rid of the check', etc versions of functions we have.

Perhaps, for example, the existing check function could use a defaultConfig, while the check' function can be renamed to checkWith and accept a PropertyConfig data type.

That detail aside, I think any PropertyConfig data type should expose a builder-esque interface, a la

module PC = Hedgehog.PropertyConfig

let config =
    PC.defaultConfig
    |> PC.withDiscardLimit 100<discards> // Allow 100 discards before failing.
    |> PC.withShrinkLimit 50<shrinks> // Allow 50 shrinks before failing.
    |> PC.withTests 500<tests> // Run each property against 500 test cases.

someProperty
|> Property.checkWith config

This creates a well-defined public API, and frees us up to represent that configuration however we wish without introducing breaking changes.

[TysonMN]

I agree with @adam-becker

[moodmosaic]

What @adam-becker describes can be a good plan 👍

[dharmaturtle]

Sounds similar to what's going on with AutoGenConfig introduced in this PR. I like it :)

[ghost]

Love these changes, thanks for doing this @dharmaturtle! Left some comments, but overall it's looking good.

[ghost]

We should probably expose a fluent builder API for consumption in other .NET languages, something like this:

var config = PropertyConfig.Default
    .WithShrinkLimit(500)
    .WithTests(100);

Then these methods can just accept a PropertyConfig parameter, like the underlying functions. The main reason to do it this way is if we add new configuration parameters later, then we just have to add that to the builder API.

[dharmaturtle]

I added support for both. The look of line 91 in this screenshot doesn't look great to me, but perhaps there's a common config a consumer wants to use everywhere. I want to keep the overloads because they're what C#/VBers are used to, and it feels weird to force them to use the config. We can add more optional overloads in the future when PropertyConfig gets more fields, and since they're optional it won't break anything.

[ghost]

This can appear a little clunky, but I also wouldn't write it this way. Maybe this is a preference thing, but I would probably write that as:

// Define an acceptable configuration as a baseline for most tests
var config = PropertyConfig.Default;

// Override when necessary locally
property.Check(config.WithTestCount(20));

The parameterized approach is less strict, to be sure. The question I want to raise is: Is that a good thing?

I think we should decide on a single approach to keep the API clean. There's nothing worse when using a library than having too many ways to do something.

Subtle note about the code above: I've chosen the name PropertyConfig.Default as this is something of an unwritten standard in C#.

var x = System.Collections.Generic.EqualityComparer<string>.Default;
var y = System.Threading.Tasks.TaskScheduler.Default;

I'm not bothered if we want to go with DefaultConfig, as it matches the underlying code better though.

[TysonMN]

PropertyConfig.DefaultConfig is verbose. No need to repeate config.

[dharmaturtle]

In your example, config is scoped to a single method. I believe most testing frameworks (xUnit, nUnit, MSTest) have a separate method per test. So in order to span multiple tests, the config will need to be a class level member. It is likely that users will want their PropertyConfigs to span multiple classes though, so we're back to a static class (e.g. TestHelper) that sets the standard PropertyConfig (e.g. TestHelper.DefaultConfig). So to override a local test, it would still look similar to what I have in the screenshot.

I personally feel like two ways of doing things is fine. My personal style is to make it as easy as possible for the end-user to consume.

Regarding PropertyConfig.Default I agree and will push an update momentarily. Sadly default is a keyword in F# so we can't mirror it there.

[ghost]

In your example, config is scoped to a single method. I believe most testing frameworks (xUnit, nUnit, MSTest) have a separate method per test. So in order to span multiple tests, the config will need to be a class level member. It is likely that users will want their PropertyConfigs to span multiple classes though, so we're back to a static class (e.g. TestHelper) that sets the standard PropertyConfig (e.g. TestHelper.DefaultConfig). So to override a local test, it would still look similar to what I have in the screenshot.

I think this might be handled better by integration packages (Hedgehog.MSTest, Hedgehog.Xunit, etc). Since Hedgehog itself isn't a test runner, maybe in a package like that we can make the integration nicer. Your concerns are valid though, just trying to decide what's best for this project.

Maybe we could punt for now and add some of these things later, as they wouldn't be breaking. I think we're going to have a constant struggle of "how do we make this idiomatic in C#/VB?" and we definitely don't need to settle that to get these changes merged, as they are very nice to have.

[dharmaturtle]

I think this might be handled better by integration packages (Hedgehog.MSTest, Hedgehog.Xunit, etc)

Sorry, I don't think I follow. I believe you're suggesting that we move the overloads with optional arguments to another integration package like Hedgehog.Xunit. However, the following overload (and its friends) don't have anything specific to xUnit, MSTest, etc:

    static member Check
        (   property : Property<bool>,
            [<Optional; DefaultParameterValue null>] ?testLimit   : int<tests>,
            [<Optional; DefaultParameterValue null>] ?shrinkLimit : int<shrinks>
        ) : unit =
        Property.checkBoolWith (Build.config testLimit shrinkLimit) property

If we were to go through with this, we would need to make an integration package for every testing framework... with code that isn't specific to any testing framework. It would be a literal copy/paste from framework to framework. Also, this code is (probably) only going to be used by C#ers and VBers, and Hedgehog.Xunit is language agnostic. (For example I use F# with xUnit.) I believe these overloads belong in Hedgehog.LINQ because overloads are what C#ers and VBers are used to.

I'll remove these overloads if you want - just thought I'd respond to that sentence.

[ghost]

Sorry, I don't think I follow.

@dharmaturtle I just mean configuration management for running tests might be better suited for a specific test runner integration, since that was a concern raised in your previous comment. I'm not saying this with certainty since I haven't looked to see how configurable these test runners are.

[dharmaturtle]

Ah, I see what you mean. I agree that configuration management for specific testing frameworks should be relegated to other packages. I was just listing them as examples of testing frameworks that separate tests as methods of a class, which will necessitate code that looks like

property.Check(TestHelper.StandardConfig.WithTestCount(20));

vs

property.Check(20);
//or
property.Check(tests:20);

At least in C#.

[dharmaturtle]

Note that shrinkLimit is not an Option. This means that they can't set the value to None. This makes for slightly cleaner usage (PropertyConfig.withShrinkLimit 0 vs PropertyConfig.withShrinkLimit (Some 0)) but I don't feel strongly about it.

[ghost]

Since calling this function is an explicit choice a user would make, I'm fine with the parameter being a simple int<shrinks>. Making the parameter an Option<int<shrinks>> exposes how we've chosen to implement this a bit too much. Also, if we later decided to make this required, this could be very confusing. Consider this code:

PC.defaultConfig |> PC.withShrinkLimit None

If we decided to require that parameter, it would be very surprising when a test failed due to a shrink limit being imposed when the user explicitly chose None.

[TysonMN]

You could add to the API withoutShrinkLimit

[dharmaturtle]

Good idea. I was torn between that and withNoShrinkLimit, but my libarts sisters talked me out of it.

Inspired by Haskell, I dropped "Limit" from withShrinkLimit and withTestLimit, but kept "limit" in the parameter name.

[dharmaturtle]

Oh. I didn't know what that button did :|

[ghost]

Oh. I didn't know what that button did :|

I disapprove all of your work from here on out! 😅 No worries.

[ghost]

LGTM 👍

[moodmosaic]

🚀 🚀 🚀