Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust tests to the batching updates feature #27230

Merged
merged 28 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
9297f16
fix tests
hannojg Aug 28, 2023
9a3cfe5
fix waitForPromise method to exhaust microtask queue
hannojg Aug 29, 2023
c4c241e
fix Emoji tests
hannojg Aug 29, 2023
b76b005
fix SidebarFilterTest
hannojg Aug 29, 2023
66c995f
wip
hannojg Aug 29, 2023
478ff87
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg Aug 31, 2023
19de1d8
Merge branch 'main' into perf/onyx-upgrade-react-batching
Szymon20000 Sep 6, 2023
ac31c74
stash work
Szymon20000 Sep 11, 2023
5200d56
stash work
Szymon20000 Sep 11, 2023
e717c1e
remove console.logs
Szymon20000 Sep 12, 2023
fba544d
make FileUtilsTest more stable
Szymon20000 Sep 12, 2023
0185bbf
make it more stable
Szymon20000 Sep 12, 2023
5b80d4b
better naming
Szymon20000 Sep 12, 2023
2f2f7b1
remove unnecessary waitForPromise
Szymon20000 Sep 12, 2023
098375f
cleanup
Szymon20000 Sep 12, 2023
3a90043
Merge branch 'main' into perf/onyx-upgrade-react-batching
Szymon20000 Sep 12, 2023
d2aa316
prettier
Szymon20000 Sep 12, 2023
36a390a
Update tests/utils/waitForPromisesToResolve.js
Szymon20000 Sep 14, 2023
c5a7d40
Update tests/actions/ReportTest.js
Szymon20000 Sep 14, 2023
577fda3
clean up
Szymon20000 Sep 14, 2023
dcc2ab2
clean up
Szymon20000 Sep 14, 2023
f386d30
rename to waitForNetworkPromises and waitForBatchedUpdates
Szymon20000 Sep 14, 2023
58f4bf5
cleanup
Szymon20000 Sep 14, 2023
dd5e6aa
bump onyx version
Szymon20000 Sep 15, 2023
12d1326
Merge remote-tracking branch 'origin/main' into perf/onyx-upgrade-rea…
Szymon20000 Sep 15, 2023
225ffe1
Update src/libs/actions/PersistedRequests.js
Szymon20000 Sep 19, 2023
e0e31ce
Merge branch 'main' into perf/onyx-upgrade-react-batching
ospfranco Sep 20, 2023
7869c5a
fix: return types are not allowe isssue
Szymon20000 Sep 20, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@ module.exports = {
},
testEnvironment: 'jsdom',
setupFiles: ['<rootDir>/jest/setup.js', './node_modules/@react-native-google-signin/google-signin/jest/build/setup.js'],
setupFilesAfterEnv: ['@testing-library/jest-native/extend-expect'],
setupFilesAfterEnv: ['@testing-library/jest-native/extend-expect', '<rootDir>/jest/setupAfterEnv.js'],
cacheDirectory: '<rootDir>/.jest-cache',
};
1 change: 1 addition & 0 deletions jest/setupAfterEnv.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
jest.useRealTimers();
28 changes: 16 additions & 12 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,10 @@
"react-native-linear-gradient": "^2.8.1",
"react-native-localize": "^2.2.6",
"react-native-modal": "^13.0.0",
"react-native-onyx": "1.0.76",
"react-native-onyx": "1.0.78",
"react-native-pager-view": "^6.2.0",
"react-native-pdf": "^6.7.1",
"react-native-performance": "^4.0.0",
"react-native-performance": "^5.1.0",
"react-native-permissions": "^3.0.1",
"react-native-picker-select": "git+https://github.com/Expensify/react-native-picker-select.git#eae05855286dc699954415cc1d629bfd8e8e47e2",
"react-native-plaid-link-sdk": "^10.0.0",
Expand Down
6 changes: 5 additions & 1 deletion src/libs/RequestThrottle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,12 @@ function getRequestWaitTime() {
return requestWaitTime;
}

