test.check

for-all should support nesting

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test

Description

Haskell QuickCheck allows for-all expressions to nest. This is useful when there are dependencies between generated values. test.check should allow this, too.

Currently, nested for-alls always succeed, which is somewhat pernicious.

I've added a patch that implements this.

Activity

Hide
Reid Draper added a comment -

Thanks Michael. I appreciate the patch, but there's a few design details that could be discussed before we get to code-level detail. As a separate, but related issue, I've been wanting to implement something like your CheckResult type, but as a record with several more fields. These fields would hold things like a plain-text description of any errors found, statistics (ala QuickCheck's collect, classify, etc.). I'd also like to write a protocol that allows basic types like booleans to be turned into this record. This would be analogous to Haskell QuickCheck's Testable Type Class. While this is technically a separate issue, I think it would behoove us to solve it in conjunction with nestable for-alls, particularly since nested for-alls can be simulated by just using bind at the generator level. Does this make sense?

Show
Reid Draper added a comment - Thanks Michael. I appreciate the patch, but there's a few design details that could be discussed before we get to code-level detail. As a separate, but related issue, I've been wanting to implement something like your CheckResult type, but as a record with several more fields. These fields would hold things like a plain-text description of any errors found, statistics (ala QuickCheck's collect, classify, etc.). I'd also like to write a protocol that allows basic types like booleans to be turned into this record. This would be analogous to Haskell QuickCheck's Testable Type Class. While this is technically a separate issue, I think it would behoove us to solve it in conjunction with nestable for-alls, particularly since nested for-alls can be simulated by just using bind at the generator level. Does this make sense?
Hide
Michael Sperber added a comment -

Absolutely.

I personally would start this patch, and work from there, unless you want to do things fundamentally differently rather than add more stuff.

Either way, how can I help make it happen?

Show
Michael Sperber added a comment - Absolutely. I personally would start this patch, and work from there, unless you want to do things fundamentally differently rather than add more stuff. Either way, how can I help make it happen?
Hide
Reid Draper added a comment -

Great question. Let me think on that and get back to you ASAP. I'd also love to make this happen soon.

Show
Reid Draper added a comment - Great question. Let me think on that and get back to you ASAP. I'd also love to make this happen soon.
Hide
Reid Draper added a comment -

Sorry for the delay, here's my sketch I've been working with:

diff --git a/src/main/clojure/clojure/test/check/properties.clj b/src/main/clojure/clojure/test/check/properties.clj
index 99b5222..139ae9a 100644
--- a/src/main/clojure/clojure/test/check/properties.clj
+++ b/src/main/clojure/clojure/test/check/properties.clj
@@ -8,13 +8,47 @@
 ;   You must not remove this notice, or any other, from this software.
 
 (ns clojure.test.check.properties
+  (:import clojure.test.check.generators.Generator)
   (:require [clojure.test.check.generators :as gen]))
 
+(defrecord Result [result pass? message stamps])
+
+(defprotocol ToResult
+  (to-result [a]))
+
+(extend java.lang.Object
+  ToResult
+  {:to-result (fn [b]
+               ;; not checking for caught exceptions here
+               (->Result b (not (false? b)) nil nil))})
+
+(extend nil
+  ToResult
+  {:to-result (fn [b]
+               (->Result b false nil nil))})
+
+(extend java.lang.Boolean
+  ToResult
+  {:to-result (fn [b]
+               (->Result b b nil nil))})
+
+(extend Generator
+  ToResult
+  {:to-result identity})
+
+(extend Result
+  ToResult
+  {:to-result identity})
+
+(defn message
+  [m property]
+  (assoc property :message m))
+
 (defn- apply-gen
   [function]
   (fn [args]
-    (let [result (try (apply function args) (catch Throwable t t))]
-      {:result result
+    (let [result (to-result (try (apply function args) (catch Throwable t t)))]
+      {:result (:result result)
        :function function
        :args args})))
 
@@ -29,9 +63,18 @@
   (for-all* [gen/int gen/int] (fn [a b] (>= (+ a b) a)))
   "
   [args function]
-  (gen/fmap
-    (apply-gen function)
-    (apply gen/tuple args)))
+  (gen/bind
+    (apply gen/tuple args)
+    (fn [a]
+      (let [result ((apply-gen function) a)]
+        (cond (gen/generator? result) (gen/fmap (fn [r] (println "foo") (update-in r :args #(conj % a))) result)
+              ;; NOTE: quick note to myself before I leave this code for the night,
+              ;; this :else is getting hit because we're wrapping the result
+              ;; with a {:result ...} map. Should probably do that conditionally.
+              ;; We also need two result types I think, a result to return from
+              ;; a property itself, and a reuslt that tacks the 'args' on top of this.
+              :else (do (println "bar") (gen/return result)))))
+    ))
 
 (defn binding-vars
   [bindings]
Show
Reid Draper added a comment - Sorry for the delay, here's my sketch I've been working with:
diff --git a/src/main/clojure/clojure/test/check/properties.clj b/src/main/clojure/clojure/test/check/properties.clj
index 99b5222..139ae9a 100644
--- a/src/main/clojure/clojure/test/check/properties.clj
+++ b/src/main/clojure/clojure/test/check/properties.clj
@@ -8,13 +8,47 @@
 ;   You must not remove this notice, or any other, from this software.
 
 (ns clojure.test.check.properties
+  (:import clojure.test.check.generators.Generator)
   (:require [clojure.test.check.generators :as gen]))
 
+(defrecord Result [result pass? message stamps])
+
+(defprotocol ToResult
+  (to-result [a]))
+
+(extend java.lang.Object
+  ToResult
+  {:to-result (fn [b]
+               ;; not checking for caught exceptions here
+               (->Result b (not (false? b)) nil nil))})
+
+(extend nil
+  ToResult
+  {:to-result (fn [b]
+               (->Result b false nil nil))})
+
+(extend java.lang.Boolean
+  ToResult
+  {:to-result (fn [b]
+               (->Result b b nil nil))})
+
+(extend Generator
+  ToResult
+  {:to-result identity})
+
+(extend Result
+  ToResult
+  {:to-result identity})
+
+(defn message
+  [m property]
+  (assoc property :message m))
+
 (defn- apply-gen
   [function]
   (fn [args]
-    (let [result (try (apply function args) (catch Throwable t t))]
-      {:result result
+    (let [result (to-result (try (apply function args) (catch Throwable t t)))]
+      {:result (:result result)
        :function function
        :args args})))
 
@@ -29,9 +63,18 @@
   (for-all* [gen/int gen/int] (fn [a b] (>= (+ a b) a)))
   "
   [args function]
-  (gen/fmap
-    (apply-gen function)
-    (apply gen/tuple args)))
+  (gen/bind
+    (apply gen/tuple args)
+    (fn [a]
+      (let [result ((apply-gen function) a)]
+        (cond (gen/generator? result) (gen/fmap (fn [r] (println "foo") (update-in r :args #(conj % a))) result)
+              ;; NOTE: quick note to myself before I leave this code for the night,
+              ;; this :else is getting hit because we're wrapping the result
+              ;; with a {:result ...} map. Should probably do that conditionally.
+              ;; We also need two result types I think, a result to return from
+              ;; a property itself, and a reuslt that tacks the 'args' on top of this.
+              :else (do (println "bar") (gen/return result)))))
+    ))
 
 (defn binding-vars
   [bindings]
Hide
Michael Sperber added a comment -

Looks OK. However, it's difficult to see why that would get you more quickly where you said you want to go than my patch ...

Show
Michael Sperber added a comment - Looks OK. However, it's difficult to see why that would get you more quickly where you said you want to go than my patch ...
Hide
Reid Draper added a comment -

Looks OK. However, it's difficult to see why that would get you more quickly where you said you want to go than my patch ...

Fair enough. Part of this it that it was easier for me to write up a sketch. The main things I'm trying to cover when supporting nested generators are making sure:

  1. We also support the upcoming ability to collect and return statistics about a test, ala collect from Haskell QuickCheck
  2. We have a sane way of returning failing tests to a user. Right now, in the :fail and :smallest keys of the returned map, we tell the user the failing arguments. They're always wrapped in at least one vector, since you may use more than one generator using prop/for-all. What do we do with nested properties? How do we distinguish between multiple generators at the 'same level', vs nested properties? Or do we not need to distinguish? Can whatever we decide to do be backward compatible?

Point being, I want to make sure we're not committing ourselves to nested-properties until we have some of those answers, and for me personally, it's easier to try and play with these things together, and see how they will fit together.

Show
Reid Draper added a comment -
Looks OK. However, it's difficult to see why that would get you more quickly where you said you want to go than my patch ...
Fair enough. Part of this it that it was easier for me to write up a sketch. The main things I'm trying to cover when supporting nested generators are making sure:
  1. We also support the upcoming ability to collect and return statistics about a test, ala collect from Haskell QuickCheck
  2. We have a sane way of returning failing tests to a user. Right now, in the :fail and :smallest keys of the returned map, we tell the user the failing arguments. They're always wrapped in at least one vector, since you may use more than one generator using prop/for-all. What do we do with nested properties? How do we distinguish between multiple generators at the 'same level', vs nested properties? Or do we not need to distinguish? Can whatever we decide to do be backward compatible?
Point being, I want to make sure we're not committing ourselves to nested-properties until we have some of those answers, and for me personally, it's easier to try and play with these things together, and see how they will fit together.
Hide
Lars Andersen added a comment -

I don't really want to have to nest for-all, I'd much rather prefer it work like let where we can refer to previous values. That said, no matter which solution, this is my biggest gripe with test.check at the moment, so any solution would be preferable to another year of hammock time.

Here's one solution, taken from the wild. I'm sure there are many others, people want this badly, but this one was taken from Nathan Marz's specter library:

(defmacro for-all+ [bindings & body]
  (let [parts (partition 2 bindings)
        vars (vec (map first parts))
        genned (reduce
                (fn [curr [v code]]
                  `(gen/bind ~code (fn [~v] ~curr)))
                `(gen/return ~vars)
                (reverse parts))]
    `(prop/for-all [~vars ~genned]
                   ~@body )))
Show
Lars Andersen added a comment - I don't really want to have to nest for-all, I'd much rather prefer it work like let where we can refer to previous values. That said, no matter which solution, this is my biggest gripe with test.check at the moment, so any solution would be preferable to another year of hammock time. Here's one solution, taken from the wild. I'm sure there are many others, people want this badly, but this one was taken from Nathan Marz's specter library:
(defmacro for-all+ [bindings & body]
  (let [parts (partition 2 bindings)
        vars (vec (map first parts))
        genned (reduce
                (fn [curr [v code]]
                  `(gen/bind ~code (fn [~v] ~curr)))
                `(gen/return ~vars)
                (reverse parts))]
    `(prop/for-all [~vars ~genned]
                   ~@body )))
Hide
Gary Fredericks added a comment -

We've been thinking about a similar problem in TCHECK-81, and in the meantime there's an even-fancier variant of for-all here.

Show
Gary Fredericks added a comment - We've been thinking about a similar problem in TCHECK-81, and in the meantime there's an even-fancier variant of for-all here.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: