Improving legacy Om code (I): Adding a test harness
Published by Manuel Rivero on 07/07/2018
Introduction.
I’m working at GreenPowerMonitor as part of a team developing a challenging SPA to monitor and manage renewable energy portfolios using ClojureScript. It’s a two years old Om application which contains a lot of legacy code. When I say legacy, I’m using Michael Feathers’ definition of legacy code as code without tests. This definition views legacy code from the perspective of code being difficult to evolve because of a lack of automated regression tests.
The legacy (untested) Om code.
Recently I had to face one of these legacy parts when I had to fix some bugs in the user interface that was presenting all the devices of a given energy facility in a hierarchy tree (devices might be comprised of other devices). This is the original legacy view code:
(ns horizon.controls.widgets.tree.hierarchy | |
(:require | |
[cljs.core.async :refer [>!]] | |
[clojure.string :as string] | |
[horizon.common.logging :as log] | |
[horizon.common.utils.keys-pressed :as kp] | |
[horizon.controls.utils.css-transitions-group :as css-transitions-group :include-macros true] | |
[horizon.controls.utils.icons :as icons] | |
[horizon.controls.utils.reactive :as utils.reactive] | |
[om.core :as om :include-macros true] | |
[om.dom :include-macros true] | |
[sablono.core :refer-macros [html]]) | |
(:require-macros | |
[cljs.core.async.macros :refer [go]] | |
[horizon.common.macros :refer [defhandler]])) | |
(defhandler expand-node [e channel expanded node-id expanded?] | |
(if expanded? | |
(go (>! channel {:type :expand-node :value (disj expanded node-id)})) | |
(go (>! channel {:type :expand-node :value (conj expanded node-id)})))) | |
(defhandler select-node [e channel value] | |
(go (>! channel {:type :select-nodes :value value}))) | |
(defhandler select-alert [e channel value] | |
(go (>! channel {:type :select-alert :value value}))) | |
(defn- selected-node? [selected node-id] | |
(contains? (set (map :id selected)) node-id)) | |
(defn- get-node-class [selected node-id level num-children] | |
(->> ["tree-node" | |
(when (selected-node? selected node-id) "selected") | |
(str "level-" level) | |
(when (zero? num-children) "clickable")] | |
(remove nil?) | |
(string/join " "))) | |
(defn- info-view [{:keys [level expanded info-data num-children selected]} owner branch-channel] | |
(reify | |
om/IRender | |
(render [_] | |
(let [{node-id :NodeId icon-type :IconType severity :Severity node-name :NodeName title :Title node-alerts :Alerts} info-data | |
expanded? (contains? expanded node-id) | |
icon (icons/build-icon-class icon-type) | |
icon-color (icons/build-icon-color severity) | |
node-class (get-node-class selected node-id level num-children)] | |
(html | |
[:div | |
(if (zero? num-children) | |
{:class node-class | |
:on-click #(select-node % branch-channel {:id node-id | |
:name node-name})} | |
{:class node-class}) | |
(if (pos? num-children) | |
[:div.expander | |
{:on-click #(expand-node % branch-channel expanded node-id expanded?)} | |
[:button.mdl-button.mdl-js-button.mdl-js-ripple-effect.mdl-button--icon | |
(if expanded? | |
[:i.fa.fa-minus] | |
[:i.fa.fa-plus])]] | |
[:div.dummy-expander]) | |
[:div.alert-icon | |
(when (some? icon-type) | |
[:button.mdl-button.mdl-js-button.mdl-js-ripple-effect.mdl-button--icon | |
{:title title | |
:on-click #(select-alert % branch-channel node-alerts)} | |
[:i {:class icon :style {:color icon-color}}]])] | |
[:div.device-name | |
{:data-id node-id} | |
node-name]]))))) | |
(defn- handle-branch-channel-messages [tree-channel {:keys [value] :as msg}] | |
(case (:type msg) | |
:select-alert (go (>! tree-channel msg)) | |
:expand-node (go (>! tree-channel {:type :expand-node :value value})) | |
:select-nodes (go (>! tree-channel {:type :select-nodes :value value})) | |
(log/log-unknown-message-type ::handle-branch-channel-messages msg))) | |
(defn- branch-view | |
[{node-id :NodeId nodes :Nodes :as data} owner {:keys [level tree-channel]}] | |
(reify | |
om/IInitState | |
(init-state [_] | |
{:expanded false | |
:selected false | |
:branch-channel (utils.reactive/build-channel-loop | |
#(handle-branch-channel-messages tree-channel %))}) | |
om/IRenderState | |
(render-state [_ {:keys [expanded branch-channel selected]}] | |
(html | |
[:div.branch-container | |
(let [num-branches (count nodes)] | |
(list | |
(om/build info-view | |
{:expanded expanded | |
:level level | |
:info-data data | |
:selected selected | |
:num-children num-branches} | |
{:opts branch-channel}) | |
(css-transitions-group/with-transition | |
:rim-list | |
(when (and (pos? num-branches) | |
(contains? expanded node-id)) | |
(om/build-all branch-view | |
nodes | |
{:opts {:level (inc level) | |
:tree-channel tree-channel} | |
:state {:selected selected | |
:expanded expanded}})))))])))) | |
(defn- update-state-global-and-local [selected channel owner node] | |
(om/set-state! owner :shift-selection node) | |
(om/set-state! owner :selected selected) | |
(go (>! channel {:type :select-nodes :value selected}))) | |
(defn- handle-control-selection [channel owner node additive] | |
(let [selected (om/get-state owner :selected) | |
to-select (if additive selected #{node})] | |
(if (contains? selected node) | |
(update-state-global-and-local (disj to-select node) channel owner nil) | |
(update-state-global-and-local (conj to-select node) channel owner (if additive nil node))))) | |
(defn- get-nodes-shift [first last] | |
(let [expanded-html-nodes (vec (array-seq (js->clj (.getElementsByClassName js/document "device-name")))) | |
expanded-nodes (mapv #(.-id (.-dataset %)) expanded-html-nodes) | |
first-index (.indexOf (to-array expanded-nodes) first) | |
last-index (.indexOf (to-array expanded-nodes) last)] | |
(subvec expanded-nodes (min first-index last-index) (inc (max first-index last-index))))) | |
(defn- handle-shift-selection [selected channel owner additive] | |
(let [shift-selection (om/get-state owner :shift-selection)] | |
(if shift-selection | |
(let [selected-range (set (get-nodes-shift shift-selection selected)) | |
current-selected (om/get-state owner :selected) | |
to-select (if additive (conj selected-range current-selected) selected-range)] | |
(update-state-global-and-local to-select channel owner shift-selection)) | |
(om/set-state! owner :shift-selection selected)))) | |
(defn- select-node-handler [owner node channel] | |
(let [pressed-keys (om/get-state owner :pressed-keys)] | |
(cond | |
(kp/is-control-pressed? pressed-keys) | |
(handle-control-selection channel owner node true) | |
(kp/is-shift-pressed? pressed-keys) | |
(handle-shift-selection node channel owner false) | |
:else | |
(handle-control-selection channel owner node false)))) | |
(defn set-state-pressed-keys [owner keys] | |
(om/set-state! owner :pressed-keys (set keys))) | |
(defn- tree-channel-handler [owner channel {:keys [value] :as msg}] | |
(case (:type msg) | |
:select-alert (go (>! channel msg)) | |
:expand-node (go (>! channel {:type :expand-node :value value})) | |
:select-nodes (select-node-handler owner value channel) | |
(log/log-unknown-message-type ::tree-channel-handler msg))) | |
(defn main-view [{:keys [values expanded selected view]} owner channel] | |
(reify | |
om/IInitState | |
(init-state [_] | |
{:shift-selection nil | |
:pressed-keys #{} | |
:selected selected | |
:tree-channel (utils.reactive/build-channel-loop | |
#(tree-channel-handler owner channel %))}) | |
om/IDidMount | |
(did-mount [_] | |
(kp/listen-to-pressed-keys | |
#{:control :shift} | |
#(set-state-pressed-keys owner %) | |
:tree-view-listen-pressed-keys)) | |
om/IWillUnmount | |
(will-unmount [_] | |
(kp/unlisten-to-pressed-keys :tree-view-listen-pressed-keys)) | |
om/IRenderState | |
(render-state [_ {:keys [tree-channel selected]}] | |
(html | |
[:div.tree-hierarchy-container | |
(om/build branch-view | |
values | |
{:opts {:level 0 | |
:tree-channel tree-channel | |
:view view} | |
:state {:selected selected | |
:expanded expanded}})])))) |
This code contains not only the layout of several components but also the logic to both conditionally render some parts of them and to respond to user interactions. This interesting logic is full of asynchronous and effectful code that is reading and updating the state of the components, extracting information from the DOM itself and reading and updating the global application state. All this makes this code very hard to test.
Humble Object pattern.
It’s very difficult to make component tests for non-component code like the one in this namespace, which makes writing end-to-end tests look like the only option.
However, following the idea of the humble object pattern, we might reduce the untested code to just the layout of the view. The humble object can be used when a code is too closely coupled to its environment to make it testable. To apply it, the interesting logic is extracted into a separate easy-to-test component that is decoupled from its environment.
In this case we extracted the interesting logic to a separate namespace, where we thoroughly tested it. With this we avoided writing the slower and more fragile end-to-end tests.
We wrote the tests using the test-doubles library (I’ve talked about it in a recent post) and some home-made tools that help testing asynchronous code based on core.async.
This is the logic we extracted:
(ns horizon.controls.widgets.tree.hierarchy-helpers | |
(:require | |
[clojure.string :as string] | |
[horizon.common.logging :as log] | |
[cljs.core.async :refer [>!]] | |
[om.core :as om :include-macros true] | |
[horizon.common.utils.keys-pressed :as kp] | |
[horizon.common.utils.collections :as utils.collections]) | |
(:require-macros | |
[cljs.core.async.macros :refer [go]])) | |
(defn- selected-node? [selected node-id] | |
(contains? (set (map :id selected)) node-id)) | |
(defn select-node-classes [selected node-id level num-children] | |
(->> ["tree-node" | |
(when (selected-node? selected node-id) "selected") | |
(str "level-" level) | |
(when (zero? num-children) "clickable")] | |
(remove nil?) | |
(string/join " "))) | |
(defn handle-branch-channel-messages [tree-channel {:keys [value] :as msg}] | |
(case (:type msg) | |
:select-alert (go (>! tree-channel msg)) | |
:expand-node (go (>! tree-channel {:type :expand-node :value value})) | |
:select-nodes (go (>! tree-channel {:type :select-nodes :value value})) | |
(log/log-unknown-message-type ::handle-branch-channel-messages msg))) | |
(defn- notify-selected-nodes! [channel selected] | |
(go (>! channel {:type :select-nodes :value selected}))) | |
(defn- update-state-global-and-local! [selected channel owner node] | |
(om/set-state! owner :shift-selection node) | |
(om/set-state! owner :selected selected) | |
(notify-selected-nodes! channel selected)) | |
(defn- get-expanded-nodes-from-dom [] | |
(let [expanded-html-nodes (vec (array-seq (js->clj (.getElementsByClassName js/document "device-name"))))] | |
(mapv #(hash-map :id (.-id (.-dataset %)) :name (.-name (.-dataset %))) expanded-html-nodes))) | |
(defn- get-shift-selected-nodes [first-selected-node last-selected-node] | |
(let [expanded-nodes (get-expanded-nodes-from-dom) | |
first-index (utils.collections/position-of first-selected-node expanded-nodes) | |
last-index (utils.collections/position-of last-selected-node expanded-nodes)] | |
(if (and first-index last-index) | |
(set (subvec expanded-nodes (min first-index last-index) (inc (max first-index last-index)))) | |
#{}))) | |
(defn- handle-shift-selection [node channel owner additive] | |
(let [shift-selection (om/get-state owner :shift-selection)] | |
(if shift-selection | |
(let [shift-selected-nodes (get-shift-selected-nodes shift-selection node) | |
current-selected (om/get-state owner :selected) | |
to-select (if additive (conj shift-selected-nodes current-selected) shift-selected-nodes)] | |
(update-state-global-and-local! to-select channel owner shift-selection)) | |
(om/set-state! owner :shift-selection node)))) | |
(defn- handle-control-selection [channel owner node additive] | |
(let [selected (om/get-state owner :selected) | |
to-select (if additive selected #{node})] | |
(if (contains? selected node) | |
(update-state-global-and-local! (disj to-select node) channel owner nil) | |
(update-state-global-and-local! (conj to-select node) channel owner (if additive nil node))))) | |
(defn- select-node-handler [owner node channel] | |
(let [pressed-keys (om/get-state owner :pressed-keys)] | |
(cond | |
(kp/is-control-pressed? pressed-keys) | |
(handle-control-selection channel owner node true) | |
(kp/is-shift-pressed? pressed-keys) | |
(handle-shift-selection node channel owner false) | |
:else | |
(handle-control-selection channel owner node false)))) | |
(defn handle-tree-channel-messages [owner channel {:keys [value] :as msg}] | |
(case (:type msg) | |
:select-alert (go (>! channel msg)) | |
:expand-node (go (>! channel {:type :expand-node :value value})) | |
:select-nodes (select-node-handler owner value channel) | |
(log/log-unknown-message-type ::handle-tree-channel-messages msg))) |
and these are the tests we wrote for it:
(ns horizon.controls.widgets.tree.hierarchy-helpers-test | |
(:require | |
[cljs.test :refer-macros [deftest testing is use-fixtures async]] | |
[horizon.controls.widgets.tree.hierarchy-helpers :as sut] | |
[horizon.test-helpers.async-test-tools :as async-test-tools] | |
[greenpowermonitor.test-doubles :as td] | |
[cljs.core.async :as core.async] | |
[horizon.common.logging :as log] | |
[om.core :as om])) | |
(deftest selecting-node-classes | |
(is (= "tree-node level-0" (sut/select-node-classes #{} 1 0 3))) | |
(is (= "tree-node level-1" (sut/select-node-classes #{} 1 1 3))) | |
(is (= "tree-node selected level-0" (sut/select-node-classes #{{:id 1}} 1 0 3))) | |
(is (= "tree-node level-0" (sut/select-node-classes #{{:id 1}} 5 0 3))) | |
(is (= "tree-node level-1 clickable" (sut/select-node-classes #{{:id 1}} 5 1 0)))) | |
;: Handling branch-channel messages | |
;;---------------------------------- | |
(deftest branch-channel-expanding-nodes-message | |
(let [msg {:type :expand-node :value :some-value} | |
channel (core.async/chan)] | |
(async done | |
(async-test-tools/expect-async-message | |
channel | |
:expected-message msg | |
:done-fn done) | |
(sut/handle-branch-channel-messages channel msg)))) | |
(deftest branch-channel-selecting-alerts-message | |
(let [msg {:type :select-alert} | |
channel (core.async/chan)] | |
(async done | |
(async-test-tools/expect-async-message | |
channel | |
:expected-message msg | |
:done-fn done) | |
(sut/handle-branch-channel-messages channel msg)))) | |
(deftest branch-channel-selecting-nodes-message | |
(let [msg {:type :select-nodes :value :some-value} | |
channel (core.async/chan)] | |
(async done | |
(async-test-tools/expect-async-message | |
channel | |
:expected-message msg | |
:done-fn done) | |
(sut/handle-branch-channel-messages channel msg)))) | |
(deftest branch-channel-any-other-type-of-message-logs-an-error | |
(td/with-doubles | |
:spying [log/log-unknown-message-type] | |
(let [msg {:type :any-other-type} | |
channel :not-used-channel] | |
(sut/handle-branch-channel-messages channel msg) | |
(is (= [[::sut/handle-branch-channel-messages msg]] | |
(td/calls-to log/log-unknown-message-type)))))) | |
(deftest branch-channel-any-other-type-of-message-produces-no-messages-in-the-channel | |
(let [msg {:type :any-other-type} | |
channel (core.async/chan)] | |
(async done | |
(async-test-tools/expect-no-messages | |
channel | |
:done-fn done) | |
(sut/handle-branch-channel-messages channel msg)))) | |
;: Handling tree-channel-handler messages | |
;;---------------------------------- | |
(deftest tree-channel-expanding-nodes-message | |
(let [owner :some-owner | |
msg {:type :expand-node :value :some-value} | |
channel (core.async/chan)] | |
(async done | |
(async-test-tools/expect-async-message | |
channel | |
:expected-message msg | |
:done-fn done) | |
(sut/handle-tree-channel-messages owner channel msg)))) | |
(deftest tree-channel-selecting-alerts-message | |
(let [owner :some-owner | |
msg {:type :select-alert} | |
channel (core.async/chan)] | |
(async done | |
(async-test-tools/expect-async-message | |
channel | |
:expected-message msg | |
:done-fn done) | |
(sut/handle-tree-channel-messages owner channel msg)))) | |
(deftest tree-channel-selecting-nodes-message | |
(let [owner :some-owner | |
msg {:type :select-nodes :value :some-value} | |
channel :not-used-channel] | |
(td/with-doubles | |
:spying [sut/select-node-handler] | |
(sut/handle-tree-channel-messages owner channel msg) | |
(is (= [[owner :some-value channel]] (td/calls-to sut/select-node-handler)))))) | |
(deftest tree-channel-any-other-type-of-message-logs-an-error | |
(td/with-doubles | |
:spying [log/log-unknown-message-type] | |
(let [msg {:type :any-other-type} | |
owner :some-owner | |
channel :not-used-channel] | |
(sut/handle-tree-channel-messages owner channel msg) | |
(is (= [[::sut/handle-tree-channel-messages msg]] | |
(td/calls-to log/log-unknown-message-type)))))) | |
(deftest tree-channel-any-other-type-of-message-produces-no-messages-in-the-channel | |
(let [msg {:type :any-other-type} | |
owner :some-owner | |
channel (core.async/chan)] | |
(async done | |
(async-test-tools/expect-no-messages | |
channel | |
:done-fn done) | |
(sut/handle-tree-channel-messages owner channel msg)))) | |
(deftest selecting-a-node-handler | |
(let [owner :some-owner | |
channel :a-channel | |
node :a-node] | |
(testing "when shift is pressed" | |
(let [pressed-keys #{:control :shift}] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :pressed-keys] pressed-keys}] | |
:spying [sut/handle-control-selection] | |
(sut/select-node-handler owner node channel) | |
(is (= [[channel owner node true]] (td/calls-to sut/handle-control-selection)))))) | |
(testing "when control but not shift is pressed" | |
(let [pressed-keys #{:shift}] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :pressed-keys] pressed-keys}] | |
:spying [sut/handle-shift-selection] | |
(sut/select-node-handler owner node channel) | |
(is (= [[node channel owner false]] (td/calls-to sut/handle-shift-selection)))))) | |
(testing "if any other keys are pressed" | |
(let [pressed-keys #{}] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :pressed-keys] pressed-keys}] | |
:spying [sut/handle-control-selection] | |
(sut/select-node-handler owner node channel) | |
(is (= [[channel owner node false]] (td/calls-to sut/handle-control-selection)))))))) | |
(deftest handling-control-selection | |
(let [owner :some-owner | |
channel :some-channel | |
clicked-node {:id 1 :name "koko"} | |
a-selected-node {:id 2 :name "moko"}] | |
(testing "when selection is additive and clicked node is already in selected nodes" | |
(let [additive true | |
selected-nodes #{clicked-node a-selected-node} | |
expected-nodes-to-select #{a-selected-node}] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :selected] selected-nodes}] | |
:spying [sut/update-state-global-and-local!] | |
(sut/handle-control-selection channel owner clicked-node additive) | |
(is (= [[expected-nodes-to-select channel owner nil]] | |
(td/calls-to sut/update-state-global-and-local!)))))) | |
(testing "when selection is additive and clicked node is not in selected nodes" | |
(let [additive true | |
selected-nodes #{a-selected-node} | |
expected-nodes-to-select #{clicked-node a-selected-node}] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :selected] selected-nodes}] | |
:spying [sut/update-state-global-and-local!] | |
(sut/handle-control-selection channel owner clicked-node additive) | |
(is (= [[expected-nodes-to-select channel owner nil]] | |
(td/calls-to sut/update-state-global-and-local!)))))) | |
(testing "when selection is not additive and clicked node is not in selected nodes" | |
(let [additive false | |
selected-nodes #{a-selected-node} | |
expected-nodes-to-select #{clicked-node}] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :selected] selected-nodes}] | |
:spying [sut/update-state-global-and-local!] | |
(sut/handle-control-selection channel owner clicked-node additive) | |
(is (= [[expected-nodes-to-select channel owner clicked-node]] | |
(td/calls-to sut/update-state-global-and-local!)))))) | |
(testing "when selection is not additive and clicked node is already in selected nodes" | |
(let [additive false | |
selected-nodes #{clicked-node a-selected-node} | |
expected-nodes-to-select #{}] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :selected] selected-nodes}] | |
:spying [sut/update-state-global-and-local!] | |
(sut/handle-control-selection channel owner clicked-node additive) | |
(is (= [[expected-nodes-to-select channel owner nil]] | |
(td/calls-to sut/update-state-global-and-local!)))))))) | |
(deftest handling-shift-selection | |
(let [owner :some-owner | |
channel :some-channel] | |
(testing "when there're no shift selected node" | |
(let [shift-selected-node nil | |
additive :any-boolean | |
clicked-node {:id 1 :name "k"}] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :shift-selection] shift-selected-node}] | |
:spying [om/set-state!] | |
(sut/handle-shift-selection clicked-node channel owner additive) | |
(is (= [[:some-owner :shift-selection clicked-node]] | |
(td/calls-to om/set-state!)))))) | |
(testing "when there are a shift selected node" | |
(let [shift-selected-node {:id 3 :name "k"} | |
currently-selected-node :any-node | |
expanded-nodes-from-dom [{:id 1 :name "k"} {:id 2 :name "o"} {:id 3 :name "k"} {:id 4 :name "o"}] | |
nodes-from-shift-selected-one-to-clicked-one #{{:id 1 :name "k"} {:id 2 :name "o"} {:id 3 :name "k"}} | |
clicked-node {:id 1 :name "k"}] | |
(testing "when selection is additive" | |
(let [additive true | |
expected-nodes-to-select (conj nodes-from-shift-selected-one-to-clicked-one currently-selected-node)] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :shift-selection] shift-selected-node | |
[owner :selected] currently-selected-node} | |
sut/get-expanded-nodes-from-dom :returns [expanded-nodes-from-dom]] | |
:spying [sut/update-state-global-and-local!] | |
(sut/handle-shift-selection clicked-node channel owner additive) | |
(is (= [[expected-nodes-to-select :some-channel :some-owner shift-selected-node]] | |
(td/calls-to sut/update-state-global-and-local!)))))) | |
(testing "when selection is not additive" | |
(let [additive false | |
expected-nodes-to-select nodes-from-shift-selected-one-to-clicked-one] | |
(td/with-doubles | |
:stubbing [om/get-state :maps {[owner :shift-selection] shift-selected-node | |
[owner :selected] currently-selected-node} | |
sut/get-expanded-nodes-from-dom :returns [expanded-nodes-from-dom]] | |
:spying [sut/update-state-global-and-local!] | |
(sut/handle-shift-selection clicked-node channel owner additive) | |
(is (= [[expected-nodes-to-select :some-channel :some-owner shift-selected-node]] | |
(td/calls-to sut/update-state-global-and-local!)))))))))) | |
(deftest updating-global-and-local-state | |
(let [selected :some-selected-nodes | |
channel :some-channel | |
owner :some-owner | |
node :some-node] | |
(td/with-doubles | |
:spying [om/set-state! | |
sut/notify-selected-nodes!] | |
(sut/update-state-global-and-local! selected channel owner node) | |
(is (= [[owner :shift-selection node] [owner :selected selected]] (td/calls-to om/set-state!))) | |
(is (= [[channel selected]] (td/calls-to sut/notify-selected-nodes!)))))) | |
(deftest notifying-selected-nodes | |
(let [selected :some-nodes | |
channel (core.async/chan)] | |
(async done | |
(async-test-tools/expect-async-message | |
channel | |
:expected-message {:type :select-nodes :value selected} | |
:done-fn done) | |
(sut/notify-selected-nodes! channel selected)))) |
See here how the view looks after this extraction. Using the humble object pattern, we managed to test the most important bits of logic with fast unit tests instead of end-to-end tests.
The real problem was the design.
We could have left the code as it was (in fact we did for a while), but its tests were highly coupled to implementation details and hard to write because its design was far from ideal. Even though, applying the humble object pattern idea, we had separated the important logic from the view, which allowed us to focus on writing tests with more ROI avoiding end-to-end tests, the extracted logic still contained many concerns. It was not only deciding how to interact with the user and what to render, but also mutating and reading state, getting data from global variables and from the DOM and making asynchronous calls. Its effectful parts were not isolated from its pure parts. This lack of separation of concerns made the code hard to test and hard to reason about, forcing us to use heavy tools: the test-doubles library and our async-test-tools assertion functions to be able to test the code.
Summary.
First, we applied the humble object pattern idea to manage to write unit tests for the interesting logic of a hard to test legacy Om view, instead of having to write more expensive end-to-end tests. Then, we saw how those unit tests were far from ideal because they were highly coupled to implementation details, and how these problems were caused by a lack of separation of concerns in the code design.
Next.
In a future post we’ll solve the lack of separation of concerns by using effects and coeffects to isolate the logic that decides how to interact with the user from all the effectful code. This new design will make the interesting logic pure and, as such, really easy to test and reason about.
Originally published in Manuel Rivero's blog.