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.

Sign up for GitHub

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

Jump to bottom

Simplifying property configuration #346

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Simplifying property configuration #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 36 additions & 45 deletions src/Hedgehog/Linq/Property.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,91 +71,46 @@ type PropertyExtensions private () =
let (Property property) = property
Property.report property

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might actually be nice to keep the config overloads. Thoughts?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should only expose a single way to configure properties; this new API should be the only API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not exactly sure what is being discussed here. However, I am not overly concerned with the C# API.

[<Extension>]
static member Report (property : Property, config : Hedgehog.PropertyConfig) : Report =
let (Property property) = property
Property.reportWith config property

[<Extension>]
static member Report (property : Property<bool>) : Report =
Property.reportBool property

[<Extension>]
static member Report (property : Property<bool>, config : Hedgehog.PropertyConfig) : Report =
Property.reportBoolWith config property

[<Extension>]
static member Check (property : Property) : unit =
let (Property property) = property
Property.check property

[<Extension>]
static member Check (property : Property, config : Hedgehog.PropertyConfig) : unit =
let (Property property) = property
Property.checkWith config property

[<Extension>]
static member Check (property : Property<bool>) : unit =
Property.checkBool property

[<Extension>]
static member Check (property : Property<bool>, config : Hedgehog.PropertyConfig) : unit =
Property.checkBoolWith config property

[<Extension>]
static member Recheck (property : Property, size : Size, seed : Seed) : unit =
let (Property property) = property
Property.recheck size seed property

[<Extension>]
static member Recheck (property : Property, size : Size, seed : Seed, config : Hedgehog.PropertyConfig) : unit =
let (Property property) = property
Property.recheckWith size seed config property

[<Extension>]
static member Recheck (property : Property<bool>, size : Size, seed : Seed) : unit =
Property.recheckBool size seed property

[<Extension>]
static member Recheck (property : Property<bool>, size : Size, seed : Seed, config : Hedgehog.PropertyConfig) : unit =
Property.recheckBoolWith size seed config property

[<Extension>]
static member ReportRecheck (property : Property, size : Size, seed : Seed) : Report =
let (Property property) = property
Property.reportRecheck size seed property

[<Extension>]
static member ReportRecheck (property : Property, size : Size, seed : Seed, config : Hedgehog.PropertyConfig) : Report =
let (Property property) = property
Property.reportRecheckWith size seed config property

[<Extension>]
static member ReportRecheck (property : Property<bool>, size : Size, seed : Seed) : Report =
Property.reportRecheckBool size seed property

[<Extension>]
static member ReportRecheck (property : Property<bool>, size : Size, seed : Seed, config : Hedgehog.PropertyConfig) : Report =
Property.reportRecheckBoolWith size seed config property

[<Extension>]
static member Render (property : Property) : string =
let (Property property) = property
Property.render property

[<Extension>]
static member Render (property : Property, config : Hedgehog.PropertyConfig) : string =
let (Property property) = property
Property.renderWith config property

[<Extension>]
static member Render (property : Property<bool>) : string =
Property.renderBool property

[<Extension>]
static member Render (property : Property<bool>, config : Hedgehog.PropertyConfig) : string =
Property.renderBoolWith config property

[<Extension>]
static member Where (property : Property<'T>, filter : Func<'T, bool>) : Property<'T> =
Property.filter filter.Invoke property
Expand Down Expand Up @@ -184,4 +139,40 @@ type PropertyExtensions private () =
Property.ofThrowing projection.Invoke (a, b)))
Property result

[<Extension>]
static member WithShrinks (property : Property, shrinkLimit : int<shrinks>) : Property =
let (Property property) = property
Property (Property.withShrinks shrinkLimit property)

[<Extension>]
static member WithoutShrinks (property : Property) : Property =
let (Property property) = property
Property (Property.withoutShrinks property)

