Possible bug in kaocha's output if test fails

[mikeananev]

Kaocha Version
1.71.1119

Platform
Operating System: macOS m1 12.5
Clojure version: 1.11.1
JDK vendor and version: OpenJDK Runtime Environment Homebrew (build 19.0.1) with --enable-preview parameter for Virtual Threads

Symptom
If test fails then I get awful kaocha's Exception output

Reproduction

1. Clone my repo SWIM
   git clone https://github.com/redstarssystems/swim/
cd swim
2. Select particular commit
   git checkout f9ef925837ff41afdb75f3cc10ba1e4fd4887841
3. Run tests to make sure that all of them are green
   clj -M:test
4. Then change value 3 to 4 on line 1993 to make test fail.
5. Run tests again and see result
   clj -M:test

[alysbrooks]

@mikeananev Thanks for the bug report (and reproduction instructions)! Do you happen to know if you see a similar problem when other tests in this project fail? I'll probably play around with your repository myself, too.

[mikeananev]

Hi @alysbrooks !
Yes, when other tests fail I often see this error. But it happens only with tests with meta ^:logic or ^:event-processing.

[plexus]

The fail-fast exception should normally never bubble to the top, so I'm labeling this as a bug. Might be related to specific user setup or config, but we should evaluate if it's something we can and should fix on our end.

@mikeananev that's a huge repro repo you shared, any chance you could make a more minimal example that still reproduces the issue?

[alysbrooks]

Hi @mikeananev, we're reviewing old issues. In the case of bug reports, we're trying to make sure we have all the information we'd need to reproduce and fix it. Would you be able to minimize the reproduction example?

[onetom]

Kaocha 1.87.1366 still has this issue.
I have a very short repro example.
Given this test:

(deftest thrown-match?-test
  (is (thrown-match? 1 1)))

without the fail fast feature:

