From 96d98c62ed3a77ce621602e732a77147fbf114a5 Mon Sep 17 00:00:00 2001 From: Icaro Motta Date: Thu, 13 Jun 2024 22:03:02 -0300 Subject: [PATCH] chore(tests)_: Facilitate writing event tests (#20424) Introduces a new macro deftest-event to facilitate writing tests for event handlers. Motivation came from the _problem of having to always extract event handlers as vars in order to test them_. Although the implementation of deftest-sub and deftest-event are similar, deftest-sub is critically important because it guarantees changes in one subscription can be caught by tests from all other related subscriptions in the graph (reference: PR https://github.com/status-im/status-mobile/pull/14472). This is not the case for the new deftest-event macro. deftest-event is essentially a way of make testing events less ceremonial by not requiring event handlers to be extracted to vars. But there are a few other small benefits: - The macro uses re-frame and "finds" the event handler by computing the interceptor chain (except :do-fx), so in a way, the tests are covering a bit more ground. - Slightly easier way to find event tests in the repo since you can just find references to deftest-event. - Possibly slightly easier to maintain by devs because now event tests and sub tests are written in a similar fashion. - Less code diff. Whether an event has a test or not, there's no var to add/remove. - The dispatch function provided by the macro makes reading the tests easier over time. For example, when we read subscription tests, the Act section of the test is always the same (rf/sub [sub-name]). Similarly for events, the Act section is always (dispatch [event-id arg1 arg2]). - Makes the re-frame code look more idiomatic because it's more common to define handlers as anonymous functions. Downside: deftest-sub and deftest-event are relatively complicated macros. Note: The test suite runs just as fast and clj-kondo can lint code within the macro just as well. Before: ```clojure (deftest process-account-from-signal-test (testing "process account from signal" (let [cofx {:db {:wallet {:accounts {}}}} effects (events/process-account-from-signal cofx [raw-account]) expected-effects {:db {:wallet {:accounts {address account}}} :fx [[:dispatch [:wallet/get-wallet-token-for-account address]] [:dispatch [:wallet/request-new-collectibles-for-account-from-signal address]] [:dispatch [:wallet/check-recent-history-for-account address]]]}] (is (match? expected-effects effects))))) ``` After ```clojure (h/deftest-event :wallet/process-account-from-signal [event-id dispatch] (let [expected-effects {:db {:wallet {:accounts {address account}}} :fx [[:dispatch [:wallet/get-wallet-token-for-account address]] [:dispatch [:wallet/request-new-collectibles-for-account-from-signal address]] [:dispatch [:wallet/check-recent-history-for-account address]]]}] (reset! rf-db/app-db {:wallet {:accounts {}}}) (is (match? expected-effects (dispatch [event-id raw-account]))))) ``` --- .clj-kondo/config.edn | 2 + .zprintrc | 1 + src/status_im/contexts/wallet/events.cljs | 111 ++++++------ .../contexts/wallet/events_test.cljs | 170 ++++++++---------- src/test_helpers/unit.clj | 76 +++++++- 5 files changed, 200 insertions(+), 160 deletions(-) diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn index c45a7a5477..a47e61b08e 100644 --- a/.clj-kondo/config.edn +++ b/.clj-kondo/config.edn @@ -10,6 +10,8 @@ legacy.status-im.utils.styles/def clojure.core/def legacy.status-im.utils.styles/defn clojure.core/defn test-helpers.unit/deftest-sub clojure.core/defn + test-helpers.unit/deftest-event clojure.core/defn + taoensso.tufte/defnp clojure.core/defn} :linters {:case-duplicate-test {:level :error} :case-quoted-test {:level :error} diff --git a/.zprintrc b/.zprintrc index b1d9628f67..f7764fa3c4 100644 --- a/.zprintrc +++ b/.zprintrc @@ -41,6 +41,7 @@ "reg-fx" :arg1-pair "testing" :arg1-body "deftest-sub" :arg1-body + "deftest-event" :arg1-body "test-async" :arg1-body "wait-for" :arg1-body "with-deps-check" :arg1-body diff --git a/src/status_im/contexts/wallet/events.cljs b/src/status_im/contexts/wallet/events.cljs index 7efddb2111..44e50ddf9f 100644 --- a/src/status_im/contexts/wallet/events.cljs +++ b/src/status_im/contexts/wallet/events.cljs @@ -105,14 +105,12 @@ :on-success [:wallet/get-accounts-success] :on-error [:wallet/log-rpc-error {:event :wallet/get-accounts}]}]]]})) -(defn process-account-from-signal - [{:keys [db]} [{:keys [address] :as account}]] - {:db (assoc-in db [:wallet :accounts address] (data-store/rpc->account account)) - :fx [[:dispatch [:wallet/get-wallet-token-for-account address]] - [:dispatch [:wallet/request-new-collectibles-for-account-from-signal address]] - [:dispatch [:wallet/check-recent-history-for-account address]]]}) - -(rf/reg-event-fx :wallet/process-account-from-signal process-account-from-signal) +(rf/reg-event-fx :wallet/process-account-from-signal + (fn [{:keys [db]} [{:keys [address] :as account}]] + {:db (assoc-in db [:wallet :accounts address] (data-store/rpc->account account)) + :fx [[:dispatch [:wallet/get-wallet-token-for-account address]] + [:dispatch [:wallet/request-new-collectibles-for-account-from-signal address]] + [:dispatch [:wallet/check-recent-history-for-account address]]]})) (rf/reg-event-fx :wallet/save-account @@ -156,26 +154,22 @@ :on-success [:wallet/remove-account-success toast-message] :on-error [:wallet/log-rpc-error {:event :wallet/remove-account}]}]]]})) -(defn get-wallet-token-for-all-accounts - [{:keys [db]}] - {:fx (->> (get-in db [:wallet :accounts]) - vals - (mapv - (fn [{:keys [address]}] - [:dispatch [:wallet/get-wallet-token-for-account address]])))}) +(rf/reg-event-fx :wallet/get-wallet-token-for-all-accounts + (fn [{:keys [db]}] + {:fx (->> (get-in db [:wallet :accounts]) + vals + (mapv + (fn [{:keys [address]}] + [:dispatch [:wallet/get-wallet-token-for-account address]])))})) -(rf/reg-event-fx :wallet/get-wallet-token-for-all-accounts get-wallet-token-for-all-accounts) - -(defn get-wallet-token-for-account - [{:keys [db]} [address]] - {:db (assoc-in db [:wallet :ui :tokens-loading address] true) - :fx [[:json-rpc/call - [{:method "wallet_getWalletToken" - :params [[address]] - :on-success [:wallet/store-wallet-token address] - :on-error [:wallet/get-wallet-token-for-account-failed address]}]]]}) - -(rf/reg-event-fx :wallet/get-wallet-token-for-account get-wallet-token-for-account) +(rf/reg-event-fx :wallet/get-wallet-token-for-account + (fn [{:keys [db]} [address]] + {:db (assoc-in db [:wallet :ui :tokens-loading address] true) + :fx [[:json-rpc/call + [{:method "wallet_getWalletToken" + :params [[address]] + :on-success [:wallet/store-wallet-token address] + :on-error [:wallet/get-wallet-token-for-account-failed address]}]]]})) (rf/reg-event-fx :wallet/get-wallet-token-for-account-failed @@ -401,14 +395,12 @@ [{:method "wallet_startWallet" :on-error [:wallet/log-rpc-error {:event :wallet/start-wallet}]}]]]})) -(defn check-recent-history-for-all-accounts - [{:keys [db]}] - {:fx (->> (get-in db [:wallet :accounts]) - vals - (mapv (fn [{:keys [address]}] - [:dispatch [:wallet/check-recent-history-for-account address]])))}) - -(rf/reg-event-fx :wallet/check-recent-history-for-all-accounts check-recent-history-for-all-accounts) +(rf/reg-event-fx :wallet/check-recent-history-for-all-accounts + (fn [{:keys [db]}] + {:fx (->> (get-in db [:wallet :accounts]) + vals + (mapv (fn [{:keys [address]}] + [:dispatch [:wallet/check-recent-history-for-account address]])))})) (rf/reg-event-fx :wallet/check-recent-history-for-account @@ -479,35 +471,32 @@ :text (i18n/label :t/provider-is-down {:chains chain-names}) :duration constants/toast-chain-down-duration}]])]}))) -(defn reset-selected-networks - [{:keys [db]}] - {:db (assoc-in db [:wallet :ui :network-filter] db/network-filter-defaults)}) +(rf/reg-event-fx :wallet/reset-selected-networks + (fn [{:keys [db]}] + {:db (assoc-in db [:wallet :ui :network-filter] db/network-filter-defaults)})) -(rf/reg-event-fx :wallet/reset-selected-networks reset-selected-networks) +(rf/reg-event-fx :wallet/update-selected-networks + (fn [{:keys [db]} [network-name]] + (let [selected-networks (get-in db [:wallet :ui :network-filter :selected-networks]) + selector-state (get-in db [:wallet :ui :network-filter :selector-state]) + contains-network? (contains? selected-networks network-name) + update-fn (if contains-network? disj conj) + networks-count (count selected-networks)] + (cond (= selector-state :default) + {:db (-> db + (assoc-in [:wallet :ui :network-filter :selected-networks] #{network-name}) + (assoc-in [:wallet :ui :network-filter :selector-state] :changed))} -(defn update-selected-networks - [{:keys [db]} [network-name]] - (let [selected-networks (get-in db [:wallet :ui :network-filter :selected-networks]) - selector-state (get-in db [:wallet :ui :network-filter :selector-state]) - contains-network? (contains? selected-networks network-name) - update-fn (if contains-network? disj conj) - networks-count (count selected-networks)] - (cond (= selector-state :default) - {:db (-> db - (assoc-in [:wallet :ui :network-filter :selected-networks] #{network-name}) - (assoc-in [:wallet :ui :network-filter :selector-state] :changed))} + ;; reset the list + ;; - if user is removing the last network in the list + ;; - if all networks is selected + (or (and (= networks-count 1) contains-network?) + (and (= (inc networks-count) constants/default-network-count) (not contains-network?))) + {:fx [[:dispatch [:wallet/reset-selected-networks]]]} - ;; reset the list - ;; - if user is removing the last network in the list - ;; - if all networks is selected - (or (and (= networks-count 1) contains-network?) - (and (= (inc networks-count) constants/default-network-count) (not contains-network?))) - {:fx [[:dispatch [:wallet/reset-selected-networks]]]} - - :else - {:db (update-in db [:wallet :ui :network-filter :selected-networks] update-fn network-name)}))) - -(rf/reg-event-fx :wallet/update-selected-networks update-selected-networks) + :else + {:db + (update-in db [:wallet :ui :network-filter :selected-networks] update-fn network-name)})))) (rf/reg-event-fx :wallet/get-crypto-on-ramps-success diff --git a/src/status_im/contexts/wallet/events_test.cljs b/src/status_im/contexts/wallet/events_test.cljs index 2bd0a090a7..15bed73bce 100644 --- a/src/status_im/contexts/wallet/events_test.cljs +++ b/src/status_im/contexts/wallet/events_test.cljs @@ -1,9 +1,11 @@ (ns status-im.contexts.wallet.events-test (:require - [cljs.test :refer-macros [deftest is testing]] + [cljs.test :refer-macros [is testing]] matcher-combinators.test + [re-frame.db :as rf-db] [status-im.constants :as constants] - [status-im.contexts.wallet.events :as events])) + status-im.contexts.wallet.events + [test-helpers.unit :as h])) (def address "0x2ee6138eb9344a8b76eca3cf7554a06c82a1e2d8") @@ -53,114 +55,98 @@ "0x04ee7c47e4b68cc05dcd3377cbd5cde6be3c89fcf20a981e55e0285ed63a50f51f8b423465eee134c51bb0255e6041e9e5b006054b0fa72a7c76942a5a1a3f4e7e" :removed false}) -(deftest scan-address-success-test - (let [db {}] - (testing "scan-address-success" - (let [expected-db {:wallet {:ui {:scanned-address address}}} - effects (events/scan-address-success {:db db} address) - result-db (:db effects)] - (is (match? result-db expected-db)))))) +(h/deftest-event :wallet/scan-address-success + [event-id dispatch] + (is (match? {:wallet {:ui {:scanned-address address}}} + (:db (dispatch [event-id address]))))) -(deftest clean-scanned-address-test - (let [db {:wallet {:ui {:scanned-address address}}}] - (testing "clean-scanned-address" - (let [expected-db {:wallet {:ui {:send nil - :scanned-address nil}}} - effects (events/clean-scanned-address {:db db}) - result-db (:db effects)] - (is (match? result-db expected-db)))))) +(h/deftest-event :wallet/clean-scanned-address + [event-id dispatch] + (reset! rf-db/app-db {:wallet {:ui {:scanned-address address}}}) + (is (match? {:wallet {:ui {}}} + (:db (dispatch [event-id]))))) -(deftest reset-selected-networks-test - (testing "reset-selected-networks" - (let [db {:wallet {}} - expected-db {:wallet {:ui {:network-filter {:selector-state :default - :selected-networks - (set constants/default-network-names)}}}} - effects (events/reset-selected-networks {:db db}) - result-db (:db effects)] - (is (match? result-db expected-db))))) +(h/deftest-event :wallet/reset-selected-networks + [event-id dispatch] + (reset! rf-db/app-db {:wallet {}}) + (is (match? {:wallet + {:ui {:network-filter {:selector-state :default + :selected-networks + (set constants/default-network-names)}}}} + (:db (dispatch [event-id]))))) -(deftest update-selected-networks-test +(h/deftest-event :wallet/update-selected-networks + [event-id dispatch] (testing "update-selected-networks" - (let [db {:wallet {:ui {:network-filter {:selected-networks - #{constants/optimism-network-name} - :selector-state :changed}}}} - network-name constants/arbitrum-network-name + (let [network-name constants/arbitrum-network-name expected-db {:wallet {:ui {:network-filter {:selected-networks #{constants/optimism-network-name network-name} - :selector-state :changed}}}} - props [network-name] - effects (events/update-selected-networks {:db db} props) - result-db (:db effects)] - (is (match? result-db expected-db)))) + :selector-state :changed}}}}] + (reset! rf-db/app-db + {:wallet {:ui {:network-filter {:selected-networks + #{constants/optimism-network-name} + :selector-state :changed}}}}) + (is (match? expected-db (:db (dispatch [event-id network-name])))))) (testing "update-selected-networks > if all networks is already selected, update to incoming network" - (let [db {:wallet {:ui {:network-filter {:selector-state :default - :selected-networks - (set constants/default-network-names)}}}} - network-name constants/arbitrum-network-name + (let [network-name constants/arbitrum-network-name expected-db {:wallet {:ui {:network-filter {:selected-networks #{network-name} - :selector-state :changed}}}} - props [network-name] - effects (events/update-selected-networks {:db db} props) - result-db (:db effects)] - (is (match? result-db expected-db)))) + :selector-state :changed}}}}] + (reset! rf-db/app-db + {:wallet {:ui {:network-filter {:selector-state :default + :selected-networks + (set constants/default-network-names)}}}}) + (is (match? expected-db (:db (dispatch [event-id network-name])))))) (testing "update-selected-networks > reset on removing last network" - (let [db {:wallet {:ui {:network-filter {:selected-networks - #{constants/optimism-network-name} - :selector-state :changed}}}} - expected-fx [[:dispatch [:wallet/reset-selected-networks]]] - props [constants/optimism-network-name] - effects (events/update-selected-networks {:db db} props) - result-fx (:fx effects)] - (is (match? result-fx expected-fx))))) + (let [expected-fx [[:dispatch [:wallet/reset-selected-networks]]]] + (reset! rf-db/app-db + {:wallet {:ui {:network-filter {:selected-networks + #{constants/optimism-network-name} + :selector-state :changed}}}}) + (is (match? expected-fx + (:fx (dispatch [event-id constants/optimism-network-name]))))))) -(deftest get-wallet-token-for-all-accounts-test - (testing "get wallet token for all accounts" - (let [address-1 "0x1" - address-2 "0x2" - cofx {:db {:wallet {:accounts {address-1 {:address address-1} - address-2 {:address address-2}}}}} - effects (events/get-wallet-token-for-all-accounts cofx) - result-fx (:fx effects) - expected-fx [[:dispatch [:wallet/get-wallet-token-for-account address-1]] - [:dispatch [:wallet/get-wallet-token-for-account address-2]]]] - (is (match? expected-fx result-fx))))) +(h/deftest-event :wallet/get-wallet-token-for-all-accounts + [event-id dispatch] + (let [address-1 "0x1" + address-2 "0x2"] + (reset! rf-db/app-db {:wallet {:accounts {address-1 {:address address-1} + address-2 {:address address-2}}}}) + (is (match? [[:dispatch [:wallet/get-wallet-token-for-account address-1]] + [:dispatch [:wallet/get-wallet-token-for-account address-2]]] + (:fx (dispatch [event-id])))))) -(deftest get-wallet-token-for-account-test - (testing "get wallet token for account" - (let [cofx {:db {}} - effects (events/get-wallet-token-for-account cofx [address]) - expected-effects {:db {:wallet {:ui {:tokens-loading {address true}}}} - :fx [[:json-rpc/call - [{:method "wallet_getWalletToken" - :params [[address]] - :on-success [:wallet/store-wallet-token address] - :on-error [:wallet/get-wallet-token-for-account-failed - address]}]]]}] - (is (match? expected-effects effects))))) +(h/deftest-event :wallet/get-wallet-token-for-account + [event-id dispatch] + (let [expected-effects {:db {:wallet {:ui {:tokens-loading {address true}}}} + :fx [[:json-rpc/call + [{:method "wallet_getWalletToken" + :params [[address]] + :on-success [:wallet/store-wallet-token address] + :on-error [:wallet/get-wallet-token-for-account-failed + address]}]]]}] + (is (match? expected-effects (dispatch [event-id address]))))) -(deftest check-recent-history-for-all-accounts-test +(h/deftest-event :wallet/check-recent-history-for-all-accounts + [event-id dispatch] (testing "check recent history for all accounts" (let [address-1 "0x1" address-2 "0x2" - cofx {:db {:wallet {:accounts {address-1 {:address address-1} - address-2 {:address address-2}}}}} - effects (events/check-recent-history-for-all-accounts cofx) - result-fx (:fx effects) expected-fx [[:dispatch [:wallet/check-recent-history-for-account address-1]] [:dispatch [:wallet/check-recent-history-for-account address-2]]]] - (is (match? expected-fx result-fx))))) + (reset! rf-db/app-db + {:wallet {:accounts {address-1 {:address address-1} + address-2 {:address address-2}}}}) + (is (match? expected-fx (:fx (dispatch [event-id]))))))) -(deftest process-account-from-signal-test - (testing "process account from signal" - (let [cofx {:db {:wallet {:accounts {}}}} - effects (events/process-account-from-signal cofx [raw-account]) - expected-effects {:db {:wallet {:accounts {address account}}} - :fx [[:dispatch [:wallet/get-wallet-token-for-account address]] - [:dispatch - [:wallet/request-new-collectibles-for-account-from-signal address]] - [:dispatch [:wallet/check-recent-history-for-account address]]]}] - (is (match? expected-effects effects))))) +(h/deftest-event :wallet/process-account-from-signal + [event-id dispatch] + (let [expected-effects {:db {:wallet {:accounts {address account}}} + :fx [[:dispatch [:wallet/get-wallet-token-for-account address]] + [:dispatch + [:wallet/request-new-collectibles-for-account-from-signal address]] + [:dispatch [:wallet/check-recent-history-for-account address]]]}] + (reset! rf-db/app-db {:wallet {:accounts {}}}) + (is (match? expected-effects (dispatch [event-id raw-account]))))) diff --git a/src/test_helpers/unit.clj b/src/test_helpers/unit.clj index 0edeeda5c5..60472f78a8 100644 --- a/src/test_helpers/unit.clj +++ b/src/test_helpers/unit.clj @@ -4,7 +4,7 @@ [clojure.string :as string] [clojure.walk :as walk])) -(defn- subscription-name->test-name +(defn- keyword->test-name [sub-name] (->> [(namespace sub-name) (name sub-name) @@ -13,7 +13,7 @@ (map #(string/replace % #"\." "-")) (string/join "-"))) -(defmacro ^:private testing-subscription +(defmacro ^:private testing-restorable-app-db [description & body] `(cljs.test/testing ~description (restore-app-db (fn [] ~@body)))) @@ -46,17 +46,79 @@ ;; Act and Assert (is (= (rf/sub [sub-name]))))) ```" - [sub-name args & body] + [sub-name bindings & body] `(let [sub-name# ~sub-name] - (cljs.test/deftest ~(symbol (subscription-name->test-name sub-name)) - (let [~args [sub-name#]] + (cljs.test/deftest ~(symbol (keyword->test-name sub-name)) + (let [~bindings [sub-name#]] (restore-app-db (fn [] ;; Do not warn about subscriptions being used in non-reactive contexts. (with-redefs [re-frame.interop/debug-enabled? false] ~@(clojure.walk/postwalk-replace - {'cljs.test/testing `testing-subscription - 'testing `testing-subscription} + {'cljs.test/testing `testing-restorable-app-db + 'testing `testing-restorable-app-db} + body)))))))) + +(defmacro ^:private event-dispatcher + "Returns an s-exp that can build an event dispatcher given an event vector." + [] + `(fn [event-v#] + (let [event-id# (first event-v#) + all-interceptors# (re-frame.registrar/get-handler :event event-id#) + interceptors-without-fx# (remove #(= :do-fx (:id %)) all-interceptors#)] + (assert (seq all-interceptors#) + (str "Event does not exist '" event-id# "'")) + (:effects (re-frame.interceptor/execute event-v# interceptors-without-fx#))))) + +(defmacro deftest-event + "Defines a test var for an event `event-id`. + + This macro primarily exists to facilitate testing anonymous event handlers + directly, without the need to extract them to vars. + + Similar to `deftest-sub`, this macro uses the re-frame machinery. + Consequently, the test body is no longer guaranteed to be pure, as all + interceptors will run (except for the standard `:do-fx`). + + Within the test, we can directly mutate `re-frame.db/app-db` atom to set up + test data. Upon test completion, it is guaranteed that the app-db will be + restored to its original state. + + Any usage of the `cljs.test/testing` macro within `body` will be modified to + ensure the app-db is restored. + + The macro offers two bindings, namely `event-id` and `dispatch`. The + `dispatch` function is similar to `re-frame.core/dispatch`, but without + executing effects. It returns the map of effects we can assert on and it's + synchronous. + + Example: + + ```clojure + (ns events-test + (:require [test-helpers.unit :as h])) + + (h/deftest-event :wallet/dummy-event + [event-id dispatch] + (let [expected {:db {:a false}}] + ;; Arrange + (swap! rf-db/app-db {:a true}) + + ;; Act and Assert + (is (match? expected (dispatch [event-id arg1 arg2]))))) + ``` + " + [event-id bindings & body] + `(let [event-id# ~event-id] + (cljs.test/deftest ~(symbol (keyword->test-name event-id)) + (let [dispatcher# (event-dispatcher) + ~bindings [event-id# dispatcher#]] + (restore-app-db + (fn [] + (with-redefs [re-frame.interop/debug-enabled? false] + ~@(clojure.walk/postwalk-replace + {'cljs.test/testing `testing-restorable-app-db + 'testing `testing-restorable-app-db} body)))))))) (defmacro use-log-fixture