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

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

Merged
merged 3 commits into from
Nov 24, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Expecto.Tests/PerformanceTests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ let runMD5() = md5.ComputeHash data
let runSHA256() = sha256.ComputeHash data


open System.Runtime.InteropServices;
[<Tests>]
let performance =
testSequenced <| testList "performance cryptography tests" [
Expand All @@ -25,13 +26,17 @@ let performance =
) |> assertTestFailsWithMsgContaining "same"

testCase "sha256 versus md5" (fun _ ->
if RuntimeInformation.IsOSPlatform(OSPlatform.Linux) then
skiptest "Doesn't hold true on this platform for unclear reasons"
Expect.isFasterThan
(runSHA256 >> ignore |> repeat10)
(runMD5 >> ignore |> repeat10)
"SHA256 is faster than MD5 should fail"
) |> assertTestFailsWithMsgContaining "slower"

testCase "md5 versus sha256" <| fun _ ->
if RuntimeInformation.IsOSPlatform(OSPlatform.Linux) then
skiptest "Doesn't hold true on this platform for unclear reasons"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's appropriate to remove them or write some other test to replace it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Expect.isFasterThan
(runMD5 >> ignore |> repeat10)
(runSHA256 >> ignore |> repeat10)
Expand Down
92 changes: 48 additions & 44 deletions Expecto.Tests/Tests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -1396,57 +1396,61 @@ let taskTests =
fun ms -> task { return ms.CanWrite ==? true }
]

testCaseTask "simple" <| task {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change any of the tests, just grouped them with testList to fix the name conflict

Expect.equal 1 1 "1=1"
}

testCaseTask "let" <| task {
let! n = async { return 1 }
Expect.equal n 1 "n=1"
}

testCaseTask "can fail" <| task {
let! n = async { return 2 }
Expect.equal n 1 "n=1"
} |> assertTestFails

testTask "simple" {
do! Task.Delay 1
Expect.equal 1 1 "1=1"
}
testList "testCaseTask" [
testCaseTask "simple" <| task {
Expect.equal 1 1 "1=1"
}

testTask "let" {
let! n = Task.FromResult 23
Expect.equal n 23 "n=23"
}
testCaseTask "let" <| task {
let! n = async { return 1 }
Expect.equal n 1 "n=1"
}

testTask "can fail" {
let! n = Task.FromResult 2
testCaseTask "can fail" <| task {
let! n = async { return 2 }
Expect.equal n 1 "n=1"
} |> assertTestFails
} |> assertTestFails
]

testTask "two" {
let! n = Task.FromResult 2
let! m = Task.FromResult (3*n)
Expect.equal m 6 "m=6"
}
testList "testTask" [
testTask "simple" {
do! Task.Delay 1
Expect.equal 1 1 "1=1"
}

testTask "two can fail" {
let! n = Task.FromResult 2
let! m = Task.FromResult (3*n)
Expect.equal m 7 "m=7"
} |> assertTestFails
testTask "let" {
let! n = Task.FromResult 23
Expect.equal n 23 "n=23"
}

testTask "two can fail middle" {
let! n = Task.FromResult 2
Expect.equal n 3 "n=3"
let! m = Task.FromResult (3*n)
Expect.equal m 6 "m=6"
} |> assertTestFails
testTask "can fail" {
let! n = Task.FromResult 2
Expect.equal n 1 "n=1"
} |> assertTestFails

testTask "inner skip" {
skiptest "skipped"
}
testTask "two" {
let! n = Task.FromResult 2
let! m = Task.FromResult (3*n)
Expect.equal m 6 "m=6"
}

testTask "two can fail" {
let! n = Task.FromResult 2
let! m = Task.FromResult (3*n)
Expect.equal m 7 "m=7"
} |> assertTestFails

testTask "two can fail middle" {
let! n = Task.FromResult 2
Expect.equal n 3 "n=3"
let! m = Task.FromResult (3*n)
Expect.equal m 6 "m=6"
} |> assertTestFails

testTask "inner skip" {
skiptest "skipped"
}
]
]

[<Tests>]
Expand Down
10 changes: 5 additions & 5 deletions Expecto/Expecto.fs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the main fix. testCaseTask and it's variants were expecting an Async instead of a Task

/// Builds a test case that will make Expecto to ignore other unfocused tests
let inline ftestCase name test = TestLabel(name, TestCase (Sync test, Focused), Focused)
/// Builds a test case with cancel that will make Expecto to ignore other unfocused tests
let inline ftestCaseWithCancel name test = TestLabel(name, TestCase (SyncWithCancel test, Focused), Focused)
/// Builds an async test case that will make Expecto to ignore other unfocused tests
let inline ftestCaseAsync name test = TestLabel(name, TestCase (Async test, Focused), Focused)
/// Builds an async test case from a task, that will make Expecto to ignore other unfocused tests
let inline ftestCaseTask name test = TestLabel(name, TestCase (Async test, Focused), Focused)
let inline ftestCaseTask name test = TestLabel(name, TestCase (Async (Async.AwaitTask test), Focused), Focused)
/// Builds a test case that will be ignored by Expecto
let inline ptestCase name test = TestLabel(name, TestCase (Sync test, Pending), Pending)
/// Builds a test case with cancel that will be ignored by Expecto
let inline ptestCaseWithCancel name test = TestLabel(name, TestCase (SyncWithCancel test, Pending), Pending)
/// Builds an async test case that will be ignored by Expecto
let inline ptestCaseAsync name test = TestLabel(name, TestCase (Async test, Pending), Pending)
/// Builds an async test case from a task, that will be ignored by Expecto
let inline ptestCaseTask name test = TestLabel(name, TestCase (Async test, Pending), Pending)
let inline ptestCaseTask name test = TestLabel(name, TestCase (Async (Async.AwaitTask test), Pending), Pending)
/// Test case or list needs to run sequenced. Use for any benchmark code or
/// for tests using `Expect.isFasterThan`
let inline testSequenced test = Sequenced (Synchronous,test)
Expand Down Expand Up @@ -235,8 +235,8 @@ module Tests =
member inline __.TryFinally(p, cf) = task.TryFinally(p, cf)
member inline __.TryWith(p, cf) = task.TryWith(p, cf)
member __.Run f =
let a = async {
do! task.Run f |> Async.AwaitTask
let a = task {
do! task.Run f
}
match focusState with
| Normal -> testCaseTask name a
Expand Down
24 changes: 12 additions & 12 deletions build.fsx
Original file line number Diff line number Diff line change
Expand Up @@ -169,17 +169,17 @@ Target.create "Release" (fun _ ->

Target.create "All" ignore

// "CheckEnv"
// ==> "Release"

// "Clean"
// ==> "BuildExpecto"
// ==> "BuildBenchmarkDotNet"
// ==> "BuildTest"
// ==> "RunTest"
// ==> "Pack"
// ==> "All"
// ==> "Push"
// ==> "Release"
"CheckEnv"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is why the build didn't fail it before

==> "Release"

"Clean"
==> "BuildExpecto"
==> "BuildBenchmarkDotNet"
==> "BuildTest"
==> "RunTest"
==> "Pack"
==> "All"
==> "Push"
==> "Release"

Target.runOrDefaultWithArguments "All"