Fix testCaseTask to accept a task, not an Async #484
Conversation
Also fix test name conficts
| @@ -1396,57 +1396,61 @@ let taskTests = | |||
| fun ms -> task { return ms.CanWrite ==? true } | |||
| ] | |||
|
|
|||
| testCaseTask "simple" <| task { | |||
There was a problem hiding this comment.
I didn't change any of the tests, just grouped them with testList to fix the name conflict
| @@ -88,23 +88,23 @@ module Tests = | |||
| /// Builds an async test case | |||
| let inline testCaseAsync name test = TestLabel(name, TestCase (Async test,Normal), Normal) | |||
| /// Builds an async test case from a task | |||
| let inline testCaseTask name test = TestLabel(name, TestCase (Async test,Normal), Normal) | |||
| let inline testCaseTask name test = TestLabel(name, TestCase (Async (Async.AwaitTask test),Normal), Normal) | |||
There was a problem hiding this comment.
This was the main fix. testCaseTask and it's variants were expecting an Async instead of a Task
| // ==> "All" | ||
| // ==> "Push" | ||
| // ==> "Release" | ||
| "CheckEnv" |
There was a problem hiding this comment.
I think this is why the build didn't fail it before
|
For some reason these two tests fail on the build server
Perhaps some cross-platform nonsense going on here |
|
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... |
|
Other than the small comment I made, this seems nice! |
|
Hmm. I'm not seeing the comment. |
Expecto.Tests/PerformanceTests.fs
Outdated
| if RuntimeInformation.IsOSPlatform(OSPlatform.Linux) then | ||
| skiptest "Doesn't hold true on this platform for unclear reasons" |
There was a problem hiding this comment.
I think it's appropriate to remove them or write some other test to replace it!
There was a problem hiding this comment.
I was hesitant to remove them, since I don't quite understand the purpose of the tests.
But it also seems like the same functionality is already covered by these tests.
I think I'll just go ahead and remove them
There appears to be other tests on the same functionality anyway https://github.com/haf/expecto/blob/d9c19fbd1d0868ed81b4ab27143f65bd3bec00cb/Expecto.Tests/Tests.fs#L1453
|
👋 I know this review is a little late but these should really take a |
|
That's a very good point. Hmm. I'm not sure how we handle that since it's already out there. |
|
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 |
|
@TheAngryByrd @ratsclub I whipped up a PR with a potential resolution, #492, if you'd be so kind as to provide thoughts |
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.