function getLastRequestWaitTime() {
return requestWaitTime;
}

function sleep(): Promise<void> {
return new Promise((resolve) => setTimeout(resolve, getRequestWaitTime()));
}

export {clear, getRequestWaitTime, sleep};
export {clear, getRequestWaitTime, sleep, getLastRequestWaitTime};
6 changes: 5 additions & 1 deletion src/libs/actions/PersistedRequests.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ Onyx.connect({
callback: (val) => (persistedRequests = val || []),
});

/**
* This promise is only used by tests. DO NOT USE THIS PROMISE IN THE APPLICATION CODE
* @returns {void}
Szymon20000 marked this conversation as resolved.
Show resolved Hide resolved
*/
function clear() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add JSdocs here for the returns that says something like:

This promise is only used by tests. DO NOT USE THIS PROMISE IN THE APPLICATION CODE

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgolen Can't we just use waitForPromisesToResolve() in the tests instead of the return statement?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []);
return Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, []);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

- Much of the logic in the app is asynchronous in nature. [`react-native-onyx`](https://github.com/expensify/react-native-onyx) relies on [`AsyncStorage`](https://github.com/react-native-async-storage/async-storage) and writes data async before updating subscribers.
- [Actions](https://github.com/Expensify/App#actions) do not typically return a `Promise` and therefore can't always be "awaited" before running an assertion.
- To test a result after some asynchronous code has run we can use [`Onyx.connect()`](https://github.com/Expensify/react-native-onyx/blob/2c94a94e51fab20330f7bd5381b72ea6c25553d9/lib/Onyx.js#L217-L231) and the helper method [`waitForPromisesToResolve()`](https://github.com/Expensify/ReactNativeChat/blob/ca2fa88a5789b82463d35eddc3d57f70a7286868/tests/utils/waitForPromisesToResolve.js#L1-L9) which returns a `Promise` and will ensure that all other `Promises` have finished running before resolving.
- To test a result after some asynchronous code has run we can use [`Onyx.connect()`](https://github.com/Expensify/react-native-onyx/blob/2c94a94e51fab20330f7bd5381b72ea6c25553d9/lib/Onyx.js#L217-L231) and the helper method [`waitForBatchedUpdates()`](https://github.com/Expensify/ReactNativeChat/blob/ca2fa88a5789b82463d35eddc3d57f70a7286868/tests/utils/waitForBatchedUpdates.js#L1-L9) which returns a `Promise` and will ensure that all other `Promises` have finished running before resolving.
- **Important Note:** When writing any asynchronous Jest test it's very important that your test itself **return a `Promise`**.

## Mocking Network Requests
Expand Down Expand Up @@ -65,7 +65,7 @@ describe('actions/Report', () => {

// When we add a new action to that report
Report.addComment(REPORT_ID, 'Hello!');
return waitForPromisesToResolve()
return waitForBatchedUpdates()
.then(() => {
const action = reportActions[ACTION_ID];

Expand Down
22 changes: 13 additions & 9 deletions tests/actions/IOUTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ import _ from 'underscore';
import Onyx from 'react-native-onyx';
import CONST from '../../src/CONST';
import ONYXKEYS from '../../src/ONYXKEYS';
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';
import * as IOU from '../../src/libs/actions/IOU';
import * as TestHelper from '../utils/TestHelper';
import DateUtils from '../../src/libs/DateUtils';
import * as NumberUtils from '../../src/libs/NumberUtils';
import * as ReportActions from '../../src/libs/actions/ReportActions';
import * as Report from '../../src/libs/actions/Report';
import OnyxUpdateManager from '../../src/libs/actions/OnyxUpdateManager';
import waitForNetworkPromises from '../utils/waitForNetworkPromises';

const CARLOS_EMAIL = 'cmartins@expensifail.com';
const CARLOS_ACCOUNT_ID = 1;
Expand All @@ -30,7 +31,7 @@ describe('actions/IOU', () => {

beforeEach(() => {
global.fetch = TestHelper.getGlobalFetchMock();
return Onyx.clear().then(waitForPromisesToResolve);
return Onyx.clear().then(waitForBatchedUpdates);
});

describe('requestMoney', () => {
Expand All @@ -44,7 +45,7 @@ describe('actions/IOU', () => {
let transactionID;
fetch.pause();
IOU.requestMoney({}, amount, CONST.CURRENCY.USD, '', merchant, RORY_EMAIL, RORY_ACCOUNT_ID, {login: CARLOS_EMAIL, accountID: CARLOS_ACCOUNT_ID}, comment);
return waitForPromisesToResolve()
return waitForBatchedUpdates()
.then(
() =>
new Promise((resolve) => {
Expand Down Expand Up @@ -209,7 +210,7 @@ describe('actions/IOU', () => {
)
.then(() => {
IOU.requestMoney(chatReport, amount, CONST.CURRENCY.USD, '', '', RORY_EMAIL, RORY_ACCOUNT_ID, {login: CARLOS_EMAIL, accountID: CARLOS_ACCOUNT_ID}, comment);
return waitForPromisesToResolve();
return waitForBatchedUpdates();
})
.then(
() =>
Expand Down Expand Up @@ -309,6 +310,7 @@ describe('actions/IOU', () => {
}),
)
.then(fetch.resume)
.then(waitForBatchedUpdates)
.then(
() =>
new Promise((resolve) => {
Expand Down Expand Up @@ -400,7 +402,7 @@ describe('actions/IOU', () => {
.then(() => Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${existingTransaction.transactionID}`, existingTransaction))
.then(() => {
IOU.requestMoney(chatReport, amount, CONST.CURRENCY.USD, '', '', RORY_EMAIL, RORY_ACCOUNT_ID, {login: CARLOS_EMAIL, accountID: CARLOS_ACCOUNT_ID}, comment);
return waitForPromisesToResolve();
return waitForBatchedUpdates();
})
.then(
() =>
Expand Down Expand Up @@ -491,6 +493,7 @@ describe('actions/IOU', () => {
}),
)
.then(fetch.resume)
.then(waitForNetworkPromises)
.then(
() =>
new Promise((resolve) => {
Expand Down Expand Up @@ -533,7 +536,7 @@ describe('actions/IOU', () => {
fetch.pause();
IOU.requestMoney({}, amount, CONST.CURRENCY.USD, '', '', RORY_EMAIL, RORY_ACCOUNT_ID, {login: CARLOS_EMAIL, accountID: CARLOS_ACCOUNT_ID}, comment);
return (
waitForPromisesToResolve()
waitForBatchedUpdates()
.then(
() =>
new Promise((resolve) => {
Expand Down Expand Up @@ -904,7 +907,7 @@ describe('actions/IOU', () => {
comment,
CONST.CURRENCY.USD,
);
return waitForPromisesToResolve();
return waitForBatchedUpdates();
})
.then(
() =>
Expand Down Expand Up @@ -1127,6 +1130,7 @@ describe('actions/IOU', () => {
}),
)
.then(fetch.resume)
.then(waitForNetworkPromises)
.then(
() =>
new Promise((resolve) => {
Expand Down Expand Up @@ -1187,7 +1191,7 @@ describe('actions/IOU', () => {
let payIOUAction;
let transaction;
IOU.requestMoney({}, amount, CONST.CURRENCY.USD, '', '', RORY_EMAIL, RORY_ACCOUNT_ID, {login: CARLOS_EMAIL, accountID: CARLOS_ACCOUNT_ID}, comment);
return waitForPromisesToResolve()
return waitForBatchedUpdates()
.then(
() =>
new Promise((resolve) => {
Expand Down Expand Up @@ -1267,7 +1271,7 @@ describe('actions/IOU', () => {
.then(() => {
fetch.pause();
IOU.payMoneyRequest(CONST.IOU.PAYMENT_TYPE.ELSEWHERE, chatReport, iouReport);
return waitForPromisesToResolve();
return waitForBatchedUpdates();
})
.then(
() =>
Expand Down
Loading