From 60ad7c8a29f54f861642e5e5aec0b0dbdd845df3 Mon Sep 17 00:00:00 2001 From: Icaro Motta Date: Wed, 24 Jul 2024 23:06:41 -0300 Subject: [PATCH] chore(tests): New match-strict? cljs.test directive (#20825) Equality checks in tests using = give a bad experience by default on test failures containing nested data structures. We use the cljs.test directive match? from matcher-combinators library to help compare nested structures. The problem with match? is that its default matcher for maps (embeds) can be too permissive, and this causes surprises. Here we upgrade matcher-combinators to latest, where a new matcher called nested-equals is available. This matcher won't allow extra keys in maps. This matcher eliminates the need for manually adding nested equals matchers as we have to do currently. - Upgrades matcher-combinators from 3.8.8 to 3.9.1 (latest as of 2024-07-19) What changes? When asserting in tests, we now have the option to use match-strict? or match?. Both directives are available by integrating with cljs.test. The code implementing the new match-strict? directive was 100% copied from the library matcher-combinators because we need to wrap the expected value ourselves with matcher-combinators.matchers/nested-equals. It's ugly code, but it's how we can integrate with cljs.test/assert-expr. --- .clj-kondo/config.edn | 3 +- nix/deps/clojure/deps.json | 6 +- nix/deps/clojure/deps.list | 2 +- shadow-cljs.edn | 2 +- .../contexts/wallet/data_store_test.cljs | 132 ++++++++---------- src/test_helpers/matchers.clj | 47 +++++++ src/test_helpers/matchers.cljs | 24 ++++ src/test_helpers/unit.cljs | 5 +- 8 files changed, 143 insertions(+), 78 deletions(-) create mode 100644 src/test_helpers/matchers.clj create mode 100644 src/test_helpers/matchers.cljs diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn index a47e61b08e..d34a725d03 100644 --- a/.clj-kondo/config.edn +++ b/.clj-kondo/config.edn @@ -74,7 +74,8 @@ ;; https://github.com/borkdude/clj-kondo/issues/867 :unresolved-symbol {:exclude [PersistentPriorityMap.EMPTY number - legacy.status-im.test-helpers/restore-app-db]} + legacy.status-im.test-helpers/restore-app-db + (cljs.test/is [match-strict?])]} :unresolved-var {:level :error} :unsorted-required-namespaces {:level :error} :unused-alias {:level :warning} diff --git a/nix/deps/clojure/deps.json b/nix/deps/clojure/deps.json index e70f714b61..7428227a14 100644 --- a/nix/deps/clojure/deps.json +++ b/nix/deps/clojure/deps.json @@ -711,11 +711,11 @@ }, { - "path": "nubank/matcher-combinators/3.8.8/matcher-combinators-3.8.8", + "path": "nubank/matcher-combinators/3.9.1/matcher-combinators-3.9.1", "host": "https://repo.clojars.org", "jar": { - "sha1": "4c94bd510f0c18a20191e46dd6becedebc640bbd", - "sha256": "0wpla2hx0s4mda58ndyd8938zmnz1gyhgfr6pzfphy27xfbvsssl" + "sha1": "f0830a112cae8ee931a90d9f39214a0ed4d44150", + "sha256": "1rjcgqhms84xmnhs7sqcd3b2dpa37mmzgz2yz33yz2rdb9skl96g" } }, diff --git a/nix/deps/clojure/deps.list b/nix/deps/clojure/deps.list index 362e8f0643..82c73717da 100644 --- a/nix/deps/clojure/deps.list +++ b/nix/deps/clojure/deps.list @@ -77,7 +77,7 @@ net/cgrand/macrovich/0.2.1/macrovich-0.2.1.jar net/java/dev/jna/jna/5.12.1/jna-5.12.1.jar nrepl/bencode/1.1.0/bencode-1.1.0.jar nrepl/nrepl/1.0.0/nrepl-1.0.0.jar -nubank/matcher-combinators/3.8.8/matcher-combinators-3.8.8.jar +nubank/matcher-combinators/3.9.1/matcher-combinators-3.9.1.jar org/apache/ant/ant/1.10.11/ant-1.10.11.jar org/apache/ant/ant-launcher/1.10.11/ant-launcher-1.10.11.jar org/apache/commons/commons-lang3/3.12.0/commons-lang3-3.12.0.jar diff --git a/shadow-cljs.edn b/shadow-cljs.edn index ad71280492..c247fc68f5 100644 --- a/shadow-cljs.edn +++ b/shadow-cljs.edn @@ -19,7 +19,7 @@ [cider/piggieback "0.4.1"] [org.slf4j/slf4j-nop "2.0.9"] [re-frisk-remote "1.6.0"] - [nubank/matcher-combinators "3.8.8"] + [nubank/matcher-combinators "3.9.1"] ;; Use the same version specified in the Nix dependency. [clj-kondo/clj-kondo "2024.03.13"] diff --git a/src/status_im/contexts/wallet/data_store_test.cljs b/src/status_im/contexts/wallet/data_store_test.cljs index 73ada0ee93..851c70bfdd 100644 --- a/src/status_im/contexts/wallet/data_store_test.cljs +++ b/src/status_im/contexts/wallet/data_store_test.cljs @@ -1,7 +1,6 @@ (ns status-im.contexts.wallet.data-store-test (:require [cljs.test :refer-macros [deftest is testing]] - [matcher-combinators.matchers :as matchers] matcher-combinators.test [status-im.contexts.wallet.data-store :as sut])) @@ -162,82 +161,73 @@ (deftest reconcile-keypairs-test (testing "reconcile-keypairs represents updated key pairs and accounts" (is - (match? - (matchers/match-with - [set? matchers/set-equals - map? matchers/equals] - {:removed-keypair-ids #{} - :removed-account-addresses #{} - :updated-accounts-by-address {"1x123" (merge account - {:key-uid "0x123" - :address "1x123"}) - "1x456" (merge account - {:key-uid "0x456" - :address "1x456" - :operable? false - :operable :no})} - :updated-keypairs-by-id {"0x123" {:key-uid "0x123" - :type :seed - :lowest-operability :fully - :accounts [(merge account - {:key-uid "0x123" - :address "1x123"})]} - "0x456" {:key-uid "0x456" - :type :key - :lowest-operability :no - :accounts [(merge account - {:key-uid "0x456" - :address "1x456" - :operable? false - :operable :no})]}}}) + (match-strict? + {:removed-keypair-ids #{} + :removed-account-addresses #{} + :updated-accounts-by-address {"1x123" (merge account + {:key-uid "0x123" + :address "1x123"}) + "1x456" (merge account + {:key-uid "0x456" + :address "1x456" + :operable? false + :operable :no})} + :updated-keypairs-by-id {"0x123" {:key-uid "0x123" + :type :seed + :lowest-operability :fully + :accounts [(merge account + {:key-uid "0x123" + :address "1x123"})]} + "0x456" {:key-uid "0x456" + :type :key + :lowest-operability :no + :accounts [(merge account + {:key-uid "0x456" + :address "1x456" + :operable? false + :operable :no})]}}} (sut/reconcile-keypairs [raw-keypair-seed-phrase raw-keypair-private-key])))) (testing "reconcile-keypairs represents removed key pairs and accounts" (is - (match? - (matchers/match-with - [set? matchers/set-equals - map? matchers/equals] - {:removed-keypair-ids #{"0x456"} - :removed-account-addresses #{"1x456"} - :updated-accounts-by-address {"1x123" (merge account - {:key-uid "0x123" - :address "1x123"})} - :updated-keypairs-by-id {"0x123" {:key-uid "0x123" - :type :seed - :lowest-operability :fully - :accounts [(merge account - {:key-uid "0x123" - :address "1x123"})]}}}) + (match-strict? + {:removed-keypair-ids #{"0x456"} + :removed-account-addresses #{"1x456"} + :updated-accounts-by-address {"1x123" (merge account + {:key-uid "0x123" + :address "1x123"})} + :updated-keypairs-by-id {"0x123" {:key-uid "0x123" + :type :seed + :lowest-operability :fully + :accounts [(merge account + {:key-uid "0x123" + :address "1x123"})]}}} (sut/reconcile-keypairs [raw-keypair-seed-phrase (assoc raw-keypair-private-key :removed true)])))) (testing "reconcile-keypairs ignores chat accounts inside updated accounts" (is - (match? - (matchers/match-with - [set? matchers/set-equals - map? matchers/equals] - {:removed-keypair-ids #{} - :removed-account-addresses #{} - :updated-accounts-by-address {"2x000" (merge account - {:key-uid "0x000" - :address "2x000" - :chat false - :wallet true - :default-account? true})} - :updated-keypairs-by-id {"0x000" {:key-uid "0x000" - :type :profile - :lowest-operability :fully - :accounts [(merge account - {:key-uid "0x000" - :address "1x000" - :chat true - :wallet false - :default-account? false}) - (merge account - {:key-uid "0x000" - :address "2x000" - :chat false - :wallet true - :default-account? true})]}}}) + (match-strict? + {:removed-keypair-ids #{} + :removed-account-addresses #{} + :updated-accounts-by-address {"2x000" (merge account + {:key-uid "0x000" + :address "2x000" + :chat false + :wallet true + :default-account? true})} + :updated-keypairs-by-id {"0x000" {:key-uid "0x000" + :type :profile + :lowest-operability :fully + :accounts [(merge account + {:key-uid "0x000" + :address "1x000" + :chat true + :wallet false + :default-account? false}) + (merge account + {:key-uid "0x000" + :address "2x000" + :chat false + :wallet true + :default-account? true})]}}} (sut/reconcile-keypairs [raw-keypair-profile]))))) diff --git a/src/test_helpers/matchers.clj b/src/test_helpers/matchers.clj new file mode 100644 index 0000000000..24693f486e --- /dev/null +++ b/src/test_helpers/matchers.clj @@ -0,0 +1,47 @@ +(ns test-helpers.matchers + "Internal use. Don't require it directly." + (:require + [cljs.test :as test] + [matcher-combinators.core :as core] + [matcher-combinators.matchers :as matchers] + [matcher-combinators.parser] + [matcher-combinators.result :as result])) + +;; This implementation is identical to `match?`, but wraps the expected value +;; with `nested-equals`. This differs from the default `embeds` matcher on maps, +;; where extra map keys are considered valid. +(defmethod test/assert-expr 'match-strict? + [_ msg form] + `(let [args# (list ~@(rest form)) + [matcher# actual#] args#] + (cond + (not (= 2 (count args#))) + (test/do-report + {:type :fail + :message ~msg + :expected (symbol "`match-strict?` expects 2 arguments: a `matcher` and the `actual`") + :actual (symbol (str (count args#) " were provided: " '~form))}) + + (core/matcher? matcher#) + (let [result# (core/match (matchers/nested-equals matcher#) actual#)] + (test/do-report + (if (core/indicates-match? result#) + {:type :pass + :message ~msg + :expected '~form + :actual (list 'match? matcher# actual#)} + (with-file+line-info + {:type :fail + :message ~msg + :expected '~form + :actual (tagged-for-pretty-printing (list '~'not (list 'match? matcher# actual#)) + result#) + :markup (::result/value result#)})))) + + :else + (test/do-report + {:type :fail + :message ~msg + :expected (str "The first argument of `match-strict?` " + "needs to be a matcher (implement the match protocol)") + :actual '~form})))) diff --git a/src/test_helpers/matchers.cljs b/src/test_helpers/matchers.cljs new file mode 100644 index 0000000000..313424a0e6 --- /dev/null +++ b/src/test_helpers/matchers.cljs @@ -0,0 +1,24 @@ +(ns test-helpers.matchers + "Some vars in this namespace solely exist to support the matchers.clj file." + (:require-macros test-helpers.matchers) + (:require + [cljs.test :as t] + [matcher-combinators.parser] + [matcher-combinators.printer :as printer] + [matcher-combinators.result :as result])) + +(defrecord Mismatch [summary match-result]) + +(defn tagged-for-pretty-printing + [actual-summary result] + (->Mismatch actual-summary result)) + +(extend-protocol IPrintWithWriter + Mismatch + (-pr-writer [this writer _] + (-write writer (printer/as-string (-> this :match-result ::result/value))))) + +(defn with-file+line-info + [report] + (merge (t/file-and-line (js/Error.) 4) + report)) diff --git a/src/test_helpers/unit.cljs b/src/test_helpers/unit.cljs index 0f3ae7ca82..d90fb649bc 100644 --- a/src/test_helpers/unit.cljs +++ b/src/test_helpers/unit.cljs @@ -12,7 +12,10 @@ [re-frame.events :as rf-events] [re-frame.registrar :as rf-registrar] [re-frame.subs :as rf-subs] - [taoensso.timbre :as log])) + [taoensso.timbre :as log] + + ;; We must require this namespace to register the custom cljs.test directive `match-strict?`. + test-helpers.matchers)) (defn db "A simple wrapper to get the latest value from the app db."