Make integration tests more enjoyable to use #19025

ilmotta · 2024-02-27T18:50:15Z

Summary

This PR brings numerous improvements to integration tests. The next step would be to apply the same improvements to contract tests, but I preferred to not touch them here because they're changing more often (manual rebase issues) and because this PR is already reasonably large.

Improvements:

Setting up the application and logged account per test is now done with an async test fixture, which is a very idiomatic way to solve this problem. No need anymore to write macros to wrap day8.re-frame.test/wait-for. The macros in test-helpers.integration will be removed once we apply the same improvements to contract tests.
Integration test timeouts can be controlled per test, with a configurable, global default (60s).
Now the integration test suite will fail-fast by default, i.e. a test failure short-circuits the entire suite immediately. This option can be overridden on a test-by-test basis. This improvement is very useful when investigating failures because the error will be shown on the spot, with no need to search backwards across lots of logs. Also, no need to wait for the whole suite to finish.
Noisy messages from re-frame can be silenced with a test fixture. We can silence even more in the future if we remove the hardcoded printf call from C++ on every signal and control it with Clojure (confirmed with Andrea this is fine). Everything is just as noisy as before this PR. It's just that now we have a better way to control it during development. We can disable most logs as well with the more direct (status-im.common.log/setup "ERROR") at the top of tests.integration-test.core. We can make verbosity even more convenient to control, but I think this should be designed outside this PR.
Removed dependency on lib day8.re-frame/test for integration tests (see detailed explanation below).
Each call to (our) wait-for can customize the timeout to process re-frame event IDs passed to it.
Syntax is now flat, instead of being nested on every call to wait-for. You can now compose other async operations anywhere in a test. This is what allows us to timeout individual wait-for calls and the whole test integration.

Notes

This PR is using the Promesa library, which will be included by PR Add Promesa to simplify working with promises #18767. But because that PR is stalled waiting for QA, I decided to open this PR anyway. Once that one is merged, I will remove the temporary commit I used in this PR to add promesa. Or perhaps better, we could consider just merging this PR because it doesn't need to be QAed and then we get Promesa in as well. Another option is to have a quick PR just adding promesa and merge it real fast because there would be no point in QA'ing it, then this PR and 18767 rebase against develop. Wdyt is the best course of action @clauxx?
promesa.core/do is essential in the integration test suite, as it makes sync & async operations play nice with each other without the developer having to promisify anything manually.
I see lots of logs related to async storage (unexpected token u in JSON at position...). I didn't try to fix this, but it would be super nice if we do in the future.

Are we not going to use `day8.re-frame.test`?

We don't need this library in integration tests and we won't need it in contract tests (follow-up PR). Whether it will be useful after we remove it from integration and contract tests is yet to be seen (probably not).

A few reasons:

The async model of promises is well understood and battle tested. The one devised in the lib is poorly understood and rigid.
There's basically no way to correctly integrate other async operations in the test, unless they can be fully controlled via re-frame events. For instance, how would you control timeouts? How would you retry in a test? How would forcefully delay an operation by a few seconds? These things are useful (to me at least) when developing integration/contract tests.
Every call to day8.re-frame.test/wait-for forces you to nest code one more level. Code readability suffers from that choice.
Have you ever looked up the implementation of wait-for? It's quite convoluted. One could say the source code is not that important, but many times I had to look it up because I couldn't understand the async model they built with their macro approach. The de facto primitive in JS for asynchronicity is promises, and we fully leverage it in this PR.
The lib has an interesting macro run-test-sync, but we have no usage for it. I used it in status-mobile for a while. At one point, all event unit tests for the Activity Center used it (e.g. commit 08fb0de), but I replaced them with the simpler pure function style.

@ulisesmac, @mohsen-ghafouri you were removed as reviewers due to the 15 reviewer limit. Your feedback is just as welcome :)

Areas that may be impacted

None. Only integration tests were touched. If they pass, the PR is good to go.

Steps to test

make test-integration or make test

status: ready

status-im-auto · 2024-02-27T18:55:56Z

Jenkins Builds

Click to see older builds (19)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`fe98ab9`	#1	2024-02-27 18:55:55	~5 min	`tests`	📄`log`
✔️	`fe98ab9`	#1	2024-02-27 18:58:01	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`fe98ab9`	#1	2024-02-27 18:58:10	~7 min	`android`	🤖`apk` 📲
✔️	`fe98ab9`	#1	2024-02-27 18:59:40	~9 min	`ios`	📱`ipa` 📲

✔️	`4c98312`	#2	2024-02-29 13:33:48	~6 min	`tests`	📄`log`
✔️	`4c98312`	#2	2024-02-29 13:36:06	~8 min	`android-e2e`	🤖`apk` 📲
✔️	`4c98312`	#2	2024-02-29 13:36:09	~8 min	`android`	🤖`apk` 📲
✔️	`4c98312`	#2	2024-02-29 13:41:10	~13 min	`ios`	📱`ipa` 📲

