Closed bhb closed 7 years ago
bhb
commented
7 years ago Or maybe just the fact that this issue exists and should show up in Google if someone searches for
karma doo karma-reporter test.chuck
is enough and we don't need to make any doc changes 😄
gfredericks
commented
7 years ago What if karma-reporter had an extension point for handling unrecognized events?
El 6 ene. 2017 16:55, "Ben Brinckerhoff" notifications@github.com escribió:
This isn't a bug with test.chuck, just a surprising result and I'm wondering how I can help others avoid it.
Repro:
;; core_test.cljs (ns test-chuck-bug-figwheel.core-test (:require [cljs.test :refer [deftest is]] [com.gfredericks.test.chuck.clojure-test :refer-macros [checking]] [clojure.test.check.generators :as gen]))
(deftest fake-test (checking "property that should fail" 12 [s gen/string] (is (< (count s) 5))))
(comment ;; running this prints out the minimal failure (cljs.test/run-tests) ;; running this does not print out the minimal failure (or anything, since reporter defines no reports) (cljs.test/run-tests (cljs.test/empty-env ::my-reporter)) )
Context: My team recently switched to using doo with karma. I later noticed that our tests were not printing out the shrunk results. After some investigation, I realized that karma is using a custom reporter https://github.com/honzabrecka/karma-reporter/blob/master/src/jx/reporter/karma.clj#L20 so the following multimethod is never called:
;; https://github.com/gfredericks/test.chuck/blob/master/src/com/gfredericks/test/chuck/clojure_test.cljc#L23-L26 (defmethod ct/report #?(:clj ::shrunk :cljs [::ct/default ::shrunk]) [m] (newline) (println "Tests failed, smallest case:" (pr-str (-> m :shrunk :smallest)) "\nSeed" (:seed m)))
This took me some time to track down, although I don't see any behavior that is buggy
- it seems reasonable for karma to use a custom reporter
- it seems reasonable for test.chuck to use the reporter mechanism to print out shrunk results, so the user can override
I started to wonder if test.chuck could provide a warning, but that would be very annoying if a user provided a custom reporter and really did not want to implement the report for shrunk results.
Perhaps the only thing we can do is add some documentation here https://github.com/gfredericks/test.chuck#alternate-clojuretest-integration. I'm happy to assist with a PR, but I wanted to discuss here first. What do you think?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gfredericks/test.chuck/issues/52, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIRE_pM2HG-Do0fgYJ5RTYs0n5E9st5ks5rPsZDgaJpZM4LdM5B .
bhb
commented
7 years ago Can you talk a little more about this? I'm afraid I don't understand.
test.chuck defines the report multimethod for the key [:cljs.test/default :com.gfredericks.test.chuck.clojure-test/shrunk]. Do you mean that karma-reporter could inspect all definitions for report (i.e. (methods cljs.test/report)), see if any are defined for reporters other than :jx.reporter.karma/karma, and give a warning?
gfredericks
commented
7 years ago I misunderstood the what you meant by "custom reporter" -- it looks like karma is just adding its own defmethods for clojure.test/report, so now I don't understand why test.chuck's implementation wouldn't be called.
I originally thought that karma was using with-redefs or something to redefine clojure.test/report and that it could use a multimethod that users could extend if they wanted to tie these two libraries together. But if clojure.test/report's multimethod is still being used then I don't see why users wouldn't be able to redefine things in whatever manner is necessary to make everything work.
But I still don't understand why it's not working in the first place.
bhb
commented
7 years ago But I still don't understand why it's not working in the first place.
Here is what I think is happening (please correct me if I'm wrong). The report multimethod switches on both the reporter and the report type:
(defmulti
^{:doc "Generic reporting function, may be overridden to plug in
different report formats (e.g., TAP, JUnit). Assertions such as
'is' call 'report' to indicate results. The argument given to
'report' will be a map with a :type key."
:dynamic true}
report (fn [m] [(:reporter (get-current-env)) (:type m)]))Karma runs the tests with a different reporter:
(cljs.test/run-tests (cljs.test/empty-env ::karma) ~@namespaces)and test.chuck implements a multimethod that works with the default reporter:
(defmethod ct/report #?(:clj ::shrunk :cljs [::ct/default ::shrunk]) [m]
(newline)
(println "Tests failed, smallest case:" (pr-str (-> m :shrunk :smallest))
"\nSeed" (:seed m)))So, when a generative test fails, test.chuck calls report with the shrunk result:
(fn [] (ct/report (shrunk-report result)))when this call occurs, the tests are running with the karma reporter (not the default one), so the dispatch value for the multimethod is [:jx.reporter.karma/karma :com.gfredericks.test.chuck.clojure-test/shrunk]. But that doesn't match any implementation of report, so nothing happens.
But if clojure.test/report's multimethod is still being used then I don't see why users wouldn't be able to redefine things in whatever manner is necessary to make everything work.
That's correct. Once I realized what was going wrong, it was easy enough to define a new multimethod and make this work.
My solution was to implement report again and call the method that was defined by test.chuck:
;; This is for my example above, for my real fix for karma, replace `::my-reporter` with `:jx.reporter.karma/karma`
(defmethod cljs.test/report [::my-reporter :com.gfredericks.test.chuck.clojure-test/shrunk] [m]
(let [f (get (methods cljs.test/report) [:cljs.test/default :com.gfredericks.test.chuck.clojure-test/shrunk])]
(f m)))The issue is that it was tricky to understand why the shrunk data was not printing. I needed to read the code of test.chuck to understand that the shrunk data was printed via a multimethod (that works with the default reporter) and then read the code of karma-reporter to figure out that they are using a non-default reporter.
I wanted to report it here in case there was a way we could make it more obvious for others that use karma (maybe a warning? More documentation here?) but frankly the right solution may be to do nothing - it might have been better just to make a blog post about my experience, or create a stack overflow question and then immediately answer it 😄
gfredericks
commented
7 years ago Okay I think I understand what's happening, thanks for the explanation.
The part that makes it difficult for me to know what to do is that I don't really know why cljs.test has this "reporter" feature; clojure.test doesn't.
I think, though, that a simpler workaround for users would be to cause the karma reporter to extend the default reporter:
(derive :jx.reporter.karma/karma :cljs.test/default)Maybe (I don't know since I don't understand why different reporters are possible) it would be appropriate for karma to do this itself.
bhb
commented
7 years ago The part that makes it difficult for me to know what to do is that I don't really know why cljs.test has this "reporter" feature; clojure.test doesn't.
I'm not really sure why it's different either. The only information I found about it in the CLJS source is this
You can plug in your own test-reporting framework by specifying a :reporter key in the test environment. It is normally set to :cljs.test/default. Set this to the desired key and supply custom implementations of the \"report\" multimethod.
But that doesn't really explain the "why".
I hadn't even realized I could use derive here - that's a good thought. I like that it's more succinct, but it does change the behavior slightly because karma-reporter (intentionally?) does not implement the :summary report type. By using derive, the summary is now printed. That's because cljs.test implements the following:
(defmethod report [::default :fail] [m]
(inc-report-counter! :fail)
(println "\nFAIL in" (testing-vars-str m))
(when (seq (:testing-contexts (get-current-env)))
(println (testing-contexts-str)))
(when-let [message (:message m)] (println message))
(print-comparison m))
(defmethod report [::default :error] [m]
(inc-report-counter! :error)
(println "\nERROR in" (testing-vars-str m))
(when (seq (:testing-contexts (get-current-env)))
(println (testing-contexts-str)))
(when-let [message (:message m)] (println message))
(print-comparison m))
(defmethod report [::default :summary] [m]
(println "\nRan" (:test m) "tests containing"
(+ (:pass m) (:fail m) (:error m)) "assertions.")
(println (:fail m) "failures," (:error m) "errors."))
(defmethod report [::default :begin-test-ns] [m]
(println "\nTesting" (name (:ns m))))
;; Ignore these message types:
(defmethod report [::default :end-test-ns] [m])
(defmethod report [::default :begin-test-var] [m]
#_(println ":begin-test-var" (testing-vars-str m)))
(defmethod report [::default :end-test-var] [m])
(defmethod report [::default :end-run-tests] [m])
(defmethod report [::default :end-test-all-vars] [m])
(defmethod report [::default :end-test-vars] [m])while karma-reporter implements
(defmethod cljs.test/report :karma [_])
(defmethod cljs.test/report [::karma :begin-test-var] [_]
(vreset! test-var-time-start (now))
(vreset! test-var-result []))
(defmethod cljs.test/report [::karma :end-test-var] [m]
(let [var-meta (meta (:var m))
result {"suite" [(:ns var-meta)]
"description" (:name var-meta)
"success" (zero? (count @test-var-result))
"skipped" nil
"time" (- (now) @test-var-time-start)
"log" (map format-log @test-var-result)}]
(karma-result! result)))
(defmethod cljs.test/report [::karma :fail] [m]
(cljs.test/inc-report-counter! :fail)
(vswap! test-var-result conj m))
(defmethod cljs.test/report [::karma :error] [m]
(cljs.test/inc-report-counter! :error)
(vswap! test-var-result conj m))
(defmethod cljs.test/report [::karma :end-run-tests] [_]
(karma-complete!))I don't think that's too bad in my project, but I can see why karma-reporter would potentially not want to default into all report types (although it easily could make a blank implementation for [:jx.reporter.karma/karma :cljs.test/summary] to avoid this issue). I'm frankly not sure if it's a wise thing to do in karma-reporter, but I can raise the issue on that project.
In any case, after the above discussion, I'm inclined to say there's nothing to be done in test.chuck, so I will close this out if you agree.
gfredericks
commented
7 years ago At least nothing to do without discussing it with the karma-reporter maintainers I think.
Thanks for the discussion.
This isn't a bug with test.chuck, just a surprising result and I'm wondering how I can help others avoid it.
Repro:
Context: My team recently switched to using
doowith karma. I later noticed that our tests were not printing out the shrunk results. After some investigation, I realized that karma is using a custom reporter so the following multimethod is never called:This took me some time to track down, although I don't see any behavior that is buggy
test.chuckto use the reporter mechanism to print out shrunk results, so the user can overrideI started to wonder if
test.chuckcould provide a warning, but that would be very annoying if a user provided a custom reporter and really did not want to implement the report for shrunk results.Perhaps the only thing we can do is add some documentation here. I'm happy to assist with a PR, but I wanted to discuss here first. What do you think?