-
Notifications
You must be signed in to change notification settings - Fork 985
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
Conversation
7d8844c
to
73c61a7
Compare
Jenkins BuildsClick to see older builds (13)
|
(fn [] | ||
(let [index (.indexOf colors/account-colors default-selected)] | ||
(when (and @ref (>= index 0)) | ||
(some-> ^js @ref |
There was a problem hiding this comment.
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.
(fn [] | ||
(h/clear-all-timers) | ||
(h/use-real-timers))) | ||
(h/setup-fake-timers) |
There was a problem hiding this comment.
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.
;; NOTE: Throws error: | ||
;; _reactNative.Animated.timing(widthValue2, { | ||
;; Cannot read properties of undefined (reading 'timing') | ||
(h/test-skip "render 1 week wallet graph" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/fire-event :on-focus (h/get-by-label-text :address-text-input)) | ||
(h/fire-event :on-blur (h/get-by-label-text :address-text-input)) | ||
(h/has-prop (h/get-by-label-text :address-text-input) :placeholder-text-color colors/neutral-30))) | ||
(-> (h/wait-for #(h/get-by-label-text :clear-button)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: Notice how it's unnecessary to surround get-by-*
with h/is-truthy
when used as a matcher for h/wait-for. The spec for RNTL
waitForclearly says the predicate should throw an exception when not found and every
get-by-*` function does that. Since this is a common need, it's nice to be able to write less.
(-> (h/wait-for #(h/is-falsy (h/query-by-label-text :clear-button))) | ||
(.then (fn [] | ||
(h/wait-for #(h/get-by-label-text :loading-button-container)))) | ||
(.then #(h/was-called on-detect-ens)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: Notice how the next assertion must go after the promise returned by the second wait-for
is fulfilled.
I think adding the library https://github.com/funcool/promesa to the project is starting to make even more sense to keep the syntax more clean, but I also think these few .then
calls look just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for promesa, I guess this discussion shiould be handled outside of the pr. It would be good to get a clearer understanding for the pros/cons of using that library in the project.
cc @clauxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, outside this PR. It's like the 5th or so time I remember we kind of wish promesa was in the project. Also even an external contributor mentioned this. I used it a lot in the past, it's kind of a no-brainer.
(take 19 | ||
(interleave (repeat [rn/view {:style (style/dashed-line-line network-name)}]) | ||
(repeat [rn/view {:style style/dashed-line-space}])))]) | ||
(into [rn/view {:style style/dashed-line}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: I resolved almost all or all (can't remember now) of the React warnings about non-unique keys. This is one example.
[react-native.audio-toolkit :as audio] | ||
[status-im.contexts.chat.messages.content.audio.view :as audio-message] | ||
[test-helpers.component :as h])) | ||
|
||
;; We can't rely on `with-redefs` with async code. | ||
(set! audio/destroy-player #()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: Using set!
is only safe because Jest isolates tests between files, so the audio/destroy-player
var is modified only within the scope of the tests in this file.
(h/describe "community options for bottom sheets" | ||
(h/setup-restorable-re-frame) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: This setup function makes the dozens of warnings about subscriptions being redefined go away. Now each test starts with a blank canvas.
@@ -48,7 +48,6 @@ | |||
|
|||
(rf/reg-event-fx :wallet/send-select-amount | |||
(fn [{:keys [db]} [{:keys [amount]}]] | |||
(js/alert "Not implemented yet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: re-frame events shouldn't have side-effects, though we have one famous exception in the project: we call log functions :)
@@ -1,25 +1,71 @@ | |||
const transformIgnorePatterns = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful
(defmacro test | ||
[description & body] | ||
`(js/global.test | ||
~description | ||
(fn [] ~@body))) | ||
|
||
(defmacro test-skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much needed!
Thank you
@@ -56,13 +55,6 @@ stdenv.mkDerivation { | |||
'-Wl,--build-id=none' | |||
''; | |||
|
|||
# Remove when we upgrade jest to 29 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less patches! YAY!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is commendable work @ilmotta
Thanks for taking care of this 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this @ilmotta !
src/status_im/core_spec.cljs
Outdated
[status-im.contexts.wallet.create-account.edit-derivation-path.component-spec] | ||
[status-im.contexts.wallet.send.input-amount.component-spec])) | ||
|
||
;; [status-im.contexts.wallet.create-account.edit-derivation-path.component-spec] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw there is one more test here (after rebasing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thanks for catching that @OmarBasem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OmarBasem, @J-Son89, when possible, please have a look at this commit I made to fix failing tests 884266b. The problem appeared after I rebased, I guess from the recently merged commit 5eb0baa
The root cause was because in quo.components.wallet.network-bridge.view/view
we were calling name
on a nil value, but this throws exceptions in the tests for status-im.contexts.wallet.send.input-amount.component-spec
. I don't know if this could be a problem in production later on, but it's safer anyway to guard against calling name
in a possibly nil value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @ilmotta 👍
@@ -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 |
There was a problem hiding this comment.
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 👌
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive improvement 🚀
Amazing PR, thanks @ilmotta! 🚀 |
Hey @vkjr, @siddarthkay said something similar, so that's two already :) I'll try then to capture some good practices in a separate document. Will create an issue to track this progress. Edit: Issue created #18287 |
73c61a7
to
884266b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
src/quo/core_spec.cljs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ilmotta This file has some commented-out require at the top, could you please remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ajayesivan, this was due to a rebase. It's fixed now.
711a3fb
to
884266b
Compare
884266b
to
fee498e
Compare
75% of end-end tests have passed
Failed tests (6)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (36)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
Fixes #18157
Closes #18235
Summary
The main motivation for this PR was to fix all component tests after the latest RN upgrade. But this PR also brings a few Christmas gifts 🎅
Dependency changes
Jest
: from26.6.3
to latest29.7.0
.@testing-library/jest-native
: from5.3.0
to latest5.4.3
@testing-library/react-native
: from11.5.4
to12.4.2
jest-circus
, this is now the default test runner.jest-environment-node
. This is handled by the package manager.jest-silent-reporter
at version0.5.0
.Why component tests were failing?
Many tests were failing because we were using RN Testing Library (RNTL) in an unreliable fashion (we still do, I haven't rewritten all of the tests, nor do I intend to). With the recent library upgrades, the unreliability was excerbated. Other times, the tests were incorrectly arranging data.
But to be fair, some degree of unreliability will always exist with RTL/RNTL, it's the nature of the beast. Next time you need to comment out a component test, consider first if the test can be rewritten in a more reliable way, even if you have your suspicions an external factor could be the cause.
with-redefs
does not work with async codeThis one may trip you up. Generally speaking,
with-redefs
should not be used with async code, assume the worst. The scope of the macro will cease to exist by the time the async code runs. In many tests we were usingwith-redefs
, then callingrender
, but for some components that useuse-effect
, JS timers, animations, etc it's unreliable and were the reason for failures.I have not rewritten all usages of with-redefs in this PR. Some tests still pass with
with-redefs
, but that's by chance, it's not what you think.Don't take my word! It's easy to reproduce:
Not waiting on
wait-for
When
test-helpers.component/wait-for
is used, subsequent assertions/etc should be done after the promise returned bywait-for
is resolved. But remember to not perform side-effects inside thewait-for
callback (check out the docs https://callstack.github.io/react-native-testing-library/docs/api#waitfor). Most, if not all of our usages ofwait-for
were not waiting. I get the impression ClojureScript confuses JS devs, because in Javascript it's more common for devs to remember to useawait
.Many tests do work without waiting, but I've seen time and time again tests behaving erratically while working on this PR.
We should strive for correctness over convenience if we want to minimize this class of problem, so await (use
.then
) afterwait-for
.🎁 1 - Silence Jest on demand
If you need to re-run component tests frequently, like me, you may want to reduce the output verbosity. By passing
JEST_USE_SILENT_REPORTER=true
tomake component-test
ormake component-test-watch
you will see a lot less noise and be able to focus on what really matters to you.🎁 2 - Selectively focus/disable tests
I know this is a given when using Jest outside CLJS, but because of our need to first compile CLJS to JS before running tests via Jest, we couldn't easily skip or focus on specific tests. This changes now!
From this PR onwards, we should never again have to change the list of requires in files
core_spec.cljs
. Commenting out required namespaces gives a bad DX because it causes constant rebasing issues.This is the screenshot for skipping the only test I couldn't fix yet. I'll do it in a follow-up, or at least try 🤷
🎁 3 - Translations now work as in prod code (but only English)
Translations performed by
*-by-translation-text
can be done now without any workaround under the hood. The query functions are now linted just likei18n/label
, which means static translation keywords must be qualified with:t/
, which is good for consistency.Steps to test
There's nothing to manually test from a QA perspective. Changes were made almost exclusively in test code. I believe my manual smoke checks and e2e tests are sufficient.
status: ready