✔️	`41fa08b`	#3	2024-03-01 15:38:20	~7 min	`android`	🤖`apk` 📲
✔️	`41fa08b`	#3	2024-03-01 15:38:23	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`41fa08b`	#3	2024-03-01 15:41:05	~10 min	`ios`	📱`ipa` 📲

✔️	`5e2114d`	#4	2024-03-01 18:20:10	~6 min	`tests`	📄`log`
✔️	`5e2114d`	#4	2024-03-01 18:20:11	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`5e2114d`	#4	2024-03-01 18:20:54	~7 min	`android`	🤖`apk` 📲
✔️	`5e2114d`	#4	2024-03-01 18:26:16	~12 min	`ios`	📱`ipa` 📲

✔️	`79bf133`	#5	2024-03-04 13:54:04	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`79bf133`	#5	2024-03-04 13:54:14	~6 min	`android`	🤖`apk` 📲
✔️	`79bf133`	#5	2024-03-04 13:54:52	~7 min	`tests`	📄`log`
✔️	`79bf133`	#5	2024-03-04 13:57:50	~10 min	`ios`	📱`ipa` 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`c8ef90f`	#6	2024-03-05 00:42:46	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`c8ef90f`	#6	2024-03-05 00:42:46	~7 min	`android`	🤖`apk` 📲
✔️	`c8ef90f`	#6	2024-03-05 00:43:16	~7 min	`ios`	📱`ipa` 📲

✔️	`f4781c9`	#8	2024-03-05 01:17:08	~6 min	`tests`	📄`log`
✔️	`f4781c9`	#8	2024-03-05 01:18:09	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`f4781c9`	#8	2024-03-05 01:18:28	~7 min	`android`	🤖`apk` 📲
✔️	`f4781c9`	#8	2024-03-05 01:18:52	~7 min	`ios`	📱`ipa` 📲

clauxx · 2024-02-29T13:43:40Z

This PR is using the Promesa library, which will be included by PR #18767. But because that PR is stalled waiting for QA, I decided to open this PR anyway. Once that one is merged, I will remove the temporary commit I used in this PR to add promesa. Or perhaps better, we could consider just merging this PR because it doesn't need to be QAed and then we get Promesa in as well. Another option is to have a quick PR just adding promesa and merge it real fast because there would be no point in QA'ing it, then this PR and 18767 rebase against develop. Wdyt is the best course of action @clauxx?

Either works for me. About that PR, if this one gets merged before it, will just close it afterwards. The changes there were mostly for demonstration purposes.

clauxx

Looks sooo much nicer to work with ❤️‍🔥

also, regarding adding the whole team as reviewers, I think you can just add status-mobile/mobile-devs and status-mobile/wallet-mobile-devs without adding each dev manually and hitting the reviewer limit.

src/test_helpers/integration.cljs

src/tests/integration_test/chat_test.cljs

seanstrom

This is very cool ✅, thanks for setting this up 🙏

seanstrom · 2024-03-01T11:57:37Z

src/test_helpers/integration.cljs

