Extensible shrunk report

[martinklepsch]

For tools like collection-check it would be useful to provide some sort of hook to obtain the results of of the quick-check invocation that's part of the checking macro.

This PR allows this extension via the regular clojure.test/report multimethod using namespaced keys similar to how test.check provides additional reporting functionality.

Adding this PR would greatly simplify the implementation of https://github.com/ztellman/collection-check/pull/13

[martinklepsch]

Initially upgraded to test.check 0.9.0 but that broke CI so for now just copied the needed with-test-out*. Should be removed when upgrading test.check. Now tests pass :+1:

[nberger]

I'm not sure about this change. I like the compact shrunk message that it outputs, but perhaps we don't want to introduce a new report dispatch value. I think the same feature could be accomplished by hooking into the :fail and :error dispatch values of clojure.test/report. Both checking and for-all should always report a failure or error there.

What do you think @martinklepsch? Does it make any sense? And of course I'd like to hear what @gfredericks thinks about this :)

[martinklepsch]

Using fail or error will add to the counts resulting in incorrect stats about number of failed tests.

I choose a new dispatch value because that's how test.check reports additional information during test runs and it seemed like an ideal way to allow default implementations but also allow tools to hook in and get the raw data.

What are the downsides of using a new dispatch value? On Sun, 15 Nov 2015 at 06:02, Nicolás Berger notifications@github.com wrote:

I'm not sure about this change. I like the compact shrunk message that it outputs, but perhaps we don't want to introduce a new report dispatch value. I think the same feature could be accomplished by hooking into the :fail and :error dispatch values of clojure.test/report. Both checking and for-all should always report a failure or error there.

What do you think @martinklepsch https://github.com/martinklepsch? Does it make any sense? And of course I'd like to hear what @gfredericks https://github.com/gfredericks thinks about this :)

— Reply to this email directly or view it on GitHub https://github.com/gfredericks/test.chuck/pull/35#issuecomment-156780872 .

[nberger]

Using fail or error will add to the counts resulting in incorrect stats about number of failed tests.

Do you mean it will not add to the counts? I mean, it currently adds to the counts, so I guess you mean that by overriding it we would lose those counts? If that's the case, we can do something like what test.check is doing in https://github.com/clojure/test.check/blob/ed0e180ce3d50d1125dc122871426bc2a8cda1fc/src/main/clojure/clojure/test/check/clojure_test.cljc#L95-L100 which is to take the current function before overriding it so we can call it as part of the new one. It might not be beautiful, but it should work.

I choose a new dispatch value because that's how test.check reports additional information during test runs and it seemed like an ideal way to allow default implementations but also allow tools to hook in and get the raw data.

What are the downsides of using a new dispatch value?

I think it's just that it becomes part of the api, so we should commit on supporting the dispatch value in the long-term. Even if we are going to support it, there's one thing that is missing: It's not being reported when there's an error in a test (that's an exception not in an assertion). So in a test like the following, shrunk will not be reported:

(deftest this-test-should-crash-and-be-caught
  (checking "an int is zero or one" 100 [i gen/int]
    ;; going for uncaught-error-not-in-assertion here
    (case i ; will throw when not in #{0 1}
      0 :zero
      1 :one)))

(it's not the best example of a test that throws not in an assertion. It doesn't even have an assertion, but I hope you can think of a test where this makes sense :) )

[nberger]

Initially upgraded to test.check 0.9.0 but that broke CI so for now just copied the needed with-test-out*. Should be removed when upgrading test.check. Now tests pass :+1:

By the way, the issue that was breaking the tests on 0.9.0 was fixed, so master is on 0.9.0 now :)

[gfredericks]

I don't have time to look at this in detail today, but it sounds like there's a lot of overlap what the stuff I want to do to the quick-check function in test.check proper (i.e., adding lots of events that can be handled in custom ways), so I mainly want to make sure we're not committing to anything that would end up wanting to be implemented differently once test.check has more support for this stuff.

I'll give it a closer look soon. Thanks for all the work @martinklepsch.

[martinklepsch]

Integrating this into test.check sounds great as well. Are you thinking of using clojure.test/report like done for trial and shrinking events?

If you provide me with some guidance how you'd like to see this implemented I'd be happy to contribute. On Sun, 15 Nov 2015 at 16:49, Gary Fredericks notifications@github.com wrote:

I don't have time to look at this in detail today, but it sounds like there's a lot of overlap what the stuff I want to do to the quick-check function in test.check proper (i.e., adding lots of events that can be handled in custom ways), so I mainly want to make sure we're not committing to anything that would end up wanting to be implemented differently once test.check has more support for this stuff.

I'll give it a closer look soon. Thanks for all the work @martinklepsch https://github.com/martinklepsch.

— Reply to this email directly or view it on GitHub https://github.com/gfredericks/test.chuck/pull/35#issuecomment-156821876 .

[gfredericks]

I was thinking that the quick-check function would optionally take a handler callback function, similar to the way clojure.test's report works, and it would call that function with various events. Then the clojure.test.check.clojure-test namespace would provide a callback that calls report I think. Or something.

[martinklepsch]

@gfredericks I assume this callback would then also be used to get information on shrinking and trial events? In the clojure-test namespace with an appropriate default report impl?

[gfredericks]

yeah, the callback would be called whenever anything interesting happened at all.

In the meantime, I think these changes look sane, and shouldn't clash with eventual changes in test.check proper, so I'll merge it.

Thanks!

[martinklepsch]

Awesome, thanks!

[gfredericks]

Released in 0.2.2.