Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix component tests, upgrade Jest & friends, and a few other goodies #18276

Merged
merged 3 commits into from
Dec 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .clj-kondo/status-im/config.edn
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
{:hooks {:analyze-call {utils.i18n/label utils.i18n/label}}
{:hooks
{:analyze-call
{utils.i18n/label utils.i18n/label
test-helpers.component/get-all-by-translation-text utils.i18n/label
test-helpers.component/get-by-translation-text utils.i18n/label
test-helpers.component/query-all-by-translation-text utils.i18n/label
test-helpers.component/query-by-translation-text utils.i18n/label}}
:linters {:status-im.linter/invalid-translation-keyword {:level :error}}}
6 changes: 5 additions & 1 deletion .zprintrc
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,17 @@
;; https://github.com/kkinnear/zprint/blob/main/doc/reference.md#respect-bl
:respect-bl

;; hang multiline left-hand-thing
;; hang multiline left-hand-thing
;; https://github.com/kkinnear/zprint/issues/273
:multi-lhs-hang]
:fn-map
{"reg-sub" :arg1-pair
"h/describe" :arg1-body
"h/describe-skip" :arg1-body
"h/describe-only" :arg1-body
"h/test" :arg1-body
"h/test-skip" :arg1-body
"h/test-only" :arg1-body
"global.describe" :arg1-body
"global.test" :arg1-body
"list-comp" :binding
Expand Down
4 changes: 3 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ lint-fix: ##@test Run code style checks and fix issues
ALL_CLOJURE_FILES=$(call find_all_clojure_files) && \
zprint '{:search-config? true}' -sw $$ALL_CLOJURE_FILES && \
zprint '{:search-config? true}' -sw $$ALL_CLOJURE_FILES && \
clojure-lsp --ns-exclude-regex ".*/src/status_im/core\.cljs$$" clean-ns && \
clojure-lsp --ns-exclude-regex ".*/src/status_im/core\.cljs|.*/src/test_helpers/component_tests_preload\.cljs$$" clean-ns && \
sh scripts/lint/trailing-newline.sh --fix && \
node_modules/.bin/prettier --write .

Expand Down Expand Up @@ -364,6 +364,7 @@ android-test:
component-test-watch: export TARGET := clojure
component-test-watch: export COMPONENT_TEST := true
component-test-watch: export BABEL_ENV := test
component-test-watch: export JEST_USE_SILENT_REPORTER := false
Copy link
Contributor

@J-Son89 J-Son89 Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice if we can call these with a shorthand arg, e.g
make component-test-watch --silent or something to that effect.
Also this pr already does so much, so not to worry about that here 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea @J-Son89, but to achieve this DX is impossible I think with make. The argument must be passed with a variable, like make component-test-watch ARG1=--silent. Maybe we should start to consider scripts instead to create a better CLI, we could even write some with babashka, something to consider cc @yqrashawn

component-test-watch: ##@ Watch tests and re-run no changes to cljs files
@@scripts/check-metro-shadow-process.sh
rm -rf ./component-spec
Expand All @@ -373,6 +374,7 @@ component-test-watch: ##@ Watch tests and re-run no changes to cljs files
component-test: export TARGET := clojure
component-test: export COMPONENT_TEST := true
component-test: export BABEL_ENV := test
component-test: export JEST_USE_SILENT_REPORTER := false
component-test: ##@test Run component tests once in NodeJS
@scripts/check-metro-shadow-process.sh
rm -rf ./component-spec
Expand Down
10 changes: 1 addition & 9 deletions nix/deps/nodejs-patched/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ stdenv.mkDerivation {
"patchBuildIdPhase"
"patchKeyChainPhase"
"patchGlogPhase"
"patchJestPhase"
"installPhase"
"installPhase"
];

# First symlink all modules as is
Expand Down Expand Up @@ -56,13 +55,6 @@ stdenv.mkDerivation {
'-Wl,--build-id=none'
'';

# Remove when we upgrade jest to 29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less patches! YAY!

patchJestPhase = ''
substituteInPlace ./node_modules/react-native/jest/setup.js --replace \
'jest.now()' \
'Date.now'
'';

installPhase = ''
mkdir -p $out
cp -R node_modules $out/
Expand Down
9 changes: 4 additions & 5 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,17 @@
"@babel/register": "7.0.0",
"@jest/globals": "^25.1.0",
"@mapbox/node-pre-gyp": "^1.0.9",
"@testing-library/jest-native": "^5.0.0",
"@testing-library/react-native": "^11.2.0",
"@testing-library/jest-native": "^5.4.3",
"@testing-library/react-native": "^12.4.2",
"@tsconfig/react-native": "^3.0.0",
"@types/jest": "^28.1.6",
"@types/react": "^18.0.24",
"@types/react-test-renderer": "^18.0.0",
"babel-jest": "^29.7.0",
"concurrently": "^7.6.0",
"jest": "^26.6.3",
"jest-circus": "^26.0.0",
"jest-environment-node": "^26.6.2",
"jest": "^29.7.0",
"jest-image-snapshot": "^5.1.0",
"jest-silent-reporter": "^0.5.0",
"metro-react-native-babel-preset": "0.76.8",
"nodemon": "^2.0.16",
"nyc": "^14.1.1",
Expand Down
2 changes: 1 addition & 1 deletion scripts/lint/require-i18n-resource-first.sh
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env sh

if rg --quiet --multiline '^\(ns.*\n^\s*\(:require\n(^\s*(;|#_).*\n)*(^\s*\[status-im\.setup\.i18n-resources\W)' src/status_im/core.cljs; then
if rg --quiet --multiline '^\(ns.*\n^\s*\(:require\n(^\s*(;|#_).*\n)*(^\s*\[status-im\.setup\.i18n-resources\W)' src/status_im/core.cljs src/test_helpers/component_tests_preload.cljs; then
exit 0
elif [ $? -eq 1 ]; then
echo "status-im.setup.i18n-resources must be loaded first (be the first one in ns :require form) in status-im.core"
Expand Down
8 changes: 5 additions & 3 deletions shadow-cljs.edn
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,10 @@
:source-map false}}
:component-test {:target :npm-module
:entries [;; We need to tell shadow-cljs to compile
;; the preloads namespace because it will
;; be used directly by Jest in the option
;; setupFilesAfterEnv.
;; the preloads namespaces because they
;; will be used directly by Jest in the
;; option setupFilesAfterEnv.
test-helpers.component-tests-preload
status-im.setup.schema-preload

quo.core-spec
Expand All @@ -161,5 +162,6 @@
:output-dir "component-spec"
:closure-defines {schema.core/throw-on-error? true}
:compiler-options {:warnings-as-errors false
:warnings {:fn-deprecated false}
:static-fns false
:infer-externs true}}}}
9 changes: 1 addition & 8 deletions src/quo/components/buttons/slide_button/component_spec.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,7 @@
:track-icon :face-id})

