From bb59f93877b4ebe5f938024da3542e43663d6a0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Tue, 2 Nov 2021 11:09:56 -0600 Subject: [PATCH 01/29] retry a request if auth+crendentials are not ready --- src/libs/API.js | 28 ++++++++++++++++++++++------ src/libs/Network.js | 18 ++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 19f70c63879a..8914b2978e1c 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -1,5 +1,6 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; +import lodashGet from 'lodash/get'; import CONST from '../CONST'; import CONFIG from '../CONFIG'; import ONYXKEYS from '../ONYXKEYS'; @@ -12,15 +13,31 @@ import LogUtil from './Log'; let isAuthenticating; let credentials; +let authToken; + +/** + * Checks if both authToken and credentials were already read from persistent storage (Onyx) + * @returns {Boolean} + */ +function isAuthStoreReady() { + return !_.isUndefined(authToken) + && !_.isUndefined(credentials); +} + Onyx.connect({ key: ONYXKEYS.CREDENTIALS, - callback: val => credentials = val, + callback: (val) => { + credentials = val || null; + Network.setIsReady(isAuthStoreReady()); + }, }); -let authToken; Onyx.connect({ key: ONYXKEYS.SESSION, - callback: val => authToken = val ? val.authToken : null, + callback: (val) => { + authToken = lodashGet(val, 'authToken', null); + Network.setIsReady(isAuthStoreReady()); + }, }); /** @@ -190,7 +207,7 @@ Network.registerErrorHandler((queuedRequest, error) => { /** * @param {Object} parameters - * @param {String} [parameters.useExpensifyLogin] + * @param {Boolean} [parameters.useExpensifyLogin] * @param {String} parameters.partnerName * @param {String} parameters.partnerPassword * @param {String} parameters.partnerUserID @@ -1005,14 +1022,13 @@ function DeleteBankAccount(parameters) { /** * @param {Object} parameters - * @param {String[]} data * @returns {Promise} */ function Mobile_GetConstants(parameters) { const commandName = 'Mobile_GetConstants'; requireParameters(['data'], parameters, commandName); - // Stringinfy the parameters object as we cannot send an object via FormData + // Stringify the parameters object as we cannot send an object via FormData const finalParameters = parameters; finalParameters.data = JSON.stringify(parameters.data); diff --git a/src/libs/Network.js b/src/libs/Network.js index aa1fd853ae82..5429a9cf59ee 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -5,7 +5,10 @@ import HttpUtils from './HttpUtils'; import ONYXKEYS from '../ONYXKEYS'; import * as ActiveClientManager from './ActiveClientManager'; import CONST from '../CONST'; +// eslint-disable-next-line import/no-cycle +import LogUtil from './Log'; +let isReady = false; let isQueuePaused = false; // Queue for network requests so we don't lose actions done by the user while offline @@ -95,6 +98,14 @@ Onyx.connect({ callback: val => email = val ? val.email : null, }); +/** + * + * @param {Boolean} val + */ +function setIsReady(val) { + isReady = val; +} + /** * Checks to see if a request can be made. * @@ -188,6 +199,12 @@ function processNetworkRequestQueue() { return; } + if (!isReady) { + LogUtil.hmmm('Trying to make a request when Network is not ready', {command: queuedRequest.command, type: queuedRequest.type}); + requestsToProcessOnNextRun.push(queuedRequest); + return; + } + const requestData = queuedRequest.data; const requestEmail = lodashGet(requestData, 'email', ''); @@ -329,4 +346,5 @@ export { clearRequestQueue, registerResponseHandler, registerErrorHandler, + setIsReady, }; From bb69c2353da7ec169970607931a2b1b804350efb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Tue, 2 Nov 2021 11:17:26 -0600 Subject: [PATCH 02/29] disable circular dep between Log-Api-Network --- src/libs/API.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 8914b2978e1c..84d96a2b4406 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -5,9 +5,9 @@ import CONST from '../CONST'; import CONFIG from '../CONFIG'; import ONYXKEYS from '../ONYXKEYS'; import redirectToSignIn from './actions/SignInRedirect'; -import * as Network from './Network'; import isViaExpensifyCashNative from './isViaExpensifyCashNative'; - +// eslint-disable-next-line import/no-cycle +import * as Network from './Network'; // eslint-disable-next-line import/no-cycle import LogUtil from './Log'; From 8552b0a57a8eeeca8c928661cdf3e8c01546e317 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Tue, 2 Nov 2021 12:22:30 -0600 Subject: [PATCH 03/29] - add Onyx initialization - add explicit globals to improve intellisense in vscode --- tests/actions/ReportTest.js | 13 ++++++++++++- tests/actions/SessionTest.js | 8 ++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/actions/ReportTest.js b/tests/actions/ReportTest.js index 718e5f9ef006..2797d8068b4b 100644 --- a/tests/actions/ReportTest.js +++ b/tests/actions/ReportTest.js @@ -1,5 +1,9 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; +import lodashGet from 'lodash/get'; +import { + beforeEach, beforeAll, afterEach, jest, describe, it, expect, +} from '@jest/globals'; import ONYXKEYS from '../../src/ONYXKEYS'; import * as Pusher from '../../src/libs/Pusher/pusher'; import PusherConnectionManager from '../../src/libs/PusherConnectionManager'; @@ -30,8 +34,15 @@ describe('actions/Report', () => { cluster: CONFIG.PUSHER.CLUSTER, authEndpoint: `${CONFIG.EXPENSIFY.URL_API_ROOT}api?command=Push_Authenticate`, }); + + Onyx.init({ + keys: ONYXKEYS, + registerStorageEventListener: () => { }, + }); }); + beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); + afterEach(() => { // Unsubscribe from account channel after each test since we subscribe in the function // subscribeToUserEvents and we don't want duplicate event subscriptions. @@ -130,7 +141,7 @@ describe('actions/Report', () => { let reportIsPinned; Onyx.connect({ key: `${ONYXKEYS.COLLECTION.REPORT}${REPORT_ID}`, - callback: val => reportIsPinned = val.isPinned, + callback: val => reportIsPinned = lodashGet(val, 'isPinned'), }); // Set up Onyx with some test user data diff --git a/tests/actions/SessionTest.js b/tests/actions/SessionTest.js index 9f6d6ee5ad31..4534ca82bf4c 100644 --- a/tests/actions/SessionTest.js +++ b/tests/actions/SessionTest.js @@ -1,4 +1,5 @@ import Onyx from 'react-native-onyx'; +import {beforeEach, jest, test} from '@jest/globals'; import * as API from '../../src/libs/API'; import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; @@ -16,6 +17,13 @@ jest.mock('../../src/libs/Notification/PushNotification', () => ({ // We test HttpUtils.xhr() since this means that our API command turned into a network request and isn't only queued. HttpUtils.xhr = jest.fn(); +Onyx.init({ + keys: ONYXKEYS, + registerStorageEventListener: () => { }, +}); + +beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); + test('Authenticate is called with saved credentials when a session expires', () => { // Given a test user and set of authToken with subscriptions to session and credentials const TEST_USER_LOGIN = 'test@testguy.com'; From 79bcfb699d8e173bd74cddc7ffeaefd7120de8ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Tue, 2 Nov 2021 15:17:44 -0600 Subject: [PATCH 04/29] add unit test to check if auth+cred are not ready --- tests/unit/NetworkTest.js | 9 +++++++ tests/unit/StoreNetworkTest.js | 47 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 tests/unit/StoreNetworkTest.js diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index ac0e411bd8b7..8c3d5c42f13b 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -1,10 +1,15 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; + +import { + beforeEach, jest, test, expect, +} from '@jest/globals'; import * as API from '../../src/libs/API'; import {signInWithTestUser} from '../utils/TestHelper'; import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ONYXKEYS from '../../src/ONYXKEYS'; +import Log from '../../src/libs/Log'; // Set up manual mocks for methods used in the actions so our test does not fail. jest.mock('../../src/libs/Notification/PushNotification', () => ({ @@ -20,6 +25,8 @@ Onyx.init({ registerStorageEventListener: () => {}, }); +jest.mock('../../src/libs/Log'); + beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); test('failing to reauthenticate while offline should not log out user', () => { @@ -92,6 +99,7 @@ test('failing to reauthenticate while offline should not log out user', () => { expect(callsToChatList.length).toBe(3); expect(callsToAuthenticate.length).toBe(2); + expect(Log.hmmm).not.toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); }); }); }); @@ -207,5 +215,6 @@ test('consecutive API calls eventually succeed when authToken is expired', () => expect(account).toEqual(TEST_ACCOUNT_DATA); expect(personalDetailsList).toEqual(TEST_PERSONAL_DETAILS); expect(chatList).toEqual(TEST_CHAT_LIST); + expect(Log.hmmm).not.toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); }); }); diff --git a/tests/unit/StoreNetworkTest.js b/tests/unit/StoreNetworkTest.js new file mode 100644 index 000000000000..fc6226d43792 --- /dev/null +++ b/tests/unit/StoreNetworkTest.js @@ -0,0 +1,47 @@ +import { + afterAll, beforeEach, jest, test, expect, describe, +} from '@jest/globals'; +import Onyx from 'react-native-onyx'; +import Log from '../../src/libs/Log'; +import * as Network from '../../src/libs/Network'; +import {fetchAccountDetails} from '../../src/libs/actions/Session'; +import ONYXKEYS from '../../src/ONYXKEYS'; +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; + +jest.useFakeTimers(); + +Onyx.init({ + keys: ONYXKEYS, + registerStorageEventListener: () => { }, +}); + +jest.mock('../../src/libs/Log'); + +beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); + +// This unit test was moved to its own file because modifies Network.setIsReady which +// could mess up other unit tests, and also beforeEach can't be used for this scenario. +describe('retry network request', () => { + const delayOrixConnect = 3000; + const origSetIsReady = Network.setIsReady; + const TEST_USER_LOGIN = 'test@testguy.com'; + + // We can't mock Onyx functions here because Onyx.connect already registered the callbacks + // when API.js is loaded, so instead we mock the setIsReady with setTimeout to simulate a + // delay in the Onyx.connect callback. + Network.setIsReady = val => setTimeout(() => origSetIsReady(val), delayOrixConnect); + + // We restore setIsReady to its original implementation. + afterAll(() => { + jest.runAllTimers(); + Network.setIsReady = origSetIsReady; + }); + + test('if auth and credentials are not read from Onyx yet', () => { + // Given a test user login and account ID + fetchAccountDetails(TEST_USER_LOGIN); + + // We should expect Log.hmmm to be called (logging an message to server when a request is made but Network is not ready) + expect(Log.hmmm).toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); + }); +}); From 2bbddebbfddcded6506b1b43be58fce90c4f7019 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Tue, 2 Nov 2021 16:58:22 -0600 Subject: [PATCH 05/29] add documentation to isReady move isReady to canMakeRequest --- src/libs/Network.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/libs/Network.js b/src/libs/Network.js index 5429a9cf59ee..ebafd6747236 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -110,11 +110,21 @@ function setIsReady(val) { * Checks to see if a request can be made. * * @param {Object} request + * @param {String} request.type + * @param {String} request.command * @param {Object} request.data * @param {Boolean} request.data.forceNetworkRequest * @return {Boolean} */ function canMakeRequest(request) { + // We'll not make the request now if the network is not ready (it should have higher priority than + // forceNetworkRequest and isQueuePaused because we wait until credentials + session objects have + // been read from persistent storage). + if (!isReady) { + LogUtil.hmmm('Trying to make a request when Network is not ready', {command: request.command, type: request.type}); + return false; + } + // These requests are always made even when the queue is paused if (request.data.forceNetworkRequest === true) { return true; @@ -199,12 +209,6 @@ function processNetworkRequestQueue() { return; } - if (!isReady) { - LogUtil.hmmm('Trying to make a request when Network is not ready', {command: queuedRequest.command, type: queuedRequest.type}); - requestsToProcessOnNextRun.push(queuedRequest); - return; - } - const requestData = queuedRequest.data; const requestEmail = lodashGet(requestData, 'email', ''); From 9c2ff14fa9ba94530563544b3db239fea8a50ec7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Tue, 2 Nov 2021 17:44:44 -0600 Subject: [PATCH 06/29] stop logging the user out in enhanceParameters (let the http request logout if the auth token is expired or inexistent) --- src/libs/API.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 84d96a2b4406..7a9f94c5677e 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -75,8 +75,6 @@ function addDefaultValuesToParameters(command, parameters) { // If we end up here with no authToken it means we are trying to make an API request before we are signed in. // In this case, we should cancel the current request by pausing the queue and clearing the remaining requests. if (!authToken) { - redirectToSignIn(); - LogUtil.info('A request was made without an authToken', false, {command, parameters}); Network.pauseRequestQueue(); Network.clearRequestQueue(); From 56b0bb894695f7dd53ae595ed84a24e9223d75e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 3 Nov 2021 11:16:05 -0600 Subject: [PATCH 07/29] Update tests/unit/StoreNetworkTest.js Co-authored-by: Marc Glasser --- tests/unit/StoreNetworkTest.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/StoreNetworkTest.js b/tests/unit/StoreNetworkTest.js index fc6226d43792..4dbd86a8c673 100644 --- a/tests/unit/StoreNetworkTest.js +++ b/tests/unit/StoreNetworkTest.js @@ -12,7 +12,7 @@ jest.useFakeTimers(); Onyx.init({ keys: ONYXKEYS, - registerStorageEventListener: () => { }, + registerStorageEventListener: () => {}, }); jest.mock('../../src/libs/Log'); From ab15fa635fbc85ed59c2ac206d39a832466c1cfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 3 Nov 2021 11:16:45 -0600 Subject: [PATCH 08/29] Update src/libs/Network.js Co-authored-by: Marc Glasser --- src/libs/Network.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/Network.js b/src/libs/Network.js index ebafd6747236..3053a3863286 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -99,7 +99,6 @@ Onyx.connect({ }); /** - * * @param {Boolean} val */ function setIsReady(val) { From acfc39a4bff7d5cfb8df011c5c75f9d8cfe19581 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 3 Nov 2021 11:16:53 -0600 Subject: [PATCH 09/29] Update tests/actions/SessionTest.js Co-authored-by: Marc Glasser --- tests/actions/SessionTest.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/actions/SessionTest.js b/tests/actions/SessionTest.js index 4534ca82bf4c..4e6fe3d71ed7 100644 --- a/tests/actions/SessionTest.js +++ b/tests/actions/SessionTest.js @@ -19,7 +19,7 @@ HttpUtils.xhr = jest.fn(); Onyx.init({ keys: ONYXKEYS, - registerStorageEventListener: () => { }, + registerStorageEventListener: () => {}, }); beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); From d0f9acc9dd956850c73e8343cad2e1abe560a904 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 3 Nov 2021 11:17:06 -0600 Subject: [PATCH 10/29] Update tests/actions/ReportTest.js Co-authored-by: Marc Glasser --- tests/actions/ReportTest.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/actions/ReportTest.js b/tests/actions/ReportTest.js index 2797d8068b4b..1696f93271ed 100644 --- a/tests/actions/ReportTest.js +++ b/tests/actions/ReportTest.js @@ -37,7 +37,7 @@ describe('actions/Report', () => { Onyx.init({ keys: ONYXKEYS, - registerStorageEventListener: () => { }, + registerStorageEventListener: () => {}, }); }); From 480b81c99a448b68b6f6df326d2f69289d9bb0d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 3 Nov 2021 12:13:13 -0600 Subject: [PATCH 11/29] add suggestions to API.js and StoreNetworkTest.js --- src/libs/API.js | 17 ++++++++--------- tests/unit/StoreNetworkTest.js | 8 ++++---- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index 7a9f94c5677e..d423fd93225e 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -15,20 +15,19 @@ let isAuthenticating; let credentials; let authToken; -/** - * Checks if both authToken and credentials were already read from persistent storage (Onyx) - * @returns {Boolean} - */ -function isAuthStoreReady() { - return !_.isUndefined(authToken) - && !_.isUndefined(credentials); +function checkRequiredDataAndSetNetworkReady() { + if (_.isUndefined(authToken) || _.isUndefined(credentials)) { + return; + } + + Network.setIsReady(true); } Onyx.connect({ key: ONYXKEYS.CREDENTIALS, callback: (val) => { credentials = val || null; - Network.setIsReady(isAuthStoreReady()); + checkRequiredDataAndSetNetworkReady(); }, }); @@ -36,7 +35,7 @@ Onyx.connect({ key: ONYXKEYS.SESSION, callback: (val) => { authToken = lodashGet(val, 'authToken', null); - Network.setIsReady(isAuthStoreReady()); + checkRequiredDataAndSetNetworkReady(); }, }); diff --git a/tests/unit/StoreNetworkTest.js b/tests/unit/StoreNetworkTest.js index 4dbd86a8c673..f20a036b17f7 100644 --- a/tests/unit/StoreNetworkTest.js +++ b/tests/unit/StoreNetworkTest.js @@ -22,19 +22,19 @@ beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); // This unit test was moved to its own file because modifies Network.setIsReady which // could mess up other unit tests, and also beforeEach can't be used for this scenario. describe('retry network request', () => { - const delayOrixConnect = 3000; - const origSetIsReady = Network.setIsReady; + const ONYX_CONNECT_DELAY_MS = 3000; + const originalSetIsReady = Network.setIsReady; const TEST_USER_LOGIN = 'test@testguy.com'; // We can't mock Onyx functions here because Onyx.connect already registered the callbacks // when API.js is loaded, so instead we mock the setIsReady with setTimeout to simulate a // delay in the Onyx.connect callback. - Network.setIsReady = val => setTimeout(() => origSetIsReady(val), delayOrixConnect); + Network.setIsReady = val => setTimeout(() => originalSetIsReady(val), ONYX_CONNECT_DELAY_MS); // We restore setIsReady to its original implementation. afterAll(() => { jest.runAllTimers(); - Network.setIsReady = origSetIsReady; + Network.setIsReady = originalSetIsReady; }); test('if auth and credentials are not read from Onyx yet', () => { From b4b694e00610dafd78323b4ebaa69d8d8dfbce22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 3 Nov 2021 14:08:03 -0600 Subject: [PATCH 12/29] remove code to pause and clear queue in enhanceParameters and its related canMakeRequest call --- src/libs/API.js | 10 ---------- src/libs/Network.js | 7 ------- 2 files changed, 17 deletions(-) diff --git a/src/libs/API.js b/src/libs/API.js index d423fd93225e..91ec7e3ba8ed 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -71,16 +71,6 @@ function addDefaultValuesToParameters(command, parameters) { const finalParameters = {...parameters}; if (isAuthTokenRequired(command) && !parameters.authToken) { - // If we end up here with no authToken it means we are trying to make an API request before we are signed in. - // In this case, we should cancel the current request by pausing the queue and clearing the remaining requests. - if (!authToken) { - LogUtil.info('A request was made without an authToken', false, {command, parameters}); - Network.pauseRequestQueue(); - Network.clearRequestQueue(); - Network.unpauseRequestQueue(); - return; - } - finalParameters.authToken = authToken; } diff --git a/src/libs/Network.js b/src/libs/Network.js index 3053a3863286..a545e594c9cd 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -220,13 +220,6 @@ function processNetworkRequestQueue() { ? enhanceParameters(queuedRequest.command, requestData) : requestData; - // Check to see if the queue has paused again. It's possible that a call to enhanceParameters() - // has paused the queue and if this is the case we must return. We don't retry these requests - // since if a request is made without an authToken we sign out the user. - if (!canMakeRequest(queuedRequest)) { - return; - } - HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure) .then(response => onResponse(queuedRequest, response)) .catch(error => onError(queuedRequest, error)); From 78da9e417da298c46f921eff45ff8fe7cb3571ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 3 Nov 2021 19:02:30 -0600 Subject: [PATCH 13/29] add delay to AsyncStorage.getItem() move StoreNetworkTest to NetworkTest --- .../async-storage.js | 22 ++++++++- tests/unit/NetworkTest.js | 27 ++++++++++- tests/unit/StoreNetworkTest.js | 47 ------------------- 3 files changed, 47 insertions(+), 49 deletions(-) delete mode 100644 tests/unit/StoreNetworkTest.js diff --git a/__mocks__/@react-native-async-storage/async-storage.js b/__mocks__/@react-native-async-storage/async-storage.js index 1051fa919c94..40873165a105 100644 --- a/__mocks__/@react-native-async-storage/async-storage.js +++ b/__mocks__/@react-native-async-storage/async-storage.js @@ -1 +1,21 @@ -export {default} from '@react-native-async-storage/async-storage/jest/async-storage-mock'; +import {jest} from '@jest/globals'; +import AsyncStorageMock from '@react-native-async-storage/async-storage/jest/async-storage-mock'; + +let asyncStorageDelay = 0; +function addDelayToGetItem(delay) { + asyncStorageDelay = delay; +} + +export default { + ...AsyncStorageMock, + getItem: jest.fn(params => AsyncStorageMock.getItem(params).then(result => new Promise((resolve) => { + if (asyncStorageDelay > 0) { + setTimeout(() => { + resolve(result); + }, asyncStorageDelay); + } else { + resolve(result); + } + }))), + addDelayToGetItem, +}; diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 8c3d5c42f13b..5e72b4423d55 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -2,14 +2,17 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; import { - beforeEach, jest, test, expect, + beforeEach, jest, test, expect, afterEach, } from '@jest/globals'; +import AsyncStorageMock from '@react-native-async-storage/async-storage'; import * as API from '../../src/libs/API'; import {signInWithTestUser} from '../utils/TestHelper'; import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ONYXKEYS from '../../src/ONYXKEYS'; import Log from '../../src/libs/Log'; +import * as Network from '../../src/libs/Network'; +import {fetchAccountDetails} from '../../src/libs/actions/Session'; // Set up manual mocks for methods used in the actions so our test does not fail. jest.mock('../../src/libs/Notification/PushNotification', () => ({ @@ -29,6 +32,11 @@ jest.mock('../../src/libs/Log'); beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); +afterEach(() => { + Network.setIsReady(false); + AsyncStorageMock.addDelayToGetItem(0); +}); + test('failing to reauthenticate while offline should not log out user', () => { // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; @@ -218,3 +226,20 @@ test('consecutive API calls eventually succeed when authToken is expired', () => expect(Log.hmmm).not.toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); }); }); + +test('retry network request if auth and credentials are not read from Onyx yet', () => { + // add a delay to AsyncStorage.getItem() to simulate a delay in the Onyx.connect callbacks + AsyncStorageMock.addDelayToGetItem(3000); + + // reset isReady for this unit test, because Onyx.clear() calls in beforeEach resolves the + // Onyx.connect callbacks with null values + Network.setIsReady(false); + + // Given a test user login and account ID + const TEST_USER_LOGIN = 'test@testguy.com'; + fetchAccountDetails(TEST_USER_LOGIN); + + // We should expect Log.hmmm to be called (logging an message to server when a request is + // made but Network is not ready) + expect(Log.hmmm).toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); +}); diff --git a/tests/unit/StoreNetworkTest.js b/tests/unit/StoreNetworkTest.js deleted file mode 100644 index f20a036b17f7..000000000000 --- a/tests/unit/StoreNetworkTest.js +++ /dev/null @@ -1,47 +0,0 @@ -import { - afterAll, beforeEach, jest, test, expect, describe, -} from '@jest/globals'; -import Onyx from 'react-native-onyx'; -import Log from '../../src/libs/Log'; -import * as Network from '../../src/libs/Network'; -import {fetchAccountDetails} from '../../src/libs/actions/Session'; -import ONYXKEYS from '../../src/ONYXKEYS'; -import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; - -jest.useFakeTimers(); - -Onyx.init({ - keys: ONYXKEYS, - registerStorageEventListener: () => {}, -}); - -jest.mock('../../src/libs/Log'); - -beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); - -// This unit test was moved to its own file because modifies Network.setIsReady which -// could mess up other unit tests, and also beforeEach can't be used for this scenario. -describe('retry network request', () => { - const ONYX_CONNECT_DELAY_MS = 3000; - const originalSetIsReady = Network.setIsReady; - const TEST_USER_LOGIN = 'test@testguy.com'; - - // We can't mock Onyx functions here because Onyx.connect already registered the callbacks - // when API.js is loaded, so instead we mock the setIsReady with setTimeout to simulate a - // delay in the Onyx.connect callback. - Network.setIsReady = val => setTimeout(() => originalSetIsReady(val), ONYX_CONNECT_DELAY_MS); - - // We restore setIsReady to its original implementation. - afterAll(() => { - jest.runAllTimers(); - Network.setIsReady = originalSetIsReady; - }); - - test('if auth and credentials are not read from Onyx yet', () => { - // Given a test user login and account ID - fetchAccountDetails(TEST_USER_LOGIN); - - // We should expect Log.hmmm to be called (logging an message to server when a request is made but Network is not ready) - expect(Log.hmmm).toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); - }); -}); From 3713f70929cadc19dca02ea3b4d89b57725333d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 3 Nov 2021 20:29:53 -0600 Subject: [PATCH 14/29] remove comment in canMakeRequest and update unit test --- src/libs/Network.js | 3 --- tests/unit/NetworkTest.js | 16 +++++++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/libs/Network.js b/src/libs/Network.js index a545e594c9cd..72f12388dcf8 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -116,9 +116,6 @@ function setIsReady(val) { * @return {Boolean} */ function canMakeRequest(request) { - // We'll not make the request now if the network is not ready (it should have higher priority than - // forceNetworkRequest and isQueuePaused because we wait until credentials + session objects have - // been read from persistent storage). if (!isReady) { LogUtil.hmmm('Trying to make a request when Network is not ready', {command: request.command, type: request.type}); return false; diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 5e72b4423d55..591eb448c6ab 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -28,8 +28,6 @@ Onyx.init({ registerStorageEventListener: () => {}, }); -jest.mock('../../src/libs/Log'); - beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); afterEach(() => { @@ -107,7 +105,6 @@ test('failing to reauthenticate while offline should not log out user', () => { expect(callsToChatList.length).toBe(3); expect(callsToAuthenticate.length).toBe(2); - expect(Log.hmmm).not.toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); }); }); }); @@ -223,7 +220,6 @@ test('consecutive API calls eventually succeed when authToken is expired', () => expect(account).toEqual(TEST_ACCOUNT_DATA); expect(personalDetailsList).toEqual(TEST_PERSONAL_DETAILS); expect(chatList).toEqual(TEST_CHAT_LIST); - expect(Log.hmmm).not.toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); }); }); @@ -231,15 +227,21 @@ test('retry network request if auth and credentials are not read from Onyx yet', // add a delay to AsyncStorage.getItem() to simulate a delay in the Onyx.connect callbacks AsyncStorageMock.addDelayToGetItem(3000); + const originalSetIsReady = Network.setIsReady; + Network.setIsReady = jest.fn(param => originalSetIsReady(param)); + // reset isReady for this unit test, because Onyx.clear() calls in beforeEach resolves the // Onyx.connect callbacks with null values Network.setIsReady(false); + HttpUtils.xhr = jest.fn(); // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; fetchAccountDetails(TEST_USER_LOGIN); - // We should expect Log.hmmm to be called (logging an message to server when a request is - // made but Network is not ready) - expect(Log.hmmm).toHaveBeenCalledWith('Trying to make a request when Network is not ready', {command: 'GetAccountStatus', type: 'post'}); + return waitForPromisesToResolve().then(() => { + // We should expect not having the Network ready and not making an http request + expect(Network.setIsReady).toHaveBeenLastCalledWith(false); + expect(HttpUtils.xhr).not.toHaveBeenCalled(); + }); }); From 9ac5627ba6efe729b40852da53d4cecbe844f292 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 09:24:49 -0600 Subject: [PATCH 15/29] check if the network request is retried in test --- tests/unit/NetworkTest.js | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 591eb448c6ab..b68554ebd7df 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -10,7 +10,6 @@ import {signInWithTestUser} from '../utils/TestHelper'; import HttpUtils from '../../src/libs/HttpUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ONYXKEYS from '../../src/ONYXKEYS'; -import Log from '../../src/libs/Log'; import * as Network from '../../src/libs/Network'; import {fetchAccountDetails} from '../../src/libs/actions/Session'; @@ -33,6 +32,9 @@ beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); afterEach(() => { Network.setIsReady(false); AsyncStorageMock.addDelayToGetItem(0); + + jest.resetAllMocks(); + jest.clearAllTimers(); }); test('failing to reauthenticate while offline should not log out user', () => { @@ -223,25 +225,40 @@ test('consecutive API calls eventually succeed when authToken is expired', () => }); }); -test('retry network request if auth and credentials are not read from Onyx yet', () => { - // add a delay to AsyncStorage.getItem() to simulate a delay in the Onyx.connect callbacks +test.only('retry network request if auth and credentials are not read from Onyx yet', () => { + // Add a delay to AsyncStorage.getItem() to simulate a delay in the Onyx.connect callbacks AsyncStorageMock.addDelayToGetItem(3000); - const originalSetIsReady = Network.setIsReady; - Network.setIsReady = jest.fn(param => originalSetIsReady(param)); - - // reset isReady for this unit test, because Onyx.clear() calls in beforeEach resolves the + // Reset isReady for this unit test, because Onyx.clear() in beforeEach resolves the // Onyx.connect callbacks with null values Network.setIsReady(false); - HttpUtils.xhr = jest.fn(); + Onyx.merge(ONYXKEYS.CREDENTIALS, {}); + Onyx.merge(ONYXKEYS.SESSION, {}); + + // Functions to mock and that we want to track + const spyNetworkSetIsReady = jest.spyOn(Network, 'setIsReady'); + const spyHttpUtilsXhr = jest.spyOn(HttpUtils, 'xhr').mockImplementation(() => Promise.resolve({})); // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; fetchAccountDetails(TEST_USER_LOGIN); - return waitForPromisesToResolve().then(() => { // We should expect not having the Network ready and not making an http request - expect(Network.setIsReady).toHaveBeenLastCalledWith(false); - expect(HttpUtils.xhr).not.toHaveBeenCalled(); + expect(spyNetworkSetIsReady).not.toHaveBeenCalled(); + expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); + + // Resolve AsyncStorage.getItem() calls + jest.runOnlyPendingTimers(); + return waitForPromisesToResolve().then(() => { + // We should expect call Network.setIsReady(true) but not making an http request yet + expect(spyNetworkSetIsReady).toHaveBeenLastCalledWith(true); + expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); + + // Run interval of processNetworkRequestQueue in Network.js + jest.runOnlyPendingTimers(); + + // We should expect a retry of the network request + expect(spyHttpUtilsXhr).toHaveBeenCalled(); + }); }); }); From a67d885013177219bf5907e797bd61e615c93eed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 12:26:25 -0600 Subject: [PATCH 16/29] restore mock async-storage to its original verison --- .../async-storage.js | 22 +------------------ 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/__mocks__/@react-native-async-storage/async-storage.js b/__mocks__/@react-native-async-storage/async-storage.js index 40873165a105..1051fa919c94 100644 --- a/__mocks__/@react-native-async-storage/async-storage.js +++ b/__mocks__/@react-native-async-storage/async-storage.js @@ -1,21 +1 @@ -import {jest} from '@jest/globals'; -import AsyncStorageMock from '@react-native-async-storage/async-storage/jest/async-storage-mock'; - -let asyncStorageDelay = 0; -function addDelayToGetItem(delay) { - asyncStorageDelay = delay; -} - -export default { - ...AsyncStorageMock, - getItem: jest.fn(params => AsyncStorageMock.getItem(params).then(result => new Promise((resolve) => { - if (asyncStorageDelay > 0) { - setTimeout(() => { - resolve(result); - }, asyncStorageDelay); - } else { - resolve(result); - } - }))), - addDelayToGetItem, -}; +export {default} from '@react-native-async-storage/async-storage/jest/async-storage-mock'; From 60df9331df933081e536784e74bcf54e68415897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 12:26:42 -0600 Subject: [PATCH 17/29] add Onyx mock to add delay in connect callbacks --- __mocks__/react-native-onyx.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 __mocks__/react-native-onyx.js diff --git a/__mocks__/react-native-onyx.js b/__mocks__/react-native-onyx.js new file mode 100644 index 000000000000..41acf1e4888d --- /dev/null +++ b/__mocks__/react-native-onyx.js @@ -0,0 +1,24 @@ +import Onyx, {withOnyx} from 'react-native-onyx'; + +let connectCallbackDelay = 0; +function addDelayToConnectCallback(delay) { + connectCallbackDelay = delay; +} + +export default { + ...Onyx, + connect: mapping => Onyx.connect({ + ...mapping, + callback: (params) => { + if (connectCallbackDelay > 0) { + setTimeout(() => { + mapping.callback(params); + }, connectCallbackDelay); + } else { + mapping.callback(params); + } + }, + }), + addDelayToConnectCallback, +}; +export {withOnyx}; From 0b2f305c3d00a757003906bc44f88cea53d36dce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 12:27:02 -0600 Subject: [PATCH 18/29] fix jsdoc param for GetAccountStatus --- src/libs/API.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libs/API.js b/src/libs/API.js index 91ec7e3ba8ed..487db2722513 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -448,6 +448,7 @@ function Get(parameters, shouldUseSecure = false) { /** * @param {Object} parameters * @param {String} parameters.email + * @param {Boolean} parameters.forceNetworkRequest * @returns {Promise} */ function GetAccountStatus(parameters) { From 1e0a1637b6da4ebbe16f7d1b3505131c32aac3a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 12:27:41 -0600 Subject: [PATCH 19/29] fix test to retry request if Network is not ready --- tests/unit/NetworkTest.js | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index b68554ebd7df..f21907a4604c 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -4,7 +4,6 @@ import Onyx from 'react-native-onyx'; import { beforeEach, jest, test, expect, afterEach, } from '@jest/globals'; -import AsyncStorageMock from '@react-native-async-storage/async-storage'; import * as API from '../../src/libs/API'; import {signInWithTestUser} from '../utils/TestHelper'; import HttpUtils from '../../src/libs/HttpUtils'; @@ -31,10 +30,8 @@ beforeEach(() => Onyx.clear().then(waitForPromisesToResolve)); afterEach(() => { Network.setIsReady(false); - AsyncStorageMock.addDelayToGetItem(0); - - jest.resetAllMocks(); - jest.clearAllTimers(); + Onyx.addDelayToConnectCallback(0); + jest.clearAllMocks(); }); test('failing to reauthenticate while offline should not log out user', () => { @@ -225,13 +222,14 @@ test('consecutive API calls eventually succeed when authToken is expired', () => }); }); -test.only('retry network request if auth and credentials are not read from Onyx yet', () => { - // Add a delay to AsyncStorage.getItem() to simulate a delay in the Onyx.connect callbacks - AsyncStorageMock.addDelayToGetItem(3000); +test('retry network request if auth and credentials are not read from Onyx yet', () => { + Onyx.addDelayToConnectCallback(3000); // Reset isReady for this unit test, because Onyx.clear() in beforeEach resolves the // Onyx.connect callbacks with null values Network.setIsReady(false); + + // Set values to trigger Onyx.connect callbacks Onyx.merge(ONYXKEYS.CREDENTIALS, {}); Onyx.merge(ONYXKEYS.SESSION, {}); @@ -242,23 +240,26 @@ test.only('retry network request if auth and credentials are not read from Onyx // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; fetchAccountDetails(TEST_USER_LOGIN); + + // Wait for the mock Onyx.callbacks to be set return waitForPromisesToResolve().then(() => { // We should expect not having the Network ready and not making an http request expect(spyNetworkSetIsReady).not.toHaveBeenCalled(); expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); - // Resolve AsyncStorage.getItem() calls + // Resolve Onyx.connect callbacks jest.runOnlyPendingTimers(); - return waitForPromisesToResolve().then(() => { - // We should expect call Network.setIsReady(true) but not making an http request yet - expect(spyNetworkSetIsReady).toHaveBeenLastCalledWith(true); - expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); - // Run interval of processNetworkRequestQueue in Network.js - jest.runOnlyPendingTimers(); + // We should expect call Network.setIsReady(true) + expect(spyNetworkSetIsReady).toHaveBeenLastCalledWith(true); - // We should expect a retry of the network request - expect(spyHttpUtilsXhr).toHaveBeenCalled(); - }); + // We should expect not making an http request yet + expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); + + // Run processNetworkRequestQueue in the setInterval of Network.js + jest.runOnlyPendingTimers(); + + // We should expect a retry of the network request + expect(spyHttpUtilsXhr).toHaveBeenCalled(); }); }); From ec2ee0874b80a4c38a0535b07493c07f45f0b774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 12:48:56 -0600 Subject: [PATCH 20/29] add missing optional param in jsdoc --- src/libs/OptionsListUtils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/OptionsListUtils.js b/src/libs/OptionsListUtils.js index 8d2e350c98d0..e42a251f7f99 100644 --- a/src/libs/OptionsListUtils.js +++ b/src/libs/OptionsListUtils.js @@ -541,7 +541,7 @@ function getOptions(reports, personalDetails, draftComments, activeReportID, { * @param {Object} reports * @param {Object} personalDetails * @param {String} searchValue - * @param {Array} betas + * @param {Array=} betas * @returns {Object} */ function getSearchOptions( @@ -638,7 +638,7 @@ function getNewChatOptions( * @param {Object} draftComments * @param {Number} activeReportID * @param {String} priorityMode - * @param {Array} betas + * @param {Array=} betas * @returns {Object} */ function getSidebarOptions( From 62ef02048c18e8e00d0618cd1ed25050aeeef700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 12:50:37 -0600 Subject: [PATCH 21/29] add waitForPromisesToResolve in test --- tests/unit/OptionsListUtilsTest.js | 33 +++++++++++++++++------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/tests/unit/OptionsListUtilsTest.js b/tests/unit/OptionsListUtilsTest.js index a761f179c3c1..f1f42df320a0 100644 --- a/tests/unit/OptionsListUtilsTest.js +++ b/tests/unit/OptionsListUtilsTest.js @@ -1,5 +1,8 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; +import { + describe, beforeAll, it, expect, +} from '@jest/globals'; import * as OptionsListUtils from '../../src/libs/OptionsListUtils'; import ONYXKEYS from '../../src/ONYXKEYS'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; @@ -593,24 +596,26 @@ describe('OptionsListUtils', () => { }); it('getSidebarOptions() with GSD priority mode', () => { - // When we call getSidebarOptions() with no search value - const results = OptionsListUtils.getSidebarOptions(REPORTS, PERSONAL_DETAILS, REPORT_DRAFT_COMMENTS, 0, CONST.PRIORITY_MODE.GSD); + waitForPromisesToResolve().then(() => { + // When we call getSidebarOptions() with no search value + const results = OptionsListUtils.getSidebarOptions(REPORTS, PERSONAL_DETAILS, REPORT_DRAFT_COMMENTS, 0, CONST.PRIORITY_MODE.GSD); - // Then expect all of the reports to be shown both multiple and single participant except the - // report that has no lastMessageTimestamp and the chat with Thor who's message is read - expect(results.recentReports.length).toBe(_.size(REPORTS) - 2); + // Then expect all of the reports to be shown both multiple and single participant except the + // report that has no lastMessageTimestamp and the chat with Thor who's message is read + expect(results.recentReports.length).toBe(_.size(REPORTS) - 2); - // That no personalDetails are shown - expect(results.personalDetails.length).toBe(0); + // That no personalDetails are shown + expect(results.personalDetails.length).toBe(0); - // And Mister Fantastic is alphabetically the fourth report and has an unread message - // despite being pinned - expect(results.recentReports[4].login).toBe('reedrichards@expensify.com'); + // And Mister Fantastic is alphabetically the fourth report and has an unread message + // despite being pinned + expect(results.recentReports[4].login).toBe('reedrichards@expensify.com'); - // And Black Panther is alphabetically the first report and has an unread message - expect(results.recentReports[0].login).toBe('tchalla@expensify.com'); + // And Black Panther is alphabetically the first report and has an unread message + expect(results.recentReports[0].login).toBe('tchalla@expensify.com'); - // And Mister Sinister is alphabetically the fifth report and has an IOU debt despite not being pinned - expect(results.recentReports[5].login).toBe('mistersinister@marauders.com'); + // And Mister Sinister is alphabetically the fifth report and has an IOU debt despite not being pinned + expect(results.recentReports[5].login).toBe('mistersinister@marauders.com'); + }); }); }); From 1226f64fd217f667b2df0eb065cbe71e5f070842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 12:50:37 -0600 Subject: [PATCH 22/29] Revert "add waitForPromisesToResolve in test" This reverts commit 62ef02048c18e8e00d0618cd1ed25050aeeef700. --- tests/unit/OptionsListUtilsTest.js | 33 +++++++++++++----------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/tests/unit/OptionsListUtilsTest.js b/tests/unit/OptionsListUtilsTest.js index f1f42df320a0..a761f179c3c1 100644 --- a/tests/unit/OptionsListUtilsTest.js +++ b/tests/unit/OptionsListUtilsTest.js @@ -1,8 +1,5 @@ import _ from 'underscore'; import Onyx from 'react-native-onyx'; -import { - describe, beforeAll, it, expect, -} from '@jest/globals'; import * as OptionsListUtils from '../../src/libs/OptionsListUtils'; import ONYXKEYS from '../../src/ONYXKEYS'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; @@ -596,26 +593,24 @@ describe('OptionsListUtils', () => { }); it('getSidebarOptions() with GSD priority mode', () => { - waitForPromisesToResolve().then(() => { - // When we call getSidebarOptions() with no search value - const results = OptionsListUtils.getSidebarOptions(REPORTS, PERSONAL_DETAILS, REPORT_DRAFT_COMMENTS, 0, CONST.PRIORITY_MODE.GSD); + // When we call getSidebarOptions() with no search value + const results = OptionsListUtils.getSidebarOptions(REPORTS, PERSONAL_DETAILS, REPORT_DRAFT_COMMENTS, 0, CONST.PRIORITY_MODE.GSD); - // Then expect all of the reports to be shown both multiple and single participant except the - // report that has no lastMessageTimestamp and the chat with Thor who's message is read - expect(results.recentReports.length).toBe(_.size(REPORTS) - 2); + // Then expect all of the reports to be shown both multiple and single participant except the + // report that has no lastMessageTimestamp and the chat with Thor who's message is read + expect(results.recentReports.length).toBe(_.size(REPORTS) - 2); - // That no personalDetails are shown - expect(results.personalDetails.length).toBe(0); + // That no personalDetails are shown + expect(results.personalDetails.length).toBe(0); - // And Mister Fantastic is alphabetically the fourth report and has an unread message - // despite being pinned - expect(results.recentReports[4].login).toBe('reedrichards@expensify.com'); + // And Mister Fantastic is alphabetically the fourth report and has an unread message + // despite being pinned + expect(results.recentReports[4].login).toBe('reedrichards@expensify.com'); - // And Black Panther is alphabetically the first report and has an unread message - expect(results.recentReports[0].login).toBe('tchalla@expensify.com'); + // And Black Panther is alphabetically the first report and has an unread message + expect(results.recentReports[0].login).toBe('tchalla@expensify.com'); - // And Mister Sinister is alphabetically the fifth report and has an IOU debt despite not being pinned - expect(results.recentReports[5].login).toBe('mistersinister@marauders.com'); - }); + // And Mister Sinister is alphabetically the fifth report and has an IOU debt despite not being pinned + expect(results.recentReports[5].login).toBe('mistersinister@marauders.com'); }); }); From 623c8a37000577c4d1122eef12387653981d63b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 15:16:44 -0600 Subject: [PATCH 23/29] fix callback params in mock Onyx --- __mocks__/react-native-onyx.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/__mocks__/react-native-onyx.js b/__mocks__/react-native-onyx.js index 41acf1e4888d..242da6e3cd17 100644 --- a/__mocks__/react-native-onyx.js +++ b/__mocks__/react-native-onyx.js @@ -9,13 +9,13 @@ export default { ...Onyx, connect: mapping => Onyx.connect({ ...mapping, - callback: (params) => { + callback: (...params) => { if (connectCallbackDelay > 0) { setTimeout(() => { - mapping.callback(params); + mapping.callback(...params); }, connectCallbackDelay); } else { - mapping.callback(params); + mapping.callback(...params); } }, }), From ea99e0b0806ae20f2bd374ebaf34b3df7b9191e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 16:32:53 -0600 Subject: [PATCH 24/29] change runOnlyPendingTimers to advanceTimersByTime --- tests/unit/NetworkTest.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index f21907a4604c..3919feed19dc 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -223,7 +223,9 @@ test('consecutive API calls eventually succeed when authToken is expired', () => }); test('retry network request if auth and credentials are not read from Onyx yet', () => { - Onyx.addDelayToConnectCallback(3000); + const ONYX_DELAY_MS = 3000; + const NETWORK_INTERVAL_MS = 1000; + Onyx.addDelayToConnectCallback(ONYX_DELAY_MS); // Reset isReady for this unit test, because Onyx.clear() in beforeEach resolves the // Onyx.connect callbacks with null values @@ -248,7 +250,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); // Resolve Onyx.connect callbacks - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(ONYX_DELAY_MS); // We should expect call Network.setIsReady(true) expect(spyNetworkSetIsReady).toHaveBeenLastCalledWith(true); @@ -257,7 +259,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); // Run processNetworkRequestQueue in the setInterval of Network.js - jest.runOnlyPendingTimers(); + jest.advanceTimersByTime(NETWORK_INTERVAL_MS); // We should expect a retry of the network request expect(spyHttpUtilsXhr).toHaveBeenCalled(); From 63d35a8e752df3c3470af5a83697c7717944c919 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 17:04:47 -0600 Subject: [PATCH 25/29] add PROCESS_REQUEST_DELAY_MS to Network change unit test comments to GIVE-WHEN-THEN format --- src/libs/Network.js | 5 ++++- tests/unit/NetworkTest.js | 34 +++++++++++++++++----------------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/libs/Network.js b/src/libs/Network.js index 72f12388dcf8..abf13558c393 100644 --- a/src/libs/Network.js +++ b/src/libs/Network.js @@ -27,6 +27,8 @@ let onError = () => {}; let didLoadPersistedRequests; let isOffline; +const PROCESS_REQUEST_DELAY_MS = 1000; + /** * Process the offline NETWORK_REQUEST_QUEUE * @param {Array | null} persistedRequests - Requests @@ -240,7 +242,7 @@ function processNetworkRequestQueue() { } // Process our write queue very often -setInterval(processNetworkRequestQueue, 1000); +setInterval(processNetworkRequestQueue, PROCESS_REQUEST_DELAY_MS); /** * @param {Object} request @@ -334,6 +336,7 @@ function registerErrorHandler(callback) { export { post, pauseRequestQueue, + PROCESS_REQUEST_DELAY_MS, unpauseRequestQueue, registerParameterEnhancer, clearRequestQueue, diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index 3919feed19dc..a4512e1f18cf 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -223,45 +223,45 @@ test('consecutive API calls eventually succeed when authToken is expired', () => }); test('retry network request if auth and credentials are not read from Onyx yet', () => { + // Given a test user login and account ID + const TEST_USER_LOGIN = 'test@testguy.com'; + + // Given a delay to the Onyx.connect callbacks const ONYX_DELAY_MS = 3000; - const NETWORK_INTERVAL_MS = 1000; Onyx.addDelayToConnectCallback(ONYX_DELAY_MS); - // Reset isReady for this unit test, because Onyx.clear() in beforeEach resolves the - // Onyx.connect callbacks with null values + // Given initial state to Network Network.setIsReady(false); - // Set values to trigger Onyx.connect callbacks + // Given initial empty values to trigger Onyx.connect callbacks in API.js Onyx.merge(ONYXKEYS.CREDENTIALS, {}); Onyx.merge(ONYXKEYS.SESSION, {}); - // Functions to mock and that we want to track + // Given some mock functions to track the isReady + // flag in Network and the http requests made const spyNetworkSetIsReady = jest.spyOn(Network, 'setIsReady'); const spyHttpUtilsXhr = jest.spyOn(HttpUtils, 'xhr').mockImplementation(() => Promise.resolve({})); - // Given a test user login and account ID - const TEST_USER_LOGIN = 'test@testguy.com'; + // When we make an arbitrary request that can be retried + // And we wait for the Onyx.callbacks to be set fetchAccountDetails(TEST_USER_LOGIN); - - // Wait for the mock Onyx.callbacks to be set return waitForPromisesToResolve().then(() => { - // We should expect not having the Network ready and not making an http request + // Then we expect not having the Network ready and not making an http request expect(spyNetworkSetIsReady).not.toHaveBeenCalled(); expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); - // Resolve Onyx.connect callbacks + // When we resolve Onyx.connect callbacks jest.advanceTimersByTime(ONYX_DELAY_MS); - // We should expect call Network.setIsReady(true) + // Then we should expect call Network.setIsReady(true) + // And We should expect not making an http request yet expect(spyNetworkSetIsReady).toHaveBeenLastCalledWith(true); - - // We should expect not making an http request yet expect(spyHttpUtilsXhr).not.toHaveBeenCalled(); - // Run processNetworkRequestQueue in the setInterval of Network.js - jest.advanceTimersByTime(NETWORK_INTERVAL_MS); + // When we run processNetworkRequestQueue in the setInterval of Network.js + jest.advanceTimersByTime(Network.PROCESS_REQUEST_DELAY_MS); - // We should expect a retry of the network request + // Then we should expect a retry of the network request expect(spyHttpUtilsXhr).toHaveBeenCalled(); }); }); From b8a580ba0b3c1be4b47f19dc870c3f7fe73240b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Fri, 5 Nov 2021 10:15:07 -0600 Subject: [PATCH 26/29] update comment in test that retrires network req --- tests/unit/NetworkTest.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unit/NetworkTest.js b/tests/unit/NetworkTest.js index a4512e1f18cf..af2d90fcb6e1 100644 --- a/tests/unit/NetworkTest.js +++ b/tests/unit/NetworkTest.js @@ -223,6 +223,12 @@ test('consecutive API calls eventually succeed when authToken is expired', () => }); test('retry network request if auth and credentials are not read from Onyx yet', () => { + // For this test we're having difficulty creating a situation where Onyx.connect() has not yet run + // because some Onyx.connect callbacks are already registered in API.js (which happens before this + // unit test is setup), so in order to test an scenario where the auth token and credentials hasn't + // been read from storage we set Network.setIsReady(false) and trigger an update in the Onyx.connect + // callbacks registered in API.js merging an empty object. + // Given a test user login and account ID const TEST_USER_LOGIN = 'test@testguy.com'; @@ -233,7 +239,7 @@ test('retry network request if auth and credentials are not read from Onyx yet', // Given initial state to Network Network.setIsReady(false); - // Given initial empty values to trigger Onyx.connect callbacks in API.js + // Given any initial value to trigger an update Onyx.merge(ONYXKEYS.CREDENTIALS, {}); Onyx.merge(ONYXKEYS.SESSION, {}); From 1f23139cdf0d0aabfd99b396378a525e13648d65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 4 Nov 2021 12:48:56 -0600 Subject: [PATCH 27/29] Revert "add missing optional param in jsdoc" This reverts commit ec2ee0874b80a4c38a0535b07493c07f45f0b774. --- src/libs/OptionsListUtils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libs/OptionsListUtils.js b/src/libs/OptionsListUtils.js index e42a251f7f99..8d2e350c98d0 100644 --- a/src/libs/OptionsListUtils.js +++ b/src/libs/OptionsListUtils.js @@ -541,7 +541,7 @@ function getOptions(reports, personalDetails, draftComments, activeReportID, { * @param {Object} reports * @param {Object} personalDetails * @param {String} searchValue - * @param {Array=} betas + * @param {Array} betas * @returns {Object} */ function getSearchOptions( @@ -638,7 +638,7 @@ function getNewChatOptions( * @param {Object} draftComments * @param {Number} activeReportID * @param {String} priorityMode - * @param {Array=} betas + * @param {Array} betas * @returns {Object} */ function getSidebarOptions( From c6fb00ffa471208bb98c88d28a8db9607ac562c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Mon, 8 Nov 2021 10:07:26 -0500 Subject: [PATCH 28/29] remove duplicated lodashGet --- src/libs/API.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/API.js b/src/libs/API.js index 88c2a480b058..f7d5678d3043 100644 --- a/src/libs/API.js +++ b/src/libs/API.js @@ -1,7 +1,6 @@ import _ from 'underscore'; import lodashGet from 'lodash/get'; import Onyx from 'react-native-onyx'; -import lodashGet from 'lodash/get'; import CONST from '../CONST'; import CONFIG from '../CONFIG'; import ONYXKEYS from '../ONYXKEYS'; From 7f22cabdd12947ad92f81fb1e50d85f3e524e76a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Mon, 8 Nov 2021 11:07:47 -0500 Subject: [PATCH 29/29] disable prefer-onyx-connect-in-libs in Onyx mock --- __mocks__/react-native-onyx.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/__mocks__/react-native-onyx.js b/__mocks__/react-native-onyx.js index 242da6e3cd17..dc9593e27502 100644 --- a/__mocks__/react-native-onyx.js +++ b/__mocks__/react-native-onyx.js @@ -1,3 +1,4 @@ +/* eslint-disable rulesdir/prefer-onyx-connect-in-libs */ import Onyx, {withOnyx} from 'react-native-onyx'; let connectCallbackDelay = 0; @@ -22,3 +23,4 @@ export default { addDelayToConnectCallback, }; export {withOnyx}; +/* eslint-enable rulesdir/prefer-onyx-connect-in-libs */