search

haf / expecto

A smooth testing lib for F#. APIs made for humans! Strong testing methodologies for everyone!
Apache License 2.0
663 stars 96 forks source link

Fix testCaseTask to accept a task, not an Async #484

Closed farlee2121 closed 7 months ago

farlee2121 commented 7 months ago

Fix testCaseTask to accept a task, not an Async. Also fix test name conflicts.

I noticed this was causing build troubles when I went back to the interactive PR, so I fixed it.

farlee2121 commented 7 months ago

For some reason these two tests fail on the build server

Perhaps some cross-platform nonsense going on here

farlee2121 commented 7 months ago

Not really sure why those cryptography tests exist, there are other tests that verify Expect.isFasterThan.

It doesn't seem like it should be, but the tests failing does seem to be a platform issue. I booted up a codespace and got the same result as the build server...

ratsclub commented 7 months ago

Other than the small comment I made, this seems nice!

farlee2121 commented 7 months ago

Hmm. I'm not seeing the comment.

TheAngryByrd commented 3 months ago

👋 I know this review is a little late but these should really take a unit -> Task or unit -> Task<unit>. Problem with just using task, is they are hot meaning it will run immediately. This will be a problem when using p on testLists and such.

farlee2121 commented 3 months ago

That's a very good point. Hmm. I'm not sure how we handle that since it's already out there. It just went out hours ago though, so no one should have ingrained it into their code yet.

farlee2121 commented 3 months ago

Looking a bit closer, this actually represents a breaking change to testTask. It previously was properly wrapping the task in an async that would defer execution.

let a = async {
        do! task.Run f |> Async.AwaitTask
    }

In that light, I think the lesser evil here is to quickly release a patch update. It breaks the new endpoints we just introduced, but fixes the defect in testTask and aligns testCaseTask with the previous expected behavior

farlee2121 commented 3 months ago

@TheAngryByrd @ratsclub I whipped up a PR with a potential resolution, #492, if you'd be so kind as to provide thoughts