(h/describe "slide-button"
(h/before-each
(fn []
(h/use-fake-timers)))

(h/after-each
(fn []
(h/clear-all-timers)
(h/use-real-timers)))
(h/setup-fake-timers)

(h/test "render the correct text"
(h/render [slide-button/view default-props])
Expand Down
2 changes: 0 additions & 2 deletions src/quo/components/colors/color_picker/component_spec.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,3 @@
(h/fire-event :press (get (h/get-all-by-label-text :color-picker-item) 0))
(-> (h/expect @selected)
(.toStrictEqual :blue)))))


20 changes: 12 additions & 8 deletions src/quo/components/colors/color_picker/view.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,18 @@
(let [selected (reagent/atom default-selected)
{window-width :width} (rn/get-window)
ref (atom nil)]
(rn/use-effect #(js/setTimeout (fn []
(.scrollToIndex ^js @ref
#js
{:animated true
:index (.indexOf colors/account-colors
default-selected)
:viewPosition 0.5}))
50))
(rn/use-effect
(fn []
(js/setTimeout
(fn []
(let [index (.indexOf colors/account-colors default-selected)]
(when (and @ref (>= index 0))
(some-> ^js @ref
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: The component test caught two potential bugs here, 1st that the index could be -1 and then scrollToIndex would throw an exception, and 2nd @ref could be nil and then an exception would be thrown as well.

(.scrollToIndex #js
{:animated true
:index index
:viewPosition 0.5})))))
50)))
[rn/flat-list
{:ref #(reset! ref %)
;; TODO: using :feng-shui? temporarily while b & w is being developed.
Expand Down
25 changes: 8 additions & 17 deletions src/quo/components/dividers/strength_divider/component_spec.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -6,34 +6,25 @@
(h/describe "select-profile component"
(h/test "render component"
(h/render [strength-divider/view {:type :okay}])
(-> (h/expect (h/get-by-label-text :strength-divider))
(.toBeTruthy)))
(h/is-truthy (h/get-by-label-text :strength-divider)))
(h/test "render component with :type :very-weak"
(h/render [strength-divider/view {:type :very-weak}])
(-> (h/expect (h/get-by-translation-text :strength-divider-very-weak-label))
(.toBeTruthy)))
(h/is-truthy (h/get-by-translation-text :t/strength-divider-very-weak-label)))
(h/test "render component with :type :weak"
(h/render [strength-divider/view {:type :weak}])
(-> (h/expect (h/get-by-translation-text :strength-divider-weak-label))
(.toBeTruthy)))
(h/is-truthy (h/get-by-translation-text :t/strength-divider-weak-label)))
(h/test "render component with :type :okay"
(h/render [strength-divider/view {:type :okay}])
(-> (h/expect (h/get-by-translation-text :strength-divider-okay-label))
(.toBeTruthy)))
(h/is-truthy (h/get-by-translation-text :t/strength-divider-okay-label)))
(h/test "render component with :type :strong"
(h/render [strength-divider/view {:type :strong}])
(-> (h/expect (h/get-by-translation-text :strength-divider-strong-label))
(.toBeTruthy)))
(h/is-truthy (h/get-by-translation-text :t/strength-divider-strong-label)))
(h/test "render component with :type :very-strong"
(h/render [strength-divider/view {:type :very-strong}])
(-> (h/expect (h/get-by-translation-text :strength-divider-very-strong-label))
(.toBeTruthy)))
(h/is-truthy (h/get-by-translation-text :t/strength-divider-very-strong-label)))
(h/test "render component with :type :alert"
(h/render [strength-divider/view {:type :alert} "Text message"])
(-> (h/expect (h/get-by-text "Text message"))
(.toBeTruthy)))
(h/is-truthy (h/get-by-text "Text message")))
(h/test "render component with :type :info"
(h/render [strength-divider/view {:type :info} "Text Info"])
(-> (h/expect (h/get-by-text "Text Info"))
(.toBeTruthy))))