[<Extension>]
static member WithTests (property : Property, testLimit : int<tests>) : Property =
let (Property property) = property
Property (Property.withTests testLimit property)

[<Extension>]
static member WithConfig (property : Property, config : PropertyConfig) : Property =
let (Property property) = property
Property (Property.withConfig config property)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get rid of this, and only expose configuration with Property.with*

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as Property.Config.set (as I strucutured/named it in another comment). I think it is better to keep. Might want to consider renaming to setConfig though.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the value be in overriding a user's preferences?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be the user calling this function, it would be their preference. For example, @dharmaturtle talks about how he runs PB tests with a time limit during development and with a test limit during integration. I think the minimal amount of duplicated code to achieve this would be constructing the correct PropertyConfig instance in a global location and then referencing it from all the PB tests in order to set it, which (when using C#) would be achieved via this method.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also make an extension method of Property that configures them to all be the same and call that on each property you want it to apply to.

public static Property ConfigureSpecifically(this Property property) =>
    property.WithTests(1000).WithoutShrinkLimit().Whatever();

// in a test

var someProperty = // ...
someProperty.ConfigureSpecifically().OverrideLocally().Check();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

I think I would still keep this myself, but I won't argue further in favor of keeping it.


[<Extension>]
static member WithShrinks (property : Property<bool>, shrinkLimit : int<shrinks>) : Property<bool> =
Property.withShrinks shrinkLimit property

[<Extension>]
static member WithoutShrinks (property : Property<bool>) : Property<bool> =
Property.withoutShrinks property

[<Extension>]
static member WithTests (property : Property<bool>, testLimit : int<tests>) : Property<bool> =
Property.withTests testLimit property

[<Extension>]
static member WithConfig (property : Property<bool>, config : PropertyConfig) : Property<bool> =
Property.withConfig config property

#endif
82 changes: 34 additions & 48 deletions src/Hedgehog/Property.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ open System

[<Struct>]
type Property<'a> =
| Property of Gen<Journal * Outcome<'a>>
| Property of PropertyConfig * Gen<Journal * Outcome<'a>>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about making this into a record? How about this?

