day8 / re-frame-test

Cross platform (cljs and clj) utilities for testing re-frame applications
MIT License
109 stars 12 forks source link

Tests #3

Closed samroberton closed 8 years ago

samroberton commented 8 years ago

These tests pass on the JVM. I haven't had much luck trying to get Karma to run anything, and if I use a Rhino CLJS REPL to run these manually in a JS environment, I get errors.

So at this stage I'm putting this up for feedback: I'll need to continue looking into getting the tests to run correctly in the JS environment.

samroberton commented 8 years ago

Well, apparently there's one test that very occasionally fails on the JVM, for reasons I don't currently fully understand -- it seems to be a timing issue, which is odd, since it's in a test where everything should be running in a single thread.

Karma's failures in CircleCI at least match the failures I see in a local CLJS REPL.

samroberton commented 8 years ago

Note that the green CI result above is a lie: the CLJS tests "pass" because the async tests are not ever calling (done), so the subsequent tests then don't run. You can tell this from the "Executed 1 of 10" text immediately before the big green "SUCCESS" in the console output. sigh

For reasons I still haven't managed to understand, the re-frame event queue doesn't appear to be firing dispatched events in the async tests. I assume it's an issue with way I've written the CLJS-specific code in re-frame-test -- or it could be a problem with the smart-arse assert-captured-test-results approach I'm trying to take. But I haven't been able to track it down yet: more investigation required.

samroberton commented 8 years ago

Ok, I think the green build is no longer I lie. I think this is good to go now.

danielcompton commented 8 years ago

Finally clicked why we were getting warnings about macros, not quite sure if I can explain it in text this late at night, but we need to wrap the defmacro with-temp-re-frame-state in a :clj reader macro, and then in day8.re-frame.test

 (defn run-test-sync* [f]
-  (with-temp-re-frame-state
+  (day8.re-frame.test/with-temp-re-frame-state
     ;; Bypass the actual re-frame EventQueue and use a local alternative over
 ...

or refer the macro into the cljs context with

(:require-macros 
    [foo.core :refer [add]])

Edit: this is because in our cljs context, we need to refer to the macro that was defined in the Clojure namespace within the same file and required with #?(:cljs (:require-macros day8.re-frame.test)).

danielcompton commented 8 years ago

And I've (mostly) cracked the other warnings we were getting for replacing the test vars. The let for the gensym needs to be outside of the macro template code, like this:

(let [captured-test (gensym "captured-test_")]
       `(do
...
            (cljs.test/deftest ~captured-test
              (let [test-result# (do ~@body)]
...

the gensyms inside the template are evaluated once at macro definition time, and then stay the same for every execution. That's not an issue as long as the symbols that we define stay inside the local context, but deftest's by their nature can't. The same thing needs to happen on the CLJ version of this macro. I'm not 100% sure how to do the ns-unmapping on the Clojure side.