bensu / doo

doo is a library and lein plugin to run cljs.test on different js environments.
Eclipse Public License 1.0
324 stars 63 forks source link

Exception in fixtures produces cryptic runs #134

Open laurentsenta opened 7 years ago

laurentsenta commented 7 years ago

When a fixture throws, it looks like doo is crashed or miscompiling.

I'd expect the library to catch the exception and explicitly describe "fixture threw, skipped this module". This would be shown as an error in the final reporting instead of looking like doo stopped.

;; ======================================================================
;; Testing with Phantom:

Testing konserve-firebase.some-test

FAIL in (some-b) (:)
expected: (= (+ 1 2) 2)
  actual: (not (= 3 2))

Testing konserve-firebase.core-test
WARNING: doo's init function was not set
#object[TypeError TypeError: undefined is not an object (evaluating 'f.cljs$lang$maxFixedArity')]
(ns konserve-firebase.core-test
   ...)

(use-fixtures :once
  {:before
   (fn []
     (throwing-code))
   :after
   (fn [])})

(deftest some-test
  (is false))

This makes the test fragile, a small change deep in a fixture setup/teardown fixture breaks the tests without much feedback.

laurentsenta commented 7 years ago

Example repo: https://github.com/lsenta/budb/tree/doo-demo-134

run make test-cljs there, you'll see the doo not set error.

MatthewDarling commented 7 years ago

I guess to resolve this, there would need to be some intermediate exception handling which says "here's the error which happened in the tests for this namespace".

For those more familiar with the code: Is there a specific part of doo which is basically saying, "run one individual test" or "run the tests for this namespace"? Or is that delegated to an underlying library?

bensu commented 7 years ago

That is delegated to the underlying library:

https://github.com/bensu/doo/blob/master/library/src/doo/runner.clj

The only decision that doo makes is to either run all ns or the selected ns. I think your latest PR might show a better error message for this case.

MatthewDarling commented 7 years ago

Hmm, it seems like clojure.test doesn't have a good way to handle this either. Leiningen actually adds an error message for exception in test fixtures. They replace clojure.test/test-ns with a version that catches exceptions and reports them.

I guess it would fall on doo to provide that same additional behaviour to cljs.test. Leiningen has started to warn people against using its testing wrappers, though, so maybe not the best approach to emulate...