From 068e62411f6ee2af6ffb0a3ddc04128061f69478 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 5 Apr 2024 08:18:54 +0200 Subject: [PATCH 01/37] Revert "Merge pull request #39668 from Expensify/revert-38997-@chrispader/prevent-simultaneous-calls-to-GetMissingOnyxMessages" This reverts commit 3864cdbbcbd672d5d434076c5ebf79c63f1bf4d7, reversing changes made to 42ee04cdeb957c2234ef7f0aad5499fd805f5f3f. --- src/libs/actions/OnyxUpdateManager.ts | 162 ++++++++++++++++++++++++-- 1 file changed, 152 insertions(+), 10 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index 9c6f30cc5e9e4..45cb2b78ecde8 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -3,6 +3,7 @@ import Log from '@libs/Log'; import * as SequentialQueue from '@libs/Network/SequentialQueue'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; +import type {OnyxUpdatesFromServer, Response} from '@src/types/onyx'; import * as App from './App'; import * as OnyxUpdates from './OnyxUpdates'; @@ -27,6 +28,134 @@ Onyx.connect({ callback: (value) => (lastUpdateIDAppliedToClient = value), }); +let queryPromise: Promise | undefined; + +type DeferredUpdatesDictionary = Record; +let deferredUpdates: DeferredUpdatesDictionary = {}; + +// This function will reset the query variables, unpause the SequentialQueue and log an info to the user. +function finalizeUpdatesAndResumeQueue() { + console.debug('[OnyxUpdateManager] Done applying all updates'); + queryPromise = undefined; + deferredUpdates = {}; + Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, null); + SequentialQueue.unpause(); +} + +// This function applies a list of updates to Onyx in order and resolves when all updates have been applied +const applyUpdates = (updates: DeferredUpdatesDictionary) => Promise.all(Object.values(updates).map((update) => OnyxUpdates.apply(update))); + +// In order for the deferred updates to be applied correctly in order, +// we need to check if there are any gaps between deferred updates. +type DetectGapAndSplitResult = {applicableUpdates: DeferredUpdatesDictionary; updatesAfterGaps: DeferredUpdatesDictionary; latestMissingUpdateID: number | undefined}; +function detectGapsAndSplit(updates: DeferredUpdatesDictionary): DetectGapAndSplitResult { + const updateValues = Object.values(updates); + const applicableUpdates: DeferredUpdatesDictionary = {}; + + let gapExists = false; + let firstUpdateAfterGaps: number | undefined; + let latestMissingUpdateID: number | undefined; + + for (const [index, update] of updateValues.entries()) { + const isFirst = index === 0; + + // If any update's previousUpdateID doesn't match the lastUpdateID from the previous update, the deferred updates aren't chained and there's a gap. + // For the first update, we need to check that the previousUpdateID of the fetched update is the same as the lastUpdateIDAppliedToClient. + // For any other updates, we need to check if the previousUpdateID of the current update is found in the deferred updates. + // If an update is chained, we can add it to the applicable updates. + const isChained = isFirst ? update.previousUpdateID === lastUpdateIDAppliedToClient : !!updates[Number(update.previousUpdateID)]; + if (isChained) { + // If a gap exists already, we will not add any more updates to the applicable updates. + // Instead, once there are two chained updates again, we can set "firstUpdateAfterGaps" to the first update after the current gap. + if (gapExists) { + // If "firstUpdateAfterGaps" hasn't been set yet and there was a gap, we need to set it to the first update after all gaps. + if (!firstUpdateAfterGaps) { + firstUpdateAfterGaps = Number(update.previousUpdateID); + } + } else { + // If no gap exists yet, we can add the update to the applicable updates + applicableUpdates[Number(update.lastUpdateID)] = update; + } + } else { + // When we find a (new) gap, we need to set "gapExists" to true and reset the "firstUpdateAfterGaps" variable, + // so that we can continue searching for the next update after all gaps + gapExists = true; + firstUpdateAfterGaps = undefined; + + // If there is a gap, it means the previous update is the latest missing update. + latestMissingUpdateID = Number(update.previousUpdateID); + } + } + + // When "firstUpdateAfterGaps" is not set yet, we need to set it to the last update in the list, + // because we will fetch all missing updates up to the previous one and can then always apply the last update in the deferred updates. + if (!firstUpdateAfterGaps) { + firstUpdateAfterGaps = Number(updateValues[updateValues.length - 1].lastUpdateID); + } + + let updatesAfterGaps: DeferredUpdatesDictionary = {}; + if (gapExists && firstUpdateAfterGaps) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + updatesAfterGaps = Object.fromEntries(Object.entries(updates).filter(([lastUpdateID]) => Number(lastUpdateID) >= firstUpdateAfterGaps!)); + } + + return {applicableUpdates, updatesAfterGaps, latestMissingUpdateID}; +} + +// This function will check for gaps in the deferred updates and +// apply the updates in order after the missing updates are fetched and applied +function validateAndApplyDeferredUpdates(): Promise { + // We only want to apply deferred updates that are newer than the last update that was applied to the client. + // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out. + const pendingDeferredUpdates = Object.fromEntries( + Object.entries(deferredUpdates).filter(([lastUpdateID]) => { + // It should not be possible for lastUpdateIDAppliedToClient to be null, + // after the missing updates have been applied. + // If still so we want to keep the deferred update in the list. + if (!lastUpdateIDAppliedToClient) { + return true; + } + return (Number(lastUpdateID) ?? 0) > lastUpdateIDAppliedToClient; + }), + ); + + // If there are no remaining deferred updates after filtering out outdated ones, + // we can just unpause the queue and return + if (Object.values(pendingDeferredUpdates).length === 0) { + return Promise.resolve(); + } + + const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(pendingDeferredUpdates); + + // If we detected a gap in the deferred updates, only apply the deferred updates before the gap, + // re-fetch the missing updates and then apply the remaining deferred updates after the gap + if (latestMissingUpdateID) { + return new Promise((resolve, reject) => { + deferredUpdates = {}; + applyUpdates(applicableUpdates).then(() => { + // After we have applied the applicable updates, there might have been new deferred updates added. + // In the next (recursive) call of "validateAndApplyDeferredUpdates", + // the initial "updatesAfterGaps" and all new deferred updates will be applied in order, + // as long as there was no new gap detected. Otherwise repeat the process. + deferredUpdates = {...deferredUpdates, ...updatesAfterGaps}; + + // It should not be possible for lastUpdateIDAppliedToClient to be null, therefore we can ignore this case. + // If lastUpdateIDAppliedToClient got updated in the meantime, we will just retrigger the validation and application of the current deferred updates. + if (!lastUpdateIDAppliedToClient || latestMissingUpdateID <= lastUpdateIDAppliedToClient) { + validateAndApplyDeferredUpdates().then(resolve).catch(reject); + return; + } + + // Then we can fetch the missing updates and apply them + App.getMissingOnyxUpdates(lastUpdateIDAppliedToClient, latestMissingUpdateID).then(validateAndApplyDeferredUpdates).then(resolve).catch(reject); + }); + }); + } + + // If there are no gaps in the deferred updates, we can apply all deferred updates in order + return applyUpdates(applicableUpdates); +} + export default () => { console.debug('[OnyxUpdateManager] Listening for updates from the server'); Onyx.connect({ @@ -66,32 +195,45 @@ export default () => { // applied in their correct and specific order. If this queue was not paused, then there would be a lot of // onyx data being applied while we are fetching the missing updates and that would put them all out of order. SequentialQueue.pause(); - let canUnpauseQueuePromise; // The flow below is setting the promise to a reconnect app to address flow (1) explained above. if (!lastUpdateIDAppliedToClient) { + // If there is a ReconnectApp query in progress, we should not start another one. + if (queryPromise) { + return; + } + Log.info('Client has not gotten reliable updates before so reconnecting the app to start the process'); // Since this is a full reconnectApp, we'll not apply the updates we received - those will come in the reconnect app request. - canUnpauseQueuePromise = App.finalReconnectAppAfterActivatingReliableUpdates(); + queryPromise = App.finalReconnectAppAfterActivatingReliableUpdates(); } else { // The flow below is setting the promise to a getMissingOnyxUpdates to address flow (2) explained above. + + // Get the number of deferred updates before adding the new one + const existingDeferredUpdatesCount = Object.keys(deferredUpdates).length; + + // Add the new update to the deferred updates + deferredUpdates[Number(updateParams.lastUpdateID)] = updateParams; + + // If there are deferred updates already, we don't need to fetch the missing updates again. + if (existingDeferredUpdatesCount > 0) { + return; + } + console.debug(`[OnyxUpdateManager] Client is behind the server by ${Number(previousUpdateIDFromServer) - lastUpdateIDAppliedToClient} so fetching incremental updates`); Log.info('Gap detected in update IDs from server so fetching incremental updates', true, { lastUpdateIDFromServer, previousUpdateIDFromServer, lastUpdateIDAppliedToClient, }); - canUnpauseQueuePromise = App.getMissingOnyxUpdates(lastUpdateIDAppliedToClient, previousUpdateIDFromServer); + + // Get the missing Onyx updates from the server and afterwards validate and apply the deferred updates. + // This will trigger recursive calls to "validateAndApplyDeferredUpdates" if there are gaps in the deferred updates. + queryPromise = App.getMissingOnyxUpdates(lastUpdateIDAppliedToClient, previousUpdateIDFromServer).then(validateAndApplyDeferredUpdates); } - canUnpauseQueuePromise.finally(() => { - OnyxUpdates.apply(updateParams).finally(() => { - console.debug('[OnyxUpdateManager] Done applying all updates'); - Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, null); - SequentialQueue.unpause(); - }); - }); + queryPromise.finally(finalizeUpdatesAndResumeQueue); }, }); }; From 1a8528c9179329e295190bf85a3ecf7ce44ad740 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 5 Apr 2024 11:55:42 +0200 Subject: [PATCH 02/37] remove queue clearing and add TS type guard for OnyxUpdate --- src/libs/actions/OnyxUpdateManager.ts | 22 ++++---------------- src/types/onyx/OnyxUpdatesFromServer.ts | 27 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index 45cb2b78ecde8..5988c40e589c8 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -1,9 +1,9 @@ import Onyx from 'react-native-onyx'; import Log from '@libs/Log'; import * as SequentialQueue from '@libs/Network/SequentialQueue'; -import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer, Response} from '@src/types/onyx'; +import {isValidOnyxUpdateFromServer} from '@src/types/onyx/OnyxUpdatesFromServer'; import * as App from './App'; import * as OnyxUpdates from './OnyxUpdates'; @@ -25,7 +25,7 @@ import * as OnyxUpdates from './OnyxUpdates'; let lastUpdateIDAppliedToClient: number | null = 0; Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => (lastUpdateIDAppliedToClient = value), + callback: (value: number) => (lastUpdateIDAppliedToClient = value), }); let queryPromise: Promise | undefined; @@ -160,22 +160,8 @@ export default () => { console.debug('[OnyxUpdateManager] Listening for updates from the server'); Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_FROM_SERVER, - callback: (value) => { - if (!value) { - return; - } - - // Since we used the same key that used to store another object, let's confirm that the current object is - // following the new format before we proceed. If it isn't, then let's clear the object in Onyx. - if ( - !(typeof value === 'object' && !!value) || - !('type' in value) || - (!(value.type === CONST.ONYX_UPDATE_TYPES.HTTPS && value.request && value.response) && - !((value.type === CONST.ONYX_UPDATE_TYPES.PUSHER || value.type === CONST.ONYX_UPDATE_TYPES.AIRSHIP) && value.updates)) - ) { - console.debug('[OnyxUpdateManager] Invalid format found for updates, cleaning and unpausing the queue'); - Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, null); - SequentialQueue.unpause(); + callback: (value: unknown) => { + if (!isValidOnyxUpdateFromServer(value)) { return; } diff --git a/src/types/onyx/OnyxUpdatesFromServer.ts b/src/types/onyx/OnyxUpdatesFromServer.ts index 3c6933da19ba0..ca88360cb2b3c 100644 --- a/src/types/onyx/OnyxUpdatesFromServer.ts +++ b/src/types/onyx/OnyxUpdatesFromServer.ts @@ -21,4 +21,31 @@ type OnyxUpdatesFromServer = { updates?: OnyxUpdateEvent[]; }; +function isValidOnyxUpdateFromServer(value: unknown): value is OnyxUpdatesFromServer { + if (!value || typeof value !== 'object') { + return false; + } + if (!('type' in value) || !value.type) { + return false; + } + if (value.type === CONST.ONYX_UPDATE_TYPES.HTTPS) { + if (!('request' in value) || !value.request) { + return false; + } + + if (!('response' in value) || !value.response) { + return false; + } + } + if (value.type === CONST.ONYX_UPDATE_TYPES.PUSHER) { + if (!('updates' in value) || !value.updates) { + return false; + } + } + + return true; +} + +export {isValidOnyxUpdateFromServer}; + export type {OnyxUpdatesFromServer, OnyxUpdateEvent, OnyxServerUpdate}; From e52db009eaaddc82963f20ec4c63096499056570 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 5 Apr 2024 12:01:45 +0200 Subject: [PATCH 03/37] fix: type check --- src/libs/actions/OnyxUpdateManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index 5988c40e589c8..affc205898f9a 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -25,7 +25,7 @@ import * as OnyxUpdates from './OnyxUpdates'; let lastUpdateIDAppliedToClient: number | null = 0; Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value: number) => (lastUpdateIDAppliedToClient = value), + callback: (value) => (lastUpdateIDAppliedToClient = value), }); let queryPromise: Promise | undefined; From 2e13b36fe2903ea4e98d50a87c8da7173e4d614b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 5 Apr 2024 12:07:41 +0200 Subject: [PATCH 04/37] simplify code --- src/libs/actions/OnyxUpdateManager.ts | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index affc205898f9a..1468145997648 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -107,17 +107,12 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary): DetectGapAndSpl function validateAndApplyDeferredUpdates(): Promise { // We only want to apply deferred updates that are newer than the last update that was applied to the client. // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out. - const pendingDeferredUpdates = Object.fromEntries( - Object.entries(deferredUpdates).filter(([lastUpdateID]) => { - // It should not be possible for lastUpdateIDAppliedToClient to be null, - // after the missing updates have been applied. - // If still so we want to keep the deferred update in the list. - if (!lastUpdateIDAppliedToClient) { - return true; - } - return (Number(lastUpdateID) ?? 0) > lastUpdateIDAppliedToClient; - }), - ); + const pendingDeferredUpdates = Object.entries(deferredUpdates).reduce((acc, [lastUpdateID, update]) => { + if (!lastUpdateIDAppliedToClient || (Number(lastUpdateID) ?? 0) > lastUpdateIDAppliedToClient) { + acc[Number(lastUpdateID)] = update; + } + return acc; + }, {}); // If there are no remaining deferred updates after filtering out outdated ones, // we can just unpause the queue and return From 4fbbd9acde49213e31c1c006abe1a718f0473817 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 5 Apr 2024 12:12:01 +0200 Subject: [PATCH 05/37] simplify code --- src/libs/actions/OnyxUpdateManager.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index 1468145997648..c5b4abf5888aa 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -95,8 +95,13 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary): DetectGapAndSpl let updatesAfterGaps: DeferredUpdatesDictionary = {}; if (gapExists && firstUpdateAfterGaps) { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - updatesAfterGaps = Object.fromEntries(Object.entries(updates).filter(([lastUpdateID]) => Number(lastUpdateID) >= firstUpdateAfterGaps!)); + updatesAfterGaps = Object.entries(updates).reduce((acc, [lastUpdateID, update]) => { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + if (Number(lastUpdateID) >= firstUpdateAfterGaps!) { + acc[Number(lastUpdateID)] = update; + } + return acc; + }, {}); } return {applicableUpdates, updatesAfterGaps, latestMissingUpdateID}; From 9eaa455e38a19431ce4a852ba68c1e2f6e7fce20 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 5 Apr 2024 12:17:01 +0200 Subject: [PATCH 06/37] fix: and simplify --- src/libs/actions/OnyxUpdateManager.ts | 28 +++++++++++++------------ src/types/onyx/OnyxUpdatesFromServer.ts | 1 + 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index c5b4abf5888aa..275a904736fed 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -95,13 +95,14 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary): DetectGapAndSpl let updatesAfterGaps: DeferredUpdatesDictionary = {}; if (gapExists && firstUpdateAfterGaps) { - updatesAfterGaps = Object.entries(updates).reduce((acc, [lastUpdateID, update]) => { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - if (Number(lastUpdateID) >= firstUpdateAfterGaps!) { - acc[Number(lastUpdateID)] = update; - } - return acc; - }, {}); + updatesAfterGaps = Object.entries(updates).reduce( + (acc, [lastUpdateID, update]) => ({ + ...acc, + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + ...(Number(lastUpdateID) >= firstUpdateAfterGaps! ? {[Number(lastUpdateID)]: update} : {}), + }), + {}, + ); } return {applicableUpdates, updatesAfterGaps, latestMissingUpdateID}; @@ -112,12 +113,13 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary): DetectGapAndSpl function validateAndApplyDeferredUpdates(): Promise { // We only want to apply deferred updates that are newer than the last update that was applied to the client. // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out. - const pendingDeferredUpdates = Object.entries(deferredUpdates).reduce((acc, [lastUpdateID, update]) => { - if (!lastUpdateIDAppliedToClient || (Number(lastUpdateID) ?? 0) > lastUpdateIDAppliedToClient) { - acc[Number(lastUpdateID)] = update; - } - return acc; - }, {}); + const pendingDeferredUpdates = Object.entries(deferredUpdates).reduce( + (acc, [lastUpdateID, update]) => ({ + ...acc, + ...(!lastUpdateIDAppliedToClient || (Number(lastUpdateID) ?? 0) > lastUpdateIDAppliedToClient ? {[Number(lastUpdateID)]: update} : {}), + }), + {}, + ); // If there are no remaining deferred updates after filtering out outdated ones, // we can just unpause the queue and return diff --git a/src/types/onyx/OnyxUpdatesFromServer.ts b/src/types/onyx/OnyxUpdatesFromServer.ts index ca88360cb2b3c..0877ea6755f85 100644 --- a/src/types/onyx/OnyxUpdatesFromServer.ts +++ b/src/types/onyx/OnyxUpdatesFromServer.ts @@ -1,4 +1,5 @@ import type {OnyxUpdate} from 'react-native-onyx'; +import CONST from '@src/CONST'; import type Request from './Request'; import type Response from './Response'; From c1342830cc59b4a7ce34c52e29096428a837a678 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 5 Apr 2024 12:20:57 +0200 Subject: [PATCH 07/37] add comment --- src/libs/actions/OnyxUpdateManager.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index 275a904736fed..d1e2aa3514dc1 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -116,6 +116,9 @@ function validateAndApplyDeferredUpdates(): Promise { const pendingDeferredUpdates = Object.entries(deferredUpdates).reduce( (acc, [lastUpdateID, update]) => ({ ...acc, + // It should not be possible for lastUpdateIDAppliedToClient to be null, + // after the missing updates have been applied. + // If still so we want to keep the deferred update in the list. ...(!lastUpdateIDAppliedToClient || (Number(lastUpdateID) ?? 0) > lastUpdateIDAppliedToClient ? {[Number(lastUpdateID)]: update} : {}), }), {}, From 36c9ad61dcf7fbe6aa6da925bbb165770da9374a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Fri, 5 Apr 2024 12:21:40 +0200 Subject: [PATCH 08/37] rename variables --- src/libs/actions/OnyxUpdateManager.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index d1e2aa3514dc1..7179cb855c90d 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -96,8 +96,8 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary): DetectGapAndSpl let updatesAfterGaps: DeferredUpdatesDictionary = {}; if (gapExists && firstUpdateAfterGaps) { updatesAfterGaps = Object.entries(updates).reduce( - (acc, [lastUpdateID, update]) => ({ - ...acc, + (accUpdates, [lastUpdateID, update]) => ({ + ...accUpdates, // eslint-disable-next-line @typescript-eslint/no-non-null-assertion ...(Number(lastUpdateID) >= firstUpdateAfterGaps! ? {[Number(lastUpdateID)]: update} : {}), }), @@ -114,8 +114,8 @@ function validateAndApplyDeferredUpdates(): Promise { // We only want to apply deferred updates that are newer than the last update that was applied to the client. // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out. const pendingDeferredUpdates = Object.entries(deferredUpdates).reduce( - (acc, [lastUpdateID, update]) => ({ - ...acc, + (accUpdates, [lastUpdateID, update]) => ({ + ...accUpdates, // It should not be possible for lastUpdateIDAppliedToClient to be null, // after the missing updates have been applied. // If still so we want to keep the deferred update in the list. From 48fa52e9685d90e68aed378e77871fcafa166679 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 15 Apr 2024 15:17:18 +0200 Subject: [PATCH 09/37] remove unnecessary pause --- src/libs/actions/OnyxUpdateManager.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index 384ae00cf7e04..aa40ca702b9a2 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -175,6 +175,7 @@ export default () => { Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_FROM_SERVER, callback: (value: unknown) => { + // When there is no value or an invalid value, there's nothing to process, so let's return early. if (!isValidOnyxUpdateFromServer(value)) { return; } @@ -207,11 +208,6 @@ export default () => { // fully migrating to the reliable updates mode. // 2. This client already has the reliable updates mode enabled, but it's missing some updates and it // needs to fetch those. - // - // For both of those, we need to pause the sequential queue. This is important so that the updates are - // applied in their correct and specific order. If this queue was not paused, then there would be a lot of - // onyx data being applied while we are fetching the missing updates and that would put them all out of order. - SequentialQueue.pause(); // The flow below is setting the promise to a reconnect app to address flow (1) explained above. if (!lastUpdateIDAppliedToClient) { From beb16a3fa14c7e51707a5fa1a2b23843df5ff49d Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 15 Apr 2024 15:18:14 +0200 Subject: [PATCH 10/37] check for isLoadingApp and ActiveClientManager first --- src/libs/actions/OnyxUpdateManager.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index aa40ca702b9a2..8714f4f6838c8 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -175,11 +175,6 @@ export default () => { Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_FROM_SERVER, callback: (value: unknown) => { - // When there is no value or an invalid value, there's nothing to process, so let's return early. - if (!isValidOnyxUpdateFromServer(value)) { - return; - } - // If isLoadingApp is positive it means that OpenApp command hasn't finished yet, and in that case // we don't have base state of the app (reports, policies, etc) setup. If we apply this update, // we'll only have them overriten by the openApp response. So let's skip it and return. @@ -197,6 +192,11 @@ export default () => { return; } + // When there is no value or an invalid value, there's nothing to process, so let's return early. + if (!isValidOnyxUpdateFromServer(value)) { + return; + } + const updateParams = value; const lastUpdateIDFromServer = value.lastUpdateID; const previousUpdateIDFromServer = value.previousUpdateID; From 26ded32c930227a9d377e27651a12cad663feeca Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sun, 21 Apr 2024 20:33:38 +0200 Subject: [PATCH 11/37] minor improvements of OnyxUpdateMangaer --- src/libs/actions/OnyxUpdateManager.ts | 28 +++++++++++++-------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index 71b0eda3465fb..9d035b84c3e77 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -19,15 +19,15 @@ import * as OnyxUpdates from './OnyxUpdates'; // 6. Restart the sequential queue // 7. Restart the Onyx updates from Pusher // This will ensure that the client is up-to-date with the server and all the updates have been applied in the correct order. -// It's important that this file is separate and not imported by OnyxUpdates.js, so that there are no circular dependencies. Onyx -// is used as a pub/sub mechanism to break out of the circular dependency. -// The circular dependency happens because this file calls API.GetMissingOnyxUpdates() which uses the SaveResponseInOnyx.js file -// (as a middleware). Therefore, SaveResponseInOnyx.js can't import and use this file directly. +// It's important that this file is separate and not imported by OnyxUpdates.js, so that there are no circular dependencies. +// Onyx is used as a pub/sub mechanism to break out of the circular dependency. +// The circular dependency happens because this file calls API.GetMissingOnyxUpdates() which uses the SaveResponseInOnyx.js file (as a middleware). +// Therefore, SaveResponseInOnyx.js can't import and use this file directly. -let lastUpdateIDAppliedToClient: number | null = 0; +let lastUpdateIDAppliedToClient = 0; Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => (lastUpdateIDAppliedToClient = value), + callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), }); let isLoadingApp = false; @@ -104,12 +104,12 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary): DetectGapAndSpl } let updatesAfterGaps: DeferredUpdatesDictionary = {}; - if (gapExists && firstUpdateAfterGaps) { + if (gapExists) { updatesAfterGaps = Object.entries(updates).reduce( (accUpdates, [lastUpdateID, update]) => ({ ...accUpdates, // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - ...(Number(lastUpdateID) >= firstUpdateAfterGaps! ? {[Number(lastUpdateID)]: update} : {}), + ...(Number(lastUpdateID) >= firstUpdateAfterGaps ? {[Number(lastUpdateID)]: update} : {}), }), {}, ); @@ -126,10 +126,7 @@ function validateAndApplyDeferredUpdates(): Promise { const pendingDeferredUpdates = Object.entries(deferredUpdates).reduce( (accUpdates, [lastUpdateID, update]) => ({ ...accUpdates, - // It should not be possible for lastUpdateIDAppliedToClient to be null, - // after the missing updates have been applied. - // If still so we want to keep the deferred update in the list. - ...(!lastUpdateIDAppliedToClient || (Number(lastUpdateID) ?? 0) > lastUpdateIDAppliedToClient ? {[Number(lastUpdateID)]: update} : {}), + ...(Number(lastUpdateID) > lastUpdateIDAppliedToClient ? {[Number(lastUpdateID)]: update} : {}), }), {}, ); @@ -147,6 +144,7 @@ function validateAndApplyDeferredUpdates(): Promise { if (latestMissingUpdateID) { return new Promise((resolve, reject) => { deferredUpdates = {}; + applyUpdates(applicableUpdates).then(() => { // After we have applied the applicable updates, there might have been new deferred updates added. // In the next (recursive) call of "validateAndApplyDeferredUpdates", @@ -156,7 +154,7 @@ function validateAndApplyDeferredUpdates(): Promise { // It should not be possible for lastUpdateIDAppliedToClient to be null, therefore we can ignore this case. // If lastUpdateIDAppliedToClient got updated in the meantime, we will just retrigger the validation and application of the current deferred updates. - if (!lastUpdateIDAppliedToClient || latestMissingUpdateID <= lastUpdateIDAppliedToClient) { + if (latestMissingUpdateID <= lastUpdateIDAppliedToClient) { validateAndApplyDeferredUpdates().then(resolve).catch(reject); return; } @@ -177,7 +175,7 @@ function validateAndApplyDeferredUpdates(): Promise { * @param clientLastUpdateID an optional override for the lastUpdateIDAppliedToClient * @returns */ -function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry, clientLastUpdateID = 0) { +function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry, clientLastUpdateID?: number) { // If isLoadingApp is positive it means that OpenApp command hasn't finished yet, and in that case // we don't have base state of the app (reports, policies, etc) setup. If we apply this update, // we'll only have them overriten by the openApp response. So let's skip it and return. @@ -203,7 +201,7 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry Date: Sun, 21 Apr 2024 20:41:22 +0200 Subject: [PATCH 12/37] implement tests --- tests/unit/OnyxUpdateManagerTest.ts | 165 ++++++++++++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 tests/unit/OnyxUpdateManagerTest.ts diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts new file mode 100644 index 0000000000000..cab9862dd90d8 --- /dev/null +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -0,0 +1,165 @@ +import Onyx from 'react-native-onyx'; +import * as App from '@libs/actions/App'; +import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; +import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type {OnyxUpdatesFromServer} from '@src/types/onyx'; + +const lastAppliedMissingUpdateIDKey = 'lastAppliedMissingUpdateID'; + +const createTriggerPromise = () => { + let trigger: () => void = () => undefined; + const resetPromise = () => + new Promise((resolve) => { + trigger = resolve; + }); + const promise = resetPromise(); + + return {promise, trigger, resetPromise}; +}; + +type AppMockType = typeof App & { + getMissingOnyxUpdates: jest.Mock>; + getMissingOnyxUpdatesTriggeredPromise: Promise; +}; + +jest.mock('@libs/actions/App', () => { + const AppImplementation: typeof App = jest.requireActual('@libs/actions/App'); + const AppOnyx: typeof Onyx = jest.requireActual('react-native-onyx').default; + const APP_ONYXKEYS: typeof ONYXKEYS = jest.requireActual('@src/ONYXKEYS').default; + + let appLastAppliedMissingUpdateID = 1; + AppOnyx.connect({ + // @ts-expect-error ignore invalid onyx key + key: 'lastAppliedMissingUpdateID', + callback: (value) => (appLastAppliedMissingUpdateID = (value as number | null) ?? 1), + }); + + const {promise: getMissingOnyxUpdatesTriggeredPromise, trigger: getMissingOnyxUpdatesWasTriggered, resetPromise: resetGetMissingOnyxUpdatesPromise} = createTriggerPromise(); + + return { + ...AppImplementation, + finalReconnectAppAfterActivatingReliableUpdates: jest.fn(() => Promise.resolve()), + getMissingOnyxUpdatesTriggeredPromise, + getMissingOnyxUpdates: jest.fn(() => { + getMissingOnyxUpdatesWasTriggered(); + + const promise = AppOnyx.set(APP_ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, appLastAppliedMissingUpdateID); + + promise.finally(() => { + resetGetMissingOnyxUpdatesPromise(); + }); + + return promise; + }), + } as AppMockType; +}); + +type OnyxUpdateManagerMockType = typeof OnyxUpdateManager & { + applyUpdates: jest.Mock>; + applyUpdatesTriggeredPromise: Promise; +}; + +jest.mock('@libs/actions/OnyxUpdateManager', () => { + const OnyxUpdateManagerImplementation: typeof OnyxUpdateManager = jest.requireActual('@libs/actions/OnyxUpdateManager'); + + const {promise: applyUpdatesTriggeredPromise, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggeredPromise} = createTriggerPromise(); + + return { + ...OnyxUpdateManagerImplementation, + applyUpdatesTriggeredPromise, + applyUpdates: jest.fn((updates: DeferredUpdatesDictionary) => { + applyUpdatesTriggered(); + + const promise = OnyxUpdateManagerImplementation.applyUpdates(updates); + + promise.finally(() => { + resetApplyUpdatesTriggeredPromise(); + }); + + return promise; + }), + } as OnyxUpdateManagerMockType; +}); + +const AppMock = App as AppMockType; +const OnyxUpdateManagerMock = OnyxUpdateManager as OnyxUpdateManagerMockType; + +let lastUpdateIDAppliedToClient = 0; +Onyx.connect({ + key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, + callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), +}); + +const createMockUpdate = (lastUpdateID: number): OnyxUpdatesFromServer => ({ + type: 'https', + lastUpdateID, + previousUpdateID: lastUpdateID - 1, + request: { + command: 'TestCommand', + successData: [], + failureData: [], + finallyData: [], + optimisticData: [], + }, + response: { + lastUpdateID, + previousUpdateID: lastUpdateID - 1, + }, + updates: [ + { + eventType: 'test', + data: [], + }, + ], +}); +const mockUpdate3 = createMockUpdate(3); +const mockUpdate4 = createMockUpdate(4); +const mockUpdate5 = createMockUpdate(5); +const mockUpdate6 = createMockUpdate(6); + +describe('OnyxUpdateManager', () => { + beforeEach(() => { + Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); + OnyxUpdateManager.resetDeferralLogicVariables(); + }); + + it('should fetch missing Onyx updates once, defer updates and apply after missing updates', async () => { + // @ts-expect-error ignore invalid onyx key + await Onyx.set(lastAppliedMissingUpdateIDKey, 1); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); + + AppMock.getMissingOnyxUpdatesTriggeredPromise.then(() => { + expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); + }); + + OnyxUpdateManager.queryPromise?.then(() => { + expect(lastUpdateIDAppliedToClient).toBe(4); + expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); + }); + }); + + it('should only apply deferred updates that are after the locally applied update', async () => { + // @ts-expect-error ignore invalid onyx key + await Onyx.set(lastAppliedMissingUpdateIDKey, 3); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); + + AppMock.getMissingOnyxUpdatesTriggeredPromise.then(() => { + expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); + Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); + }); + + OnyxUpdateManager.queryPromise?.then(() => { + expect(lastUpdateIDAppliedToClient).toBe(6); + expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); + }); + }); +}); From cbe2d9f19020c705ca388e05e5244281e987e38d Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sun, 21 Apr 2024 20:41:35 +0200 Subject: [PATCH 13/37] add exports for testing --- src/libs/actions/OnyxUpdateManager.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager.ts index 9d035b84c3e77..66bfccde89cb0 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager.ts @@ -38,16 +38,22 @@ Onyx.connect({ }, }); +// eslint-disable-next-line import/no-mutable-exports let queryPromise: Promise | undefined; type DeferredUpdatesDictionary = Record; +// eslint-disable-next-line import/no-mutable-exports let deferredUpdates: DeferredUpdatesDictionary = {}; +const resetDeferralLogicVariables = () => { + queryPromise = undefined; + deferredUpdates = {}; +}; + // This function will reset the query variables, unpause the SequentialQueue and log an info to the user. function finalizeUpdatesAndResumeQueue() { console.debug('[OnyxUpdateManager] Done applying all updates'); - queryPromise = undefined; - deferredUpdates = {}; + resetDeferralLogicVariables(); Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, null); SequentialQueue.unpause(); } @@ -260,3 +266,5 @@ export default () => { }; export {handleOnyxUpdateGap}; +export {queryPromise, deferredUpdates, applyUpdates, resetDeferralLogicVariables}; +export type {DeferredUpdatesDictionary}; From 2a290797b7c93903b796ce006f8b208321fc2a0b Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sun, 21 Apr 2024 22:36:29 +0200 Subject: [PATCH 14/37] fix: test implementation --- tests/unit/OnyxUpdateManagerTest.ts | 79 ++++++++++++++++------------- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index cab9862dd90d8..9fcf81850be92 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -1,11 +1,12 @@ import Onyx from 'react-native-onyx'; -import * as App from '@libs/actions/App'; -import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; +import * as AppImport from '@libs/actions/App'; +import * as OnyxUpdateManagerImport from '@libs/actions/OnyxUpdateManager'; import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager'; +// import {deferredUpdates, handleOnyxUpdateGap, queryPromise} from '@libs/actions/OnyxUpdateManager'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; -const lastAppliedMissingUpdateIDKey = 'lastAppliedMissingUpdateID'; +const lastAppliedMissingUpdateIDOnyxKey = 'lastAppliedMissingUpdateID'; const createTriggerPromise = () => { let trigger: () => void = () => undefined; @@ -18,21 +19,21 @@ const createTriggerPromise = () => { return {promise, trigger, resetPromise}; }; -type AppMockType = typeof App & { +type AppActionsMock = typeof AppImport & { getMissingOnyxUpdates: jest.Mock>; getMissingOnyxUpdatesTriggeredPromise: Promise; }; jest.mock('@libs/actions/App', () => { - const AppImplementation: typeof App = jest.requireActual('@libs/actions/App'); + const AppImplementation: typeof AppImport = jest.requireActual('@libs/actions/App'); const AppOnyx: typeof Onyx = jest.requireActual('react-native-onyx').default; const APP_ONYXKEYS: typeof ONYXKEYS = jest.requireActual('@src/ONYXKEYS').default; - let appLastAppliedMissingUpdateID = 1; + let appLastAppliedMissingUpdateID = 2; AppOnyx.connect({ // @ts-expect-error ignore invalid onyx key key: 'lastAppliedMissingUpdateID', - callback: (value) => (appLastAppliedMissingUpdateID = (value as number | null) ?? 1), + callback: (value) => (appLastAppliedMissingUpdateID = (value as number | null) ?? 2), }); const {promise: getMissingOnyxUpdatesTriggeredPromise, trigger: getMissingOnyxUpdatesWasTriggered, resetPromise: resetGetMissingOnyxUpdatesPromise} = createTriggerPromise(); @@ -52,38 +53,45 @@ jest.mock('@libs/actions/App', () => { return promise; }), - } as AppMockType; + } as AppActionsMock; }); -type OnyxUpdateManagerMockType = typeof OnyxUpdateManager & { +type OnyxUpdateManagerMock = typeof OnyxUpdateManagerImport & { applyUpdates: jest.Mock>; applyUpdatesTriggeredPromise: Promise; }; jest.mock('@libs/actions/OnyxUpdateManager', () => { - const OnyxUpdateManagerImplementation: typeof OnyxUpdateManager = jest.requireActual('@libs/actions/OnyxUpdateManager'); + const OnyxUpdateManagerImplementation: typeof OnyxUpdateManagerImport = jest.requireActual('@libs/actions/OnyxUpdateManager'); const {promise: applyUpdatesTriggeredPromise, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggeredPromise} = createTriggerPromise(); - return { - ...OnyxUpdateManagerImplementation, - applyUpdatesTriggeredPromise, - applyUpdates: jest.fn((updates: DeferredUpdatesDictionary) => { - applyUpdatesTriggered(); - - const promise = OnyxUpdateManagerImplementation.applyUpdates(updates); - - promise.finally(() => { - resetApplyUpdatesTriggeredPromise(); - }); - - return promise; - }), - } as OnyxUpdateManagerMockType; + return new Proxy(OnyxUpdateManagerImplementation, { + get: (target, prop) => { + switch (prop) { + case 'applyUpdatesTriggeredPromise': + return applyUpdatesTriggeredPromise; + case 'applyUpdates': + return jest.fn((updates: DeferredUpdatesDictionary) => { + applyUpdatesTriggered(); + + const promise = OnyxUpdateManagerImplementation.applyUpdates(updates); + + promise.finally(() => { + resetApplyUpdatesTriggeredPromise(); + }); + + return promise; + }); + default: + return target[prop as keyof typeof OnyxUpdateManagerImport]; + } + }, + }) as OnyxUpdateManagerMock; }); -const AppMock = App as AppMockType; -const OnyxUpdateManagerMock = OnyxUpdateManager as OnyxUpdateManagerMockType; +const App = AppImport as AppActionsMock; +const OnyxUpdateManager = OnyxUpdateManagerImport as OnyxUpdateManagerMock; let lastUpdateIDAppliedToClient = 0; Onyx.connect({ @@ -119,19 +127,19 @@ const mockUpdate5 = createMockUpdate(5); const mockUpdate6 = createMockUpdate(6); describe('OnyxUpdateManager', () => { - beforeEach(() => { - Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); + beforeEach(async () => { + // @ts-expect-error ignore invalid onyx key + await Onyx.set(lastAppliedMissingUpdateIDOnyxKey, 2); + await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); OnyxUpdateManager.resetDeferralLogicVariables(); }); - it('should fetch missing Onyx updates once, defer updates and apply after missing updates', async () => { - // @ts-expect-error ignore invalid onyx key - await Onyx.set(lastAppliedMissingUpdateIDKey, 1); + it('should fetch missing Onyx updates once, defer updates and apply after missing updates', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); - AppMock.getMissingOnyxUpdatesTriggeredPromise.then(() => { + App.getMissingOnyxUpdatesTriggeredPromise.then(() => { expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); }); @@ -145,12 +153,13 @@ describe('OnyxUpdateManager', () => { it('should only apply deferred updates that are after the locally applied update', async () => { // @ts-expect-error ignore invalid onyx key - await Onyx.set(lastAppliedMissingUpdateIDKey, 3); + await Onyx.set(lastAppliedMissingUpdateIDOnyxKey, 3); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - AppMock.getMissingOnyxUpdatesTriggeredPromise.then(() => { + App.getMissingOnyxUpdatesTriggeredPromise.then(() => { expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); }); From 28004cb03c6e25052514ed8fa87dc9a861d4b3c9 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sun, 21 Apr 2024 22:52:35 +0200 Subject: [PATCH 15/37] implement proxy for OnyxUpdateManager --- tests/unit/OnyxUpdateManagerTest.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 9fcf81850be92..3593770cac07f 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -2,7 +2,6 @@ import Onyx from 'react-native-onyx'; import * as AppImport from '@libs/actions/App'; import * as OnyxUpdateManagerImport from '@libs/actions/OnyxUpdateManager'; import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager'; -// import {deferredUpdates, handleOnyxUpdateGap, queryPromise} from '@libs/actions/OnyxUpdateManager'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; @@ -143,7 +142,7 @@ describe('OnyxUpdateManager', () => { expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); }); - OnyxUpdateManager.queryPromise?.then(() => { + OnyxUpdateManager.queryPromise?.finally(() => { expect(lastUpdateIDAppliedToClient).toBe(4); expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention @@ -164,7 +163,7 @@ describe('OnyxUpdateManager', () => { Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); }); - OnyxUpdateManager.queryPromise?.then(() => { + OnyxUpdateManager.queryPromise?.finally(() => { expect(lastUpdateIDAppliedToClient).toBe(6); expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention From 70e428ea38fe5f089c3516be190220403322513c Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Sun, 21 Apr 2024 23:24:56 +0200 Subject: [PATCH 16/37] update tests --- tests/unit/OnyxUpdateManagerTest.ts | 103 +++++++++++++++++++--------- 1 file changed, 72 insertions(+), 31 deletions(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 3593770cac07f..4d7684a662514 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -4,6 +4,7 @@ import * as OnyxUpdateManagerImport from '@libs/actions/OnyxUpdateManager'; import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; const lastAppliedMissingUpdateIDOnyxKey = 'lastAppliedMissingUpdateID'; @@ -65,6 +66,22 @@ jest.mock('@libs/actions/OnyxUpdateManager', () => { const {promise: applyUpdatesTriggeredPromise, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggeredPromise} = createTriggerPromise(); + // return { + // ...OnyxUpdateManagerImplementation, + // applyUpdatesTriggeredPromise, + // applyUpdates: jest.fn((updates: DeferredUpdatesDictionary) => { + // applyUpdatesTriggered(); + + // const promise = OnyxUpdateManagerImplementation.applyUpdates(updates); + + // promise.finally(() => { + // resetApplyUpdatesTriggeredPromise(); + // }); + + // return promise; + // }), + // } as OnyxUpdateManagerMock; + return new Proxy(OnyxUpdateManagerImplementation, { get: (target, prop) => { switch (prop) { @@ -74,6 +91,8 @@ jest.mock('@libs/actions/OnyxUpdateManager', () => { return jest.fn((updates: DeferredUpdatesDictionary) => { applyUpdatesTriggered(); + console.log('apply updates triggered'); + const promise = OnyxUpdateManagerImplementation.applyUpdates(updates); promise.finally(() => { @@ -92,12 +111,6 @@ jest.mock('@libs/actions/OnyxUpdateManager', () => { const App = AppImport as AppActionsMock; const OnyxUpdateManager = OnyxUpdateManagerImport as OnyxUpdateManagerMock; -let lastUpdateIDAppliedToClient = 0; -Onyx.connect({ - key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), -}); - const createMockUpdate = (lastUpdateID: number): OnyxUpdatesFromServer => ({ type: 'https', lastUpdateID, @@ -125,32 +138,58 @@ const mockUpdate4 = createMockUpdate(4); const mockUpdate5 = createMockUpdate(5); const mockUpdate6 = createMockUpdate(6); +const resetOnyxUpdateManager = async () => { + // @ts-expect-error ignore invalid onyx key + await Onyx.set(lastAppliedMissingUpdateIDOnyxKey, 2); + await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); + OnyxUpdateManager.resetDeferralLogicVariables(); +}; + describe('OnyxUpdateManager', () => { + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + return waitForBatchedUpdates(); + }); + beforeEach(async () => { - // @ts-expect-error ignore invalid onyx key - await Onyx.set(lastAppliedMissingUpdateIDOnyxKey, 2); - await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); - OnyxUpdateManager.resetDeferralLogicVariables(); + jest.clearAllMocks(); + Onyx.clear(); + await resetOnyxUpdateManager(); + return waitForBatchedUpdates(); }); it('should fetch missing Onyx updates once, defer updates and apply after missing updates', () => { + let lastUpdateIDAppliedToClient = 0; + Onyx.connect({ + key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, + callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), + }); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); - App.getMissingOnyxUpdatesTriggeredPromise.then(() => { - expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); - }); - - OnyxUpdateManager.queryPromise?.finally(() => { - expect(lastUpdateIDAppliedToClient).toBe(4); - expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); - }); + return App.getMissingOnyxUpdatesTriggeredPromise + .then(() => { + expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); + }) + .then(waitForBatchedUpdates) + .then(() => OnyxUpdateManager.queryPromise) + ?.then(() => { + expect(lastUpdateIDAppliedToClient).toBe(5); + expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); + }); }); it('should only apply deferred updates that are after the locally applied update', async () => { + let lastUpdateIDAppliedToClient = 0; + Onyx.connect({ + key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, + callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), + }); + // @ts-expect-error ignore invalid onyx key await Onyx.set(lastAppliedMissingUpdateIDOnyxKey, 3); @@ -158,16 +197,18 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - App.getMissingOnyxUpdatesTriggeredPromise.then(() => { - expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); - Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); - }); - - OnyxUpdateManager.queryPromise?.finally(() => { - expect(lastUpdateIDAppliedToClient).toBe(6); - expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); - }); + return App.getMissingOnyxUpdatesTriggeredPromise + .then(() => { + expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); + Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); + }) + .then(waitForBatchedUpdates) + .then(() => OnyxUpdateManager.queryPromise) + ?.then(() => { + expect(lastUpdateIDAppliedToClient).toBe(6); + expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); + }); }); }); From 089069614f68742bd7a99a895189fda5cf3b4b78 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 22 Apr 2024 10:55:38 +0200 Subject: [PATCH 17/37] extract util logic from OnyxUpdateManager --- .../index.ts} | 129 +----------------- src/libs/actions/OnyxUpdateManager/types.ts | 5 + src/libs/actions/OnyxUpdateManager/utils.ts | 129 ++++++++++++++++++ 3 files changed, 141 insertions(+), 122 deletions(-) rename src/libs/actions/{OnyxUpdateManager.ts => OnyxUpdateManager/index.ts} (50%) create mode 100644 src/libs/actions/OnyxUpdateManager/types.ts create mode 100644 src/libs/actions/OnyxUpdateManager/utils.ts diff --git a/src/libs/actions/OnyxUpdateManager.ts b/src/libs/actions/OnyxUpdateManager/index.ts similarity index 50% rename from src/libs/actions/OnyxUpdateManager.ts rename to src/libs/actions/OnyxUpdateManager/index.ts index 66bfccde89cb0..176fd10899fbf 100644 --- a/src/libs/actions/OnyxUpdateManager.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -3,11 +3,12 @@ import Onyx from 'react-native-onyx'; import * as ActiveClientManager from '@libs/ActiveClientManager'; import Log from '@libs/Log'; import * as SequentialQueue from '@libs/Network/SequentialQueue'; +import * as App from '@userActions/App'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer, Response} from '@src/types/onyx'; import {isValidOnyxUpdateFromServer} from '@src/types/onyx/OnyxUpdatesFromServer'; -import * as App from './App'; -import * as OnyxUpdates from './OnyxUpdates'; +import type DeferredUpdatesDictionary from './types'; +import * as OnyxUpdateManagerUtils from './utils'; // This file is in charge of looking at the updateIDs coming from the server and comparing them to the last updateID that the client has. // If the client is behind the server, then we need to @@ -41,7 +42,6 @@ Onyx.connect({ // eslint-disable-next-line import/no-mutable-exports let queryPromise: Promise | undefined; -type DeferredUpdatesDictionary = Record; // eslint-disable-next-line import/no-mutable-exports let deferredUpdates: DeferredUpdatesDictionary = {}; @@ -58,123 +58,6 @@ function finalizeUpdatesAndResumeQueue() { SequentialQueue.unpause(); } -// This function applies a list of updates to Onyx in order and resolves when all updates have been applied -const applyUpdates = (updates: DeferredUpdatesDictionary) => Promise.all(Object.values(updates).map((update) => OnyxUpdates.apply(update))); - -// In order for the deferred updates to be applied correctly in order, -// we need to check if there are any gaps between deferred updates. -type DetectGapAndSplitResult = {applicableUpdates: DeferredUpdatesDictionary; updatesAfterGaps: DeferredUpdatesDictionary; latestMissingUpdateID: number | undefined}; -function detectGapsAndSplit(updates: DeferredUpdatesDictionary): DetectGapAndSplitResult { - const updateValues = Object.values(updates); - const applicableUpdates: DeferredUpdatesDictionary = {}; - - let gapExists = false; - let firstUpdateAfterGaps: number | undefined; - let latestMissingUpdateID: number | undefined; - - for (const [index, update] of updateValues.entries()) { - const isFirst = index === 0; - - // If any update's previousUpdateID doesn't match the lastUpdateID from the previous update, the deferred updates aren't chained and there's a gap. - // For the first update, we need to check that the previousUpdateID of the fetched update is the same as the lastUpdateIDAppliedToClient. - // For any other updates, we need to check if the previousUpdateID of the current update is found in the deferred updates. - // If an update is chained, we can add it to the applicable updates. - const isChained = isFirst ? update.previousUpdateID === lastUpdateIDAppliedToClient : !!updates[Number(update.previousUpdateID)]; - if (isChained) { - // If a gap exists already, we will not add any more updates to the applicable updates. - // Instead, once there are two chained updates again, we can set "firstUpdateAfterGaps" to the first update after the current gap. - if (gapExists) { - // If "firstUpdateAfterGaps" hasn't been set yet and there was a gap, we need to set it to the first update after all gaps. - if (!firstUpdateAfterGaps) { - firstUpdateAfterGaps = Number(update.previousUpdateID); - } - } else { - // If no gap exists yet, we can add the update to the applicable updates - applicableUpdates[Number(update.lastUpdateID)] = update; - } - } else { - // When we find a (new) gap, we need to set "gapExists" to true and reset the "firstUpdateAfterGaps" variable, - // so that we can continue searching for the next update after all gaps - gapExists = true; - firstUpdateAfterGaps = undefined; - - // If there is a gap, it means the previous update is the latest missing update. - latestMissingUpdateID = Number(update.previousUpdateID); - } - } - - // When "firstUpdateAfterGaps" is not set yet, we need to set it to the last update in the list, - // because we will fetch all missing updates up to the previous one and can then always apply the last update in the deferred updates. - if (!firstUpdateAfterGaps) { - firstUpdateAfterGaps = Number(updateValues[updateValues.length - 1].lastUpdateID); - } - - let updatesAfterGaps: DeferredUpdatesDictionary = {}; - if (gapExists) { - updatesAfterGaps = Object.entries(updates).reduce( - (accUpdates, [lastUpdateID, update]) => ({ - ...accUpdates, - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - ...(Number(lastUpdateID) >= firstUpdateAfterGaps ? {[Number(lastUpdateID)]: update} : {}), - }), - {}, - ); - } - - return {applicableUpdates, updatesAfterGaps, latestMissingUpdateID}; -} - -// This function will check for gaps in the deferred updates and -// apply the updates in order after the missing updates are fetched and applied -function validateAndApplyDeferredUpdates(): Promise { - // We only want to apply deferred updates that are newer than the last update that was applied to the client. - // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out. - const pendingDeferredUpdates = Object.entries(deferredUpdates).reduce( - (accUpdates, [lastUpdateID, update]) => ({ - ...accUpdates, - ...(Number(lastUpdateID) > lastUpdateIDAppliedToClient ? {[Number(lastUpdateID)]: update} : {}), - }), - {}, - ); - - // If there are no remaining deferred updates after filtering out outdated ones, - // we can just unpause the queue and return - if (Object.values(pendingDeferredUpdates).length === 0) { - return Promise.resolve(); - } - - const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(pendingDeferredUpdates); - - // If we detected a gap in the deferred updates, only apply the deferred updates before the gap, - // re-fetch the missing updates and then apply the remaining deferred updates after the gap - if (latestMissingUpdateID) { - return new Promise((resolve, reject) => { - deferredUpdates = {}; - - applyUpdates(applicableUpdates).then(() => { - // After we have applied the applicable updates, there might have been new deferred updates added. - // In the next (recursive) call of "validateAndApplyDeferredUpdates", - // the initial "updatesAfterGaps" and all new deferred updates will be applied in order, - // as long as there was no new gap detected. Otherwise repeat the process. - deferredUpdates = {...deferredUpdates, ...updatesAfterGaps}; - - // It should not be possible for lastUpdateIDAppliedToClient to be null, therefore we can ignore this case. - // If lastUpdateIDAppliedToClient got updated in the meantime, we will just retrigger the validation and application of the current deferred updates. - if (latestMissingUpdateID <= lastUpdateIDAppliedToClient) { - validateAndApplyDeferredUpdates().then(resolve).catch(reject); - return; - } - - // Then we can fetch the missing updates and apply them - App.getMissingOnyxUpdates(lastUpdateIDAppliedToClient, latestMissingUpdateID).then(validateAndApplyDeferredUpdates).then(resolve).catch(reject); - }); - }); - } - - // If there are no gaps in the deferred updates, we can apply all deferred updates in order - return applyUpdates(applicableUpdates); -} - /** * * @param onyxUpdatesFromServer @@ -251,7 +134,9 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(deferredUpdates, lastUpdateIDFromClient)) + .then((newDeferredUpdates) => (deferredUpdates = newDeferredUpdates)); } queryPromise.finally(finalizeUpdatesAndResumeQueue); @@ -266,5 +151,5 @@ export default () => { }; export {handleOnyxUpdateGap}; -export {queryPromise, deferredUpdates, applyUpdates, resetDeferralLogicVariables}; +export {queryPromise, deferredUpdates, resetDeferralLogicVariables}; export type {DeferredUpdatesDictionary}; diff --git a/src/libs/actions/OnyxUpdateManager/types.ts b/src/libs/actions/OnyxUpdateManager/types.ts new file mode 100644 index 0000000000000..a081dff006653 --- /dev/null +++ b/src/libs/actions/OnyxUpdateManager/types.ts @@ -0,0 +1,5 @@ +import type {OnyxUpdatesFromServer} from '@src/types/onyx'; + +type DeferredUpdatesDictionary = Record; + +export default DeferredUpdatesDictionary; diff --git a/src/libs/actions/OnyxUpdateManager/utils.ts b/src/libs/actions/OnyxUpdateManager/utils.ts new file mode 100644 index 0000000000000..bdbcfa13e37af --- /dev/null +++ b/src/libs/actions/OnyxUpdateManager/utils.ts @@ -0,0 +1,129 @@ +import * as App from '@userActions/App'; +import * as OnyxUpdates from '@userActions/OnyxUpdates'; +import type DeferredUpdatesDictionary from './types'; + +// This function applies a list of updates to Onyx in order and resolves when all updates have been applied +const applyUpdates = (updates: DeferredUpdatesDictionary) => Promise.all(Object.values(updates).map((update) => OnyxUpdates.apply(update))); + +// In order for the deferred updates to be applied correctly in order, +// we need to check if there are any gaps between deferred updates. +type DetectGapAndSplitResult = {applicableUpdates: DeferredUpdatesDictionary; updatesAfterGaps: DeferredUpdatesDictionary; latestMissingUpdateID: number | undefined}; +function detectGapsAndSplit(updates: DeferredUpdatesDictionary, lastUpdateIDFromClient: number): DetectGapAndSplitResult { + const updateValues = Object.values(updates); + const applicableUpdates: DeferredUpdatesDictionary = {}; + + let gapExists = false; + let firstUpdateAfterGaps: number | undefined; + let latestMissingUpdateID: number | undefined; + + for (const [index, update] of updateValues.entries()) { + const isFirst = index === 0; + + // If any update's previousUpdateID doesn't match the lastUpdateID from the previous update, the deferred updates aren't chained and there's a gap. + // For the first update, we need to check that the previousUpdateID of the fetched update is the same as the lastUpdateIDAppliedToClient. + // For any other updates, we need to check if the previousUpdateID of the current update is found in the deferred updates. + // If an update is chained, we can add it to the applicable updates. + const isChained = isFirst ? update.previousUpdateID === lastUpdateIDFromClient : !!updates[Number(update.previousUpdateID)]; + if (isChained) { + // If a gap exists already, we will not add any more updates to the applicable updates. + // Instead, once there are two chained updates again, we can set "firstUpdateAfterGaps" to the first update after the current gap. + if (gapExists) { + // If "firstUpdateAfterGaps" hasn't been set yet and there was a gap, we need to set it to the first update after all gaps. + if (!firstUpdateAfterGaps) { + firstUpdateAfterGaps = Number(update.previousUpdateID); + } + } else { + // If no gap exists yet, we can add the update to the applicable updates + applicableUpdates[Number(update.lastUpdateID)] = update; + } + } else { + // When we find a (new) gap, we need to set "gapExists" to true and reset the "firstUpdateAfterGaps" variable, + // so that we can continue searching for the next update after all gaps + gapExists = true; + firstUpdateAfterGaps = undefined; + + // If there is a gap, it means the previous update is the latest missing update. + latestMissingUpdateID = Number(update.previousUpdateID); + } + } + + // When "firstUpdateAfterGaps" is not set yet, we need to set it to the last update in the list, + // because we will fetch all missing updates up to the previous one and can then always apply the last update in the deferred updates. + if (!firstUpdateAfterGaps) { + firstUpdateAfterGaps = Number(updateValues[updateValues.length - 1].lastUpdateID); + } + + let updatesAfterGaps: DeferredUpdatesDictionary = {}; + if (gapExists) { + updatesAfterGaps = Object.entries(updates).reduce( + (accUpdates, [lastUpdateID, update]) => ({ + ...accUpdates, + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + ...(Number(lastUpdateID) >= firstUpdateAfterGaps ? {[Number(lastUpdateID)]: update} : {}), + }), + {}, + ); + } + + return {applicableUpdates, updatesAfterGaps, latestMissingUpdateID}; +} + +// This function will check for gaps in the deferred updates and +// apply the updates in order after the missing updates are fetched and applied +function validateAndApplyDeferredUpdates(deferredUpdates: DeferredUpdatesDictionary, lastUpdateIDFromClient: number): Promise { + let newDeferredUpdates = {...deferredUpdates}; + + // We only want to apply deferred updates that are newer than the last update that was applied to the client. + // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out. + const pendingDeferredUpdates = Object.entries(newDeferredUpdates).reduce( + (accUpdates, [lastUpdateID, update]) => ({ + ...accUpdates, + ...(Number(lastUpdateID) > lastUpdateIDFromClient ? {[Number(lastUpdateID)]: update} : {}), + }), + {}, + ); + + // If there are no remaining deferred updates after filtering out outdated ones, + // we can just unpause the queue and return + if (Object.values(pendingDeferredUpdates).length === 0) { + return Promise.resolve(newDeferredUpdates); + } + + const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(pendingDeferredUpdates, lastUpdateIDFromClient); + + // If we detected a gap in the deferred updates, only apply the deferred updates before the gap, + // re-fetch the missing updates and then apply the remaining deferred updates after the gap + if (latestMissingUpdateID) { + return new Promise((resolve, reject) => { + newDeferredUpdates = {}; + + applyUpdates(applicableUpdates).then(() => { + // After we have applied the applicable updates, there might have been new deferred updates added. + // In the next (recursive) call of "validateAndApplyDeferredUpdates", + // the initial "updatesAfterGaps" and all new deferred updates will be applied in order, + // as long as there was no new gap detected. Otherwise repeat the process. + newDeferredUpdates = {...newDeferredUpdates, ...updatesAfterGaps}; + + // It should not be possible for lastUpdateIDAppliedToClient to be null, therefore we can ignore this case. + // If lastUpdateIDAppliedToClient got updated in the meantime, we will just retrigger the validation and application of the current deferred updates. + if (latestMissingUpdateID <= lastUpdateIDFromClient) { + validateAndApplyDeferredUpdates(newDeferredUpdates, lastUpdateIDFromClient) + .then(() => resolve(newDeferredUpdates)) + .catch(reject); + return; + } + + // Then we can fetch the missing updates and apply them + App.getMissingOnyxUpdates(lastUpdateIDFromClient, latestMissingUpdateID) + .then(() => validateAndApplyDeferredUpdates(newDeferredUpdates, lastUpdateIDFromClient)) + .then(() => resolve(newDeferredUpdates)) + .catch(reject); + }); + }); + } + + // If there are no gaps in the deferred updates, we can apply all deferred updates in order + return applyUpdates(applicableUpdates).then(() => newDeferredUpdates); +} + +export {applyUpdates, detectGapsAndSplit, validateAndApplyDeferredUpdates}; From 909b5a8f7f9df4427b2792bfd95d718ddabbf9be Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 22 Apr 2024 14:30:02 +0200 Subject: [PATCH 18/37] extract createProxyForValue and further split up utils --- src/libs/actions/OnyxUpdateManager/index.ts | 16 +++----- .../OnyxUpdateManager/utils/applyUpdates.ts | 9 +++++ .../utils/deferredUpdates.ts | 8 ++++ .../{utils.ts => utils/index.ts} | 37 +++++++++++-------- src/utils/createProxyForValue.ts | 19 ++++++++++ 5 files changed, 64 insertions(+), 25 deletions(-) create mode 100644 src/libs/actions/OnyxUpdateManager/utils/applyUpdates.ts create mode 100644 src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts rename src/libs/actions/OnyxUpdateManager/{utils.ts => utils/index.ts} (82%) create mode 100644 src/utils/createProxyForValue.ts diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 176fd10899fbf..0fc84222d4142 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -9,6 +9,7 @@ import type {OnyxUpdatesFromServer, Response} from '@src/types/onyx'; import {isValidOnyxUpdateFromServer} from '@src/types/onyx/OnyxUpdatesFromServer'; import type DeferredUpdatesDictionary from './types'; import * as OnyxUpdateManagerUtils from './utils'; +import deferredUpdatesProxy from './utils/deferredUpdates'; // This file is in charge of looking at the updateIDs coming from the server and comparing them to the last updateID that the client has. // If the client is behind the server, then we need to @@ -42,12 +43,9 @@ Onyx.connect({ // eslint-disable-next-line import/no-mutable-exports let queryPromise: Promise | undefined; -// eslint-disable-next-line import/no-mutable-exports -let deferredUpdates: DeferredUpdatesDictionary = {}; - const resetDeferralLogicVariables = () => { queryPromise = undefined; - deferredUpdates = {}; + deferredUpdatesProxy.deferredUpdates = {}; }; // This function will reset the query variables, unpause the SequentialQueue and log an info to the user. @@ -115,10 +113,10 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry 0) { @@ -134,9 +132,7 @@ function handleOnyxUpdateGap(onyxUpdatesFromServer: OnyxEntry OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(deferredUpdates, lastUpdateIDFromClient)) - .then((newDeferredUpdates) => (deferredUpdates = newDeferredUpdates)); + queryPromise = App.getMissingOnyxUpdates(lastUpdateIDFromClient, previousUpdateIDFromServer).then(() => OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates(clientLastUpdateID)); } queryPromise.finally(finalizeUpdatesAndResumeQueue); @@ -151,5 +147,5 @@ export default () => { }; export {handleOnyxUpdateGap}; -export {queryPromise, deferredUpdates, resetDeferralLogicVariables}; +export {queryPromise, resetDeferralLogicVariables}; export type {DeferredUpdatesDictionary}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/applyUpdates.ts new file mode 100644 index 0000000000000..37a1ec70728d8 --- /dev/null +++ b/src/libs/actions/OnyxUpdateManager/utils/applyUpdates.ts @@ -0,0 +1,9 @@ +// We need to keep this in a separate file, so that we can mock this function in tests. +import type DeferredUpdatesDictionary from '@libs/actions/OnyxUpdateManager/types'; +import * as OnyxUpdates from '@userActions/OnyxUpdates'; + +// This function applies a list of updates to Onyx in order and resolves when all updates have been applied +const applyUpdates = (updates: DeferredUpdatesDictionary) => Promise.all(Object.values(updates).map((update) => OnyxUpdates.apply(update))); + +// eslint-disable-next-line import/prefer-default-export +export {applyUpdates}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts new file mode 100644 index 0000000000000..4463e8ea5a7bf --- /dev/null +++ b/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts @@ -0,0 +1,8 @@ +import type DeferredUpdatesDictionary from '@libs/actions/OnyxUpdateManager/types'; +import createProxyForValue from '@src/utils/createProxyForValue'; + +const deferredUpdatesValue = {deferredUpdates: {} as DeferredUpdatesDictionary}; + +const deferredUpdatesProxy = createProxyForValue(deferredUpdatesValue, 'deferredUpdates'); + +export default deferredUpdatesProxy; diff --git a/src/libs/actions/OnyxUpdateManager/utils.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts similarity index 82% rename from src/libs/actions/OnyxUpdateManager/utils.ts rename to src/libs/actions/OnyxUpdateManager/utils/index.ts index bdbcfa13e37af..42451fae2a28c 100644 --- a/src/libs/actions/OnyxUpdateManager/utils.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -1,9 +1,15 @@ +import Onyx from 'react-native-onyx'; import * as App from '@userActions/App'; -import * as OnyxUpdates from '@userActions/OnyxUpdates'; -import type DeferredUpdatesDictionary from './types'; +import type DeferredUpdatesDictionary from '@userActions/OnyxUpdateManager/types'; +import ONYXKEYS from '@src/ONYXKEYS'; +import {applyUpdates} from './applyUpdates'; +import deferredUpdatesProxy from './deferredUpdates'; -// This function applies a list of updates to Onyx in order and resolves when all updates have been applied -const applyUpdates = (updates: DeferredUpdatesDictionary) => Promise.all(Object.values(updates).map((update) => OnyxUpdates.apply(update))); +let lastUpdateIDAppliedToClient = 0; +Onyx.connect({ + key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, + callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), +}); // In order for the deferred updates to be applied correctly in order, // we need to check if there are any gaps between deferred updates. @@ -70,12 +76,12 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary, lastUpdateIDFrom // This function will check for gaps in the deferred updates and // apply the updates in order after the missing updates are fetched and applied -function validateAndApplyDeferredUpdates(deferredUpdates: DeferredUpdatesDictionary, lastUpdateIDFromClient: number): Promise { - let newDeferredUpdates = {...deferredUpdates}; +function validateAndApplyDeferredUpdates(clientLastUpdateID?: number): Promise { + const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0; // We only want to apply deferred updates that are newer than the last update that was applied to the client. // At this point, the missing updates from "GetMissingOnyxUpdates" have been applied already, so we can safely filter out. - const pendingDeferredUpdates = Object.entries(newDeferredUpdates).reduce( + const pendingDeferredUpdates = Object.entries(deferredUpdatesProxy.deferredUpdates).reduce( (accUpdates, [lastUpdateID, update]) => ({ ...accUpdates, ...(Number(lastUpdateID) > lastUpdateIDFromClient ? {[Number(lastUpdateID)]: update} : {}), @@ -86,7 +92,7 @@ function validateAndApplyDeferredUpdates(deferredUpdates: DeferredUpdatesDiction // If there are no remaining deferred updates after filtering out outdated ones, // we can just unpause the queue and return if (Object.values(pendingDeferredUpdates).length === 0) { - return Promise.resolve(newDeferredUpdates); + return Promise.resolve(); } const {applicableUpdates, updatesAfterGaps, latestMissingUpdateID} = detectGapsAndSplit(pendingDeferredUpdates, lastUpdateIDFromClient); @@ -95,35 +101,36 @@ function validateAndApplyDeferredUpdates(deferredUpdates: DeferredUpdatesDiction // re-fetch the missing updates and then apply the remaining deferred updates after the gap if (latestMissingUpdateID) { return new Promise((resolve, reject) => { - newDeferredUpdates = {}; + deferredUpdatesProxy.deferredUpdates = {}; applyUpdates(applicableUpdates).then(() => { // After we have applied the applicable updates, there might have been new deferred updates added. // In the next (recursive) call of "validateAndApplyDeferredUpdates", // the initial "updatesAfterGaps" and all new deferred updates will be applied in order, // as long as there was no new gap detected. Otherwise repeat the process. - newDeferredUpdates = {...newDeferredUpdates, ...updatesAfterGaps}; + deferredUpdatesProxy.deferredUpdates = {...deferredUpdatesProxy.deferredUpdates, ...updatesAfterGaps}; + // updateDeferredUpdates(newDeferredUpdates); // It should not be possible for lastUpdateIDAppliedToClient to be null, therefore we can ignore this case. // If lastUpdateIDAppliedToClient got updated in the meantime, we will just retrigger the validation and application of the current deferred updates. if (latestMissingUpdateID <= lastUpdateIDFromClient) { - validateAndApplyDeferredUpdates(newDeferredUpdates, lastUpdateIDFromClient) - .then(() => resolve(newDeferredUpdates)) + validateAndApplyDeferredUpdates(clientLastUpdateID) + .then(() => resolve(undefined)) .catch(reject); return; } // Then we can fetch the missing updates and apply them App.getMissingOnyxUpdates(lastUpdateIDFromClient, latestMissingUpdateID) - .then(() => validateAndApplyDeferredUpdates(newDeferredUpdates, lastUpdateIDFromClient)) - .then(() => resolve(newDeferredUpdates)) + .then(() => validateAndApplyDeferredUpdates(clientLastUpdateID)) + .then(() => resolve(undefined)) .catch(reject); }); }); } // If there are no gaps in the deferred updates, we can apply all deferred updates in order - return applyUpdates(applicableUpdates).then(() => newDeferredUpdates); + return applyUpdates(applicableUpdates).then(() => undefined); } export {applyUpdates, detectGapsAndSplit, validateAndApplyDeferredUpdates}; diff --git a/src/utils/createProxyForValue.ts b/src/utils/createProxyForValue.ts new file mode 100644 index 0000000000000..e9e8eb5dca6bf --- /dev/null +++ b/src/utils/createProxyForValue.ts @@ -0,0 +1,19 @@ +const createProxyForValue = (value: Record, variableName: VariableName) => + new Proxy(value, { + get: (target, prop) => { + if (prop !== variableName) { + return undefined; + } + return target[prop as VariableName]; + }, + set: (target, prop, newValue) => { + if (prop !== variableName) { + return false; + } + // eslint-disable-next-line no-param-reassign + target[prop as VariableName] = newValue; + return true; + }, + }); + +export default createProxyForValue; From c502513afd4699d29bc6c6103a5656662f0bae94 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 22 Apr 2024 14:30:40 +0200 Subject: [PATCH 19/37] fix: tests --- tests/unit/OnyxUpdateManagerTest.ts | 129 +++++++++++----------------- 1 file changed, 50 insertions(+), 79 deletions(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 4d7684a662514..f96d339c8df25 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -1,13 +1,13 @@ import Onyx from 'react-native-onyx'; import * as AppImport from '@libs/actions/App'; -import * as OnyxUpdateManagerImport from '@libs/actions/OnyxUpdateManager'; +import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager'; +import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; +import deferredUpdatesProxy from '@libs/actions/OnyxUpdateManager/utils/deferredUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; -const lastAppliedMissingUpdateIDOnyxKey = 'lastAppliedMissingUpdateID'; - const createTriggerPromise = () => { let trigger: () => void = () => undefined; const resetPromise = () => @@ -20,35 +20,40 @@ const createTriggerPromise = () => { }; type AppActionsMock = typeof AppImport & { + lastUpdateIdFromUpdatesProxy: Record<'lastUpdateIdFromUpdates', number>; getMissingOnyxUpdates: jest.Mock>; getMissingOnyxUpdatesTriggeredPromise: Promise; + getMissingOnyxUpdatesDonePromise: Promise; }; jest.mock('@libs/actions/App', () => { const AppImplementation: typeof AppImport = jest.requireActual('@libs/actions/App'); const AppOnyx: typeof Onyx = jest.requireActual('react-native-onyx').default; const APP_ONYXKEYS: typeof ONYXKEYS = jest.requireActual('@src/ONYXKEYS').default; + const appCreateProxyForValue = jest.requireActual('@src/utils/createProxyForValue').default; - let appLastAppliedMissingUpdateID = 2; - AppOnyx.connect({ - // @ts-expect-error ignore invalid onyx key - key: 'lastAppliedMissingUpdateID', - callback: (value) => (appLastAppliedMissingUpdateID = (value as number | null) ?? 2), - }); + const lastUpdateIdFromUpdatesValueInternal = {lastUpdateIdFromUpdates: 2}; + const lastUpdateIdFromUpdatesProxy = appCreateProxyForValue(lastUpdateIdFromUpdatesValueInternal, 'lastUpdateIdFromUpdates'); + const {lastUpdateIdFromUpdates} = lastUpdateIdFromUpdatesValueInternal; - const {promise: getMissingOnyxUpdatesTriggeredPromise, trigger: getMissingOnyxUpdatesWasTriggered, resetPromise: resetGetMissingOnyxUpdatesPromise} = createTriggerPromise(); + const {promise: getMissingOnyxUpdatesTriggeredPromise, trigger: getMissingOnyxUpdatesWasTriggered, resetPromise: resetGetMissingOnyxUpdatesTriggeredPromise} = createTriggerPromise(); + const {promise: getMissingOnyxUpdatesDonePromise, trigger: getMissingOnyxUpdatesDone, resetPromise: resetGetMissingOnyxUpdatesDonePromise} = createTriggerPromise(); return { ...AppImplementation, + lastUpdateIdFromUpdatesProxy, finalReconnectAppAfterActivatingReliableUpdates: jest.fn(() => Promise.resolve()), getMissingOnyxUpdatesTriggeredPromise, + getMissingOnyxUpdatesDonePromise, getMissingOnyxUpdates: jest.fn(() => { + resetGetMissingOnyxUpdatesDonePromise(); getMissingOnyxUpdatesWasTriggered(); - const promise = AppOnyx.set(APP_ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, appLastAppliedMissingUpdateID); + const promise = AppOnyx.set(APP_ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIdFromUpdates); promise.finally(() => { - resetGetMissingOnyxUpdatesPromise(); + getMissingOnyxUpdatesDone(); + resetGetMissingOnyxUpdatesTriggeredPromise(); }); return promise; @@ -56,60 +61,38 @@ jest.mock('@libs/actions/App', () => { } as AppActionsMock; }); -type OnyxUpdateManagerMock = typeof OnyxUpdateManagerImport & { - applyUpdates: jest.Mock>; +type ApplyUpdatesMock = typeof ApplyUpdatesImport & { + applyUpdates: jest.Mock>; applyUpdatesTriggeredPromise: Promise; }; -jest.mock('@libs/actions/OnyxUpdateManager', () => { - const OnyxUpdateManagerImplementation: typeof OnyxUpdateManagerImport = jest.requireActual('@libs/actions/OnyxUpdateManager'); +jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates', () => { + const AppOnyx: typeof Onyx = jest.requireActual('react-native-onyx').default; + const APP_ONYXKEYS: typeof ONYXKEYS = jest.requireActual('@src/ONYXKEYS').default; const {promise: applyUpdatesTriggeredPromise, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggeredPromise} = createTriggerPromise(); - // return { - // ...OnyxUpdateManagerImplementation, - // applyUpdatesTriggeredPromise, - // applyUpdates: jest.fn((updates: DeferredUpdatesDictionary) => { - // applyUpdatesTriggered(); - - // const promise = OnyxUpdateManagerImplementation.applyUpdates(updates); - - // promise.finally(() => { - // resetApplyUpdatesTriggeredPromise(); - // }); - - // return promise; - // }), - // } as OnyxUpdateManagerMock; - - return new Proxy(OnyxUpdateManagerImplementation, { - get: (target, prop) => { - switch (prop) { - case 'applyUpdatesTriggeredPromise': - return applyUpdatesTriggeredPromise; - case 'applyUpdates': - return jest.fn((updates: DeferredUpdatesDictionary) => { - applyUpdatesTriggered(); + return { + applyUpdatesTriggeredPromise, + applyUpdates: jest.fn((updates: DeferredUpdatesDictionary) => { + applyUpdatesTriggered(); - console.log('apply updates triggered'); + const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); - const promise = OnyxUpdateManagerImplementation.applyUpdates(updates); + const promise = AppOnyx.set(APP_ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIdFromUpdates); - promise.finally(() => { - resetApplyUpdatesTriggeredPromise(); - }); + promise.finally(() => { + resetApplyUpdatesTriggeredPromise(); + }); - return promise; - }); - default: - return target[prop as keyof typeof OnyxUpdateManagerImport]; - } - }, - }) as OnyxUpdateManagerMock; + return promise; + }), + } as ApplyUpdatesMock; }); const App = AppImport as AppActionsMock; -const OnyxUpdateManager = OnyxUpdateManagerImport as OnyxUpdateManagerMock; +const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; +const {applyUpdates} = ApplyUpdates; const createMockUpdate = (lastUpdateID: number): OnyxUpdatesFromServer => ({ type: 'https', @@ -139,59 +122,47 @@ const mockUpdate5 = createMockUpdate(5); const mockUpdate6 = createMockUpdate(6); const resetOnyxUpdateManager = async () => { - // @ts-expect-error ignore invalid onyx key - await Onyx.set(lastAppliedMissingUpdateIDOnyxKey, 2); + App.lastUpdateIdFromUpdatesProxy.lastUpdateIdFromUpdates = 2; await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); OnyxUpdateManager.resetDeferralLogicVariables(); }; describe('OnyxUpdateManager', () => { - beforeAll(() => { - Onyx.init({keys: ONYXKEYS}); - return waitForBatchedUpdates(); - }); + let lastUpdateIDAppliedToClient = 1; beforeEach(async () => { jest.clearAllMocks(); Onyx.clear(); await resetOnyxUpdateManager(); - return waitForBatchedUpdates(); - }); - - it('should fetch missing Onyx updates once, defer updates and apply after missing updates', () => { - let lastUpdateIDAppliedToClient = 0; + Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), + callback: (value) => (lastUpdateIDAppliedToClient = value ?? 1), }); + return waitForBatchedUpdates(); + }); + it('should fetch missing Onyx updates once, defer updates and apply after missing updates', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); return App.getMissingOnyxUpdatesTriggeredPromise .then(() => { - expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); + expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); }) .then(waitForBatchedUpdates) .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { expect(lastUpdateIDAppliedToClient).toBe(5); - expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); + expect(applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention - expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); + expect(applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); }); }); it('should only apply deferred updates that are after the locally applied update', async () => { - let lastUpdateIDAppliedToClient = 0; - Onyx.connect({ - key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, - callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), - }); - - // @ts-expect-error ignore invalid onyx key - await Onyx.set(lastAppliedMissingUpdateIDOnyxKey, 3); + App.lastUpdateIdFromUpdatesProxy.lastUpdateIdFromUpdates = 3; OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); @@ -199,16 +170,16 @@ describe('OnyxUpdateManager', () => { return App.getMissingOnyxUpdatesTriggeredPromise .then(() => { - expect(Object.keys(OnyxUpdateManager.deferredUpdates)).toHaveLength(3); + expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); }) .then(waitForBatchedUpdates) .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { expect(lastUpdateIDAppliedToClient).toBe(6); - expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledTimes(1); + expect(applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention - expect(OnyxUpdateManager.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); + expect(applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); }); }); }); From 001eb3aa4fe0a53df03a69b1664b28cc8901b396 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 22 Apr 2024 15:25:03 +0200 Subject: [PATCH 20/37] extract mocks from test --- .../utils/__mocks__/applyUpdates.ts | 32 ++++ .../actions/OnyxUpdateManager/utils/index.ts | 4 + src/libs/actions/__mocks__/App.ts | 74 +++++++++ src/utils/createTriggerPromise.ts | 12 ++ tests/unit/OnyxUpdateManagerTest.ts | 150 ++++++++---------- 5 files changed, 191 insertions(+), 81 deletions(-) create mode 100644 src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts create mode 100644 src/libs/actions/__mocks__/App.ts create mode 100644 src/utils/createTriggerPromise.ts diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts new file mode 100644 index 0000000000000..a5a916cadfe4a --- /dev/null +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts @@ -0,0 +1,32 @@ +import Onyx from 'react-native-onyx'; +import type DeferredUpdatesDictionary from '@libs/actions/OnyxUpdateManager/types'; +import ONYXKEYS from '@src/ONYXKEYS'; +import createTriggerPromise from '@src/utils/createTriggerPromise'; + +const {promise: applyUpdatesTriggeredPromise, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggeredPromise} = createTriggerPromise(); + +const applyUpdates = jest.fn((updates: DeferredUpdatesDictionary) => { + console.log('apply updates'); + + applyUpdatesTriggered(); + + const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); + + const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIdFromUpdates); + + promise.finally(() => { + resetApplyUpdatesTriggeredPromise(); + }); + + promise + .then(() => { + console.log('applyUpdates succeeded'); + }) + .catch((e) => { + console.log('applyUpdates failed', e); + }); + + return promise; +}); + +export {applyUpdates, applyUpdatesTriggeredPromise}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index 42451fae2a28c..0ca42a46db7ad 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -103,11 +103,15 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number): Promise { deferredUpdatesProxy.deferredUpdates = {}; + console.log({applicableUpdates}); + applyUpdates(applicableUpdates).then(() => { // After we have applied the applicable updates, there might have been new deferred updates added. // In the next (recursive) call of "validateAndApplyDeferredUpdates", // the initial "updatesAfterGaps" and all new deferred updates will be applied in order, // as long as there was no new gap detected. Otherwise repeat the process. + + console.log({updatesAfterGaps}); deferredUpdatesProxy.deferredUpdates = {...deferredUpdatesProxy.deferredUpdates, ...updatesAfterGaps}; // updateDeferredUpdates(newDeferredUpdates); diff --git a/src/libs/actions/__mocks__/App.ts b/src/libs/actions/__mocks__/App.ts new file mode 100644 index 0000000000000..81f3cbec41f34 --- /dev/null +++ b/src/libs/actions/__mocks__/App.ts @@ -0,0 +1,74 @@ +import Onyx from 'react-native-onyx'; +import type * as AppImport from '@libs/actions/App'; +import ONYXKEYS from '@src/ONYXKEYS'; +import createProxyForValue from '@src/utils/createProxyForValue'; +import createTriggerPromise from '@src/utils/createTriggerPromise'; + +const AppImplementation: typeof AppImport = jest.requireActual('@libs/actions/App'); +const { + setLocale, + setLocaleAndNavigate, + setSidebarLoaded, + setUpPoliciesAndNavigate, + openProfile, + redirectThirdPartyDesktopSignIn, + openApp, + reconnectApp, + confirmReadyToOpenApp, + handleRestrictedEvent, + beginDeepLinkRedirect, + beginDeepLinkRedirectAfterTransition, + finalReconnectAppAfterActivatingReliableUpdates, + savePolicyDraftByNewWorkspace, + createWorkspaceWithPolicyDraftAndNavigateToIt, + updateLastVisitedPath, + KEYS_TO_PRESERVE, +} = AppImplementation; + +const shouldGetMissingOnyxUpdatesUpToIdValue = {shouldGetMissingOnyxUpdatesUpToId: 2}; +const shouldGetMissingOnyxUpdatesUpToIdProxy = createProxyForValue(shouldGetMissingOnyxUpdatesUpToIdValue, 'shouldGetMissingOnyxUpdatesUpToId'); +const {shouldGetMissingOnyxUpdatesUpToId} = shouldGetMissingOnyxUpdatesUpToIdValue; + +const {promise: getMissingOnyxUpdatesTriggeredPromise, trigger: getMissingOnyxUpdatesWasTriggered, resetPromise: resetGetMissingOnyxUpdatesTriggeredPromise} = createTriggerPromise(); +const {promise: getMissingOnyxUpdatesDonePromise, trigger: getMissingOnyxUpdatesDone, resetPromise: resetGetMissingOnyxUpdatesDonePromise} = createTriggerPromise(); + +const getMissingOnyxUpdates = jest.fn(() => { + resetGetMissingOnyxUpdatesDonePromise(); + getMissingOnyxUpdatesWasTriggered(); + + const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, shouldGetMissingOnyxUpdatesUpToId); + + promise.finally(() => { + getMissingOnyxUpdatesDone(); + resetGetMissingOnyxUpdatesTriggeredPromise(); + }); + + return promise; +}); + +export { + // Mocks + shouldGetMissingOnyxUpdatesUpToIdProxy, + getMissingOnyxUpdatesTriggeredPromise, + getMissingOnyxUpdatesDonePromise, + getMissingOnyxUpdates, + + // Actual App implementation + setLocale, + setLocaleAndNavigate, + setSidebarLoaded, + setUpPoliciesAndNavigate, + openProfile, + redirectThirdPartyDesktopSignIn, + openApp, + reconnectApp, + confirmReadyToOpenApp, + handleRestrictedEvent, + beginDeepLinkRedirect, + beginDeepLinkRedirectAfterTransition, + finalReconnectAppAfterActivatingReliableUpdates, + savePolicyDraftByNewWorkspace, + createWorkspaceWithPolicyDraftAndNavigateToIt, + updateLastVisitedPath, + KEYS_TO_PRESERVE, +}; diff --git a/src/utils/createTriggerPromise.ts b/src/utils/createTriggerPromise.ts new file mode 100644 index 0000000000000..9060ba22ec86f --- /dev/null +++ b/src/utils/createTriggerPromise.ts @@ -0,0 +1,12 @@ +const createTriggerPromise = () => { + let trigger: () => void = () => undefined; + const resetPromise = () => + new Promise((resolve) => { + trigger = resolve; + }); + const promise = resetPromise(); + + return {promise, trigger, resetPromise}; +}; + +export default createTriggerPromise; diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index f96d339c8df25..996891f725c6c 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -1,98 +1,29 @@ import Onyx from 'react-native-onyx'; import * as AppImport from '@libs/actions/App'; import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; -import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager'; import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; import deferredUpdatesProxy from '@libs/actions/OnyxUpdateManager/utils/deferredUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; -const createTriggerPromise = () => { - let trigger: () => void = () => undefined; - const resetPromise = () => - new Promise((resolve) => { - trigger = resolve; - }); - const promise = resetPromise(); - - return {promise, trigger, resetPromise}; -}; +jest.mock('@libs/actions/App'); +jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); type AppActionsMock = typeof AppImport & { - lastUpdateIdFromUpdatesProxy: Record<'lastUpdateIdFromUpdates', number>; + shouldGetMissingOnyxUpdatesUpToIdProxy: Record<'shouldGetMissingOnyxUpdatesUpToId', number>; getMissingOnyxUpdates: jest.Mock>; getMissingOnyxUpdatesTriggeredPromise: Promise; getMissingOnyxUpdatesDonePromise: Promise; }; -jest.mock('@libs/actions/App', () => { - const AppImplementation: typeof AppImport = jest.requireActual('@libs/actions/App'); - const AppOnyx: typeof Onyx = jest.requireActual('react-native-onyx').default; - const APP_ONYXKEYS: typeof ONYXKEYS = jest.requireActual('@src/ONYXKEYS').default; - const appCreateProxyForValue = jest.requireActual('@src/utils/createProxyForValue').default; - - const lastUpdateIdFromUpdatesValueInternal = {lastUpdateIdFromUpdates: 2}; - const lastUpdateIdFromUpdatesProxy = appCreateProxyForValue(lastUpdateIdFromUpdatesValueInternal, 'lastUpdateIdFromUpdates'); - const {lastUpdateIdFromUpdates} = lastUpdateIdFromUpdatesValueInternal; - - const {promise: getMissingOnyxUpdatesTriggeredPromise, trigger: getMissingOnyxUpdatesWasTriggered, resetPromise: resetGetMissingOnyxUpdatesTriggeredPromise} = createTriggerPromise(); - const {promise: getMissingOnyxUpdatesDonePromise, trigger: getMissingOnyxUpdatesDone, resetPromise: resetGetMissingOnyxUpdatesDonePromise} = createTriggerPromise(); - - return { - ...AppImplementation, - lastUpdateIdFromUpdatesProxy, - finalReconnectAppAfterActivatingReliableUpdates: jest.fn(() => Promise.resolve()), - getMissingOnyxUpdatesTriggeredPromise, - getMissingOnyxUpdatesDonePromise, - getMissingOnyxUpdates: jest.fn(() => { - resetGetMissingOnyxUpdatesDonePromise(); - getMissingOnyxUpdatesWasTriggered(); - - const promise = AppOnyx.set(APP_ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIdFromUpdates); - - promise.finally(() => { - getMissingOnyxUpdatesDone(); - resetGetMissingOnyxUpdatesTriggeredPromise(); - }); - - return promise; - }), - } as AppActionsMock; -}); - type ApplyUpdatesMock = typeof ApplyUpdatesImport & { applyUpdates: jest.Mock>; applyUpdatesTriggeredPromise: Promise; }; -jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates', () => { - const AppOnyx: typeof Onyx = jest.requireActual('react-native-onyx').default; - const APP_ONYXKEYS: typeof ONYXKEYS = jest.requireActual('@src/ONYXKEYS').default; - - const {promise: applyUpdatesTriggeredPromise, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggeredPromise} = createTriggerPromise(); - - return { - applyUpdatesTriggeredPromise, - applyUpdates: jest.fn((updates: DeferredUpdatesDictionary) => { - applyUpdatesTriggered(); - - const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); - - const promise = AppOnyx.set(APP_ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIdFromUpdates); - - promise.finally(() => { - resetApplyUpdatesTriggeredPromise(); - }); - - return promise; - }), - } as ApplyUpdatesMock; -}); - const App = AppImport as AppActionsMock; const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; -const {applyUpdates} = ApplyUpdates; const createMockUpdate = (lastUpdateID: number): OnyxUpdatesFromServer => ({ type: 'https', @@ -122,7 +53,7 @@ const mockUpdate5 = createMockUpdate(5); const mockUpdate6 = createMockUpdate(6); const resetOnyxUpdateManager = async () => { - App.lastUpdateIdFromUpdatesProxy.lastUpdateIdFromUpdates = 2; + App.shouldGetMissingOnyxUpdatesUpToIdProxy.shouldGetMissingOnyxUpdatesUpToId = 2; await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); OnyxUpdateManager.resetDeferralLogicVariables(); }; @@ -134,7 +65,6 @@ describe('OnyxUpdateManager', () => { jest.clearAllMocks(); Onyx.clear(); await resetOnyxUpdateManager(); - Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, callback: (value) => (lastUpdateIDAppliedToClient = value ?? 1), @@ -149,20 +79,27 @@ describe('OnyxUpdateManager', () => { return App.getMissingOnyxUpdatesTriggeredPromise .then(() => { + // Missing updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) should have been fetched from the server + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); + // All scheduled updates should have been added to the deferred list expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); }) .then(waitForBatchedUpdates) .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(5); - expect(applyUpdates).toHaveBeenCalledTimes(1); + + // There should be only one call to applyUpdates. The call should contain all the deferred update, + // since the locally applied updates have changed in the meantime. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention - expect(applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); }); }); - it('should only apply deferred updates that are after the locally applied update', async () => { - App.lastUpdateIdFromUpdatesProxy.lastUpdateIdFromUpdates = 3; + it('should only apply deferred updates that are after the locally applied update (pending updates)', async () => { + App.shouldGetMissingOnyxUpdatesUpToIdProxy.shouldGetMissingOnyxUpdatesUpToId = 3; OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); @@ -170,16 +107,67 @@ describe('OnyxUpdateManager', () => { return App.getMissingOnyxUpdatesTriggeredPromise .then(() => { + // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); + // All scheduled updates should have been added to the deferred list expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); - Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); + + // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, + // while we are waiting for the missing updates to be fetched. + // Only the deferred updates after the lastUpdateIDAppliedToClient should be applied. + return Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); }) + .then(() => ApplyUpdates.applyUpdatesTriggeredPromise) .then(waitForBatchedUpdates) .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(6); - expect(applyUpdates).toHaveBeenCalledTimes(1); + + // There should be only one call to applyUpdates. The call should only contain the last deferred update, + // since the locally applied updates have changed in the meantime. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); // eslint-disable-next-line @typescript-eslint/naming-convention - expect(applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); }); }); + + it('should re-fetch missing updates if the deferred updates have a gap', async () => { + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); + + return ( + App.getMissingOnyxUpdatesTriggeredPromise + .then(() => { + // All scheduled updates should have been added to the deferred list + expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); + }) + // The applicable updates (only update 3) should be applied first + .then(() => ApplyUpdates.applyUpdatesTriggeredPromise) + // After applying the applicable updates, the missing updates from 3 to 4 should be re-fetched + .then(() => App.getMissingOnyxUpdatesTriggeredPromise) + .then(() => { + // The deferred updates after the gap should now be in the list of dererred updates. + expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(2); + }) + .then(waitForBatchedUpdates) + .then(() => OnyxUpdateManager.queryPromise) + ?.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(6); + + // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. + // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + + // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); + }) + ); + }); }); From 4eaf83a4f0f094d3a5241686168a1f3f0ffd3ec7 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 22 Apr 2024 17:41:12 +0200 Subject: [PATCH 21/37] apply updaes --- .../utils/__mocks__/applyUpdates.ts | 29 +-- .../utils/deferredUpdates.ts | 2 +- .../actions/OnyxUpdateManager/utils/index.ts | 14 +- src/libs/actions/__mocks__/App.ts | 32 +-- src/utils/createProxyForValue.ts | 15 +- src/utils/createTriggerPromise.ts | 12 -- tests/unit/OnyxUpdateManagerTest.ts | 189 ++++++++++++------ tests/utils/createTriggerPromise.ts | 44 ++++ 8 files changed, 218 insertions(+), 119 deletions(-) delete mode 100644 src/utils/createTriggerPromise.ts create mode 100644 tests/utils/createTriggerPromise.ts diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts index a5a916cadfe4a..43a96a757b98d 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts @@ -1,32 +1,35 @@ import Onyx from 'react-native-onyx'; import type DeferredUpdatesDictionary from '@libs/actions/OnyxUpdateManager/types'; +import createTriggerPromise from '@src/../tests/utils/createTriggerPromise'; import ONYXKEYS from '@src/ONYXKEYS'; -import createTriggerPromise from '@src/utils/createTriggerPromise'; +import createProxyForValue from '@src/utils/createProxyForValue'; -const {promise: applyUpdatesTriggeredPromise, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggeredPromise} = createTriggerPromise(); +const {initialPromises: initialApplyUpdatesTriggeredPromises, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggered} = createTriggerPromise(); -const applyUpdates = jest.fn((updates: DeferredUpdatesDictionary) => { - console.log('apply updates'); +const mockValues = { + applyUpdatesTriggered: initialApplyUpdatesTriggeredPromises, +}; +const mockValuesProxy = createProxyForValue(mockValues); + +const resetApplyUpdatesTriggeredPromise = () => + resetApplyUpdatesTriggered((newPromise, index) => { + mockValuesProxy.applyUpdatesTriggered[index] = newPromise; + }); +const applyUpdates = jest.fn((updates: DeferredUpdatesDictionary) => { applyUpdatesTriggered(); const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); + console.log({lastUpdateIdFromUpdates}); + const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIdFromUpdates); promise.finally(() => { resetApplyUpdatesTriggeredPromise(); }); - promise - .then(() => { - console.log('applyUpdates succeeded'); - }) - .catch((e) => { - console.log('applyUpdates failed', e); - }); - return promise; }); -export {applyUpdates, applyUpdatesTriggeredPromise}; +export {applyUpdates, mockValuesProxy}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts index 4463e8ea5a7bf..d27aacff9911f 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts @@ -3,6 +3,6 @@ import createProxyForValue from '@src/utils/createProxyForValue'; const deferredUpdatesValue = {deferredUpdates: {} as DeferredUpdatesDictionary}; -const deferredUpdatesProxy = createProxyForValue(deferredUpdatesValue, 'deferredUpdates'); +const deferredUpdatesProxy = createProxyForValue(deferredUpdatesValue); export default deferredUpdatesProxy; diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index 0ca42a46db7ad..6f45d6d648162 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -14,7 +14,9 @@ Onyx.connect({ // In order for the deferred updates to be applied correctly in order, // we need to check if there are any gaps between deferred updates. type DetectGapAndSplitResult = {applicableUpdates: DeferredUpdatesDictionary; updatesAfterGaps: DeferredUpdatesDictionary; latestMissingUpdateID: number | undefined}; -function detectGapsAndSplit(updates: DeferredUpdatesDictionary, lastUpdateIDFromClient: number): DetectGapAndSplitResult { +function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdateID?: number): DetectGapAndSplitResult { + const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0; + const updateValues = Object.values(updates); const applicableUpdates: DeferredUpdatesDictionary = {}; @@ -103,21 +105,19 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number): Promise { deferredUpdatesProxy.deferredUpdates = {}; - console.log({applicableUpdates}); - applyUpdates(applicableUpdates).then(() => { // After we have applied the applicable updates, there might have been new deferred updates added. // In the next (recursive) call of "validateAndApplyDeferredUpdates", // the initial "updatesAfterGaps" and all new deferred updates will be applied in order, // as long as there was no new gap detected. Otherwise repeat the process. - console.log({updatesAfterGaps}); + const newLastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0; + deferredUpdatesProxy.deferredUpdates = {...deferredUpdatesProxy.deferredUpdates, ...updatesAfterGaps}; // updateDeferredUpdates(newDeferredUpdates); - // It should not be possible for lastUpdateIDAppliedToClient to be null, therefore we can ignore this case. // If lastUpdateIDAppliedToClient got updated in the meantime, we will just retrigger the validation and application of the current deferred updates. - if (latestMissingUpdateID <= lastUpdateIDFromClient) { + if (latestMissingUpdateID <= newLastUpdateIDFromClient) { validateAndApplyDeferredUpdates(clientLastUpdateID) .then(() => resolve(undefined)) .catch(reject); @@ -125,7 +125,7 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number): Promise validateAndApplyDeferredUpdates(clientLastUpdateID)) .then(() => resolve(undefined)) .catch(reject); diff --git a/src/libs/actions/__mocks__/App.ts b/src/libs/actions/__mocks__/App.ts index 81f3cbec41f34..443103ad72092 100644 --- a/src/libs/actions/__mocks__/App.ts +++ b/src/libs/actions/__mocks__/App.ts @@ -1,8 +1,8 @@ import Onyx from 'react-native-onyx'; import type * as AppImport from '@libs/actions/App'; +import createTriggerPromise from '@src/../tests/utils/createTriggerPromise'; import ONYXKEYS from '@src/ONYXKEYS'; import createProxyForValue from '@src/utils/createProxyForValue'; -import createTriggerPromise from '@src/utils/createTriggerPromise'; const AppImplementation: typeof AppImport = jest.requireActual('@libs/actions/App'); const { @@ -25,21 +25,29 @@ const { KEYS_TO_PRESERVE, } = AppImplementation; -const shouldGetMissingOnyxUpdatesUpToIdValue = {shouldGetMissingOnyxUpdatesUpToId: 2}; -const shouldGetMissingOnyxUpdatesUpToIdProxy = createProxyForValue(shouldGetMissingOnyxUpdatesUpToIdValue, 'shouldGetMissingOnyxUpdatesUpToId'); -const {shouldGetMissingOnyxUpdatesUpToId} = shouldGetMissingOnyxUpdatesUpToIdValue; +const { + initialPromises: initialGetMissingOnyxUpdatesTriggeredPromises, + trigger: getMissingOnyxUpdatesWasTriggered, + resetPromise: resetGetMissingOnyxUpdatesTriggered, +} = createTriggerPromise(); + +const mockValues = { + getMissingOnyxUpdatesTriggered: initialGetMissingOnyxUpdatesTriggeredPromises, +}; +const mockValuesProxy = createProxyForValue(mockValues); -const {promise: getMissingOnyxUpdatesTriggeredPromise, trigger: getMissingOnyxUpdatesWasTriggered, resetPromise: resetGetMissingOnyxUpdatesTriggeredPromise} = createTriggerPromise(); -const {promise: getMissingOnyxUpdatesDonePromise, trigger: getMissingOnyxUpdatesDone, resetPromise: resetGetMissingOnyxUpdatesDonePromise} = createTriggerPromise(); +const resetGetMissingOnyxUpdatesTriggeredPromise = () => { + resetGetMissingOnyxUpdatesTriggered((newPromise, index) => { + mockValuesProxy.getMissingOnyxUpdatesTriggered[index] = newPromise; + }); +}; -const getMissingOnyxUpdates = jest.fn(() => { - resetGetMissingOnyxUpdatesDonePromise(); +const getMissingOnyxUpdates = jest.fn((_fromID: number, toID: number) => { getMissingOnyxUpdatesWasTriggered(); - const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, shouldGetMissingOnyxUpdatesUpToId); + const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, toID); promise.finally(() => { - getMissingOnyxUpdatesDone(); resetGetMissingOnyxUpdatesTriggeredPromise(); }); @@ -48,9 +56,7 @@ const getMissingOnyxUpdates = jest.fn(() => { export { // Mocks - shouldGetMissingOnyxUpdatesUpToIdProxy, - getMissingOnyxUpdatesTriggeredPromise, - getMissingOnyxUpdatesDonePromise, + mockValuesProxy, getMissingOnyxUpdates, // Actual App implementation diff --git a/src/utils/createProxyForValue.ts b/src/utils/createProxyForValue.ts index e9e8eb5dca6bf..ace0951aa720c 100644 --- a/src/utils/createProxyForValue.ts +++ b/src/utils/createProxyForValue.ts @@ -1,17 +1,18 @@ -const createProxyForValue = (value: Record, variableName: VariableName) => +const createProxyForValue = >(value: Value) => new Proxy(value, { - get: (target, prop) => { - if (prop !== variableName) { + get: (target, property) => { + if (typeof property === 'symbol') { return undefined; } - return target[prop as VariableName]; + + return target[property]; }, - set: (target, prop, newValue) => { - if (prop !== variableName) { + set: (target, property, newValue) => { + if (typeof property === 'symbol') { return false; } // eslint-disable-next-line no-param-reassign - target[prop as VariableName] = newValue; + target[property as keyof Value] = newValue; return true; }, }); diff --git a/src/utils/createTriggerPromise.ts b/src/utils/createTriggerPromise.ts deleted file mode 100644 index 9060ba22ec86f..0000000000000 --- a/src/utils/createTriggerPromise.ts +++ /dev/null @@ -1,12 +0,0 @@ -const createTriggerPromise = () => { - let trigger: () => void = () => undefined; - const resetPromise = () => - new Promise((resolve) => { - trigger = resolve; - }); - const promise = resetPromise(); - - return {promise, trigger, resetPromise}; -}; - -export default createTriggerPromise; diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 996891f725c6c..c0923ce2f6788 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -11,15 +11,17 @@ jest.mock('@libs/actions/App'); jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); type AppActionsMock = typeof AppImport & { - shouldGetMissingOnyxUpdatesUpToIdProxy: Record<'shouldGetMissingOnyxUpdatesUpToId', number>; getMissingOnyxUpdates: jest.Mock>; - getMissingOnyxUpdatesTriggeredPromise: Promise; - getMissingOnyxUpdatesDonePromise: Promise; + mockValuesProxy: { + getMissingOnyxUpdatesTriggered: Array>; + }; }; type ApplyUpdatesMock = typeof ApplyUpdatesImport & { applyUpdates: jest.Mock>; - applyUpdatesTriggeredPromise: Promise; + mockValuesProxy: { + applyUpdatesTriggered: Promise; + }; }; const App = AppImport as AppActionsMock; @@ -53,22 +55,22 @@ const mockUpdate5 = createMockUpdate(5); const mockUpdate6 = createMockUpdate(6); const resetOnyxUpdateManager = async () => { - App.shouldGetMissingOnyxUpdatesUpToIdProxy.shouldGetMissingOnyxUpdatesUpToId = 2; await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); OnyxUpdateManager.resetDeferralLogicVariables(); }; describe('OnyxUpdateManager', () => { let lastUpdateIDAppliedToClient = 1; - - beforeEach(async () => { - jest.clearAllMocks(); - Onyx.clear(); - await resetOnyxUpdateManager(); + beforeAll(() => { Onyx.connect({ key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, callback: (value) => (lastUpdateIDAppliedToClient = value ?? 1), }); + }); + + beforeEach(async () => { + jest.clearAllMocks(); + await resetOnyxUpdateManager(); return waitForBatchedUpdates(); }); @@ -77,14 +79,27 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); - return App.getMissingOnyxUpdatesTriggeredPromise - .then(() => { - // Missing updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) should have been fetched from the server - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); - // All scheduled updates should have been added to the deferred list - expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); - }) - .then(waitForBatchedUpdates) + // return App.mockValuesProxy.getMissingOnyxUpdatesTriggered[0] + // .then(() => { + // // Missing updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) should have been fetched from the server + // expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); + // // All scheduled updates should have been added to the deferred list + // expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); + // }) + // .then(waitForBatchedUpdates) + // .then(() => OnyxUpdateManager.queryPromise) + // ?.then(() => { + // // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + // expect(lastUpdateIDAppliedToClient).toBe(5); + + // // There should be only one call to applyUpdates. The call should contain all the deferred update, + // // since the locally applied updates have changed in the meantime. + // expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + // // eslint-disable-next-line @typescript-eslint/naming-convention + // expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); + // }); + + return waitForBatchedUpdates() .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. @@ -99,34 +114,51 @@ describe('OnyxUpdateManager', () => { }); it('should only apply deferred updates that are after the locally applied update (pending updates)', async () => { - App.shouldGetMissingOnyxUpdatesUpToIdProxy.shouldGetMissingOnyxUpdatesUpToId = 3; - OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - return App.getMissingOnyxUpdatesTriggeredPromise - .then(() => { - // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); - // All scheduled updates should have been added to the deferred list - expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); - - // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, - // while we are waiting for the missing updates to be fetched. - // Only the deferred updates after the lastUpdateIDAppliedToClient should be applied. - return Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); - }) - .then(() => ApplyUpdates.applyUpdatesTriggeredPromise) - .then(waitForBatchedUpdates) + // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, + // while we are waiting for the missing updates to be fetched. + // Only the deferred updates after the lastUpdateIDAppliedToClient should be applied. + Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); + + // return App.mockValuesProxy.getMissingOnyxUpdatesTriggered[0] + // .then(() => { + // // All scheduled updates should have been added to the deferred list + // expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); + // }) + // .then(() => ApplyUpdates.mockValuesProxy.applyUpdatesTriggered) + // .then(waitForBatchedUpdates) + // .then(() => OnyxUpdateManager.queryPromise) + // ?.then(() => { + // // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + // expect(lastUpdateIDAppliedToClient).toBe(6); + + // // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server in the first and only call to getMissingOnyxUpdates + // expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); + + // // There should be only one call to applyUpdates. The call should only contain the last deferred update, + // // since the locally applied updates have changed in the meantime. + // expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + + // // eslint-disable-next-line @typescript-eslint/naming-convention + // expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); + // }); + + return waitForBatchedUpdates() .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(6); + // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server in the first and only call to getMissingOnyxUpdates + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); + // There should be only one call to applyUpdates. The call should only contain the last deferred update, // since the locally applied updates have changed in the meantime. expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); }); @@ -137,37 +169,62 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - return ( - App.getMissingOnyxUpdatesTriggeredPromise - .then(() => { - // All scheduled updates should have been added to the deferred list - expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); - }) - // The applicable updates (only update 3) should be applied first - .then(() => ApplyUpdates.applyUpdatesTriggeredPromise) - // After applying the applicable updates, the missing updates from 3 to 4 should be re-fetched - .then(() => App.getMissingOnyxUpdatesTriggeredPromise) - .then(() => { - // The deferred updates after the gap should now be in the list of dererred updates. - expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(2); - }) - .then(waitForBatchedUpdates) - .then(() => OnyxUpdateManager.queryPromise) - ?.then(() => { - // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - expect(lastUpdateIDAppliedToClient).toBe(6); - - // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. - // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); - - // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); - }) - ); + // return ( + // App.mockValuesProxy.getMissingOnyxUpdatesTriggered[0] + // .then(() => { + // // All scheduled updates should have been added to the deferred list + // expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); + // }) + // // The applicable updates (only update 3) should be applied first + // .then(() => ApplyUpdates.mockValuesProxy.applyUpdatesTriggered) + // // After applying the applicable updates, the missing updates from 3 to 4 should be re-fetched + // .then(() => { + // App.mockValuesProxy.shouldGetMissingOnyxUpdatesUpToId = 4; + // }) + // .then(() => App.mockValuesProxy.getMissingOnyxUpdatesTriggered[1]) + // .then(() => { + // // The deferred updates after the gap should now be in the list of dererred updates. + // expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(2); + // }) + // .then(waitForBatchedUpdates) + // .then(() => OnyxUpdateManager.queryPromise) + // ?.then(() => { + // // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + // expect(lastUpdateIDAppliedToClient).toBe(6); + + // // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. + // // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. + // expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); + // // eslint-disable-next-line @typescript-eslint/naming-convention + // expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + + // // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. + // expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); + // // eslint-disable-next-line @typescript-eslint/naming-convention + // expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); + // }) + // ); + + return waitForBatchedUpdates() + .then(() => OnyxUpdateManager.queryPromise) + ?.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(6); + + // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. + // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); + + // After the initial missing updates have been applied, the applicable updates (3) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + + // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates. 3-4 + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); + + // After the gap in the deferred updates has been resolve, the remaining deferred updates (5, 6) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); + }); }); }); diff --git a/tests/utils/createTriggerPromise.ts b/tests/utils/createTriggerPromise.ts new file mode 100644 index 0000000000000..8afc4f06ef745 --- /dev/null +++ b/tests/utils/createTriggerPromise.ts @@ -0,0 +1,44 @@ +const createTriggerPromise = (count = 1) => { + let promiseIndex = 0; + let resolves: Array<() => void> = []; + const initialPromises = Array(count) + .fill(0) + .map( + (index) => + new Promise((resolve) => { + if (index !== 0) { + return; + } + resolves.push(resolve); + }), + ); + + let trigger: () => void = () => undefined; + + const resetPromise = (resetPromiseCallback?: (resettedPromise: Promise, index: number) => void) => { + const newPromise = new Promise((resolve) => { + trigger = resolve; + }); + + if (resetPromiseCallback) { + return resetPromiseCallback(newPromise, promiseIndex); + } + + initialPromises[promiseIndex] = newPromise; + if (promiseIndex < count - 1) { + promiseIndex++; + } + }; + + if (resolves.length === 0) { + resetPromise(); + } else { + trigger = resolves[0]; + resolves = []; + } + + resetPromise(); + return {initialPromises, trigger, resetPromise}; +}; + +export default createTriggerPromise; From 99109431ead2f4287a7d151fee3c8fc33f4301db Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 22 Apr 2024 17:45:43 +0200 Subject: [PATCH 22/37] remove unnecessary promise trigger code --- .../utils/__mocks__/applyUpdates.ts | 27 +----- src/libs/actions/__mocks__/App.ts | 27 ------ tests/unit/OnyxUpdateManagerTest.ts | 86 ------------------- tests/utils/createTriggerPromise.ts | 44 ---------- 4 files changed, 2 insertions(+), 182 deletions(-) delete mode 100644 tests/utils/createTriggerPromise.ts diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts index 43a96a757b98d..1a3f0f3e8a95b 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts @@ -1,35 +1,12 @@ import Onyx from 'react-native-onyx'; import type DeferredUpdatesDictionary from '@libs/actions/OnyxUpdateManager/types'; -import createTriggerPromise from '@src/../tests/utils/createTriggerPromise'; import ONYXKEYS from '@src/ONYXKEYS'; -import createProxyForValue from '@src/utils/createProxyForValue'; - -const {initialPromises: initialApplyUpdatesTriggeredPromises, trigger: applyUpdatesTriggered, resetPromise: resetApplyUpdatesTriggered} = createTriggerPromise(); - -const mockValues = { - applyUpdatesTriggered: initialApplyUpdatesTriggeredPromises, -}; -const mockValuesProxy = createProxyForValue(mockValues); - -const resetApplyUpdatesTriggeredPromise = () => - resetApplyUpdatesTriggered((newPromise, index) => { - mockValuesProxy.applyUpdatesTriggered[index] = newPromise; - }); const applyUpdates = jest.fn((updates: DeferredUpdatesDictionary) => { - applyUpdatesTriggered(); - const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); - - console.log({lastUpdateIdFromUpdates}); - const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIdFromUpdates); - - promise.finally(() => { - resetApplyUpdatesTriggeredPromise(); - }); - return promise; }); -export {applyUpdates, mockValuesProxy}; +// eslint-disable-next-line import/prefer-default-export +export {applyUpdates}; diff --git a/src/libs/actions/__mocks__/App.ts b/src/libs/actions/__mocks__/App.ts index 443103ad72092..6b834cf4f3710 100644 --- a/src/libs/actions/__mocks__/App.ts +++ b/src/libs/actions/__mocks__/App.ts @@ -1,8 +1,6 @@ import Onyx from 'react-native-onyx'; import type * as AppImport from '@libs/actions/App'; -import createTriggerPromise from '@src/../tests/utils/createTriggerPromise'; import ONYXKEYS from '@src/ONYXKEYS'; -import createProxyForValue from '@src/utils/createProxyForValue'; const AppImplementation: typeof AppImport = jest.requireActual('@libs/actions/App'); const { @@ -25,38 +23,13 @@ const { KEYS_TO_PRESERVE, } = AppImplementation; -const { - initialPromises: initialGetMissingOnyxUpdatesTriggeredPromises, - trigger: getMissingOnyxUpdatesWasTriggered, - resetPromise: resetGetMissingOnyxUpdatesTriggered, -} = createTriggerPromise(); - -const mockValues = { - getMissingOnyxUpdatesTriggered: initialGetMissingOnyxUpdatesTriggeredPromises, -}; -const mockValuesProxy = createProxyForValue(mockValues); - -const resetGetMissingOnyxUpdatesTriggeredPromise = () => { - resetGetMissingOnyxUpdatesTriggered((newPromise, index) => { - mockValuesProxy.getMissingOnyxUpdatesTriggered[index] = newPromise; - }); -}; - const getMissingOnyxUpdates = jest.fn((_fromID: number, toID: number) => { - getMissingOnyxUpdatesWasTriggered(); - const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, toID); - - promise.finally(() => { - resetGetMissingOnyxUpdatesTriggeredPromise(); - }); - return promise; }); export { // Mocks - mockValuesProxy, getMissingOnyxUpdates, // Actual App implementation diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index c0923ce2f6788..ca7fefb4c28ed 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -2,7 +2,6 @@ import Onyx from 'react-native-onyx'; import * as AppImport from '@libs/actions/App'; import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; -import deferredUpdatesProxy from '@libs/actions/OnyxUpdateManager/utils/deferredUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; @@ -12,16 +11,10 @@ jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); type AppActionsMock = typeof AppImport & { getMissingOnyxUpdates: jest.Mock>; - mockValuesProxy: { - getMissingOnyxUpdatesTriggered: Array>; - }; }; type ApplyUpdatesMock = typeof ApplyUpdatesImport & { applyUpdates: jest.Mock>; - mockValuesProxy: { - applyUpdatesTriggered: Promise; - }; }; const App = AppImport as AppActionsMock; @@ -79,26 +72,6 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); - // return App.mockValuesProxy.getMissingOnyxUpdatesTriggered[0] - // .then(() => { - // // Missing updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) should have been fetched from the server - // expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); - // // All scheduled updates should have been added to the deferred list - // expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); - // }) - // .then(waitForBatchedUpdates) - // .then(() => OnyxUpdateManager.queryPromise) - // ?.then(() => { - // // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - // expect(lastUpdateIDAppliedToClient).toBe(5); - - // // There should be only one call to applyUpdates. The call should contain all the deferred update, - // // since the locally applied updates have changed in the meantime. - // expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); - // // eslint-disable-next-line @typescript-eslint/naming-convention - // expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); - // }); - return waitForBatchedUpdates() .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { @@ -123,29 +96,6 @@ describe('OnyxUpdateManager', () => { // Only the deferred updates after the lastUpdateIDAppliedToClient should be applied. Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); - // return App.mockValuesProxy.getMissingOnyxUpdatesTriggered[0] - // .then(() => { - // // All scheduled updates should have been added to the deferred list - // expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); - // }) - // .then(() => ApplyUpdates.mockValuesProxy.applyUpdatesTriggered) - // .then(waitForBatchedUpdates) - // .then(() => OnyxUpdateManager.queryPromise) - // ?.then(() => { - // // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - // expect(lastUpdateIDAppliedToClient).toBe(6); - - // // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server in the first and only call to getMissingOnyxUpdates - // expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); - - // // There should be only one call to applyUpdates. The call should only contain the last deferred update, - // // since the locally applied updates have changed in the meantime. - // expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); - - // // eslint-disable-next-line @typescript-eslint/naming-convention - // expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); - // }); - return waitForBatchedUpdates() .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { @@ -169,42 +119,6 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - // return ( - // App.mockValuesProxy.getMissingOnyxUpdatesTriggered[0] - // .then(() => { - // // All scheduled updates should have been added to the deferred list - // expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(3); - // }) - // // The applicable updates (only update 3) should be applied first - // .then(() => ApplyUpdates.mockValuesProxy.applyUpdatesTriggered) - // // After applying the applicable updates, the missing updates from 3 to 4 should be re-fetched - // .then(() => { - // App.mockValuesProxy.shouldGetMissingOnyxUpdatesUpToId = 4; - // }) - // .then(() => App.mockValuesProxy.getMissingOnyxUpdatesTriggered[1]) - // .then(() => { - // // The deferred updates after the gap should now be in the list of dererred updates. - // expect(Object.keys(deferredUpdatesProxy.deferredUpdates)).toHaveLength(2); - // }) - // .then(waitForBatchedUpdates) - // .then(() => OnyxUpdateManager.queryPromise) - // ?.then(() => { - // // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - // expect(lastUpdateIDAppliedToClient).toBe(6); - - // // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. - // // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. - // expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); - // // eslint-disable-next-line @typescript-eslint/naming-convention - // expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); - - // // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. - // expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); - // // eslint-disable-next-line @typescript-eslint/naming-convention - // expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); - // }) - // ); - return waitForBatchedUpdates() .then(() => OnyxUpdateManager.queryPromise) ?.then(() => { diff --git a/tests/utils/createTriggerPromise.ts b/tests/utils/createTriggerPromise.ts deleted file mode 100644 index 8afc4f06ef745..0000000000000 --- a/tests/utils/createTriggerPromise.ts +++ /dev/null @@ -1,44 +0,0 @@ -const createTriggerPromise = (count = 1) => { - let promiseIndex = 0; - let resolves: Array<() => void> = []; - const initialPromises = Array(count) - .fill(0) - .map( - (index) => - new Promise((resolve) => { - if (index !== 0) { - return; - } - resolves.push(resolve); - }), - ); - - let trigger: () => void = () => undefined; - - const resetPromise = (resetPromiseCallback?: (resettedPromise: Promise, index: number) => void) => { - const newPromise = new Promise((resolve) => { - trigger = resolve; - }); - - if (resetPromiseCallback) { - return resetPromiseCallback(newPromise, promiseIndex); - } - - initialPromises[promiseIndex] = newPromise; - if (promiseIndex < count - 1) { - promiseIndex++; - } - }; - - if (resolves.length === 0) { - resetPromise(); - } else { - trigger = resolves[0]; - resolves = []; - } - - resetPromise(); - return {initialPromises, trigger, resetPromise}; -}; - -export default createTriggerPromise; From b078cfad52330c12cc7fce24d1f397b79f8204c6 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 22 Apr 2024 19:45:55 +0200 Subject: [PATCH 23/37] update tests and mocks --- src/libs/actions/OnyxUpdateManager/index.ts | 5 +- src/libs/actions/OnyxUpdateManager/types.ts | 4 +- .../utils/__mocks__/applyUpdates.ts | 10 +- .../utils/__mocks__/index.ts | 12 ++ .../OnyxUpdateManager/utils/applyUpdates.ts | 2 +- .../utils/deferredUpdates.ts | 2 +- .../actions/OnyxUpdateManager/utils/index.ts | 4 +- tests/unit/OnyxUpdateManagerTest.ts | 183 +++++++++++++++++- 8 files changed, 204 insertions(+), 18 deletions(-) create mode 100644 src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index 0fc84222d4142..d46f31fe952bd 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -7,7 +7,6 @@ import * as App from '@userActions/App'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer, Response} from '@src/types/onyx'; import {isValidOnyxUpdateFromServer} from '@src/types/onyx/OnyxUpdatesFromServer'; -import type DeferredUpdatesDictionary from './types'; import * as OnyxUpdateManagerUtils from './utils'; import deferredUpdatesProxy from './utils/deferredUpdates'; @@ -146,6 +145,4 @@ export default () => { }); }; -export {handleOnyxUpdateGap}; -export {queryPromise, resetDeferralLogicVariables}; -export type {DeferredUpdatesDictionary}; +export {handleOnyxUpdateGap, queryPromise, resetDeferralLogicVariables}; diff --git a/src/libs/actions/OnyxUpdateManager/types.ts b/src/libs/actions/OnyxUpdateManager/types.ts index a081dff006653..119dfb82ba1e6 100644 --- a/src/libs/actions/OnyxUpdateManager/types.ts +++ b/src/libs/actions/OnyxUpdateManager/types.ts @@ -2,4 +2,6 @@ import type {OnyxUpdatesFromServer} from '@src/types/onyx'; type DeferredUpdatesDictionary = Record; -export default DeferredUpdatesDictionary; +type DetectGapAndSplitResult = {applicableUpdates: DeferredUpdatesDictionary; updatesAfterGaps: DeferredUpdatesDictionary; latestMissingUpdateID: number | undefined}; + +export type {DeferredUpdatesDictionary, DetectGapAndSplitResult}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts index 1a3f0f3e8a95b..412214cdf2ec2 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts @@ -1,10 +1,16 @@ import Onyx from 'react-native-onyx'; -import type DeferredUpdatesDictionary from '@libs/actions/OnyxUpdateManager/types'; +import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; import ONYXKEYS from '@src/ONYXKEYS'; +let lastUpdateIDAppliedToClient = 0; +Onyx.connect({ + key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, + callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), +}); + const applyUpdates = jest.fn((updates: DeferredUpdatesDictionary) => { const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); - const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, lastUpdateIdFromUpdates); + const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, Math.max(lastUpdateIDAppliedToClient, lastUpdateIdFromUpdates)); return promise; }); diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts new file mode 100644 index 0000000000000..3cfb0abefbeba --- /dev/null +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts @@ -0,0 +1,12 @@ +import type * as UtilsImport from '..'; + +const UtilsImplementation: typeof UtilsImport = jest.requireActual('@libs/actions/OnyxUpdateManager/utils'); + +const detectGapsAndSplit = jest.fn(UtilsImplementation.detectGapsAndSplit); +const validateAndApplyDeferredUpdates = jest.fn(UtilsImplementation.validateAndApplyDeferredUpdates); + +export { + // Mocks + detectGapsAndSplit, + validateAndApplyDeferredUpdates, +}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/applyUpdates.ts index 37a1ec70728d8..5857b079c1ba1 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/applyUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/applyUpdates.ts @@ -1,5 +1,5 @@ // We need to keep this in a separate file, so that we can mock this function in tests. -import type DeferredUpdatesDictionary from '@libs/actions/OnyxUpdateManager/types'; +import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; import * as OnyxUpdates from '@userActions/OnyxUpdates'; // This function applies a list of updates to Onyx in order and resolves when all updates have been applied diff --git a/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts index d27aacff9911f..1bf251fbd3df9 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts @@ -1,4 +1,4 @@ -import type DeferredUpdatesDictionary from '@libs/actions/OnyxUpdateManager/types'; +import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; import createProxyForValue from '@src/utils/createProxyForValue'; const deferredUpdatesValue = {deferredUpdates: {} as DeferredUpdatesDictionary}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index 6f45d6d648162..8fb08c505f736 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -1,6 +1,6 @@ import Onyx from 'react-native-onyx'; import * as App from '@userActions/App'; -import type DeferredUpdatesDictionary from '@userActions/OnyxUpdateManager/types'; +import type {DeferredUpdatesDictionary, DetectGapAndSplitResult} from '@userActions/OnyxUpdateManager/types'; import ONYXKEYS from '@src/ONYXKEYS'; import {applyUpdates} from './applyUpdates'; import deferredUpdatesProxy from './deferredUpdates'; @@ -13,7 +13,7 @@ Onyx.connect({ // In order for the deferred updates to be applied correctly in order, // we need to check if there are any gaps between deferred updates. -type DetectGapAndSplitResult = {applicableUpdates: DeferredUpdatesDictionary; updatesAfterGaps: DeferredUpdatesDictionary; latestMissingUpdateID: number | undefined}; + function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdateID?: number): DetectGapAndSplitResult { const lastUpdateIDFromClient = clientLastUpdateID ?? lastUpdateIDAppliedToClient ?? 0; diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index ca7fefb4c28ed..d80b4da0a8a44 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -1,12 +1,15 @@ import Onyx from 'react-native-onyx'; import * as AppImport from '@libs/actions/App'; import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; +import type {DeferredUpdatesDictionary, DetectGapAndSplitResult} from '@libs/actions/OnyxUpdateManager/types'; +import * as OnyxUpdateManagerUtilsImport from '@libs/actions/OnyxUpdateManager/utils'; import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; jest.mock('@libs/actions/App'); +jest.mock('@libs/actions/OnyxUpdateManager/utils'); jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); type AppActionsMock = typeof AppImport & { @@ -14,11 +17,41 @@ type AppActionsMock = typeof AppImport & { }; type ApplyUpdatesMock = typeof ApplyUpdatesImport & { - applyUpdates: jest.Mock>; + applyUpdates: jest.Mock, [updates: DeferredUpdatesDictionary]>; +}; + +type OnyxUpdateManagerUtilsMock = typeof OnyxUpdateManagerUtilsImport & { + detectGapsAndSplit: jest.Mock, [updates: DeferredUpdatesDictionary, clientLastUpdateID?: number]>; + validateAndApplyDeferredUpdates: jest.Mock, [clientLastUpdateID?: number]>; }; const App = AppImport as AppActionsMock; const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; +const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock; + +let onApplyUpdates: ((updates: DeferredUpdatesDictionary) => Promise) | undefined; +ApplyUpdates.applyUpdates.mockImplementation((updates: DeferredUpdatesDictionary) => { + const ApplyUpdatesMock: ApplyUpdatesMock = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'); + + if (onApplyUpdates === undefined) { + return ApplyUpdatesMock.applyUpdates(updates).then(() => []); + } + + return onApplyUpdates(updates) + .then(() => ApplyUpdatesMock.applyUpdates(updates)) + .then(() => []); +}); + +let onValidateAndApplyDeferredUpdates: ((clientLastUpdateID?: number) => Promise) | undefined; +OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates.mockImplementation((clientLastUpdateID?: number) => { + const UtilsMock: OnyxUpdateManagerUtilsMock = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/__mocks__'); + + if (onValidateAndApplyDeferredUpdates === undefined) { + return UtilsMock.validateAndApplyDeferredUpdates(clientLastUpdateID); + } + + return onValidateAndApplyDeferredUpdates(clientLastUpdateID).then(() => UtilsMock.validateAndApplyDeferredUpdates(clientLastUpdateID)); +}); const createMockUpdate = (lastUpdateID: number): OnyxUpdatesFromServer => ({ type: 'https', @@ -46,6 +79,8 @@ const mockUpdate3 = createMockUpdate(3); const mockUpdate4 = createMockUpdate(4); const mockUpdate5 = createMockUpdate(5); const mockUpdate6 = createMockUpdate(6); +const mockUpdate7 = createMockUpdate(7); +const mockUpdate8 = createMockUpdate(8); const resetOnyxUpdateManager = async () => { await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); @@ -78,6 +113,11 @@ describe('OnyxUpdateManager', () => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(5); + // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + // There should be only one call to applyUpdates. The call should contain all the deferred update, // since the locally applied updates have changed in the meantime. expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); @@ -86,15 +126,18 @@ describe('OnyxUpdateManager', () => { }); }); - it('should only apply deferred updates that are after the locally applied update (pending updates)', async () => { + it('should only apply deferred updates that are newer than the last locally applied update (pending updates)', async () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, - // while we are waiting for the missing updates to be fetched. - // Only the deferred updates after the lastUpdateIDAppliedToClient should be applied. - Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); + onValidateAndApplyDeferredUpdates = async () => { + // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, + // while we are waiting for the missing updates to be fetched. + // Only the deferred updates after the lastUpdateIDAppliedToClient should be applied. + await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); + onValidateAndApplyDeferredUpdates = undefined; + }; return waitForBatchedUpdates() .then(() => OnyxUpdateManager.queryPromise) @@ -102,6 +145,11 @@ describe('OnyxUpdateManager', () => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(6); + // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server in the first and only call to getMissingOnyxUpdates expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); @@ -125,6 +173,11 @@ describe('OnyxUpdateManager', () => { // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. expect(lastUpdateIDAppliedToClient).toBe(6); + // Even though there is a gap in the deferred updates, we only want to fetch missing updates once per batch. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); + // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); @@ -136,9 +189,125 @@ describe('OnyxUpdateManager', () => { // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates. 3-4 expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); - // After the gap in the deferred updates has been resolve, the remaining deferred updates (5, 6) should be applied. + // After the gap in the deferred updates has been resolved, the remaining deferred updates (5, 6) should be applied. // eslint-disable-next-line @typescript-eslint/naming-convention expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); }); }); + + it('should re-fetch missing deferred updates only once per batch', async () => { + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate8); + + return waitForBatchedUpdates() + .then(() => OnyxUpdateManager.queryPromise) + ?.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(8); + + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); + + // After the initial missing updates have been applied, the applicable updates (3-4) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3, 4: mockUpdate4}); + + // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap (4-7) in the deferred updates. + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 4, 7); + + // After the gap in the deferred updates has been resolved, the remaining deferred updates (8) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {8: mockUpdate8}); + }); + }); + + it('should not re-fetch missing updates if the locally applied update has been updated', async () => { + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate7); + + onApplyUpdates = async () => { + // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, + // while the applicable updates have been applied. + // When this happens, the OnyxUpdateManager should trigger another validation of the deferred updates, + // without triggering another re-fetching of missing updates from the server. + await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); + onApplyUpdates = undefined; + }; + + return waitForBatchedUpdates() + .then(() => OnyxUpdateManager.queryPromise) + ?.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(7); + + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + + // validateAndApplyDeferredUpdates should only be called once, since the locally applied update id has been updated in the meantime, + // and the deferred updates after the locally applied update don't have any gaps. + expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // Since there is a gap in the deferred updates, we need to run applyUpdates twice. + // Once for the applicable updates (before the gap) and then for the remaining deferred updates. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + + // After the initial missing updates have been applied, the applicable updates (3) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + + // Since the lastUpdateIDAppliedToClient has changed to 5 in the meantime, we only need to apply the remaining deferred updates (6-7). + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {6: mockUpdate6, 7: mockUpdate7}); + }); + }); + + it('should re-fetch missing updates if the locally applied update has been updated, but there are still gaps after the locally applied update', async () => { + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); + OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate7); + + onApplyUpdates = async () => { + // We manually update the lastUpdateIDAppliedToClient to 4, to simulate local updates being applied, + // while the applicable updates have been applied. + // When this happens, the OnyxUpdateManager should trigger another validation of the deferred updates, + // without triggering another re-fetching of missing updates from the server. + await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 4); + onApplyUpdates = undefined; + }; + + return waitForBatchedUpdates() + .then(() => OnyxUpdateManager.queryPromise) + ?.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(7); + + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function, since it recursively calls itself. + expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // Since there is a gap in the deferred updates, we need to run applyUpdates twice. + // Once for the applicable updates (before the gap) and then for the remaining deferred updates. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + + // After the initial missing updates have been applied, the applicable updates (3) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + + // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates, + // that are later than the locally applied update (4-6). (including the last locally applied update) + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 4, 6); + + // Since the lastUpdateIDAppliedToClient has changed to 4 in the meantime and we're fetching updates 5-6 we only need to apply the remaining deferred updates (7). + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {7: mockUpdate7}); + }); + }); }); From 2c50853637fc3d6909ecc65d306f8e525393c447 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 22 Apr 2024 19:53:12 +0200 Subject: [PATCH 24/37] fix: ts --- src/libs/actions/OnyxUpdateManager/utils/index.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index 8fb08c505f736..a04c7e8842191 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -57,9 +57,7 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdate // When "firstUpdateAfterGaps" is not set yet, we need to set it to the last update in the list, // because we will fetch all missing updates up to the previous one and can then always apply the last update in the deferred updates. - if (!firstUpdateAfterGaps) { - firstUpdateAfterGaps = Number(updateValues[updateValues.length - 1].lastUpdateID); - } + const firstUpdateAfterGapWithFallback = firstUpdateAfterGaps ?? Number(updateValues[updateValues.length - 1].lastUpdateID); let updatesAfterGaps: DeferredUpdatesDictionary = {}; if (gapExists) { @@ -67,7 +65,7 @@ function detectGapsAndSplit(updates: DeferredUpdatesDictionary, clientLastUpdate (accUpdates, [lastUpdateID, update]) => ({ ...accUpdates, // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - ...(Number(lastUpdateID) >= firstUpdateAfterGaps ? {[Number(lastUpdateID)]: update} : {}), + ...(Number(lastUpdateID) >= firstUpdateAfterGapWithFallback ? {[Number(lastUpdateID)]: update} : {}), }), {}, ); From 3904ef4a167ca630dd5c793afbc61431d06def20 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 11:20:08 +0200 Subject: [PATCH 25/37] fix: tests --- tests/unit/OnyxUpdateManagerTest.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index d80b4da0a8a44..2110f8f49d675 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -176,7 +176,7 @@ describe('OnyxUpdateManager', () => { // Even though there is a gap in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); - expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. @@ -210,7 +210,7 @@ describe('OnyxUpdateManager', () => { // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); - expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); // After the initial missing updates have been applied, the applicable updates (3-4) should be applied. // eslint-disable-next-line @typescript-eslint/naming-convention @@ -291,7 +291,7 @@ describe('OnyxUpdateManager', () => { // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. // Unfortunately, we cannot easily count the calls of this function, since it recursively calls itself. - expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); // Since there is a gap in the deferred updates, we need to run applyUpdates twice. // Once for the applicable updates (before the gap) and then for the remaining deferred updates. From abff1aabc8ab9ce946ac11f16b240feddbec5cd2 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 12:15:25 +0200 Subject: [PATCH 26/37] move specific mocks to mock folders --- .../utils/__mocks__/applyUpdates.ts | 24 +++++++-- .../utils/__mocks__/index.ts | 35 +++++++++--- src/libs/actions/__mocks__/App.ts | 5 ++ tests/unit/OnyxUpdateManagerTest.ts | 53 ++++--------------- 4 files changed, 61 insertions(+), 56 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts index 412214cdf2ec2..d02a034c29cc1 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts @@ -1,6 +1,7 @@ import Onyx from 'react-native-onyx'; import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; import ONYXKEYS from '@src/ONYXKEYS'; +import createProxyForValue from '@src/utils/createProxyForValue'; let lastUpdateIDAppliedToClient = 0; Onyx.connect({ @@ -8,11 +9,26 @@ Onyx.connect({ callback: (value) => (lastUpdateIDAppliedToClient = value ?? 0), }); +type ApplyUpdatesMockValues = { + onApplyUpdates: ((updates: DeferredUpdatesDictionary) => Promise) | undefined; +}; + +type ApplyUpdatesMock = { + applyUpdates: jest.Mock, [updates: DeferredUpdatesDictionary]>; + mockValues: ApplyUpdatesMockValues; +}; + +const mockValues: ApplyUpdatesMockValues = { + onApplyUpdates: undefined, +}; +const mockValuesProxy = createProxyForValue(mockValues); + const applyUpdates = jest.fn((updates: DeferredUpdatesDictionary) => { const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); - const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, Math.max(lastUpdateIDAppliedToClient, lastUpdateIdFromUpdates)); - return promise; + return (mockValuesProxy.onApplyUpdates === undefined ? Promise.resolve() : mockValuesProxy.onApplyUpdates(updates)).then(() => + Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, Math.max(lastUpdateIDAppliedToClient, lastUpdateIdFromUpdates)), + ); }); -// eslint-disable-next-line import/prefer-default-export -export {applyUpdates}; +export {applyUpdates, mockValuesProxy as mockValues}; +export type {ApplyUpdatesMock}; diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts index 3cfb0abefbeba..461e074dd8816 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts @@ -1,12 +1,31 @@ -import type * as UtilsImport from '..'; +import type {DeferredUpdatesDictionary, DetectGapAndSplitResult} from '@libs/actions/OnyxUpdateManager/types'; +import createProxyForValue from '@src/utils/createProxyForValue'; +import type * as OnyxUpdateManagerUtilsImport from '..'; -const UtilsImplementation: typeof UtilsImport = jest.requireActual('@libs/actions/OnyxUpdateManager/utils'); +const UtilsImplementation: typeof OnyxUpdateManagerUtilsImport = jest.requireActual('@libs/actions/OnyxUpdateManager/utils'); -const detectGapsAndSplit = jest.fn(UtilsImplementation.detectGapsAndSplit); -const validateAndApplyDeferredUpdates = jest.fn(UtilsImplementation.validateAndApplyDeferredUpdates); +type OnyxUpdateManagerUtilsMockValues = { + onValidateAndApplyDeferredUpdates: ((clientLastUpdateID?: number) => Promise) | undefined; +}; + +type OnyxUpdateManagerUtilsMock = typeof UtilsImplementation & { + detectGapsAndSplit: jest.Mock, [updates: DeferredUpdatesDictionary, clientLastUpdateID?: number]>; + validateAndApplyDeferredUpdates: jest.Mock, [clientLastUpdateID?: number]>; + mockValues: OnyxUpdateManagerUtilsMockValues; +}; -export { - // Mocks - detectGapsAndSplit, - validateAndApplyDeferredUpdates, +const mockValues: OnyxUpdateManagerUtilsMockValues = { + onValidateAndApplyDeferredUpdates: undefined, }; +const mockValuesProxy = createProxyForValue(mockValues); + +const detectGapsAndSplit = jest.fn(UtilsImplementation.detectGapsAndSplit); + +const validateAndApplyDeferredUpdates = jest.fn((clientLastUpdateID?: number) => + (mockValuesProxy.onValidateAndApplyDeferredUpdates === undefined ? Promise.resolve() : mockValuesProxy.onValidateAndApplyDeferredUpdates(clientLastUpdateID)).then(() => + UtilsImplementation.validateAndApplyDeferredUpdates(clientLastUpdateID), + ), +); + +export {detectGapsAndSplit, validateAndApplyDeferredUpdates, mockValuesProxy as mockValues}; +export type {OnyxUpdateManagerUtilsMock}; diff --git a/src/libs/actions/__mocks__/App.ts b/src/libs/actions/__mocks__/App.ts index 6b834cf4f3710..3de0c3d946c9b 100644 --- a/src/libs/actions/__mocks__/App.ts +++ b/src/libs/actions/__mocks__/App.ts @@ -23,6 +23,10 @@ const { KEYS_TO_PRESERVE, } = AppImplementation; +type AppActionsMock = typeof AppImport & { + getMissingOnyxUpdates: jest.Mock>; +}; + const getMissingOnyxUpdates = jest.fn((_fromID: number, toID: number) => { const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, toID); return promise; @@ -51,3 +55,4 @@ export { updateLastVisitedPath, KEYS_TO_PRESERVE, }; +export type {AppActionsMock}; diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 2110f8f49d675..57884fa8fd7c3 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -1,8 +1,10 @@ import Onyx from 'react-native-onyx'; +import type {AppActionsMock} from '@libs/actions/__mocks__/App'; import * as AppImport from '@libs/actions/App'; import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; -import type {DeferredUpdatesDictionary, DetectGapAndSplitResult} from '@libs/actions/OnyxUpdateManager/types'; import * as OnyxUpdateManagerUtilsImport from '@libs/actions/OnyxUpdateManager/utils'; +import type {OnyxUpdateManagerUtilsMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__'; +import type {ApplyUpdatesMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'; import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; @@ -12,47 +14,10 @@ jest.mock('@libs/actions/App'); jest.mock('@libs/actions/OnyxUpdateManager/utils'); jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); -type AppActionsMock = typeof AppImport & { - getMissingOnyxUpdates: jest.Mock>; -}; - -type ApplyUpdatesMock = typeof ApplyUpdatesImport & { - applyUpdates: jest.Mock, [updates: DeferredUpdatesDictionary]>; -}; - -type OnyxUpdateManagerUtilsMock = typeof OnyxUpdateManagerUtilsImport & { - detectGapsAndSplit: jest.Mock, [updates: DeferredUpdatesDictionary, clientLastUpdateID?: number]>; - validateAndApplyDeferredUpdates: jest.Mock, [clientLastUpdateID?: number]>; -}; - const App = AppImport as AppActionsMock; const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock; -let onApplyUpdates: ((updates: DeferredUpdatesDictionary) => Promise) | undefined; -ApplyUpdates.applyUpdates.mockImplementation((updates: DeferredUpdatesDictionary) => { - const ApplyUpdatesMock: ApplyUpdatesMock = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'); - - if (onApplyUpdates === undefined) { - return ApplyUpdatesMock.applyUpdates(updates).then(() => []); - } - - return onApplyUpdates(updates) - .then(() => ApplyUpdatesMock.applyUpdates(updates)) - .then(() => []); -}); - -let onValidateAndApplyDeferredUpdates: ((clientLastUpdateID?: number) => Promise) | undefined; -OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates.mockImplementation((clientLastUpdateID?: number) => { - const UtilsMock: OnyxUpdateManagerUtilsMock = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/__mocks__'); - - if (onValidateAndApplyDeferredUpdates === undefined) { - return UtilsMock.validateAndApplyDeferredUpdates(clientLastUpdateID); - } - - return onValidateAndApplyDeferredUpdates(clientLastUpdateID).then(() => UtilsMock.validateAndApplyDeferredUpdates(clientLastUpdateID)); -}); - const createMockUpdate = (lastUpdateID: number): OnyxUpdatesFromServer => ({ type: 'https', lastUpdateID, @@ -131,12 +96,12 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - onValidateAndApplyDeferredUpdates = async () => { + OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, // while we are waiting for the missing updates to be fetched. // Only the deferred updates after the lastUpdateIDAppliedToClient should be applied. await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); - onValidateAndApplyDeferredUpdates = undefined; + OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = undefined; }; return waitForBatchedUpdates() @@ -231,13 +196,13 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate7); - onApplyUpdates = async () => { + ApplyUpdates.mockValues.onApplyUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 5, to simulate local updates being applied, // while the applicable updates have been applied. // When this happens, the OnyxUpdateManager should trigger another validation of the deferred updates, // without triggering another re-fetching of missing updates from the server. await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 5); - onApplyUpdates = undefined; + ApplyUpdates.mockValues.onApplyUpdates = undefined; }; return waitForBatchedUpdates() @@ -271,13 +236,13 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate7); - onApplyUpdates = async () => { + ApplyUpdates.mockValues.onApplyUpdates = async () => { // We manually update the lastUpdateIDAppliedToClient to 4, to simulate local updates being applied, // while the applicable updates have been applied. // When this happens, the OnyxUpdateManager should trigger another validation of the deferred updates, // without triggering another re-fetching of missing updates from the server. await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 4); - onApplyUpdates = undefined; + ApplyUpdates.mockValues.onApplyUpdates = undefined; }; return waitForBatchedUpdates() From 39387f64b8b4b08a98eaf275423edcc4396f1ac9 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 12:29:05 +0200 Subject: [PATCH 27/37] fix: update mocks and tests --- .../utils/__mocks__/index.ts | 3 +- tests/unit/OnyxUpdateManagerTest.ts | 29 +++++++++++++++---- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts index 461e074dd8816..e548a9e22b1a3 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts @@ -1,6 +1,7 @@ import type {DeferredUpdatesDictionary, DetectGapAndSplitResult} from '@libs/actions/OnyxUpdateManager/types'; import createProxyForValue from '@src/utils/createProxyForValue'; import type * as OnyxUpdateManagerUtilsImport from '..'; +import {applyUpdates} from './applyUpdates'; const UtilsImplementation: typeof OnyxUpdateManagerUtilsImport = jest.requireActual('@libs/actions/OnyxUpdateManager/utils'); @@ -27,5 +28,5 @@ const validateAndApplyDeferredUpdates = jest.fn((clientLastUpdateID?: number) => ), ); -export {detectGapsAndSplit, validateAndApplyDeferredUpdates, mockValuesProxy as mockValues}; +export {applyUpdates, detectGapsAndSplit, validateAndApplyDeferredUpdates, mockValuesProxy as mockValues}; export type {OnyxUpdateManagerUtilsMock}; diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 57884fa8fd7c3..b7b6e103fbde7 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -81,7 +81,11 @@ describe('OnyxUpdateManager', () => { // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); - expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); // There should be only one call to applyUpdates. The call should contain all the deferred update, // since the locally applied updates have changed in the meantime. @@ -113,7 +117,11 @@ describe('OnyxUpdateManager', () => { // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); - expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server in the first and only call to getMissingOnyxUpdates expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); @@ -141,6 +149,10 @@ describe('OnyxUpdateManager', () => { // Even though there is a gap in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. @@ -175,6 +187,9 @@ describe('OnyxUpdateManager', () => { // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); // After the initial missing updates have been applied, the applicable updates (3-4) should be applied. @@ -214,9 +229,10 @@ describe('OnyxUpdateManager', () => { // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); - // validateAndApplyDeferredUpdates should only be called once, since the locally applied update id has been updated in the meantime, - // and the deferred updates after the locally applied update don't have any gaps. - expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); // Since there is a gap in the deferred updates, we need to run applyUpdates twice. // Once for the applicable updates (before the gap) and then for the remaining deferred updates. @@ -255,7 +271,8 @@ describe('OnyxUpdateManager', () => { expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. - // Unfortunately, we cannot easily count the calls of this function, since it recursively calls itself. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); // Since there is a gap in the deferred updates, we need to run applyUpdates twice. From 63279686e75b5072f3e08dd9f35bf44a77017438 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 12:33:36 +0200 Subject: [PATCH 28/37] simplify --- tests/unit/OnyxUpdateManagerTest.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index b7b6e103fbde7..5cb636e3a572c 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -47,11 +47,6 @@ const mockUpdate6 = createMockUpdate(6); const mockUpdate7 = createMockUpdate(7); const mockUpdate8 = createMockUpdate(8); -const resetOnyxUpdateManager = async () => { - await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); - OnyxUpdateManager.resetDeferralLogicVariables(); -}; - describe('OnyxUpdateManager', () => { let lastUpdateIDAppliedToClient = 1; beforeAll(() => { @@ -63,7 +58,8 @@ describe('OnyxUpdateManager', () => { beforeEach(async () => { jest.clearAllMocks(); - await resetOnyxUpdateManager(); + await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); + OnyxUpdateManager.resetDeferralLogicVariables(); return waitForBatchedUpdates(); }); From b0a70babbfc10787742d742e9c736e43b3e8d74c Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 15:27:43 +0200 Subject: [PATCH 29/37] fix: update mocks, tests and test utils to allow for broader E2E tests --- src/libs/actions/OnyxUpdateManager/index.ts | 15 ++- src/libs/actions/__mocks__/App.ts | 24 +++- tests/actions/OnyxUpdateManagerTest.ts | 131 ++++++++++++++++++++ tests/unit/OnyxUpdateManagerTest.ts | 36 ++---- tests/utils/createOnyxMockUpdate.ts | 23 ++++ 5 files changed, 195 insertions(+), 34 deletions(-) create mode 100644 tests/actions/OnyxUpdateManagerTest.ts create mode 100644 tests/utils/createOnyxMockUpdate.ts diff --git a/src/libs/actions/OnyxUpdateManager/index.ts b/src/libs/actions/OnyxUpdateManager/index.ts index d46f31fe952bd..8c66953796143 100644 --- a/src/libs/actions/OnyxUpdateManager/index.ts +++ b/src/libs/actions/OnyxUpdateManager/index.ts @@ -39,9 +39,16 @@ Onyx.connect({ }, }); -// eslint-disable-next-line import/no-mutable-exports let queryPromise: Promise | undefined; +let resolveQueryPromiseWrapper: () => void; +const createQueryPromiseWrapper = () => + new Promise((resolve) => { + resolveQueryPromiseWrapper = resolve; + }); +// eslint-disable-next-line import/no-mutable-exports +let queryPromiseWrapper = createQueryPromiseWrapper(); + const resetDeferralLogicVariables = () => { queryPromise = undefined; deferredUpdatesProxy.deferredUpdates = {}; @@ -50,6 +57,10 @@ const resetDeferralLogicVariables = () => { // This function will reset the query variables, unpause the SequentialQueue and log an info to the user. function finalizeUpdatesAndResumeQueue() { console.debug('[OnyxUpdateManager] Done applying all updates'); + + resolveQueryPromiseWrapper(); + queryPromiseWrapper = createQueryPromiseWrapper(); + resetDeferralLogicVariables(); Onyx.set(ONYXKEYS.ONYX_UPDATES_FROM_SERVER, null); SequentialQueue.unpause(); @@ -145,4 +156,4 @@ export default () => { }); }; -export {handleOnyxUpdateGap, queryPromise, resetDeferralLogicVariables}; +export {handleOnyxUpdateGap, queryPromiseWrapper as queryPromise, resetDeferralLogicVariables}; diff --git a/src/libs/actions/__mocks__/App.ts b/src/libs/actions/__mocks__/App.ts index 3de0c3d946c9b..574a5b51e1b9b 100644 --- a/src/libs/actions/__mocks__/App.ts +++ b/src/libs/actions/__mocks__/App.ts @@ -1,6 +1,9 @@ import Onyx from 'react-native-onyx'; import type * as AppImport from '@libs/actions/App'; +import type * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; +import type {OnyxUpdatesFromServer} from '@src/types/onyx'; +import createProxyForValue from '@src/utils/createProxyForValue'; const AppImplementation: typeof AppImport = jest.requireActual('@libs/actions/App'); const { @@ -23,18 +26,33 @@ const { KEYS_TO_PRESERVE, } = AppImplementation; +type AppMockValues = { + missingOnyxUpdatesToBeApplied: OnyxUpdatesFromServer[] | undefined; +}; + type AppActionsMock = typeof AppImport & { - getMissingOnyxUpdates: jest.Mock>; + getMissingOnyxUpdates: jest.Mock>; + mockValues: AppMockValues; }; +const mockValues: AppMockValues = { + missingOnyxUpdatesToBeApplied: undefined, +}; +const mockValuesProxy = createProxyForValue(mockValues); + +const ApplyUpdatesImplementation: typeof ApplyUpdatesImport = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); const getMissingOnyxUpdates = jest.fn((_fromID: number, toID: number) => { - const promise = Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, toID); - return promise; + if (mockValuesProxy.missingOnyxUpdatesToBeApplied === undefined) { + return Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, toID); + } + + return ApplyUpdatesImplementation.applyUpdates(mockValuesProxy.missingOnyxUpdatesToBeApplied); }); export { // Mocks getMissingOnyxUpdates, + mockValuesProxy as mockValues, // Actual App implementation setLocale, diff --git a/tests/actions/OnyxUpdateManagerTest.ts b/tests/actions/OnyxUpdateManagerTest.ts new file mode 100644 index 0000000000000..e51285879e3b2 --- /dev/null +++ b/tests/actions/OnyxUpdateManagerTest.ts @@ -0,0 +1,131 @@ +import type {OnyxEntry} from 'react-native-onyx'; +import Onyx from 'react-native-onyx'; +import OnyxUtils from 'react-native-onyx/dist/OnyxUtils'; +import type {AppActionsMock} from '@libs/actions/__mocks__/App'; +import * as AppImport from '@libs/actions/App'; +import applyOnyxUpdatesReliably from '@libs/actions/applyOnyxUpdatesReliably'; +import * as OnyxUpdateManagerExports from '@libs/actions/OnyxUpdateManager'; +import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; +import type {ApplyUpdatesMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'; +import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; +import CONST from '@src/CONST'; +import OnyxUpdateManager from '@src/libs/actions/OnyxUpdateManager'; +import ONYXKEYS from '@src/ONYXKEYS'; +import type * as OnyxTypes from '@src/types/onyx'; +import createOnyxMockUpdate from '../utils/createOnyxMockUpdate'; +import * as TestHelper from '../utils/TestHelper'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; + +jest.mock('@libs/actions/App'); +jest.mock('@libs/actions/OnyxUpdateManager/utils'); +jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates', () => { + const ApplyUpdatesImplementation: typeof ApplyUpdatesImport = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); + + return { + applyUpdates: jest.fn((updates: DeferredUpdatesDictionary) => ApplyUpdatesImplementation.applyUpdates(updates)), + }; +}); + +const App = AppImport as AppActionsMock; +const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; + +const TEST_USER_ACCOUNT_ID = 1; +const REPORT_ID = 'testReport1'; +const ONYX_KEY = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}` as const; + +const exampleReportAction: Partial = { + actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, + actorAccountID: TEST_USER_ACCOUNT_ID, + automatic: false, + avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_3.png', + message: [{type: 'COMMENT', html: 'Testing a comment', text: 'Testing a comment', translationKey: ''}], + person: [{type: 'TEXT', style: 'strong', text: 'Test User'}], + shouldShow: true, +}; + +const initialData = {report1: exampleReportAction, report2: exampleReportAction, report3: exampleReportAction} as OnyxTypes.ReportActions; + +const mockUpdate2 = createOnyxMockUpdate(2, [ + { + onyxMethod: OnyxUtils.METHOD.MERGE, + key: ONYX_KEY, + value: { + report1: null, + }, + }, +]); + +const report2PersonDiff = [ + {type: 'TEXT', style: 'light', text: 'Other Test User'}, + {type: 'TEXT', style: 'light', text: 'Other Test User 2'}, +]; +const mockUpdate3 = createOnyxMockUpdate(3, [ + { + onyxMethod: OnyxUtils.METHOD.MERGE, + key: ONYX_KEY, + value: { + report2: { + person: report2PersonDiff, + }, + report3: { + avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_5.png', + }, + }, + }, +]); +const mockUpdate4 = createOnyxMockUpdate(4, [ + { + onyxMethod: OnyxUtils.METHOD.MERGE, + key: ONYX_KEY, + value: { + report3: null, + }, + }, +]); + +OnyxUpdateManager(); + +describe('actions/OnyxUpdateManager', () => { + let reportActions: OnyxEntry; + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + Onyx.connect({ + key: ONYX_KEY, + callback: (val) => (reportActions = val), + }); + }); + + beforeEach(async () => { + // @ts-expect-error TODO: Remove this once TestHelper (https://github.com/Expensify/App/issues/25318) is migrated to TypeScript. + global.fetch = TestHelper.getGlobalFetchMock(); + jest.clearAllMocks(); + await Onyx.clear(); + await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); + await Onyx.set(ONYX_KEY, initialData); + + App.mockValues.missingOnyxUpdatesToBeApplied = undefined; + OnyxUpdateManagerExports.resetDeferralLogicVariables(); + return waitForBatchedUpdates(); + }); + + it('should trigger Onyx update gap handling', async () => { + App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate3]; + + applyOnyxUpdatesReliably(mockUpdate2); + applyOnyxUpdatesReliably(mockUpdate4); + applyOnyxUpdatesReliably(mockUpdate3); + + return OnyxUpdateManagerExports.queryPromise.then(() => { + const expectedResult: Record> = { + report2: { + ...exampleReportAction, + person: report2PersonDiff, + }, + }; + + expect(reportActions).toEqual(expectedResult); + + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 5cb636e3a572c..22c46203a53ee 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -1,4 +1,5 @@ import Onyx from 'react-native-onyx'; +import createOnyxMockUpdate from 'tests/utils/createOnyxMockUpdate'; import type {AppActionsMock} from '@libs/actions/__mocks__/App'; import * as AppImport from '@libs/actions/App'; import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; @@ -7,7 +8,6 @@ import type {OnyxUpdateManagerUtilsMock} from '@libs/actions/OnyxUpdateManager/u import type {ApplyUpdatesMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'; import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; -import type {OnyxUpdatesFromServer} from '@src/types/onyx'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; jest.mock('@libs/actions/App'); @@ -18,34 +18,12 @@ const App = AppImport as AppActionsMock; const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock; -const createMockUpdate = (lastUpdateID: number): OnyxUpdatesFromServer => ({ - type: 'https', - lastUpdateID, - previousUpdateID: lastUpdateID - 1, - request: { - command: 'TestCommand', - successData: [], - failureData: [], - finallyData: [], - optimisticData: [], - }, - response: { - lastUpdateID, - previousUpdateID: lastUpdateID - 1, - }, - updates: [ - { - eventType: 'test', - data: [], - }, - ], -}); -const mockUpdate3 = createMockUpdate(3); -const mockUpdate4 = createMockUpdate(4); -const mockUpdate5 = createMockUpdate(5); -const mockUpdate6 = createMockUpdate(6); -const mockUpdate7 = createMockUpdate(7); -const mockUpdate8 = createMockUpdate(8); +const mockUpdate3 = createOnyxMockUpdate(3); +const mockUpdate4 = createOnyxMockUpdate(4); +const mockUpdate5 = createOnyxMockUpdate(5); +const mockUpdate6 = createOnyxMockUpdate(6); +const mockUpdate7 = createOnyxMockUpdate(7); +const mockUpdate8 = createOnyxMockUpdate(8); describe('OnyxUpdateManager', () => { let lastUpdateIDAppliedToClient = 1; diff --git a/tests/utils/createOnyxMockUpdate.ts b/tests/utils/createOnyxMockUpdate.ts new file mode 100644 index 0000000000000..f8e28e364cf73 --- /dev/null +++ b/tests/utils/createOnyxMockUpdate.ts @@ -0,0 +1,23 @@ +import type {OnyxUpdate} from 'react-native-onyx'; +import type {OnyxUpdatesFromServer} from '@src/types/onyx'; + +const createOnyxMockUpdate = (lastUpdateID: number, successData: OnyxUpdate[] = []): OnyxUpdatesFromServer => ({ + type: 'https', + lastUpdateID, + previousUpdateID: lastUpdateID - 1, + request: { + command: 'TestCommand', + successData, + failureData: [], + finallyData: [], + optimisticData: [], + }, + response: { + jsonCode: 200, + lastUpdateID, + previousUpdateID: lastUpdateID - 1, + onyxData: successData, + }, +}); + +export default createOnyxMockUpdate; From d0e3b689baa07c679e77077fb97f5b2c3511e643 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 17:36:12 +0200 Subject: [PATCH 30/37] add more tests and comments --- tests/actions/OnyxUpdateManagerTest.ts | 131 +++++++++++++++++++++---- 1 file changed, 113 insertions(+), 18 deletions(-) diff --git a/tests/actions/OnyxUpdateManagerTest.ts b/tests/actions/OnyxUpdateManagerTest.ts index e51285879e3b2..f8a2f8fcb98fe 100644 --- a/tests/actions/OnyxUpdateManagerTest.ts +++ b/tests/actions/OnyxUpdateManagerTest.ts @@ -6,6 +6,8 @@ import * as AppImport from '@libs/actions/App'; import applyOnyxUpdatesReliably from '@libs/actions/applyOnyxUpdatesReliably'; import * as OnyxUpdateManagerExports from '@libs/actions/OnyxUpdateManager'; import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; +import * as OnyxUpdateManagerUtilsImport from '@libs/actions/OnyxUpdateManager/utils'; +import type {OnyxUpdateManagerUtilsMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__'; import type {ApplyUpdatesMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'; import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; import CONST from '@src/CONST'; @@ -13,7 +15,6 @@ import OnyxUpdateManager from '@src/libs/actions/OnyxUpdateManager'; import ONYXKEYS from '@src/ONYXKEYS'; import type * as OnyxTypes from '@src/types/onyx'; import createOnyxMockUpdate from '../utils/createOnyxMockUpdate'; -import * as TestHelper from '../utils/TestHelper'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; jest.mock('@libs/actions/App'); @@ -28,12 +29,13 @@ jest.mock('@libs/actions/OnyxUpdateManager/utils/applyUpdates', () => { const App = AppImport as AppActionsMock; const ApplyUpdates = ApplyUpdatesImport as ApplyUpdatesMock; +const OnyxUpdateManagerUtils = OnyxUpdateManagerUtilsImport as OnyxUpdateManagerUtilsMock; const TEST_USER_ACCOUNT_ID = 1; const REPORT_ID = 'testReport1'; const ONYX_KEY = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}` as const; -const exampleReportAction: Partial = { +const exampleReportAction = { actionName: CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT, actorAccountID: TEST_USER_ACCOUNT_ID, automatic: false, @@ -41,10 +43,17 @@ const exampleReportAction: Partial = { message: [{type: 'COMMENT', html: 'Testing a comment', text: 'Testing a comment', translationKey: ''}], person: [{type: 'TEXT', style: 'strong', text: 'Test User'}], shouldShow: true, -}; +} satisfies Partial; -const initialData = {report1: exampleReportAction, report2: exampleReportAction, report3: exampleReportAction} as OnyxTypes.ReportActions; +const initialData = {report1: exampleReportAction, report2: exampleReportAction, report3: exampleReportAction} as unknown as OnyxTypes.ReportActions; +const mockUpdate1 = createOnyxMockUpdate(1, [ + { + onyxMethod: OnyxUtils.METHOD.SET, + key: ONYX_KEY, + value: initialData, + }, +]); const mockUpdate2 = createOnyxMockUpdate(2, [ { onyxMethod: OnyxUtils.METHOD.MERGE, @@ -55,21 +64,22 @@ const mockUpdate2 = createOnyxMockUpdate(2, [ }, ]); -const report2PersonDiff = [ - {type: 'TEXT', style: 'light', text: 'Other Test User'}, - {type: 'TEXT', style: 'light', text: 'Other Test User 2'}, -]; +const report2PersonDiff = { + person: [ + {type: 'TEXT', style: 'light', text: 'Other Test User'}, + {type: 'TEXT', style: 'light', text: 'Other Test User 2'}, + ], +} satisfies Partial; +const report3AvatarDiff: Partial = { + avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_5.png', +}; const mockUpdate3 = createOnyxMockUpdate(3, [ { onyxMethod: OnyxUtils.METHOD.MERGE, key: ONYX_KEY, value: { - report2: { - person: report2PersonDiff, - }, - report3: { - avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_5.png', - }, + report2: report2PersonDiff, + report3: report3AvatarDiff, }, }, ]); @@ -83,6 +93,24 @@ const mockUpdate4 = createOnyxMockUpdate(4, [ }, ]); +const report2AvatarDiff = { + avatar: 'https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/avatar_6.png', +} satisfies Partial; +const report4 = { + ...exampleReportAction, + automatic: true, +} satisfies Partial; +const mockUpdate5 = createOnyxMockUpdate(5, [ + { + onyxMethod: OnyxUtils.METHOD.MERGE, + key: ONYX_KEY, + value: { + report2: report2AvatarDiff, + report4, + }, + }, +]); + OnyxUpdateManager(); describe('actions/OnyxUpdateManager', () => { @@ -96,8 +124,6 @@ describe('actions/OnyxUpdateManager', () => { }); beforeEach(async () => { - // @ts-expect-error TODO: Remove this once TestHelper (https://github.com/Expensify/App/issues/25318) is migrated to TypeScript. - global.fetch = TestHelper.getGlobalFetchMock(); jest.clearAllMocks(); await Onyx.clear(); await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); @@ -109,7 +135,10 @@ describe('actions/OnyxUpdateManager', () => { }); it('should trigger Onyx update gap handling', async () => { - App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate3]; + // Since we don't want to trigger actual GetMissingOnyxUpdates calls to the server/backend, + // we have to mock the results of these calls. By setting the missingOnyxUpdatesToBeApplied + // property on the mock, we can simulate the results of the GetMissingOnyxUpdates calls. + App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate2, mockUpdate3]; applyOnyxUpdatesReliably(mockUpdate2); applyOnyxUpdatesReliably(mockUpdate4); @@ -119,13 +148,79 @@ describe('actions/OnyxUpdateManager', () => { const expectedResult: Record> = { report2: { ...exampleReportAction, - person: report2PersonDiff, + ...report2PersonDiff, }, }; expect(reportActions).toEqual(expectedResult); + // GetMissingOnyxUpdates should have been called for the gap between update 2 and 4. + // Since we queued update 4 before update 3, there's a gap to resolve, before we apply the deferred updates. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 2, 3); + + // After the missing updates have been applied, the applicable updates after + // all locally applied updates should be applied. (4) expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {4: mockUpdate4}); }); }); + + it('should trigger 2 GetMissingOnyxUpdates calls, because the deferred updates have gaps', async () => { + // Since we don't want to trigger actual GetMissingOnyxUpdates calls to the server/backend, + // we have to mock the results of these calls. By setting the missingOnyxUpdatesToBeApplied + // property on the mock, we can simulate the results of the GetMissingOnyxUpdates calls. + App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate1, mockUpdate2]; + + applyOnyxUpdatesReliably(mockUpdate3); + applyOnyxUpdatesReliably(mockUpdate5); + + let finishFirstCall: () => void; + const firstGetMissingOnyxUpdatesCallFinished = new Promise((resolve) => { + finishFirstCall = resolve; + }); + + OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = () => { + finishFirstCall(); + return Promise.resolve(); + }; + + return firstGetMissingOnyxUpdatesCallFinished + .then(() => { + // After the first GetMissingOnyxUpdates call has been resolved, + // we have to set the mocked results of for the second call. + App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate3, mockUpdate4]; + }) + .then(() => OnyxUpdateManagerExports.queryPromise) + .then(() => { + const expectedResult: Record> = { + report2: { + ...exampleReportAction, + ...report2PersonDiff, + ...report2AvatarDiff, + }, + report4, + }; + + expect(reportActions).toEqual(expectedResult); + + // GetMissingOnyxUpdates should have been called twice, once for the gap between update 1 and 3, + // and once for the gap between update 3 and 5. + // We always fetch missing updates from the lastUpdateIDAppliedToClient + // to previousUpdateID of the first deferred update. First 1-2, second 3-4 + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); + + // Since we have two GetMissingOnyxUpdates calls, there will be two sets of applicable updates. + // The first applicable update will be 3, after missing updates 1-2 have been applied. + // The second applicable update will be 5, after missing updates 3-4 have been applied. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5}); + }); + }); }); From 52ad15a5a958ebc55d7755a1641da12134a2929e Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 17:41:37 +0200 Subject: [PATCH 31/37] remove waitForBatchedChanges --- tests/unit/OnyxUpdateManagerTest.ts | 266 +++++++++++++--------------- 1 file changed, 126 insertions(+), 140 deletions(-) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index 22c46203a53ee..e2bd0daec081c 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -1,5 +1,4 @@ import Onyx from 'react-native-onyx'; -import createOnyxMockUpdate from 'tests/utils/createOnyxMockUpdate'; import type {AppActionsMock} from '@libs/actions/__mocks__/App'; import * as AppImport from '@libs/actions/App'; import * as OnyxUpdateManager from '@libs/actions/OnyxUpdateManager'; @@ -8,7 +7,7 @@ import type {OnyxUpdateManagerUtilsMock} from '@libs/actions/OnyxUpdateManager/u import type {ApplyUpdatesMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'; import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; -import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; +import createOnyxMockUpdate from '../utils/createOnyxMockUpdate'; jest.mock('@libs/actions/App'); jest.mock('@libs/actions/OnyxUpdateManager/utils'); @@ -38,7 +37,6 @@ describe('OnyxUpdateManager', () => { jest.clearAllMocks(); await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); OnyxUpdateManager.resetDeferralLogicVariables(); - return waitForBatchedUpdates(); }); it('should fetch missing Onyx updates once, defer updates and apply after missing updates', () => { @@ -46,27 +44,25 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate4); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); - return waitForBatchedUpdates() - .then(() => OnyxUpdateManager.queryPromise) - ?.then(() => { - // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - expect(lastUpdateIDAppliedToClient).toBe(5); - - // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered - expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); - - // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. - // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. - // The intended assertion would look like this: - // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); - - // There should be only one call to applyUpdates. The call should contain all the deferred update, - // since the locally applied updates have changed in the meantime. - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); - }); + return OnyxUpdateManager.queryPromise.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(5); + + // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + + // There should be only one call to applyUpdates. The call should contain all the deferred update, + // since the locally applied updates have changed in the meantime. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({3: mockUpdate3, 4: mockUpdate4, 5: mockUpdate5}); + }); }); it('should only apply deferred updates that are newer than the last locally applied update (pending updates)', async () => { @@ -82,31 +78,29 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = undefined; }; - return waitForBatchedUpdates() - .then(() => OnyxUpdateManager.queryPromise) - ?.then(() => { - // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - expect(lastUpdateIDAppliedToClient).toBe(6); + return OnyxUpdateManager.queryPromise.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(6); - // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered - expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + // There are no gaps in the deferred updates, therefore only one call to getMissingOnyxUpdates should be triggered + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); - // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. - // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. - // The intended assertion would look like this: - // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); - // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server in the first and only call to getMissingOnyxUpdates - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); + // Missing updates from 1 (last applied to client) to 3 (last "previousUpdateID" from first deferred update) should have been fetched from the server in the first and only call to getMissingOnyxUpdates + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 3); - // There should be only one call to applyUpdates. The call should only contain the last deferred update, - // since the locally applied updates have changed in the meantime. - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); + // There should be only one call to applyUpdates. The call should only contain the last deferred update, + // since the locally applied updates have changed in the meantime. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(1); - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); - }); + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledWith({6: mockUpdate6}); + }); }); it('should re-fetch missing updates if the deferred updates have a gap', async () => { @@ -114,36 +108,34 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); - return waitForBatchedUpdates() - .then(() => OnyxUpdateManager.queryPromise) - ?.then(() => { - // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - expect(lastUpdateIDAppliedToClient).toBe(6); + return OnyxUpdateManager.queryPromise.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(6); - // Even though there is a gap in the deferred updates, we only want to fetch missing updates once per batch. - expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + // Even though there is a gap in the deferred updates, we only want to fetch missing updates once per batch. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); - // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. - // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. - // The intended assertion would look like this: - // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); - // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. - // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); + // There should be multiple calls getMissingOnyxUpdates and applyUpdates, since we detect a gap in the deferred updates. + // The first call to getMissingOnyxUpdates should fetch updates from 1 (last applied to client) to 2 (last "previousUpdateID" from first deferred update) from the server. + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); - // After the initial missing updates have been applied, the applicable updates (3) should be applied. - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + // After the initial missing updates have been applied, the applicable updates (3) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); - // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates. 3-4 - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); + // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates. 3-4 + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); - // After the gap in the deferred updates has been resolved, the remaining deferred updates (5, 6) should be applied. - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); - }); + // After the gap in the deferred updates has been resolved, the remaining deferred updates (5, 6) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5, 6: mockUpdate6}); + }); }); it('should re-fetch missing deferred updates only once per batch', async () => { @@ -152,34 +144,32 @@ describe('OnyxUpdateManager', () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate8); - return waitForBatchedUpdates() - .then(() => OnyxUpdateManager.queryPromise) - ?.then(() => { - // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - expect(lastUpdateIDAppliedToClient).toBe(8); - - // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. - expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); - // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. - // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. - // The intended assertion would look like this: - // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); - - // After the initial missing updates have been applied, the applicable updates (3-4) should be applied. - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3, 4: mockUpdate4}); - - // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap (4-7) in the deferred updates. - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 4, 7); - - // After the gap in the deferred updates has been resolved, the remaining deferred updates (8) should be applied. - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {8: mockUpdate8}); - }); + return OnyxUpdateManager.queryPromise.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(8); + + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); + + // After the initial missing updates have been applied, the applicable updates (3-4) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3, 4: mockUpdate4}); + + // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap (4-7) in the deferred updates. + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 4, 7); + + // After the gap in the deferred updates has been resolved, the remaining deferred updates (8) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {8: mockUpdate8}); + }); }); - it('should not re-fetch missing updates if the locally applied update has been updated', async () => { + it('should not re-fetch missing updates if the lastUpdateIDFromClient has been updated', async () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate5); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate6); @@ -194,35 +184,33 @@ describe('OnyxUpdateManager', () => { ApplyUpdates.mockValues.onApplyUpdates = undefined; }; - return waitForBatchedUpdates() - .then(() => OnyxUpdateManager.queryPromise) - ?.then(() => { - // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - expect(lastUpdateIDAppliedToClient).toBe(7); + return OnyxUpdateManager.queryPromise.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(7); - // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. - expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); - // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. - // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. - // The intended assertion would look like this: - // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(1); - // Since there is a gap in the deferred updates, we need to run applyUpdates twice. - // Once for the applicable updates (before the gap) and then for the remaining deferred updates. - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + // Since there is a gap in the deferred updates, we need to run applyUpdates twice. + // Once for the applicable updates (before the gap) and then for the remaining deferred updates. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); - // After the initial missing updates have been applied, the applicable updates (3) should be applied. - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + // After the initial missing updates have been applied, the applicable updates (3) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); - // Since the lastUpdateIDAppliedToClient has changed to 5 in the meantime, we only need to apply the remaining deferred updates (6-7). - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {6: mockUpdate6, 7: mockUpdate7}); - }); + // Since the lastUpdateIDAppliedToClient has changed to 5 in the meantime, we only need to apply the remaining deferred updates (6-7). + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {6: mockUpdate6, 7: mockUpdate7}); + }); }); - it('should re-fetch missing updates if the locally applied update has been updated, but there are still gaps after the locally applied update', async () => { + it('should re-fetch missing updates if the lastUpdateIDFromClient has increased, but there are still gaps after the locally applied update', async () => { OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate3); OnyxUpdateManager.handleOnyxUpdateGap(mockUpdate7); @@ -235,35 +223,33 @@ describe('OnyxUpdateManager', () => { ApplyUpdates.mockValues.onApplyUpdates = undefined; }; - return waitForBatchedUpdates() - .then(() => OnyxUpdateManager.queryPromise) - ?.then(() => { - // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. - expect(lastUpdateIDAppliedToClient).toBe(7); + return OnyxUpdateManager.queryPromise.then(() => { + // After all missing and deferred updates have been applied, the lastUpdateIDAppliedToClient should be 6. + expect(lastUpdateIDAppliedToClient).toBe(7); - // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. - expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + // Even though there are multiple gaps in the deferred updates, we only want to fetch missing updates once per batch. + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); - // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. - // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. - // The intended assertion would look like this: - // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); + // validateAndApplyDeferredUpdates should be called twice, once for the initial deferred updates and once for the remaining deferred updates with gaps. + // Unfortunately, we cannot easily count the calls of this function with Jest, since it recursively calls itself. + // The intended assertion would look like this: + // expect(OnyxUpdateManagerUtils.validateAndApplyDeferredUpdates).toHaveBeenCalledTimes(2); - // Since there is a gap in the deferred updates, we need to run applyUpdates twice. - // Once for the applicable updates (before the gap) and then for the remaining deferred updates. - expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); + // Since there is a gap in the deferred updates, we need to run applyUpdates twice. + // Once for the applicable updates (before the gap) and then for the remaining deferred updates. + expect(ApplyUpdates.applyUpdates).toHaveBeenCalledTimes(2); - // After the initial missing updates have been applied, the applicable updates (3) should be applied. - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); + // After the initial missing updates have been applied, the applicable updates (3) should be applied. + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(1, {3: mockUpdate3}); - // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates, - // that are later than the locally applied update (4-6). (including the last locally applied update) - expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 4, 6); + // The second call to getMissingOnyxUpdates should fetch the missing updates from the gap in the deferred updates, + // that are later than the locally applied update (4-6). (including the last locally applied update) + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 4, 6); - // Since the lastUpdateIDAppliedToClient has changed to 4 in the meantime and we're fetching updates 5-6 we only need to apply the remaining deferred updates (7). - // eslint-disable-next-line @typescript-eslint/naming-convention - expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {7: mockUpdate7}); - }); + // Since the lastUpdateIDAppliedToClient has changed to 4 in the meantime and we're fetching updates 5-6 we only need to apply the remaining deferred updates (7). + // eslint-disable-next-line @typescript-eslint/naming-convention + expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {7: mockUpdate7}); + }); }); }); From a7aac11d1e50781c4c359be55b6e867130fd9e7a Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 18:17:45 +0200 Subject: [PATCH 32/37] rename and move helper function --- .../OnyxUpdateManager/utils/__mocks__/applyUpdates.ts | 4 ++-- .../actions/OnyxUpdateManager/utils/__mocks__/index.ts | 4 ++-- .../actions/OnyxUpdateManager/utils/deferredUpdates.ts | 4 ++-- src/libs/actions/__mocks__/App.ts | 4 ++-- .../{createProxyForValue.ts => createProxyForObject.ts} | 9 +++++++-- 5 files changed, 15 insertions(+), 10 deletions(-) rename src/utils/{createProxyForValue.ts => createProxyForObject.ts} (56%) diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts index d02a034c29cc1..5cd66df6b0b0c 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates.ts @@ -1,7 +1,7 @@ import Onyx from 'react-native-onyx'; import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; import ONYXKEYS from '@src/ONYXKEYS'; -import createProxyForValue from '@src/utils/createProxyForValue'; +import createProxyForObject from '@src/utils/createProxyForObject'; let lastUpdateIDAppliedToClient = 0; Onyx.connect({ @@ -21,7 +21,7 @@ type ApplyUpdatesMock = { const mockValues: ApplyUpdatesMockValues = { onApplyUpdates: undefined, }; -const mockValuesProxy = createProxyForValue(mockValues); +const mockValuesProxy = createProxyForObject(mockValues); const applyUpdates = jest.fn((updates: DeferredUpdatesDictionary) => { const lastUpdateIdFromUpdates = Math.max(...Object.keys(updates).map(Number)); diff --git a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts index e548a9e22b1a3..b4d97a4399db3 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/__mocks__/index.ts @@ -1,5 +1,5 @@ import type {DeferredUpdatesDictionary, DetectGapAndSplitResult} from '@libs/actions/OnyxUpdateManager/types'; -import createProxyForValue from '@src/utils/createProxyForValue'; +import createProxyForObject from '@src/utils/createProxyForObject'; import type * as OnyxUpdateManagerUtilsImport from '..'; import {applyUpdates} from './applyUpdates'; @@ -18,7 +18,7 @@ type OnyxUpdateManagerUtilsMock = typeof UtilsImplementation & { const mockValues: OnyxUpdateManagerUtilsMockValues = { onValidateAndApplyDeferredUpdates: undefined, }; -const mockValuesProxy = createProxyForValue(mockValues); +const mockValuesProxy = createProxyForObject(mockValues); const detectGapsAndSplit = jest.fn(UtilsImplementation.detectGapsAndSplit); diff --git a/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts b/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts index 1bf251fbd3df9..838c27821aaef 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/deferredUpdates.ts @@ -1,8 +1,8 @@ import type {DeferredUpdatesDictionary} from '@libs/actions/OnyxUpdateManager/types'; -import createProxyForValue from '@src/utils/createProxyForValue'; +import createProxyForObject from '@src/utils/createProxyForObject'; const deferredUpdatesValue = {deferredUpdates: {} as DeferredUpdatesDictionary}; -const deferredUpdatesProxy = createProxyForValue(deferredUpdatesValue); +const deferredUpdatesProxy = createProxyForObject(deferredUpdatesValue); export default deferredUpdatesProxy; diff --git a/src/libs/actions/__mocks__/App.ts b/src/libs/actions/__mocks__/App.ts index 574a5b51e1b9b..3d2b5814684bb 100644 --- a/src/libs/actions/__mocks__/App.ts +++ b/src/libs/actions/__mocks__/App.ts @@ -3,7 +3,7 @@ import type * as AppImport from '@libs/actions/App'; import type * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; import ONYXKEYS from '@src/ONYXKEYS'; import type {OnyxUpdatesFromServer} from '@src/types/onyx'; -import createProxyForValue from '@src/utils/createProxyForValue'; +import createProxyForObject from '@src/utils/createProxyForObject'; const AppImplementation: typeof AppImport = jest.requireActual('@libs/actions/App'); const { @@ -38,7 +38,7 @@ type AppActionsMock = typeof AppImport & { const mockValues: AppMockValues = { missingOnyxUpdatesToBeApplied: undefined, }; -const mockValuesProxy = createProxyForValue(mockValues); +const mockValuesProxy = createProxyForObject(mockValues); const ApplyUpdatesImplementation: typeof ApplyUpdatesImport = jest.requireActual('@libs/actions/OnyxUpdateManager/utils/applyUpdates'); const getMissingOnyxUpdates = jest.fn((_fromID: number, toID: number) => { diff --git a/src/utils/createProxyForValue.ts b/src/utils/createProxyForObject.ts similarity index 56% rename from src/utils/createProxyForValue.ts rename to src/utils/createProxyForObject.ts index ace0951aa720c..c18e5e30a0d9c 100644 --- a/src/utils/createProxyForValue.ts +++ b/src/utils/createProxyForObject.ts @@ -1,4 +1,9 @@ -const createProxyForValue = >(value: Value) => +/** + * Creates a proxy around an object variable that can be exported from modules, to allow modification from outside the module. + * @param value the object that should be wrapped in a proxy + * @returns A proxy object that can be modified from outside the module + */ +const createProxyForObject = >(value: Value) => new Proxy(value, { get: (target, property) => { if (typeof property === 'symbol') { @@ -17,4 +22,4 @@ const createProxyForValue = >(value: Value }, }); -export default createProxyForValue; +export default createProxyForObject; From 95fc4f6c74e95c708e940918fe08e8bce65f8053 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 18:17:59 +0200 Subject: [PATCH 33/37] add isPaused checker function to SequentialQueue --- src/libs/Network/SequentialQueue.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 38b0549b28bca..b94166c0249df 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -156,6 +156,10 @@ function isRunning(): boolean { return isSequentialQueueRunning; } +function isPaused(): boolean { + return isQueuePaused; +} + // Flush the queue when the connection resumes NetworkStore.onReconnection(flush); @@ -191,4 +195,4 @@ function waitForIdle(): Promise { return isReadyPromise; } -export {flush, getCurrentRequest, isRunning, push, waitForIdle, pause, unpause}; +export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause}; From 3e8bc28ead47b5b65c4ca6fcd0b5cc44aebbb6d0 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 18:18:11 +0200 Subject: [PATCH 34/37] reset more things before each test --- tests/unit/OnyxUpdateManagerTest.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/OnyxUpdateManagerTest.ts b/tests/unit/OnyxUpdateManagerTest.ts index e2bd0daec081c..20b29ca18a56a 100644 --- a/tests/unit/OnyxUpdateManagerTest.ts +++ b/tests/unit/OnyxUpdateManagerTest.ts @@ -36,6 +36,8 @@ describe('OnyxUpdateManager', () => { beforeEach(async () => { jest.clearAllMocks(); await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); + OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = undefined; + ApplyUpdates.mockValues.onApplyUpdates = undefined; OnyxUpdateManager.resetDeferralLogicVariables(); }); From 5e42e99a6ad57a0d5ff1222f1c5891dbeff56ca2 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 18:18:38 +0200 Subject: [PATCH 35/37] add another test --- tests/actions/OnyxUpdateManagerTest.ts | 61 +++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 7 deletions(-) diff --git a/tests/actions/OnyxUpdateManagerTest.ts b/tests/actions/OnyxUpdateManagerTest.ts index f8a2f8fcb98fe..8a4e70fb2faa2 100644 --- a/tests/actions/OnyxUpdateManagerTest.ts +++ b/tests/actions/OnyxUpdateManagerTest.ts @@ -10,12 +10,12 @@ import * as OnyxUpdateManagerUtilsImport from '@libs/actions/OnyxUpdateManager/u import type {OnyxUpdateManagerUtilsMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__'; import type {ApplyUpdatesMock} from '@libs/actions/OnyxUpdateManager/utils/__mocks__/applyUpdates'; import * as ApplyUpdatesImport from '@libs/actions/OnyxUpdateManager/utils/applyUpdates'; +import * as SequentialQueue from '@libs/Network/SequentialQueue'; import CONST from '@src/CONST'; import OnyxUpdateManager from '@src/libs/actions/OnyxUpdateManager'; import ONYXKEYS from '@src/ONYXKEYS'; import type * as OnyxTypes from '@src/types/onyx'; import createOnyxMockUpdate from '../utils/createOnyxMockUpdate'; -import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; jest.mock('@libs/actions/App'); jest.mock('@libs/actions/OnyxUpdateManager/utils'); @@ -129,9 +129,9 @@ describe('actions/OnyxUpdateManager', () => { await Onyx.set(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT, 1); await Onyx.set(ONYX_KEY, initialData); + OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = undefined; App.mockValues.missingOnyxUpdatesToBeApplied = undefined; OnyxUpdateManagerExports.resetDeferralLogicVariables(); - return waitForBatchedUpdates(); }); it('should trigger Onyx update gap handling', async () => { @@ -182,16 +182,14 @@ describe('actions/OnyxUpdateManager', () => { }); OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = () => { + // After the first GetMissingOnyxUpdates call has been resolved, + // we have to set the mocked results of for the second call. + App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate3, mockUpdate4]; finishFirstCall(); return Promise.resolve(); }; return firstGetMissingOnyxUpdatesCallFinished - .then(() => { - // After the first GetMissingOnyxUpdates call has been resolved, - // we have to set the mocked results of for the second call. - App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate3, mockUpdate4]; - }) .then(() => OnyxUpdateManagerExports.queryPromise) .then(() => { const expectedResult: Record> = { @@ -223,4 +221,53 @@ describe('actions/OnyxUpdateManager', () => { expect(ApplyUpdates.applyUpdates).toHaveBeenNthCalledWith(2, {5: mockUpdate5}); }); }); + + it('should pause SequentialQueue while missing updates are being fetched', async () => { + // Since we don't want to trigger actual GetMissingOnyxUpdates calls to the server/backend, + // we have to mock the results of these calls. By setting the missingOnyxUpdatesToBeApplied + // property on the mock, we can simulate the results of the GetMissingOnyxUpdates calls. + App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate1, mockUpdate2]; + + applyOnyxUpdatesReliably(mockUpdate3); + applyOnyxUpdatesReliably(mockUpdate5); + + const assertAfterFirstGetMissingOnyxUpdates = () => { + // While the fetching of missing udpates and the validation and application of the deferred updaes is running, + // the SequentialQueue should be paused. + expect(SequentialQueue.isPaused()).toBeTruthy(); + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(1); + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(1, 1, 2); + }; + + const assertAfterSecondGetMissingOnyxUpdates = () => { + // The SequentialQueue should still be paused. + expect(SequentialQueue.isPaused()).toBeTruthy(); + expect(SequentialQueue.isRunning()).toBeFalsy(); + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + expect(App.getMissingOnyxUpdates).toHaveBeenNthCalledWith(2, 3, 4); + }; + + let firstCallFinished = false; + OnyxUpdateManagerUtils.mockValues.onValidateAndApplyDeferredUpdates = () => { + if (firstCallFinished) { + assertAfterSecondGetMissingOnyxUpdates(); + return Promise.resolve(); + } + + assertAfterFirstGetMissingOnyxUpdates(); + + // After the first GetMissingOnyxUpdates call has been resolved, + // we have to set the mocked results of for the second call. + App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate3, mockUpdate4]; + firstCallFinished = true; + return Promise.resolve(); + }; + + return OnyxUpdateManagerExports.queryPromise.then(() => { + // Once the OnyxUpdateManager has finished filling the gaps, the SequentialQueue should be unpaused again. + // It must not necessarily be running, because it might not have been flushed yet. + expect(SequentialQueue.isPaused()).toBeFalsy(); + expect(App.getMissingOnyxUpdates).toHaveBeenCalledTimes(2); + }); + }); }); From f57a149f788e0d2db56395f026b17b80671f3630 Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Tue, 23 Apr 2024 18:34:40 +0200 Subject: [PATCH 36/37] fix: first test --- tests/actions/OnyxUpdateManagerTest.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/actions/OnyxUpdateManagerTest.ts b/tests/actions/OnyxUpdateManagerTest.ts index 8a4e70fb2faa2..d1a10f8a47751 100644 --- a/tests/actions/OnyxUpdateManagerTest.ts +++ b/tests/actions/OnyxUpdateManagerTest.ts @@ -141,6 +141,13 @@ describe('actions/OnyxUpdateManager', () => { App.mockValues.missingOnyxUpdatesToBeApplied = [mockUpdate2, mockUpdate3]; applyOnyxUpdatesReliably(mockUpdate2); + + // Delay all later updates, so that the update 2 has time to be written to storage and for the + // ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT to be updated. + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); + applyOnyxUpdatesReliably(mockUpdate4); applyOnyxUpdatesReliably(mockUpdate3); From a738a518abdd082ec5ff77bef44e75d170ae33da Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Thu, 25 Apr 2024 11:14:35 +0200 Subject: [PATCH 37/37] remove unused line --- src/libs/actions/OnyxUpdateManager/utils/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/actions/OnyxUpdateManager/utils/index.ts b/src/libs/actions/OnyxUpdateManager/utils/index.ts index a04c7e8842191..4df22d292d197 100644 --- a/src/libs/actions/OnyxUpdateManager/utils/index.ts +++ b/src/libs/actions/OnyxUpdateManager/utils/index.ts @@ -112,7 +112,6 @@ function validateAndApplyDeferredUpdates(clientLastUpdateID?: number): Promise