(kaocha.repl/run #'thrown-match?-test {:kaocha/fail-fast? false})

results in a concise test summary:

EXPECTED:
(thrown-match? 1 1)

ACTUAL:
the expected exception wasn't thrown
1 tests, 1 assertions, 1 failures.
=>
{:kaocha.result/count 1,
 :kaocha.result/pass 0,
 :kaocha.result/error 0,
 :kaocha.result/fail 1,
 :kaocha.result/pending 0}

with with fail fast active

(kaocha.repl/run #'thrown-match?-test {:kaocha/fail-fast? true})

we would get the whole kaocha test plan printed, unless we restrict printing depth with (set! *print-level* 6):

EXPECTED:
(thrown-match? 1 1)

ACTUAL:
(mismatch
 (expected 1)
 (actual
  {:kaocha/fail-fast true,
        :caused-by
        {:file destination_test.clj,
         :line 469,
         :kaocha/testable #,
         :kaocha/test-plan #,
         :type :fail,
         :expected #,
         :actual the expected exception wasn't thrown,
         :message nil}}))

1 tests, 2 assertions, 2 failures.
=>
{:kaocha.result/count 1,
 :kaocha.result/pass 0,
 :kaocha.result/error 0,
 :kaocha.result/fail 2,
 :kaocha.result/pending 0}

I was wondering whether this is a kaocha or matcher-combinators issue, but I'm not sure.
Maybe it's an issue between the two?
Maybe fail-fast in kaocha shouldn't be implemented via throwing an exception?
Borkdude mentioned here that implementing fail-fast behaviour with clojure.test is pretty easy

[onetom]

actually, i have a custom version of kaocha.report/testing-vars-str and kaocha.report/fail-summary for :kaocha/fail-type & :error dispatch values, but even without that customization the problem is the same, just the output is slightly different

FAIL in ..../thrown-match?-test (xxx_test.clj:468)
expected: (thrown-match? 1 1)
  actual: the expected exception wasn't thrown

FAIL in ..../thrown-match?-test (xxx_test.clj:468)
expected: (thrown-match? 1 1)
  actual: (mismatch
 (expected 1)
 (actual
  {:kaocha/fail-fast true,
        :caused-by
        {:file "destination_test.clj",
         :line 468,
         :kaocha/testable #,
         :kaocha/test-plan #,
         :type :fail,
         :expected #,
         :actual the expected exception wasn't thrown,
         :message nil}}))

[plexus]

I see now, fail-fast as you mention is implemented by throwing an exception. It has to be, because it's the only way we can break out of a test block early. We normally catch it higher up so it's invisible to the user, but thrown-match? catches it instead.

Note that borkdude's "fail-fast" is not actually fail fast, it short circuits after the first failing test var, not after the first failing assertion.

I'd have to go through the code path and implementations to see what the right way is to solve this. We could redefine (monkey patch) thrown-match? but maybe there's a more elegant solution.

[onetom]

We hit this issue again last Friday, but luckily we were pair-programming and I remembered this issue.

[alysbrooks]

This isn't a very elegant solution either, but I wonder if the reporter could detect this situation and display "the expected exception wasn't thrown" instead of the fail-fast exception.

[onetom]

to support short-circuiting a test function would also be very useful for writing integration tests, which are usually story-like and failing an assertion would likely cause the rest of the assertions fail too, flooding the test report with distracting failure messages.

it would be nice, if we could just put a ^:kaocha/fail-fast? on such test definitions...

for example, this is how one of our such tests look like with the kaocha.report/documentation reporter:

ginoco.journey-test
  create-flow-test
    sign up
    authorize Google Sheets
    authorize Xero
    create an Excel flow
    wait for snapshot generation to finish
    delete default snapshot
    create snapshot

just mentioning this, because seeing a more concrete use-case often sparks some new ideas.

currently we manually sprinkled some explicit exceptions after every http call:

        (testing "authorize Xero"
...
          (-> (call-authorize-source! call-gini!
                                      :oauth.provider/xero
                                      {:oauth.grant/code xero-authz-code})
              expect-http-ok!)
...
        (testing "create an Excel flow"
...
          (-> (http-mock/request :post "/flows"
                {:flow/type        "flow.type/spreadsheet-db"
                 :flow/source     123
                 :flow/destination {:app/type "app/microsoft-excel"}})
              call-gini! expect-http-ok!))

where expect-http-ok! looks like this:

(ns xxx
  (:require
...
    [ring.mock.request :as http-mock]
    [ring.util.http-response :as http-resp]
    [ring.util.http-status :as http-code]))

(defmacro expect-http-status! [resp & expected-http-status-codes]
  `(doto ~resp
     (as-> ~'resp
           (when-not
             (is (~'match? {:status (m/pred ~(set expected-http-status-codes))}
                   ~'resp)
                 (-> "Unexpected HTTP code %s. Expected one of %s"
                     (format (:status ~'resp)
                             (str/join ", " ~(vec expected-http-status-codes)))))
             (throw (doto (Throwable. "Unexpected HTTP status")
                      (.setStackTrace (make-array StackTraceElement 0))))))))

(defmacro expect-http-ok! [resp]
  `(expect-http-status! ~resp
                        http-code/ok
                        http-code/created
                        http-code/accepted
                        http-code/temporary-redirect))

[alysbrooks]

@onetom I'm no longer a particularly active maintainer, but I suspect it would be easiest to deal with that proposal as a separate feature request. Unless you're saying that if you could short-circuit tests, you wouldn't need this issue to be fixed?

[onetom]

@alysbrooks

@onetom ... Unless you're saying that if you could short-circuit tests, you wouldn't need this issue to be fixed?

we are keeping the fail-fast? option true by default for the sake of these few integration test,
but then we see these enormous test-plan printouts, when some of our unit tests fail.

im just saying, if fail-fast? could be localized to individual tests, then we cloud keep fail-fast? turned off by default and we would face this issue less often.

i do understand that discussing ^:kaocha/fail-fast? would be off-topic here and this issue wouldn't be circumvented by it either. i was just trying to put the issue into a larger context.

[plexus]

Thanks for the additional context @onetom , but @alysbrooks is right. Tickets work best when they have a single clear call to action. In this case that's fixing the interaction between fail-fast and thrown/thrown-match, because that's advertised behavior that isn't working, ie a bug. A PR for that would very much be welcomed, either by monkey patching the macros, or through a different approach if one can be found.

Setting fail-fast through metadata would be an interesting feature, and a PR for that would be welcome as well.

At the end of the day Kaocha is free software which, per the license, is provided as is. It's good to report issues so they are known and documented, but reporting them alone doesn't fix them. Either someone in the community steps up, or you have to wait and see if we decide at some point to pay someone to fix certain issues, as we have done in the past.