Using `Collect` and `Classify` in conjunction with `And` or `Or`

[OkkeHendriks]

Hello,

When I try to collect and /or classify two or more properties only the left hand side of the and or or operator seems to propagate its statistics.
I am not sure if this is a bug or intended behavior?
Using FsCheck 2.16.5.

open FsCheck

let prop1 = 1 = 1 |@ "1 = 1" |> Prop.collect "Property 1" |> Prop.classify true "P1"
let prop2 = 1 <> 2 |@ "1 <> 2" |> Prop.collect "Property 2" |> Prop.classify true "P2"

printfn "\nprop1"
prop1 |> Check.Quick
printfn "\nprop2"
prop2 |> Check.Quick

printfn "\nprop1 .&. prop2"
prop1 .&. prop2 |> Check.Quick
printfn "\nprop2 .&. prop1"
prop2 .&. prop1 |> Check.Quick

printfn "\nprop1 .|. prop2"
prop1 .|. prop2 |> Check.Quick

printfn "\nprop2 .|. prop1"
prop2 .|. prop1 |> Check.Quick

Output:

prop1
Ok, passed 100 tests (100% P1, "Property 1").

prop2
Ok, passed 100 tests (100% P2, "Property 2").

prop1 .&. prop2
Ok, passed 100 tests (100% P1, "Property 1").

prop2 .&. prop1
Ok, passed 100 tests (100% P2, "Property 2").

prop1 .|. prop2
Ok, passed 100 tests (100% P1, "Property 1").

prop2 .|. prop1
Ok, passed 100 tests (100% P2, "Property 2").

If I add a failing prop3 1 = 3, the correct label is shown:

let prop3 = 1 = 3 |@ "1 = 3" |> Prop.collect "Property 3" |> Prop.classify true "P3"

printfn "\nprop1 .&. prop2  .&. prop3"
prop1 .&. prop2 .&. prop3 |> Check.Quick
printfn "\nprop3 .&. prop1  .&. prop2"
prop3 .&. prop1 .&. prop2 |> Check.Quick

Output:

prop1 .&. prop2  .&. prop3
Falsifiable, after 1 test (0 shrinks) (StdGen (674262175, 297181018)):
Label of failing property: 1 = 3
Original:


prop3 .&. prop1  .&. prop2
Falsifiable, after 1 test (0 shrinks) (StdGen (675076347, 297181018)):
Label of failing property: 1 = 3
Original:

But this happens probably because it 'fails fast'.

[kurtschelfthout]

Hm yes good point. Probably a bug here:

https://github.com/fscheck/FsCheck/blob/master/src/FsCheck/Testable.fs#L22

Some (all?) of those match arms should merge labels. (labels is where things like collect and classify add a string per test case)

[OkkeHendriks]

Ran into this one again today.
I think the labels from all Outcomes which 'matter' should be combined.

I.e. for and this would imply combining the labels from both left and right (when Outcome.Passed), but for or we are only interested in the labels from the valid (Outcome.Passed) property or properties?

Do the labels only really matter in case the total combined property is Outcome.Passed?

[kurtschelfthout]

I.e. for and this would imply combining the labels from both left and right (when Outcome.Passed), but for or we are only interested in the labels from the valid (Outcome.Passed) property or properties?

Yes, that sounds right.

[rynoV]

How about this

--- a/src/FsCheck/Testable.fs
+++ b/src/FsCheck/Testable.fs
@@ -21,23 +21,26 @@ open FsCheck.Internals
     {   Outcome     : Outcome
         Stamp       : list<string>
         Labels      : Set<string>
         Arguments   : list<obj> } 
     ///Returns a new result that is Passed if and only if both this
     ///and the given Result are Passed.
     static member internal ResAnd(l, r) = 
         match (l.Outcome,r.Outcome) with
         | (Outcome.Failed _,_) -> l //here a potential Failed in r is thrown away...
         | (_,Outcome.Failed _) -> r
