-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA][TS migration] Migrate multiple test to typescript #37396
[No QA][TS migration] Migrate multiple test to typescript #37396
Conversation
tests/unit/CurrencyUtilsTest.ts
Outdated
if (isEmptyObject(currencyCodeList)) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this check is needed?
tests/unit/IOUUtilsTest.ts
Outdated
const usdPendingTransaction = TransactionUtils.buildOptimisticTransaction(100, 'USD', iouReport.reportID); | ||
const aedPendingTransaction = TransactionUtils.buildOptimisticTransaction(100, 'AED', iouReport.reportID); | ||
const MergeQueries: Record<`${typeof ONYXKEYS.COLLECTION.TRANSACTION}${string}`, NullishDeep<Transaction>> = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can use CollectionDataSet type here
tests/unit/IOUUtilsTest.ts
Outdated
pendingAction: null, | ||
}, | ||
}).then(() => { | ||
const MergeQueries: Record<`${typeof ONYXKEYS.COLLECTION.TRANSACTION}${string}`, NullishDeep<Transaction>> = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
tests/unit/SidebarTest.ts
Outdated
@@ -50,13 +38,15 @@ describe('Sidebar', () => { | |||
describe('archived chats', () => { | |||
it('renders the archive reason as the preview message of the chat', () => { | |||
const report = { | |||
// @ts-expect-error TODO: Remove this once LHNTestUtils (https://github.com/Expensify/App/issues/25320) is migrated to TypeScript. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's merged, so please merge the latest main
tests/utils/TestHelper.ts
Outdated
import type {PersonalDetails, Report} from '@src/types/onyx'; | ||
import waitForBatchedUpdates from './waitForBatchedUpdates'; | ||
|
||
type MockFetch = ReturnType<typeof jest.fn> & {pause?: () => void; fail?: () => void; succeed?: () => void; resume?: () => Promise<void>}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe we can use just jest.Mock &
instead of ReturnType<typeof jest.fn> &
type MockFetch = ReturnType<typeof jest.fn> & {pause?: () => void; fail?: () => void; succeed?: () => void; resume?: () => Promise<void>}; | |
type MockFetch = ReturnType<typeof jest.fn> & { | |
pause?: () => void; | |
fail?: () => void; | |
succeed?: () => void; | |
resume?: () => Promise<void> | |
}; |
tests/e2e/compare/compare.ts
Outdated
// Unique test scenario names | ||
const names = [...new Set([..._.keys(compareEntries), ..._.keys(baselineEntries || {})])]; | ||
const compareKeys = Object.keys(compareEntries); | ||
const baselineKeys = baselineEntries ? Object.keys(baselineEntries) : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const baselineKeys = baselineEntries ? Object.keys(baselineEntries) : []; | |
const baselineKeys = Object.keys(baselineEntries ?? {}); |
wdyt?
# Conflicts: # tests/utils/TestHelper.js
# Conflicts: # tests/e2e/measure/math.ts
tests/unit/SidebarTest.ts
Outdated
.then(() => | ||
Onyx.multiSet({ | ||
.then(() => { | ||
const reportCollection = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const reportCollection = { | |
const reportCollection: ReportActionCollectionDataSet = { |
this works?
tests/unit/SidebarTest.ts
Outdated
[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report, | ||
} as ReportCollectionDataSet; | ||
|
||
const reportAction = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
tests/unit/SidebarTest.ts
Outdated
.then(() => | ||
Onyx.multiSet({ | ||
.then(() => { | ||
const reportCollection = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
tests/unit/SidebarTest.ts
Outdated
[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report, | ||
} as ReportCollectionDataSet; | ||
|
||
const reportAction = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
tests/utils/TestHelper.ts
Outdated
// Response is the same for calls to Authenticate and BeginSignIn | ||
HttpUtils.xhr = jest.fn().mockImplementation(() => { | ||
// Your mocked response object | ||
const mockedResponse = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this comment?
tests/utils/TestHelper.ts
Outdated
const originalXhr = HttpUtils.xhr; | ||
HttpUtils.xhr = jest.fn().mockImplementation(() => { | ||
// Your mocked response object | ||
const mockedResponse = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this comment?
tests/utils/TestHelper.ts
Outdated
|
||
HttpUtils.xhr = jest.fn().mockImplementation(() => { | ||
// Your mocked response object | ||
const mockedResponse = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const mockedResponse = { | |
const mockedResponse: Response = { |
Could we type this object?
tests/utils/TestHelper.ts
Outdated
@@ -18,7 +19,13 @@ type MockFetch = ReturnType<typeof jest.fn> & { | |||
resume?: () => Promise<void>; | |||
}; | |||
|
|||
type Response = {ok: boolean; json: () => Promise<{jsonCode: number}>}; | |||
type Response = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use Response
from src/types/onyx/Response.ts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response type differs from the OnyxType, should we update the OnyxType.Reponse to comply with the type of the TestHelper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my suggestion
diff --git a/tests/utils/TestHelper.ts b/tests/utils/TestHelper.ts
index 097433e881..10313ad46d 100644
--- a/tests/utils/TestHelper.ts
+++ b/tests/utils/TestHelper.ts
@@ -1,12 +1,11 @@
import Str from 'expensify-common/lib/str';
-import type {OnyxUpdate} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import CONST from '@src/CONST';
import * as Session from '@src/libs/actions/Session';
import HttpUtils from '@src/libs/HttpUtils';
import * as NumberUtils from '@src/libs/NumberUtils';
import ONYXKEYS from '@src/ONYXKEYS';
-import type {PersonalDetails, Report} from '@src/types/onyx';
+import type {Response as OnyxResponse, PersonalDetails, Report} from '@src/types/onyx';
import waitForBatchedUpdates from './waitForBatchedUpdates';
type MockFetch = ReturnType<typeof jest.fn> & {
@@ -19,14 +18,7 @@ type MockFetch = ReturnType<typeof jest.fn> & {
resume?: () => Promise<void>;
};
-type Response = {
- ok?: boolean;
- json?: () => Promise<{jsonCode: number}>;
- jsonCode?: number;
- onyxData?: OnyxUpdate[];
-};
-
-type QueueItem = (value: Response | PromiseLike<Response>) => void;
+type QueueItem = (value: Partial<Response> | PromiseLike<Partial<Response>>) => void;
type FormData = {
entries: () => Array<[string, string | Blob]>;
@@ -58,7 +50,7 @@ function signInWithTestUser(accountID = 1, login = '[email protected]', password = '
HttpUtils.xhr = jest.fn().mockImplementation(() => {
// Your mocked response object
- const mockedResponse: Response = {
+ const mockedResponse: OnyxResponse = {
onyxData: [
{
onyxMethod: Onyx.METHOD.MERGE,
@@ -176,7 +168,7 @@ function getGlobalFetchMock() {
let isPaused = false;
let shouldFail = false;
- const getResponse = () =>
+ const getResponse = (): Partial<Response> =>
shouldFail
? {
ok: true,
@@ -256,4 +248,4 @@ const createAddListenerMock = () => {
return {triggerTransitionEnd, addListener};
};
-export {getGlobalFetchMock, signInWithTestUser, signOutTestUser, setPersonalDetails, buildPersonalDetails, buildTestReportComment, assertFormDataMatchesObject, createAddListenerMock};
+export {assertFormDataMatchesObject, buildPersonalDetails, buildTestReportComment, createAddListenerMock, getGlobalFetchMock, setPersonalDetails, signInWithTestUser, signOutTestUser};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Got it 👍
Updated
tests/utils/TestHelper.ts
Outdated
pause?: () => void; | ||
|
||
fail?: () => void; | ||
|
||
succeed?: () => void; | ||
|
||
resume?: () => Promise<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pause?: () => void; | |
fail?: () => void; | |
succeed?: () => void; | |
resume?: () => Promise<void>; | |
pause?: () => void; | |
fail?: () => void; | |
succeed?: () => void; | |
resume?: () => Promise<void>; |
tests/utils/TestHelper.ts
Outdated
const originalXhr = HttpUtils.xhr; | ||
HttpUtils.xhr = jest.fn().mockImplementation(() => { | ||
// Your mocked response object | ||
const mockedResponse = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this comment?
tests/utils/TestHelper.ts
Outdated
// Response is the same for calls to Authenticate and BeginSignIn | ||
HttpUtils.xhr = jest.fn().mockImplementation(() => { | ||
// Your mocked response object | ||
const mockedResponse = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this comment?
tests/unit/SidebarTest.ts
Outdated
const reportCollection: ReportActionCollectionDataSet = { | ||
[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report, | ||
} as ReportCollectionDataSet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const reportCollection: ReportActionCollectionDataSet = { | |
[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report, | |
} as ReportCollectionDataSet; | |
const reportCollection: ReportCollectionDataSet = { | |
[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report, | |
}; |
tests/unit/SidebarTest.ts
Outdated
.then(() => { | ||
const reportCollection: ReportCollectionDataSet = { | ||
[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`]: report, | ||
} as ReportCollectionDataSet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} as ReportCollectionDataSet; | |
}; |
# Conflicts: # tests/unit/SidebarTest.ts
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@ruben-rebelo TypeScript checks and e2e test build failing. |
# Conflicts: # tests/utils/TestHelper.js
@akinwale I've updated the PR. |
looks like jest is failing now |
@ruben-rebelo merge conflicts again 😕 and tests are failing |
# Conflicts: # src/types/onyx/ReportAction.ts
@akinwale @roryabraham all yours! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, thank you very much!
@@ -39,7 +39,7 @@ describe('Middleware', () => { | |||
}, | |||
{ | |||
command: 'AddComment', | |||
data: {authToken: 'testToken', reportID: '1234', reportActionID: '5678', reportComment: 'foo'}, | |||
data: {authToken: 'testToken', reportID: '1234', reportActionID: '5678'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing the reportComment
here? is that not needed at all in the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed for the test.
This was wrong because Report doesn't have any reportComment, with the addition of the TS on this file the type started to complain.
This was working before as we are directly testing the Middleware and we expected exactly that data.
So in my opinion this can be safely removed from the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you!
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
The tests were passing |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.58-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.58-8 🚀
|
Hey @ruben-rebelo or anyone familiar with these changes. It's not a problem, but a PR was merged that didn't recently merge main, and we now need to align the changes to prevent a failing check on main. Would you mind reviewing this PR to ensure your code, and the unaligned code is being resolved correctly? Thanks |
Details
[TS migration] Migrate multiple test to TypeScript
Fixed Issues
$ #25314
$ #25315
$ #25316
$ #25317
$ #25318
PROPOSAL: N/A
Tests
Verify that no errors appear in the JS console
Run E2E test works as before
Run Unit tests works as before
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
N/a
Android: mWeb Chrome
N/a
iOS: Native
N/a
iOS: mWeb Safari
N/a
MacOS: Chrome / Safari
N/a
MacOS: Desktop
N/a