Fork me on GitHub
#test-check
<
2017-06-20
>
mfikes16:06:22

There is an evidently breaking change in the 0.10.0-alpha1 release, somehow related to the introduction of the result protocol (https://github.com/clojure/test.check/commit/95112167af9636a48d3174ddd03409e8ac752179). Was it intentional that this be a breaking change (does it require clients to update?) Here is a related ticket in the ClojureScript compiler regarding it: https://dev.clojure.org/jira/browse/CLJS-2110

gfredericks16:06:45

@mfikes the issue is spec returning an exception object?

mfikes16:06:11

To be honest, I haven’t sorted out why things are regressing.

gfredericks16:06:53

So spec used a hacky workaround that the result protocol was meant to obviate

mfikes16:06:58

I only know empirically right now based on bisecting. I’ve read the code to at least attempt to grok why, but haven’t gained comprehension yet.

gfredericks16:06:36

So ideally spec should change to the better version, but I don't think I meant it to break. Though I might have

mfikes16:06:06

(I also don’t know if this affects Clojure—my experience with it is with JVM and self-hosted ClojureScript, where it breaks in the same way for both.)

gfredericks16:06:08

I don't have a sense for what parts are changing at what speed 😕

gfredericks16:06:49

And whether it would be bad for spec to require tc alpha

mfikes16:06:49

Cool. If there was no clear intent to be a breaking change, I’ll dig further to see if can suss out what is going on.

gfredericks16:06:20

I expect making it nonbreaking would consist of having t.c continue special-casing return values that are Exceptions

mfikes16:06:56

That helps… I’ll dig deeper—t.c. 0.10.0 is still alpha, after all.

Alex Miller (Clojure team)17:06:34

I would say it’s possible that spec.alpha could depend on an alpha

Alex Miller (Clojure team)17:06:46

but that regardless of that, t.c shouldn’t introduce a breaking change

Alex Miller (Clojure team)17:06:09

and if the semantics need to change, it should have a new name

mfikes17:06:22

FWIW, t.c 0.10.0-alpha1 has a breaking change for Clojure 1.9.0-alpha17 for this case as well:

$ lein repl
nREPL server started on port 55971 on host 127.0.0.1 - 
REPL-y 0.3.7, nREPL 0.2.12
Clojure 1.9.0-alpha17
Java HotSpot(TM) 64-Bit Server VM 1.8.0_131-b11
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
 Javadoc: (javadoc java-object-or-class-here)
    Exit: Control+D or (exit) or (quit)
 Results: Stored in vars *1, *2, *3, an exception in *e

user=> (require '[clojure.spec.alpha :as s]
  #_=>   '[clojure.spec.test.alpha :as st])
nil
user=> (defn ranged-rand [start end] 0)
#'user/ranged-rand
user=> (s/fdef ranged-rand
  #_=>   :args (s/and (s/cat :start int? :end int?)
  #_=>                #(< (:start %) (:end %)))
  #_=>   :ret int?
  #_=>   :fn (s/and #(>= (:ret %) (-> % :args :start))
  #_=>              #(< (:ret %) (-> % :args :end))))
user/ranged-rand
user=> (st/check `ranged-rand)
({:spec #object[clojure.spec.alpha$fspec_impl$reify__1215 0x2af92bb9 "clojure.spec.alpha$fspec_impl$reify__1215@2af92bb9"], :clojure.spec.test.check/ret {:result true, :num-tests 1000, :seed 1497980057359}, :sym user/ranged-rand})

Alex Miller (Clojure team)17:06:22

at a glance I don’t see anything amiss there

mfikes17:06:11

Right, it might not be a breaking change, but an internal bug. Here is what it used to do…

mfikes17:06:17

Internal bug because: It doesn’t return a falsey :result Breaking change because the :result used to be much richer than just a Boolean value, and clients might not know to look for :result-data

Alex Miller (Clojure team)17:06:14

it just looks to me like it found a failing example in the old one but didn’t in the new one

Alex Miller (Clojure team)17:06:37

which might just be due to what the random seed turned up, not a real difference

mfikes17:06:47

Ahh. Right.

mfikes17:06:03

I’ll increase the number of tests to see what occurs.

Alex Miller (Clojure team)17:06:47

I guess maybe it’s surprising that t.c found 1000 successful cases with a hard-coded result in the new one

mfikes17:06:32

True. Seems unlikely to have pulled that off 🙂

Alex Miller (Clojure team)17:06:26

so maybe just looking at (gen/sample (s/gen (:args (s/get-spec user/ranged-rand))) 1000) might be interesting as a before / after

mfikes17:06:17

user=> (gen/sample (s/gen (:args (s/get-spec 'user/ranged-rand))) 10)
((0 1) (-1 0) (-1 2) (0 1) (-23 2) (1 3) (-16 -6) (-92 -10) (-1 2) (2 7))

mfikes17:06:38

(2 7) should fail, right?

mfikes17:06:30

Yep… many don’t include 0

Alex Miller (Clojure team)17:06:44

or (-1 0) - which is the failing example in the old one

Alex Miller (Clojure team)17:06:04

so the generator seems fine

Alex Miller (Clojure team)17:06:14

and next we should suspect the prop checking

mfikes17:06:41

I revised my ranged-rand to prn its args, and also revised the :fn part of the fdef to print its argument, and at least those parts look correct

user=> (st/check `ranged-rand {:clojure.spec.test.check/opts {:num-tests 2}})
-1 1
{:args {:start -1, :end 1}, :ret 0}
-4 -2
{:args {:start -4, :end -2}, :ret 0}
({:spec #object[clojure.spec.alpha$fspec_impl$reify__1215 0x77b18e9a "clojure.spec.alpha$fspec_impl$reify__1215@77b18e9a"], :clojure.spec.test.check/ret {:result true, :num-tests 2, :seed 1497981519034}, :sym user/ranged-rand})

mfikes18:06:37

TL;DR of the above is just reproing it in Clojure

mfikes18:06:30

I suppose there is the twist that there might be an internal bug in t.c (disregarding any potential interface changes)

gfredericks18:06:01

@mfikes regarding the first issue, it definitely looks like I didn't retain the old behavior; an extra clause here should fix it: https://github.com/clojure/test.check/blob/23e1fcc1bb7f3f2d0202ed99eee202a0b1f2c652/src/main/clojure/clojure/test/check/results.cljc#L18

gfredericks18:06:24

I'm assuming cljs has something like Throwable for extending error types, but maybe that's not true?

mfikes18:06:48

It might be possible to extend js/Error or somesuch

mfikes18:06:52

@gfredericks Evidently you can. Not sure what the implications might be.

cljs.user=> (defprotocol Result
       #_=>   (passing? [result])
       #_=>   (result-data [result] "A map of data about the trial."))
nil
cljs.user=> (extend-protocol Result
       #_=>   js/Error
       #_=>   (passing? [this] false)
       #_=>   (result-data [this] (ex-data this)))
#object[Function "function (this$){
var this$__$1 = this;
return cljs.core.ex_data.call(null,this$__$1);
}"]
cljs.user=> (result-data (ex-info "hi" {:a 1}))
{:a 1}

mfikes18:06:10

^ works in JVM and self-hosted ClojureScript

gfredericks18:06:16

@mfikes does it work for (throw "some string")?

mfikes18:06:54

@gfredericks If you extract using ex-message:

cljs.user=> (extend-protocol Result
       #_=>   js/Error
       #_=>   (passing? [this] false)
       #_=>   (result-data [this] (ex-message this)))
#object[Function "function (this$){
var this$__$1 = this;
return cljs.core.ex_message.call(null,this$__$1);
}"]
cljs.user=> (result-data (js/Error. "some string"))
"some string"

mfikes18:06:43

I’ll see what happens if I build a t.c that further extends Result to exceptions

gfredericks18:06:28

I was just concerned that if you throw a string, the thrown thing is not a js/Error and so wouldn't match that clause

mfikes18:06:48

At least for the exception thrown for the case above, further extending does the trick

mfikes18:06:40

cljs.user=> (st/check `ranged-rand)
[{:spec #object[cljs.spec.alpha.t_cljs$spec$alpha5600],
  :clojure.test.check/ret {:result false,
                           :result-data {:a 1},
                           :seed 1497984200982,
                           :failing-size 0,
                           :num-tests 1,
                           :fail [(-1 0)],
                           :shrunk {:total-nodes-visited 0,
                                    :depth 0,
                                    :result false,
                                    :result-data {:a 1},
                                    :smallest [(-1 0)]}},
  :sym cljs.user/ranged-rand,
  :failure false}]
With this kind of extension:
(passing? [this] false)
-  (result-data [this] {}))
+  (result-data [this] {})
+
+  #?(:clj Throwable :cljs js/Error)
+  (passing? [this] false)
+  (result-data [this] {:a 1}))

mfikes18:06:06

Perhaps :failure false is not what you’d want. Hrm.

gfredericks21:06:19

I'll make sure to get another alpha out shortly