Closed lackita closed 9 years ago
I thought about that some when I was implementing it, but what I realized is that, even with parallelization the binary search finding the smallest failure will still need to be sequential. Could still break it, but you're right that we'd have to wait until test.check changes to know how to avoid the problem.
On Thu, Jan 29, 2015 at 2:15 PM, Gary Fredericks notifications@github.com wrote:
Thanks! This is pretty cool; I've wanted something like this many times before.
The usage of with-redefs with the atom makes me wonder what will happen if/when test.check starts supporting parallelize tests. But I assume that's an implementation detail that can be messed with once we know more what the changes to test.check will be.
— Reply to this email directly or view it on GitHub https://github.com/gfredericks/test.chuck/pull/4#issuecomment-72085572.
You're saying that shrinking can't be parallelized? I don't think that's the case, but even still just running several tests in parallel would mean the're all reporting to the same global atom, I believe.
Yeah, that's a good point, things will probably get muddled in the atom if they're all running in parallel.
I was just thinking the other day that I had never in my life needed to use with-local-vars
but maybe this would be a sensible place for it
Toyed around with with-local-vars
, but I still found state management pretty confusing. Then I realized that an agent that gets sent save-to-final-reports
was a much better fit for managing state in this case.
I've made that change, does it look better to you too?
I just noticed there were two atoms;
The atom I was referring to originally is the one on line 10, and the problem with it has to do with how with-redefs
works (which is globally).
This should illustrate the problem:
(defn report [x] (println x))
(defmacro capture-reports
[& body]
`(let [reports# (atom [])]
(with-redefs [report (fn [x#] (swap! reports# conj x#))]
~@body
@reports#)))
(defn report-foo-bar
[]
(capture-reports
(report :foo)
(report :bar)))
(report-foo-bar) => [:foo :bar] ;; good
(let [captured (atom [])]
(dotimes [_ 100]
(.start (Thread. (fn [] (swap! captured conj (report-foo-bar))))))
(Thread/sleep 1000)
(frequencies @captured))
=> {[] 5, [:bar] 2, [:foo] 2, [:foo :bar] 90, [:foo :bar :foo] 1} ;; whoops
Alright, with-local-vars
addresses the problem in your example, but when applied to my original code complains about not being able to let fully qualified names. with-bindings
, on the other hand, doesn't throw that error and also seems to address the concurrency problem. I've pushed that change.
I can't figure out how you would've gotten code to run with with-bindings
-- it's supposed to take a map with dynamic vars, so something like this can't work:
(with-bindings [first identity] (first [1 2 3]))
;; ClassCastException clojure.core$first__4091 cannot be cast to clojure.lang.IMapEntry clojure.lang.Var.pushThreadBindings (Var.java:317)
the fact that the tests pass is completely baffling me; trying the code out locally to debug my understanding.
Yeah, I've got a couple more tests that intentionally fail, I'm going to try it out on those and see what happens. I can't find any good examples of with-bindings, do you know of any?
Well I don't know of any reason to use with-bindings instead of just binding, but neither one is useful without an actual ^:dynamic
var to use it with, which I don't think you have; so I'm not sure what you were expecting to happen actually; what did your with-local-vars
code look like?
It looks like what's happening is it's always passing, the error must be caught and ignored silently.
It was just replacing with-redefs
with with-local-vars
:
(defmacro capture-reports [body]
`(let [reports# (atom [])]
(with-redefs [report #(swap! reports# conj %)]
~@body)
@reports#))
But I get this error:
Can't let qualified name: clojure.test/report
yeah I just confirmed that the with-binding
fails the way I'd expect it to, but the test orchestration code doesn't notice
binding
seems to do what I'd expect, do you know of any problems with that?
actually, no, not quite, it has the same localization problem with-redefs
had
binding
can only work if the underlying var is ^:dynamic
which I was about to say doesn't apply to clojure.test/report
but in fact I was wrong about that
so I'm thinking binding
might be the way to go -- what problem did you just observe with it?
user> (defmacro capture-reports
[& body]
`(let [reports# (atom [])]
(binding [report (fn [x#] (swap! reports# conj x#))]
~@body
@reports#)))
#'user/capture-reports
user> (let [captured (atom [])]
(dotimes [_ 100]
(.start (Thread. (fn [] (swap! captured conj (report-foo-bar))))))
(Thread/sleep 1000)
(frequencies @captured))
{[:foo :bar] 98, [:foo] 1, [:bar :foo] 1}
yeah I just did that myself and was confused, but you have to recompile report-foo-bar
too, then it works
(when you change a macro, you have to recompile all the uses of the macro to have the change be completely effective)
Okay so I think using binding
with your fd981d6 version of the code is the first thing we want (I don't think there's any need for the agent change, correct me if I'm wrong); then the second thing is to figure out what it is about the orchestration code was swallowing exceptions.
I think it should be possible to add a test that catches the exception-swallowing problem
Ah, that would explain why my with-bindings
experiments passed.
OK, I'll see if I can add a test for exception-swallowing after work today. In the meantime, I've pushed the binding change.
I'll also take out the agent, as I can resolve the race condition just as easily with swap!
if I keep the restructuring of save-to-final-reports
from the agent change.
I haven't figured out how to test for the uncaught exception, but I did figure out how to report it.
So other than improving test coverage, am I missing anything?
I think this test catches the problem: https://www.refheap.com/96731
I can add that myself though. I think everything else is good. There are a few awkward feeling things that might be able to be improved at some point (e.g., failures get reported twice it looks like), but this definitely seems useful as-is.
Thanks again!
Thanks again for all the advice, hope you find it useful.
FYI I added an alternate macro for when (for some reason) you'd rather use defspec
than deftest
:
Thanks! This is pretty cool; I've wanted something like this many times before.
The usage of
with-redefs
with the atom makes me wonder what will happen if/when test.check starts supporting parallelize tests. But I assume that's an implementation detail that can be messed with once we know more what the changes to test.check will be.