search

hedgehogqa / fsharp-hedgehog

Release with confidence, state-of-the-art property testing for .NET.
https://hedgehogqa.github.io/fsharp-hedgehog/
Other
271 stars 31 forks source link

Allow Property.check to work with Property<_> and not just Property<unit> #429

Open cmeeren opened 1 year ago

cmeeren commented 1 year ago

Fluent assertion libraries like FluentAssertions generally make the test implementation return a non-unit value. Property.check and similar require Property<unit>, which means I have to explicitly |> ignore the result of the fluent assertion chain. This explicit ignoring is not necessary if not using properties (for example, xUnit by itself allows test functions to return arbitrary values), and I can't see a good reason why Property.check and similar could not just ignore the generic value and therefore work with any Property<_>.

AlexeyRaga commented 1 year ago

With https://github.com/hedgehogqa/fsharp-hedgehog/issues/431 in mind, the consideration is that these properties may become non-recheckable...

TysonMN commented 12 months ago

I agree in principle, but I think there is a good reason not to do this.

We currently have functions like Property.checkBool that accept Property<bool>, but I am about to remove them. The behavior is for them to essentially call Property.bind, which means such a test will not benefit from efficient rechecking, which is where the code just after the last generated value through the end of the computational expression or through all calls to Property.map will only be run once on the failed input.

After I remove functions like Property.checkBool, then code that used them will no longer compile. Instead, the authors will need to assert that the value of the bool is true. Then their test will benefit from efficient rechecking.

If I implement the suggestion proposed in this issue as well as remove functions like Property.checkBool, then code that used them will no longer compile. Just as above, the authors should assert that the value of the bool is true. However, I am concerned that some might change Property.checkBool to Property.check, which would compile but create a vacuous test (i.e. a test that always passes).

Therefore, I am willing to make the suggested change, but I want to wait until I think nobody is using any of the functions like Property.checkBool.

AlexeyRaga commented 12 months ago

but I want to wait until I think nobody is using any of the functions like Property.checkBool.

I am not sure if this is assertable. Most of .NET code is in private repositories, and most of the devs do not follow issues on GH but instead just use nuget packages.

Maybe leaving these functions but marking them with Obsolete(IsError = true) (with a brief description) attribute would be a good move? This way people will at least have a chance to understand the issue better?

AlexeyRaga commented 12 months ago

But I truly don't have any suggestions on how to fix the return issue. With return or without, the types align, the code compiles, the test runs, but yet it does not behave properly...

TysonMN commented 12 months ago

I am not sure if this is assertable. Most of .NET code is in private repositories, and most of the devs do not follow issues on GH but instead just use nuget packages.

If almost all downloads are for a version without Property.checkBool and friends, then I know that (probably almost) nobody is using any of those functions.

Maybe leaving these functions but marking them with Obsolete(IsError = true) (with a brief description) attribute would be a good move? This way people will at least have a chance to understand the issue better?

I am fine with that. Is this what you prefer? I will go with your preference on this.

But I truly don't have any suggestions on how to fix the return issue. With return or without, the types align, the code compiles, the test runs, but yet it does not behave properly...

...the property/behavior of efficient rechecking (i.e. when rechecking, the non-generation code test code is only executed on the shrunken input?

I will double check if there is anything that can be done about that. I willing to require the use of return. I suppose I can dereplicate computational expression methods.

AlexeyRaga commented 12 months ago

Yes, you can remove Zero from Property.Builder and it will require return.

I would like it to be this way, but I am not sure why it is there in the first place...

AlexeyRaga commented 12 months ago

I am fine with that. Is this what you prefer? I will go with your preference on this.

yes, I would prefer a breaking change (with a message showing how to fix it, which Obsolete can provide) to a situation where team members who know would have to police team members who don't know :)

TysonMN commented 12 months ago

We currently have functions like Property.checkBool that accept Property<bool>, but I am about to remove them.

This is no longer true; these functions will stay. See https://github.com/hedgehogqa/fsharp-hedgehog/issues/419#issuecomment-1636883075 for more information.