+
+;;;; Fixtures
+
+(defn fixture-logged


small nit comment: Reading fixture-logged makes me think this might have something to do with logging of events or something. Perhaps this could be fixture-login-logout or fixture-session? Thoughts?

I like your fixture-session suggestion. Thanks @seanstrom

ulisesmac

Sorry for the late review @ilmotta

It looks a lot better! 💯
I hope this API continues improving as we create more tests

ulisesmac · 2024-03-01T20:46:13Z

src/test_helpers/integration.cljs

+
+;;;; Fixtures
+
+(defn fixture-session


While trying to use async fixtures I remembered of your suggestion in one of our Discord channels. Thanks @ulisesmac

ulisesmac · 2024-03-01T20:46:37Z

src/tests/integration_test/chat_test.cljs

+(use-fixtures :each (h/fixture-session))
+


J-Son89

Thanks for this @ilmotta - I'll wait with my pr until this one is merged! Much better! 🙏

smohamedjavid

Clean and tidy 🚀
Thanks a lot for this PR! ❤️

clauxx · 2024-03-04T12:00:52Z

@ilmotta the promesa PR passed the QA and was merged, so I guess you can rebase

…ssages

ilmotta requested a review from jakubgs as a code owner February 27, 2024 18:50

ilmotta force-pushed the ilmotta-18676/make-integration-tests-more-enjoyable branch from fe98ab9 to 4c98312 Compare February 29, 2024 13:26

ilmotta changed the title ~~[WIP] Make integration tests more enjoyable to use~~ Make integration tests more enjoyable to use Feb 29, 2024

ilmotta added skip-manual-qa integration-tests developer-xp labels Feb 29, 2024

ilmotta self-assigned this Feb 29, 2024

ilmotta requested review from alwx, cammellos, shivekkhurana, vkjr, yqrashawn, briansztamfater, ajayesivan, smohamedjavid, clauxx, J-Son89, BalogunofAfrica, mohsen-ghafouri and ulisesmac February 29, 2024 13:41

ilmotta requested review from seanstrom, FFFra and OmarBasem and removed request for mohsen-ghafouri and ulisesmac February 29, 2024 14:55

clauxx approved these changes Mar 1, 2024

View reviewed changes

yqrashawn approved these changes Mar 1, 2024

View reviewed changes

seanstrom approved these changes Mar 1, 2024

View reviewed changes

ilmotta force-pushed the ilmotta-18676/make-integration-tests-more-enjoyable branch 2 times, most recently from 41fa08b to 5e2114d Compare March 1, 2024 18:13

ilmotta mentioned this pull request Mar 1, 2024

add integration test for send #18910

Closed

ulisesmac approved these changes Mar 1, 2024

View reviewed changes

J-Son89 approved these changes Mar 1, 2024

View reviewed changes

smohamedjavid approved these changes Mar 4, 2024

View reviewed changes

ilmotta force-pushed the ilmotta-18676/make-integration-tests-more-enjoyable branch from 5e2114d to 79bf133 Compare March 4, 2024 13:47

OmarBasem approved these changes Mar 4, 2024

View reviewed changes

ilmotta added 13 commits March 4, 2024 21:35

Rewrite integration tests without lib day8.re-frame/test

2ce3496

Rewrite tests to use promesa

43835b3

Log full keyword instead of just its name

63dcf6e

Respect order of expected event IDs

b3564e8

Renaming and reject with a better error message

d0c3a54

Fail fast when int tests fail and support silencing re-frame debug me…

6bd75cf

…ssages

Make fail-fast behavior configurable and merge two functions

fcf84d7

Update zprint rule for h/integration-test

9cbbc7f

Add docstrings to fixtures

097f4ad

Allow integration tests to time out (default 60s)

389b40a

Improve docstrings

8865e0d

Rename fixture

d658fa4

Document the done function will be passed to f

33f4c35

ilmotta force-pushed the ilmotta-18676/make-integration-tests-more-enjoyable branch 2 times, most recently from c8ef90f to e76a48f Compare March 5, 2024 01:10

Use promesa create instead of interop

f4781c9

ilmotta force-pushed the ilmotta-18676/make-integration-tests-more-enjoyable branch from e76a48f to f4781c9 Compare March 5, 2024 01:10

ilmotta merged commit 5d1e1f8 into develop Mar 5, 2024
6 checks passed

ilmotta deleted the ilmotta-18676/make-integration-tests-more-enjoyable branch March 5, 2024 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make integration tests more enjoyable to use #19025

Make integration tests more enjoyable to use #19025

ilmotta commented Feb 27, 2024 •

edited

Loading

status-im-auto commented Feb 27, 2024 •

edited

Loading

clauxx commented Feb 29, 2024 •

edited

Loading

clauxx left a comment

seanstrom left a comment

seanstrom Mar 1, 2024

ilmotta Mar 1, 2024

ulisesmac left a comment

ulisesmac Mar 1, 2024

ilmotta Mar 1, 2024

ulisesmac Mar 1, 2024

J-Son89 left a comment

smohamedjavid left a comment

clauxx commented Mar 4, 2024


		;;;; Fixtures

		(defn fixture-logged


		;;;; Fixtures

		(defn fixture-session

Make integration tests more enjoyable to use #19025

Make integration tests more enjoyable to use #19025

Conversation

ilmotta commented Feb 27, 2024 • edited Loading

Summary

Notes

Are we not going to use day8.re-frame.test?

Areas that may be impacted

Steps to test

status-im-auto commented Feb 27, 2024 • edited Loading

Jenkins Builds

clauxx commented Feb 29, 2024 • edited Loading

clauxx left a comment

Choose a reason for hiding this comment

seanstrom left a comment

Choose a reason for hiding this comment

seanstrom Mar 1, 2024

Choose a reason for hiding this comment

ilmotta Mar 1, 2024

Choose a reason for hiding this comment

ulisesmac left a comment

Choose a reason for hiding this comment

ulisesmac Mar 1, 2024

Choose a reason for hiding this comment

ilmotta Mar 1, 2024

Choose a reason for hiding this comment

ulisesmac Mar 1, 2024

Choose a reason for hiding this comment

J-Son89 left a comment

Choose a reason for hiding this comment

smohamedjavid left a comment

Choose a reason for hiding this comment

clauxx commented Mar 4, 2024

ilmotta commented Feb 27, 2024 •

edited

Loading

Are we not going to use `day8.re-frame.test`?

status-im-auto commented Feb 27, 2024 •

edited

Loading

clauxx commented Feb 29, 2024 •

edited

Loading