-        | (_,Outcome.Passed) -> l
-        | (Outcome.Passed,_) -> r
-        | (Outcome.Rejected,Outcome.Rejected) -> l //or r, whatever
+        | (Outcome.Rejected,_) -> l
+        | (_,Outcome.Rejected) -> r
+        | (Outcome.Passed,Outcome.Passed) ->
+            { l with
+                Stamp = List.append l.Stamp r.Stamp
+                Labels = Set.union l.Labels r.Labels }
     static member internal ResOr(l, r) =
         match (l.Outcome, r.Outcome) with
         | (Outcome.Failed _,_) -> r
         | (_,Outcome.Failed _) -> l
         | (Outcome.Passed,_) -> l
         | (_,Outcome.Passed) -> r
         | (Outcome.Rejected,Outcome.Rejected) -> l //or r, whatever

I don't think OR needs any change, the current code should keep the stamps from the first passing property, which I think would be the desired behaviour, to mimic short-circuiting?

[kurtschelfthout]

I think we should merge in all cases, including or. One use of these stats is determining what sort of data coverage generator and tests are getting. since FsCheck is not short circuiting, we shouldn’t pretend it is. On 4 Mar 2025, at 23:35, Calum Sieppert ***@***.***> wrote: How about this --- a/src/FsCheck/Testable.fs +++ b/src/FsCheck/Testable.fs @@ -21,23 +21,26 @@ open FsCheck.Internals { Outcome : Outcome Stamp : list<string> Labels : Set<string> Arguments : list<obj> } ///Returns a new result that is Passed if and only if both this ///and the given Result are Passed. static member internal ResAnd(l, r) = match (l.Outcome,r.Outcome) with | (Outcome.Failed _,_) -> l //here a potential Failed in r is thrown away... | (_,Outcome.Failed _) -> r - | (_,Outcome.Passed) -> l - | (Outcome.Passed,_) -> r - | (Outcome.Rejected,Outcome.Rejected) -> l //or r, whatever + | (Outcome.Rejected,_) -> l + | (_,Outcome.Rejected) -> r + | (Outcome.Passed,Outcome.Passed) -> + { l with + Stamp = List.append l.Stamp r.Stamp + Labels = Set.union l.Labels r.Labels } static member internal ResOr(l, r) = match (l.Outcome, r.Outcome) with | (Outcome.Failed _,_) -> r | (_,Outcome.Failed _) -> l | (Outcome.Passed,_) -> l | (_,Outcome.Passed) -> r | (Outcome.Rejected,Outcome.Rejected) -> l //or r, whatever I don't think OR needs any change, the current code should keep the stamps from the first passing property, which I think would be the desired behaviour, to mimic short-circuiting?—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***> rynoV left a comment (fscheck/FsCheck#629) How about this --- a/src/FsCheck/Testable.fs +++ b/src/FsCheck/Testable.fs @@ -21,23 +21,26 @@ open FsCheck.Internals { Outcome : Outcome Stamp : list<string> Labels : Set<string> Arguments : list<obj> } ///Returns a new result that is Passed if and only if both this ///and the given Result are Passed. static member internal ResAnd(l, r) = match (l.Outcome,r.Outcome) with | (Outcome.Failed _,_) -> l //here a potential Failed in r is thrown away... | (_,Outcome.Failed _) -> r - | (_,Outcome.Passed) -> l - | (Outcome.Passed,_) -> r - | (Outcome.Rejected,Outcome.Rejected) -> l //or r, whatever + | (Outcome.Rejected,_) -> l + | (_,Outcome.Rejected) -> r + | (Outcome.Passed,Outcome.Passed) -> + { l with + Stamp = List.append l.Stamp r.Stamp + Labels = Set.union l.Labels r.Labels } static member internal ResOr(l, r) = match (l.Outcome, r.Outcome) with | (Outcome.Failed _,_) -> r | (_,Outcome.Failed _) -> l | (Outcome.Passed,_) -> l | (_,Outcome.Passed) -> r | (Outcome.Rejected,Outcome.Rejected) -> l //or r, whatever I don't think OR needs any change, the current code should keep the stamps from the first passing property, which I think would be the desired behaviour, to mimic short-circuiting? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>

[brianrourkeboll]

I think we should merge in all cases, including or. One use of these stats is determining what sort of data coverage generator and tests are getting. since FsCheck is not short circuiting, we shouldn’t pretend it is.

Indeed, another use-case is seeing the labels of all failing subproperties when multiple subproperties are composed with AND.