Adjust tests to the batching updates feature #27230

Szymon20000 · 2023-09-12T11:00:17Z

The purpose of this pull-request is to prepare app tests for batching update feature link.
Before that feature all Onyx.set or Onyx.merge were notifying its subscribers about a change right away.
Once we apply batching solution the notifications can be delayed and at the end of the current micro task cycle.
Because of that we had to reimplement waitForPromisesToResolve and sometimes use it twice in a row.

Details

All network requests in tests now require 1 micro task and then one macro task which is setTimout. So to properly wait for onyx updates caused by network request we need to call waitForPromisesToResolve twice in a row.
when using fake timers we need to call jest.runOnlyPendingTimers() after every onyx update so that setTimeout used by batching logic is executed. In order to reduce the number of places where we have to use it we use real timer by default and fake timers only when time precision is needed to make the test stable.

Fixed Issues

$ #27470
PROPOSAL:

Tests

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

…f/onyx-upgrade-react-batching

melvin-bot · 2023-09-12T11:38:44Z

@pecanoro Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

tests/actions/ReportTest.js

tests/utils/waitForPromisesToResolve.js

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

janicduplessis · 2023-09-14T15:52:02Z

tests/actions/ReportTest.js

-    beforeEach(() => Onyx.clear().then(waitForPromisesToResolve));
+    beforeEach(() => {
+        const promise = Onyx.clear().then(jest.useRealTimers);
+        waitForPromisesToResolve();


Should that be .then(wairForPromisesToResolve)?

Actually the purpose is to flush timers if we use fake timers.
I change it so it's hopefully more clear now

janicduplessis · 2023-09-14T15:58:46Z

tests/utils/fastForwardTwoMicrotasksCycles.js

+ *
+ * @returns {Promise}
+ */
+export default () => waitForPromisesToResolve().then(waitForPromisesToResolve);


Should we call this something like waitForOnyxPromise that is onyx specific as fastForwardTwoMicrotasksCycles sounds too implementation specific.

No Idea to be honest. @tgolen WDYT?

I really like the idea of having waitForNetworkPromise and waitForOnyxPromise. Would that work?

janicduplessis · 2023-09-14T15:59:53Z

tests/unit/APITest.js

                    // We need to advance past the request throttle back off timer because the request won't be retried until then
-                    jest.advanceTimersByTime(CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS);
-                    return waitForPromisesToResolve();
+                    return new Promise((resolve) => setTimeout(resolve, 1000)).then(waitForPromisesToResolve);


This isn't going to actually take 1s to resolve right?

True! Fixed!

mvtglobally · 2023-09-18T14:26:20Z

QA Regression is completed on this Adhock build @tgolen @mountiny

allroundexperts · 2023-09-18T17:46:14Z

@allroundexperts would you be able to help with this?

Just started my day @mountiny. I'll confirm @ospfranco's findings.

OSBotify · 2023-09-19T03:18:02Z

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/27230/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/27230/index.html

desktop 💻	web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/27230/NewExpensify.dmg	https://27230.pr-testing.expensify.com

marcaaron

Changes LGTM - I think at this point we are just waiting to confirm whether any fix is needed for a regression.

src/libs/actions/PersistedRequests.js

marcaaron · 2023-09-19T05:36:37Z

src/libs/actions/PersistedRequests.js

@@ -10,7 +10,7 @@ Onyx.connect({
 });

 function clear() {


I believe we are trying to avoid using that and prefer to wait for the Onyx method to finish because waitForPromisesToResolve is not compatible with the batched updates solution.

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

# Conflicts: # package-lock.json # package.json

ospfranco · 2023-09-20T06:53:58Z

Seems some of the issues where caused by errors on the main branch. E.g. 2FA setup was caused by a FocusTrap component that crashed with a null ref, it was reverted on main and I've now updated this branch to main. I'm testing again, and will take a look into remaining issues:

Emoji - The previously tone appears for a few seconds when fast switching between skin tones
❌ Not reproducible on main
❌ Not reproducible on branch

Security - 2FA setup seems that it is not completed, finish button does not continue process
❌ Not reproducible on main
❌ Not reproducible on branch

Workspace - User is redirected to /not-found page after refreshing workspace chat page
❌ Not reproducible on main
❌ Not reproducible on branch

IOU - When you return to selecting RM amount and continue, the request will be reset
❌ Not reproducible on main
❌ Not reproducible on branch

IOU - Incorrect animation display when selecting Split bill members
❌ Not reproducible on main
❌ Not reproducible on branch

Task - Task reverts to previous state briefly when checking or unchecking checkbox
❌ Not reproducible on main
❌ Not reproducible on branch

Preferences - Priority mode reverts to #focus briefly when changing it to Most recent
❌ Not reproducible on main
❌ Not reproducible on branch

mountiny

Thanks for the patience on this one!

OSBotify · 2023-09-20T09:02:03Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.72-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

situchan · 2023-09-21T11:43:20Z

I suggest to retest all bugs what QA team found in this PR build.
i.e. #27230 (comment) was reproducible on this branch.
And it now became deploy blocker - #27928

thienlnam · 2023-09-21T11:55:28Z

^ @Szymon20000 Are you able to investigate this?

Szymon20000 · 2023-09-21T12:02:43Z

I'm busy in another project but @ospfranco has better context.

ospfranco · 2023-09-21T12:06:02Z

Just tested #27230 (comment) and indeed it is not working, but it doesn't look like it is the same problem that we ignored because of the < FocusTrap/> component that got reverted. The screens mount but nothing is thrown or happening.

ospfranco · 2023-09-21T12:35:55Z

Found the problem and a potential solution

#27938

@situchan @thienlnam @mountiny

OSBotify · 2023-09-22T13:25:01Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.72-11 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-09-25T10:09:27Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.74-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-09-28T18:51:48Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.74-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

hannojg and others added 17 commits August 28, 2023 15:48

fix tests

9297f16

fix waitForPromise method to exhaust microtask queue

9a3cfe5

fix Emoji tests

c4c241e

fix SidebarFilterTest

b76b005

wip

66c995f

Merge branch 'main' of github.com:margelo/expensify-app-fork into per…

478ff87

…f/onyx-upgrade-react-batching

Merge branch 'main' into perf/onyx-upgrade-react-batching

19de1d8

stash work

ac31c74

stash work

5200d56

remove console.logs

e717c1e

make FileUtilsTest more stable

fba544d

make it more stable

0185bbf

better naming

5b80d4b

remove unnecessary waitForPromise

2f2f7b1

cleanup

098375f

Merge branch 'main' into perf/onyx-upgrade-react-batching

3a90043

prettier

d2aa316

Szymon20000 changed the title ~~Adjust tests to batching updates~~ Adjust tests to the batching updates feature Sep 12, 2023

Szymon20000 marked this pull request as ready for review September 12, 2023 11:38

Szymon20000 requested a review from a team as a code owner September 12, 2023 11:38

melvin-bot bot removed the request for review from a team September 12, 2023 11:38

melvin-bot bot requested a review from pecanoro September 12, 2023 11:38

marcaaron requested changes Sep 12, 2023

View reviewed changes

tests/actions/ReportTest.js Outdated Show resolved Hide resolved

tests/utils/waitForPromisesToResolve.js Outdated Show resolved Hide resolved

Szymon20000 and others added 3 commits September 14, 2023 13:11

Update tests/utils/waitForPromisesToResolve.js

36a390a

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Update tests/actions/ReportTest.js

c5a7d40

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

clean up

577fda3

Szymon20000 mentioned this pull request Sep 14, 2023

Batch updates second try Expensify/react-native-onyx#315

Merged

janicduplessis suggested changes Sep 14, 2023

View reviewed changes

clean up

dcc2ab2

marcaaron previously approved these changes Sep 19, 2023

View reviewed changes

mountiny mentioned this pull request Sep 19, 2023

Use initialValue in withOnyx and other optimizations for ReportScreen #26772

Merged

59 tasks

Update src/libs/actions/PersistedRequests.js

225ffe1

Co-authored-by: Marc Glasser <marc.aaron.glasser@gmail.com>

Szymon20000 dismissed stale reviews from marcaaron and tgolen via 225ffe1 September 19, 2023 08:32

blazejkustra mentioned this pull request Sep 19, 2023

[NoQA] [TS migration] Migrate 'CloseAccount.js' lib to TypeScript #27355

Merged

58 tasks

mountiny mentioned this pull request Sep 20, 2023

Add focus trap to the RHP (v2) #27670

Merged

Merge branch 'main' into perf/onyx-upgrade-react-batching

e0e31ce

# Conflicts: # package-lock.json # package.json

fix: return types are not allowe isssue

7869c5a

mountiny approved these changes Sep 20, 2023

View reviewed changes

mountiny merged commit d763b8d into Expensify:main Sep 20, 2023

OSBotify mentioned this pull request Sep 20, 2023

Deploy Checklist: New Expensify 2023-09-20 #27837

Closed

situchan mentioned this pull request Sep 21, 2023

[HOLD for payment 2023-10-02] [HOLD for payment 2023-09-29] Pressing 'Next' while setting up 2FA does not redirect the user #27928

Closed

6 tasks

ospfranco mentioned this pull request Sep 21, 2023

[CP Staging] Add currentStep to navigation hook to prevent lock in 2fa setup step #27938

Merged

59 tasks

melvin-bot bot mentioned this pull request Sep 22, 2023

[HOLD for payment 2023-09-29] Tests in the App repo are not passing when we use onyx version with Batched updates #27470

Closed

6 tasks

This was referenced Sep 25, 2023

Deploy Checklist: New Expensify DUPE 2023-09-25 #28118

Closed

Deploy Checklist: New Expensify 2023-09-25 #28119

Closed

Adjust tests to the batching updates feature #27230

Adjust tests to the batching updates feature #27230

Uh oh!

Conversation

Szymon20000 commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Sep 12, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvtglobally commented Sep 18, 2023

Uh oh!

allroundexperts commented Sep 18, 2023

Uh oh!

OSBotify commented Sep 19, 2023

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ospfranco commented Sep 20, 2023

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Sep 20, 2023

Uh oh!

situchan commented Sep 21, 2023

Uh oh!

thienlnam commented Sep 21, 2023

Uh oh!

Szymon20000 commented Sep 21, 2023

Uh oh!

ospfranco commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ospfranco commented Sep 21, 2023

Uh oh!

OSBotify commented Sep 22, 2023

Uh oh!

OSBotify commented Sep 25, 2023

Uh oh!

OSBotify commented Sep 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

Szymon20000 commented Sep 12, 2023 •

edited

Loading

ospfranco commented Sep 21, 2023 •

edited

Loading