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

boolTask<bool>, PropertyTask<Property> #684

Open
brianrourkeboll opened this issue Oct 7, 2024 · 4 comments · May be fixed by #694
Open

boolTask<bool>, PropertyTask<Property> #684

brianrourkeboll opened this issue Oct 7, 2024 · 4 comments · May be fixed by #694

Comments

@brianrourkeboll
Copy link

brianrourkeboll commented Oct 7, 2024

The behavior added in #634 to address #633 feels confusing to me when contrasted with the behavior of tests that return Task<bool>, especially for xUnit/NUnit tests annotated with PropertyAttribute.

That is:

  1. Tests marked with PropertyAttribute that return bool fail when the bool is false.
  2. Tests marked with PropertyAttribute that return Task<bool> fail when the bool is false.
  3. Tests marked with PropertyAttribute that return Property fail when the Property is falsy.
  4. Tests marked with PropertyAttribute that return Task<Property> pass when the Property is falsy.

I would expect 4 to fail just as 1–3 do.

// Fails as expected.
[Property]
public bool Test(bool b) =>
    b && !b;

// Fails as expected.
[Property]
public Property Test(bool b) =>
    Prop.Label(b && !b, "Some label.");

// Fails as expected.
[Property]
public Task<bool> Test(bool b) =>
    Task.FromResult(b && !b);

// Passes. I would expect this to fail.
[Property]
public Task<Property> Test(bool b) =>
    Task.FromResult(Prop.Label(b && !b, "Some label."));

// Passes. I would expect this to fail.
[Property]
public Task<Property> Test(bool b) =>
    Task.FromResult(Prop.ToProperty(b).And(!b));
// Fails as expected.
[<Property>]
let test1 b =
    b && not b

// Fails as expected.
[<Property>]
let test2 b =
    (b && not b) |> Prop.label "Some label."

// Fails as expected.
[<Property>]
let test3 b =
    Task.FromResult (b && not b)

// Passes. I would expect this to fail.
[<Property>]
let test4 b =
    Task.FromResult ((b && not b) |> Prop.label "Some label.")

// Passes. I would expect this to fail.
[<Property>]
let test5 b =
    Task.FromResult (b .&. not b)

I understand the original desire not to need to explicitly upcast Task<unit> to Task, but the difference in treatment between Task<bool> and Task<Property> is dangerous, because it is far too easy to write a test that returns Task<Property> and have it pass for the wrong reason.

That is, a developer might expect the test to fail if the Property inside the returned Task<Property> was falsy, just as such a test would fail for a false or falsy bool, Task<bool>, or Property — but the test returning Task<Property> would actually always pass as long as no exceptions were thrown.

Is there is any chance we could make Task<Property> behave like Task<bool> and Property here? Or, ideally, Task<'T> when 'T is testable behave like 'T?

I'd be willing to open a PR.

CC @ploeh, @kurtschelfthout.

@kurtschelfthout
Copy link
Member

kurtschelfthout commented Oct 8, 2024 via email

@ploeh
Copy link
Member

ploeh commented Oct 8, 2024

I agree that the behaviour reported here looks undesirable.

@brianrourkeboll
Copy link
Author

Hmm.

Given the current design (originally added in 3e90689), it seems obvious that it is impossible to create a Property purely from a Task<'Testable> or a Task<Property> without either: (1) having a recursive relationship between Testable.fs and Runner.fs; (2) having a recursive relationship between Property and ResultContainer, and a way for ResultContainer.Future to represent an unevaluated Property; or (3) lifting the Task<_> even higher (e.g., type Property = Property of (IArbMap -> Gen<Shrink<ResultContainer>>) | Future of Task<(IArbMap -> Gen<Shrink<ResultContainer>>)>).

Something like this, while it works for the trivial examples I have tried, is surely wrong because of the call to Shrink.getValue and the ignoring of any remaining shrinks:

static member TaskGeneric() =
    { new ITestable<Task<'T>> with
        member __.Property t =
            Property (fun arbMap ->
                Gen.promote (fun runner ->
                    Shrink.ofValue (Future (
                        t.ContinueWith (fun (t : Task<'T>) ->
                            match t.IsCanceled, t.IsFaulted with
                            | _, true -> Task.FromResult (Res.failedException t.Exception)
                            | true, _ -> Task.FromResult Res.failedCancelled
                            | false, false ->
                                let prop = property t.Result
                                let gen = Property.GetGen arbMap prop
                                let shrink = runner gen
            
                                match Shrink.getValue shrink with
                                | Value result, _ -> Task.FromResult result
                                | Future resultTask, _ -> resultTask)
                        |> _.Unwrap())))) }

Or am I missing something or failing to do the proper type Tetris?

@kurtschelfthout
Copy link
Member

Yes, I think your analysis is essentially correct. I remember something about struggling with this issue - ResultContainer was probably part of trying to break the recursive relationship, before F# properly supported recursive modules. My memory is very hazy on the whole thing though...if you see a reasonable way through I am happy to defer to your judgement.

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

Successfully merging a pull request may close this issue.

3 participants