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

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

Open
cmeeren opened this issue Jun 16, 2023 · 8 comments
Open

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Jun 16, 2023

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
Copy link

With #431 in mind, the consideration is that these properties may become non-recheckable...

@TysonMN
Copy link
Member

TysonMN commented Jul 15, 2023

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
Copy link

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
Copy link

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
Copy link
Member

TysonMN commented Jul 15, 2023

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
Copy link

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
Copy link

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
Copy link
Member

TysonMN commented Jul 15, 2023

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 #419 (comment) for more information.

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