Clojurians Log v2

@borkdude you put a call to done in finally but that probably is not right? what if the user calls done themselves?

oh but I see, done is not available if you use ^:async

Hrm but I think the deftest macro should probably say something about ^:async, no?

it wasn't updated for the change, it should also probably call out the behavior - it will implicitly catch the exception and fail the test, same as the sync variant

I can add that tweak though separately.

I'll be back in 20 minutes

@dnolen done cannot be called in deftest ^async, its intention is that it does that for you. if you want manual, use a normal deftest with async/done. yeah updates to docstring welcome

@djblue deftest + ^:async is a thin macro that expands into something like:

(deftest foo (t/async done ((^:async fn [](try ...(finally done))))

I think with the async macro, the test would never complete as the done callback would never be called:

% clj -Sdeps '{:deps {org.clojure/clojurescript {:mvn/version "1.12.145"}}}' -M -m cljs.main --repl-env node
ClojureScript 1.12.145
cljs.user=> (require '[clojure.test :refer [deftest async run-tests]])
nil
cljs.user=> (deftest throw-test (async done (throw (ex-info "hi" {}))))
#'cljs.user/throw-test
cljs.user=> (run-tests)

Testing cljs.user
#object[Promise [object Promise]]
cljs.user=> Error: hi
    at new cljs$core$ExceptionInfo (/tmp/out639186978284392609715639532341/cljs/core.js:37900:10)
    at cljs$core$ex_info.cljs$core$IFn$_invoke$arity$3 (/tmp/out639186978284392609715639532341/cljs/core.js:37961:9)
    at cljs$core$ex_info.cljs$core$IFn$_invoke$arity$2 (/tmp/out639186978284392609715639532341/cljs/core.js:37957:26)
    at cljs$core$ex_info (/tmp/out639186978284392609715639532341/cljs/core.js:37943:26)
    at repl:43:25
    at Object.call (repl:44:4)
    at cljs$test$run_block (/tmp/out639186978284392609715639532341/cljs/test.js:372:12)
    at repl:1:109
    at repl:46:3
    at repl:51:4

I mean by manually using promises, not async/await

(our messages raced)

With the async macro you would manually need to .then/.catch/.finally

So are you thinking with async/await, users should also need to manually try/catch their tests body as well?

I don't know, I'm just trying to get a mental picture of what happened in the old situation

perhaps deftest ^:async should add a catch and then do whatever the old thing did, but I don't know what that is

I think with the async macro, everything was more manual. https://clojurescript.org/tools/testing#async-testing doesn't show a way to signal an error with the done callback. I think I accidentally assumed that ^:async /`await` would do more. I do wonder if we should fix both by allowing done to be called with an error? 🤔

It was pretty common in node.js to call a done callback with an error as a first parameter if there is one

we could protect escaping errors by emitting a catch in there and then connect that to the reporting.

does your test suite actually fail in the old way or is the exception just printed accidentally?

Here is how my test code used to look, vs what I updated it to look like

(deftest throw-test
  (async done
         (-> (run-code)
             (.catch (fn [e]
                       (is (nil? e))))
             (.finally done))))

(deftest ^:async throw-test
  (await (run-code)))

With the async macro, everything with manual, and the only way to signal an error was via is

ok so nothing changed basically?

I mean we could change this, I'm not against catching and reporting uncaught errors

the sync deftest macro does it too right?

> the sync deftest macro does it too right? Sync tests will report errors correctly

cc @dnolen proposal: catch escaping errors in (deftest ^:async ...) since the sync one does it too.

hrm, is somebody proposing a way to handle done? It's still requires explicitly handling no? only alternative I see is amore invasive change to the runner to make it work w/ promises, but that seems like too big of a change to me.

I think what's there right now is more or less backwards compatible?

I think if we update the current runner to work with (done error) we could solve the issue for both?

isn't it just a matter of adding a catch and report that error to the test runner callback? ... ish?

I'll take a look at the code

I mean, something like this could work?

(let [done-sym (gensym "done")]
                  [`(cljs.test/async ~done-sym
                                     (try ~@body
+                                         (catch :default e#
+                                           (cljs.test/do-report
+                                             {:type :error
+                                              :message "Uncaught exception, not in assertion."
+                                              :expected nil
+                                              :actual e#}))
                                          (finally (~done-sym))))])

not much code for a lot better UX

then I can also remove all my (catch :default _ (is false)) in my async/await tests in cljs.async-await-test which was a smell that something was off probably

that seems reasonable to me, patch welcome.

ok will submit one tomorrow

@dnolen uploaded a patch to CLJS-3483

thanks, I saw!

I tried out your patch locally @borkdude and it seems to work for me, 🙏 :