Testable.cover would fail to add coverage labels if the condition was always false, leading to false "sufficient coverage" test results. Take the added regression test in PropertySpec as an example:
// CHECK-NEXT: +++ OK, failed as expected.
// CHECK-NEXT: *** Insufficient coverage after 100 tests
// CHECK-NEXT: (only {{[0-9]+}}% never, not {{[0-9]+}}%)
property("Cover reports failures for conditions that never happen") <- forAll { (s : Set<Int>) in
let somethingHappened = false
return (s.count == [Int](s).count).cover(somethingHappened, percentage: 1, label: "something happened")
}.expectFailure
The expected failure never occurs because somethingHappened is always false and, as a result, the `"something happened" label was never added.
This PR fixes the cover logic so that the labels are always upserted, but the stamp is only added if the condition is true. With this change, the above test passes because cover fails the test due to insufficient coverage ("something happened" only 0%, not 1%).
Why merge this pull request?
Fixes the expected behavior of cover in an edge case.
What's worth discussing about this pull request?
Whether the changes to cover could cause unintended side effects, as it's reused in multiple other cases (classify, label, etc.). I'd be willing to add more tests if people think of other scenarios worth checking.
What downsides are there to merging this pull request?
What's in this pull request?
Testable.cover
would fail to add coverage labels if the condition was always false, leading to false "sufficient coverage" test results. Take the added regression test inPropertySpec
as an example:The expected failure never occurs because
somethingHappened
is alwaysfalse
and, as a result, the `"something happened" label was never added.This PR fixes the
cover
logic so that the labels are always upserted, but thestamp
is only added if thecondition
is true. With this change, the above test passes becausecover
fails the test due to insufficient coverage ("something happened" only 0%, not 1%
).Why merge this pull request?
Fixes the expected behavior of
cover
in an edge case.What's worth discussing about this pull request?
Whether the changes to
cover
could cause unintended side effects, as it's reused in multiple other cases (classify
,label
, etc.). I'd be willing to add more tests if people think of other scenarios worth checking.What downsides are there to merging this pull request?
None that I can think of.