(h/is-truthy (h/get-by-text "Text Info"))))
10 changes: 1 addition & 9 deletions src/quo/components/drawers/drawer_buttons/component_spec.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,7 @@
[test-helpers.component :as h]))

(h/describe "drawer-buttons"
(h/before-each
(fn []
(h/use-fake-timers)))

(h/after-each
(fn []
(h/clear-all-timers)
(h/use-real-timers)))
(h/setup-fake-timers)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Fake timers are a nice feature. When I tried to enable them in more places, sometimes they could help make the tests more deterministic due to the elimination of some timing issues, but tests ended up failing for other reasons. It's a tool, and as such should be used only when needed.


(h/test "the top heading and subheading render"
(h/render [drawer-buttons/view
Expand All @@ -37,7 +30,6 @@
(-> (js/expect (h/get-by-text "bottom-sub-heading"))
(.toBeTruthy)))


(h/test "it clicks the top card"
(let [event (h/mock-fn)]
(with-redefs [safe-area/get-top (fn [] 10)]
Expand Down
4 changes: 2 additions & 2 deletions src/quo/components/drawers/drawer_top/component_spec.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
:icon-avatar :i/placeholder
:type :keypair}])
(h/is-truthy (h/get-by-text "Title"))
(-> (h/expect (h/get-by-translation-text :on-device))
(-> (h/expect (h/get-by-translation-text :t/on-device))
(.toBeTruthy)))

(h/test "component renders in keypair type when keycard? is true"
Expand All @@ -81,7 +81,7 @@
:icon-avatar :i/placeholder
:type :keypair}])
(h/is-truthy (h/get-by-text "Title"))
(-> (h/expect (h/get-by-translation-text :on-keycard))
(-> (h/expect (h/get-by-translation-text :t/on-keycard))
(.toBeTruthy)))

(h/test "component renders in default-keypair type"
Expand Down
5 changes: 4 additions & 1 deletion src/quo/components/graph/wallet_graph/component_spec.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
{:time-frame :empty}])
(h/is-truthy (h/get-by-label-text :illustration)))

(h/test "render 1 week wallet graph"
;; NOTE: Throws error:
;; _reactNative.Animated.timing(widthValue2, {
;; Cannot read properties of undefined (reading 'timing')
(h/test-skip "render 1 week wallet graph"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Using h/test-skip here shows how we can skip tests without commenting them out. This is important, for instance, if you comment all the tests in a test file, Jest will throw an error and the test suite will fail saying at least one test should exist per file.

For this particular test, as mentioned in the PR description, I couldn't fix it yet, but it's just one among so many fixed. We will find a way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep it's the way to go 👍

(h/render [wallet-graph/view
{:time-frame :1-week
:data (data 7)}])
Expand Down
Loading