match? seems broken in CLJS

wilkerlucio commented 3 years ago

Hello, I'm trying to use it to test with clojure.test in CLJS, but the match? always returns true, example case:

(deftest sample-test
  (is (match? (m/embeds {:foo "bar"}) {})))

The test passes, I expect it to fail.

philomates commented 3 years ago

Hey @wilkerlucio! I'm pretty unfamiliar with cljs setups, but I copied your example into test/cljs/matcher_combinators/cljs_example_test.cljs and ran it both with lein test-node and lein test-phantom and in both cases it fails. In what context are you running the example?

wilkerlucio commented 3 years ago

Hello @philomates , thanks for checking!

The setup I'm trying is with Shadow CLJS on the browser, I made a complete repro demo, can you try this? https://github.com/wilkerlucio/match-comb-shadow-repro

philomates commented 3 years ago

nice, I got it working (err, that is passing incorrectly) Some notes before I log off for the day:

expected failure behavior is demonstrated when you target nodejs via shadow-cljs compile test-λ && node out/node-tests.js after adding the following to shadow-cljs.edn
```
      :test-λ {:target    :node-test
               :output-to "out/node-tests.js"
               :ns-regexp "-test$"
               :autorun   true}
```
shadow-cljs has some weird monkey-patching of cljs.test https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/test.cljs. Found this thanks to a comment in https://github.com/lambdaisland/chui/blob/master/modules/chui-core/src/lambdaisland/chui/runner.cljs#L85-L87
matcher-combinators does some weird stuff to register match? in cljs: https://github.com/nubank/matcher-combinators/blob/master/src/cljc/matcher_combinators/cljs_test.cljc#L15-L25

I'm suspecting that the test reporting for match? isn't properly being handled in when targeting :browser-test. I'll try to dig a little farther tomorrow but maybe @plexus knows right off the bat what's going on here, given the work with chui?

plexus commented 3 years ago

Hey @philomates :wave: I don't remember a lot of details, but looking at that Chui comment it might have to do with the difference between report and do-report. The latter calls the former, adding file/line numbers, but in a hacky way. I think Thomas redefined the assertions to call report directly, getting the file/line number from form metadata. But... I'm looking around the shadow-cljs code and I can't seem to find where that is happening now.

philomates commented 3 years ago

I spent another hour with this and wasn't able to fully follow what is happening in shadow-cljs with regards to the difference of running tests using the :browser-test target vs. :node-test target

@thheller could you lend any insights? Do you know why running tests with :browser-test would result in custom clojure.test assert-expr/reports not being respected while everything seems fine when targeting :node-test?

thheller commented 3 years ago

This is related to https://github.com/bhauman/cljs-test-display which is used by :browser-test but not :node-test.

What it does is set its own :reporter and then implement all the other defmethod impls for cljs.test/report to hook up the UI and stuff.

This library on the other hand expects the default reporter to be used (ie. :cljs.test/default) and since there is no defmulti match for [:cljs-test-display.core/default :matcher-combinators/exception-mismatch] will just do the default which is to do nothing.

https://github.com/nubank/matcher-combinators/blob/b3da18923f0aa6a5ef924e9303f690a3efa1d978/src/cljc/matcher_combinators/cljs_test.cljc#L104

The tests all run, just the report is dropped.

josefigueroa-nedap commented 3 years ago

This is a workaround for the combination of cljs-test-display and matcher-combinators that works in my setup.

The html version of the report doesn't looks nice, but the printed version in the Web Console keeps format and color.

(ns sample.with-cljs-test-display-and-match-combinators
  (:require
   [cljs.test :as t :refer [run-tests deftest is]]
   [cljs-test-display.core]
   [matcher-combinators.test]
   [matcher-combinators.cljs-test]
   [matcher-combinators.printer]))

(defmethod cljs.test/report [:cljs-test-display.core/default :matcher-combinators/mismatch]
 [m]
 ; cljs-test-display
 (t/inc-report-counter! :fail)
 (cljs-test-display.core/add-fail-node! m)
 ; console display with color and format
 (println "\nFAIL in" (t/testing-vars-str m))
 (when (seq (:testing-contexts (t/get-current-env)))
   (println (t/testing-contexts-str)))
 (when-let [message (:message m)]
   (println message))
 (println "mismatch:")
 (matcher-combinators.printer/pretty-print (:markup m)))

(deftest this-should-be-seen
  (is (match? {:a {:b 100}} {:a {:b 200}})))

(run-tests (cljs-test-display.core/init! "app-tests"))

mauricioszabo commented 3 years ago

@wilkerlucio maybe it's related to https://github.com/nubank/matcher-combinators/issues/83?

ottonascarella commented 2 years ago

The workaround proposed by @josefigueroa-nedap seems to work for me too.

philomates commented 1 year ago

https://github.com/nubank/matcher-combinators/pull/18a9 has been released in 3.8.2 and should address this

pesterhazy commented 1 year ago

Just tried the latest version, and it seems to have fixed the issue with match? succeeding despite problems. We were able to remove the custom defmethod call that was used to work around the issue

Thanks @philomates !

nubank / matcher-combinators

match? seems broken in CLJS #156