{ Config: PropertyConfig
  Generator: Gen<Journal * Outcome<'a>> }

Copy link

@ghost ghost Sep 11, 2021 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this, I think we should also mark the internals as private, and use lower camel case for the names:

// unsure of the validity of this syntax, just typing into the comment editor right now
type Property<'a> =
    private
    {
        config: PropertyConfig
        generator: Gen<Journal * Outcome<'a>>
    }

In any case, we should make 2 accessor functions, and only use them. They can be marked as inline if we have performance concerns.

let inline config (property : Property<'a>) : PropertyConfig =
    property.config

// This already exists as 'toGen'
let inline generator (property : Property<'a>) : PropertyConfig =
    property.generator

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with removing the public access scope. However, I think the access scope should be internal instead of private.

I think we should follow the F# naming guidelines and use Pascal case for record field names.

The most common operation is mapping, not (just) getting. As such, I think we should create a submodule inside Property like this (@dharmaturtle knows what I mean):

module Config =
  let get p = p.Config
  let set p c = { p with Config = c }
  let map f p = { p with Config = f p.Config }


module Property =

let ofGen (x : Gen<Journal * Outcome<'a>>) : Property<'a> =
Property x
let ofGen (gen : Gen<Journal * Outcome<'a>>) : Property<'a> =
Property (PropertyConfig.defaultConfig, gen)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave the PropertyConfig.defaultConfig zero thought.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to audit the use of this function, if it's being used as prop |> toGen |> doGenThing |> ofGen, then we'll lose the original configuration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. All such calls should be replaced with Property.Generator.map (as I structured/named it in another comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about, it a PR before this one, we change Property from a single case discriminated union to a record with a single field as well as expressing all maps over that field as such? Then I think it would easier to verify where PropertyConfig.defaultConfig should be used.


let toGen (Property x : Property<'a>) : Gen<Journal * Outcome<'a>> =
x
let toGen (Property (_, gen) : Property<'a>) : Gen<Journal * Outcome<'a>> =
gen

let tryFinally (after : unit -> unit) (m : Property<'a>) : Property<'a> =
Gen.tryFinally after (toGen m) |> ofGen
Expand Down Expand Up @@ -136,8 +136,8 @@ module Property =
| _, Some tree -> loop (nshrinks + 1<shrinks>) tree
loop 0<shrinks>

let private reportWith' (args : PropertyArgs) (config : PropertyConfig) (p : Property<unit>) : Report =
let random = toGen p |> Gen.toRandom
let private report' (args : PropertyArgs) (Property (config, gen) : Property<unit>) : Report =
let random = gen |> Gen.toRandom

let nextSize size =
if size >= 100 then
Expand Down Expand Up @@ -175,32 +175,19 @@ module Property =

loop args 0<tests> 0<discards>

let reportWith (config : PropertyConfig) (p : Property<unit>) : Report =
let args = PropertyArgs.init
p |> reportWith' args config

let report (p : Property<unit>) : Report =
p |> reportWith PropertyConfig.defaultConfig

let reportBoolWith (config : PropertyConfig) (p : Property<bool>) : Report =
p |> bind ofBool |> reportWith config
let args = PropertyArgs.init
p |> report' args

let reportBool (p : Property<bool>) : Report =
p |> bind ofBool |> report

let checkWith (config : PropertyConfig) (p : Property<unit>) : unit =
reportWith config p
|> Report.tryRaise

let check (p : Property<unit>) : unit =
report p
|> Report.tryRaise

let checkBool (g : Property<bool>) : unit =
g |> bind ofBool |> check

let checkBoolWith (config : PropertyConfig) (g : Property<bool>) : unit =
g |> bind ofBool |> checkWith config
let checkBool (p : Property<bool>) : unit =
p |> bind ofBool |> check

/// Converts a possibly-throwing function to
/// a property by treating an exception as a failure.
Expand All @@ -210,41 +197,24 @@ module Property =
with e ->
handle e

let reportRecheckWith (size : Size) (seed : Seed) (config : PropertyConfig) (p : Property<unit>) : Report =
let reportRecheck (size : Size) (seed : Seed) (p : Property<unit>) : Report =
let args = {
PropertyArgs.init with
RecheckType = RecheckType.None
Seed = seed
Size = size
}
reportWith' args config p

let reportRecheck (size : Size) (seed : Seed) (p : Property<unit>) : Report =
reportRecheckWith size seed PropertyConfig.defaultConfig p

let reportRecheckBoolWith (size : Size) (seed : Seed) (config : PropertyConfig) (p : Property<bool>) : Report =
p |> bind ofBool |> reportRecheckWith size seed config
report' args p

let reportRecheckBool (size : Size) (seed : Seed) (p : Property<bool>) : Report =
p |> bind ofBool |> reportRecheck size seed

let recheckWith (size : Size) (seed : Seed) (config : PropertyConfig) (p : Property<unit>) : unit =
reportRecheckWith size seed config p
|> Report.tryRaise

let recheck (size : Size) (seed : Seed) (p : Property<unit>) : unit =
reportRecheck size seed p
|> Report.tryRaise

let recheckBoolWith (size : Size) (seed : Seed) (config : PropertyConfig) (g : Property<bool>) : unit =
g |> bind ofBool |> recheckWith size seed config

let recheckBool (size : Size) (seed : Seed) (g : Property<bool>) : unit =
g |> bind ofBool |> recheck size seed

let renderWith (n : PropertyConfig) (p : Property<unit>) : string =
reportWith n p
|> Report.render
let recheckBool (size : Size) (seed : Seed) (p : Property<bool>) : unit =
p |> bind ofBool |> recheck size seed

let render (p : Property<unit>) : string =
report p
Expand All @@ -254,9 +224,25 @@ module Property =
reportBool property
|> Report.render

let renderBoolWith (config : PropertyConfig) (p : Property<bool>) : string =
reportBoolWith config p
|> Report.render
/// Set the number of times a property is allowed to shrink before the test
/// runner gives up and displays the counterexample.
let withShrinks (shrinkLimit : int<shrinks>) (Property (config, gen) : Property<'a>) : Property<'a> =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to just take an int instead of an int<shrinks>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on this?

let config = { config with ShrinkLimit = Some shrinkLimit }
Property (config, gen)

/// Restores the default shrinking behavior.
let withoutShrinks (Property (config, gen) : Property<'a>) : Property<'a> =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be called withDefaultShrinks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this, the current name withoutShrinks sounds as if no shrinking will occur.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better name that withDefaultShrinks (which is a syntactic name) is withUnlimitedShrinking (which is a semantic name)

let config = { config with ShrinkLimit = None }
Property (config, gen)

/// Set the number of times a property should be executed before it is
/// considered successful.
let withTests (testLimit : int<tests>) (Property (config, gen) : Property<'a>) : Property<'a> =
let config = { config with TestLimit = testLimit }
Property (config, gen)

let withConfig (config : PropertyConfig) (Property (_, gen) : Property<'a>) : Property<'a> =
Property (config, gen)

[<AutoOpen>]
module PropertyBuilder =
Expand Down
6 changes: 3 additions & 3 deletions tests/Hedgehog.Linq.Tests/LinqTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using Xunit;

// Import ForAll:
Expand Down Expand Up @@ -118,7 +118,7 @@ from y in ForAll(Gen.FromValue(false))
where y == false
select Assert.True(x && !y);

property.Check(PropertyConfig.Default.WithTests(20));
property.WithTests(20).Check();
}

[Fact]
Expand All @@ -131,7 +131,7 @@ from y in ForAll(Gen.FromValue(false))
where y == false
select x && !y;

property.Check(PropertyConfig.Default.WithTests(20));
property.WithTests(20).Check();
}

[Fact]
Expand Down
5 changes: 3 additions & 2 deletions tests/Hedgehog.Tests/PropertyTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module Hedgehog.Tests.PropertyTests
module Hedgehog.Tests.PropertyTests

open Hedgehog
open Expecto
Expand All @@ -11,7 +11,8 @@ let propertyTests = testList "Property tests" [
let! xs = Range.singleton 0 |> Gen.int |> Gen.list (Range.singleton 5) |> Gen.map ResizeArray
return false
}
|> Property.renderWith (PropertyConfig.withShrinks 0<shrinks> PropertyConfig.defaultConfig)
|> Property.withShrinks 0<shrinks>
|> Property.render
Expect.isNotMatch report "\.\.\." "Abbreviation (...) found"

]
7 changes: 2 additions & 5 deletions tests/Hedgehog.Tests/ShrinkTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module Hedgehog.Tests.ShrinkTests
module Hedgehog.Tests.ShrinkTests

open Hedgehog
open TestDsl
Expand Down Expand Up @@ -199,14 +199,11 @@ let shrinkTests = testList "Shrink tests" [
yield! testCases "Property.reportWith respects shrinkLimit"
[ 0<shrinks>; 1<shrinks>; 2<shrinks> ] <| fun shrinkLimit ->

let propConfig =
PropertyConfig.defaultConfig
|> PropertyConfig.withShrinks shrinkLimit
let report =
property {
let! actual = Range.linear 1 1_000_000 |> Gen.int
return actual < 500_000
} |> Property.reportWith propConfig
} |> Property.withShrinks shrinkLimit |> Property.report
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two |> should be on separate lines for readability.

match report.Status with
| Failed failureData ->
failureData.Shrinks =! shrinkLimit
Expand Down