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.

Sign up for GitHub

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 '[Remaining]' test to TypeScript #36867

Merged
merged 28 commits into from
Mar 18, 2024
Merged

[NO QA][TS migration] Migrate '[Remaining]' test to TypeScript #36867

Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
33b1400
wip migrate ViolationUtilsTest to ts
HezekielT Feb 17, 2024
dbe4d7c
finished migrating ViolationUtilsTest to ts
HezekielT Feb 17, 2024
9d10ab7
migrate UserUtilsTest to ts
HezekielT Feb 17, 2024
0bfbaee
migrate PhoneNumberTest to ts
HezekielT Feb 17, 2024
cb7e261
wip migrate SearchPage.perf-test to ts
HezekielT Feb 17, 2024
8d354d1
created type SearchPageProps
HezekielT Feb 17, 2024
4d4628c
created type for createAddListenerMock()
HezekielT Feb 17, 2024
6ff1321
wip migrate OptionsSelector.perf-test to ts
HezekielT Feb 18, 2024
6cae028
used Awaited utility type to fix Promise related issue
HezekielT Feb 18, 2024
7b5f839
code cleanup
HezekielT Feb 19, 2024
46506f0
migrate PolicyTest to ts
HezekielT Feb 19, 2024
2d930dc
used Array.prototype.map correctly
HezekielT Feb 19, 2024
59071f1
removed unnecessary changes
HezekielT Feb 19, 2024
3cc250c
run prettier
HezekielT Feb 20, 2024
b0a88ed
run prettier
HezekielT Mar 4, 2024
dff82ff
added return type for jest mock function
HezekielT Mar 4, 2024
54deb99
added ts-expect-error
HezekielT Mar 4, 2024
093e674
added ts-expect-error
HezekielT Mar 4, 2024
c79782d
fix ts issues
HezekielT Mar 9, 2024
0c050cc
remove registerStorageEventListener
HezekielT Mar 9, 2024
f82a45b
small fix
HezekielT Mar 9, 2024
f5e2c6a
run prettier
HezekielT Mar 9, 2024
8deff43
Update tests/perf-test/SearchPage.perf-test.tsx
HezekielT Mar 15, 2024
b914a43
code changes based on review
HezekielT Mar 15, 2024
ac4284e
Merge branch 'main' into migrate/36135
HezekielT Mar 15, 2024
d2c5348
fix ts error
HezekielT Mar 15, 2024
599adad
updated SearchPage props
HezekielT Mar 15, 2024
aaad266
remove unwanted check
HezekielT Mar 15, 2024
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 src/types/onyx/PolicyCategory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ type PolicyCategory = OnyxCommon.OnyxValueWithOfflineFeedback<{

/** "General Ledger code" that corresponds to this category in an accounting system. Similar to an ID. */
// eslint-disable-next-line @typescript-eslint/naming-convention
'GL Code': string;
'GL Code'?: string;

/** An ID for this category from an external accounting system */
externalID: string;
Expand Down
2 changes: 1 addition & 1 deletion src/types/onyx/PolicyTag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ type PolicyTag = {

/** "General Ledger code" that corresponds to this tag in an accounting system. Similar to an ID. */
// eslint-disable-next-line @typescript-eslint/naming-convention
'GL Code': string;
'GL Code'?: string;

/** A list of errors keyed by microtime */
errors?: OnyxCommon.Errors | null;
Expand Down
86 changes: 47 additions & 39 deletions tests/actions/PolicyTest.js → tests/actions/PolicyTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import _ from 'lodash';
import Onyx from 'react-native-onyx';
import type {OnyxCollection} from 'react-native-onyx';
import CONST from '@src/CONST';
import OnyxUpdateManager from '../../src/libs/actions/OnyxUpdateManager';
import * as Policy from '../../src/libs/actions/Policy';
import ONYXKEYS from '../../src/ONYXKEYS';
import OnyxUpdateManager from '@src/libs/actions/OnyxUpdateManager';
import * as Policy from '@src/libs/actions/Policy';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PolicyMembers, Policy as PolicyType, Report, ReportAction, ReportActions} from '@src/types/onyx';
import * as TestHelper from '../utils/TestHelper';
import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';

Expand All @@ -20,12 +21,14 @@ describe('actions/Policy', () => {
});

beforeEach(() => {
// @ts-expect-error TODO: Remove this once TestHelper (https://github.com/Expensify/App/issues/25318) is migrated to TypeScript.
global.fetch = TestHelper.getGlobalFetchMock();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't put TODOs in the code. If we have an issue, that's already good enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included the TODO part because of the guidelines

Suppress a TypeScript error stemming from the bug with @ts-expect-error. Create a separate GH issue. Prefix the issue title with [TS ERROR #]. Cross-link the migration PR and the created GH issue. On the same line as @ts-expect-error, put down the GH issue number prefixed with TODO:.
The @ts-expect-error annotation tells the TS compiler to ignore any errors in the line that follows it. However, if there's no error in the line, TypeScript will also raise an error.

Link

Copy link
Contributor

Choose a reason for hiding this comment

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

@hayata-suenaga any reason we need TODOs in the code, or are GH issues sufficient? I ask cause we normally don't put TODOs in code (see SO), and this guideline is kinda contradicts that. In any case, @HezekielT don't block on this, and sorry for the confusion

Copy link
Contributor

@blazejkustra blazejkustra Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@cead22 We were at a point where every issue was blocked by some other and without @ts-expect-error we would be stuck or move very slowly. That's why we decided to allow TODOs in the codebase, the good part is that once TestHelper gets migrated here, this line will throw an error so these TODOs will disappear over time (once we migrate blocking files).

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @blazejkustra for the explanation!

Copy link
Contributor

@cead22 cead22 Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the context. I'm still curious, why do we need TODOs when we have GH issues? Don't the GH issues serve as TODOs? Could we link the lines of codes in the issues, instead of linking the issues in the code?

return Onyx.clear().then(waitForBatchedUpdates);
});

describe('createWorkspace', () => {
it('creates a new workspace', async () => {
// @ts-expect-error TODO: Remove this once TestHelper (https://github.com/Expensify/App/issues/25318) is migrated to TypeScript.
fetch.pause();
Onyx.set(ONYXKEYS.SESSION, {email: ESH_EMAIL, accountID: ESH_ACCOUNT_ID});
await waitForBatchedUpdates();
Expand All @@ -38,7 +41,7 @@ describe('actions/Policy', () => {
Policy.createWorkspace(ESH_EMAIL, true, WORKSPACE_NAME, policyID);
await waitForBatchedUpdates();

let policy = await new Promise((resolve) => {
let policy: OnyxCollection<PolicyType> = await new Promise((resolve) => {
const connectionID = Onyx.connect({
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
waitForCollectionCallback: true,
Expand All @@ -50,15 +53,15 @@ describe('actions/Policy', () => {
});

// check if policy was created with correct values
expect(policy.id).toBe(policyID);
expect(policy.name).toBe(WORKSPACE_NAME);
expect(policy.type).toBe(CONST.POLICY.TYPE.FREE);
expect(policy.role).toBe(CONST.POLICY.ROLE.ADMIN);
expect(policy.owner).toBe(ESH_EMAIL);
expect(policy.isPolicyExpenseChatEnabled).toBe(true);
expect(policy.pendingAction).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);

const policyMembers = await new Promise((resolve) => {
expect(policy?.id).toBe(policyID);
expect(policy?.name).toBe(WORKSPACE_NAME);
expect(policy?.type).toBe(CONST.POLICY.TYPE.FREE);
expect(policy?.role).toBe(CONST.POLICY.ROLE.ADMIN);
expect(policy?.owner).toBe(ESH_EMAIL);
expect(policy?.isPolicyExpenseChatEnabled).toBe(true);
expect(policy?.pendingAction).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);

const policyMembers: OnyxCollection<PolicyMembers> = await new Promise((resolve) => {
const connectionID = Onyx.connect({
key: `${ONYXKEYS.COLLECTION.POLICY_MEMBERS}${policyID}`,
waitForCollectionCallback: true,
Expand All @@ -70,9 +73,9 @@ describe('actions/Policy', () => {
});

// check if the user was added as an admin to the policy
expect(policyMembers[ESH_ACCOUNT_ID].role).toBe(CONST.POLICY.ROLE.ADMIN);
expect(policyMembers?.[ESH_ACCOUNT_ID]?.role).toBe(CONST.POLICY.ROLE.ADMIN);

let allReports = await new Promise((resolve) => {
let allReports: OnyxCollection<Report> = await new Promise((resolve) => {
const connectionID = Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
Expand All @@ -84,12 +87,12 @@ describe('actions/Policy', () => {
});

// Three reports should be created: #announce, #admins and expense report
const workspaceReports = _.filter(allReports, (report) => report.policyID === policyID);
expect(_.size(workspaceReports)).toBe(3);
_.forEach(workspaceReports, (report) => {
expect(report.pendingFields.addWorkspaceRoom).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);
expect(report.participantAccountIDs).toEqual([ESH_ACCOUNT_ID]);
switch (report.chatType) {
const workspaceReports = Object.values(allReports ?? {}).filter((report) => report?.policyID === policyID);
expect(workspaceReports.length).toBe(3);
HezekielT marked this conversation as resolved.
Show resolved Hide resolved
workspaceReports.forEach((report) => {
expect(report?.pendingFields?.addWorkspaceRoom).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);
expect(report?.participantAccountIDs).toEqual([ESH_ACCOUNT_ID]);
switch (report?.chatType) {
case CONST.REPORT.CHAT_TYPE.POLICY_ADMINS: {
HezekielT marked this conversation as resolved.
Show resolved Hide resolved
adminReportID = report.reportID;
break;
Expand All @@ -107,7 +110,7 @@ describe('actions/Policy', () => {
}
});

let reportActions = await new Promise((resolve) => {
let reportActions: OnyxCollection<ReportActions> = await new Promise((resolve) => {
const connectionID = Onyx.connect({
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
waitForCollectionCallback: true,
Expand All @@ -119,20 +122,21 @@ describe('actions/Policy', () => {
});

// Each of the three reports should have a a `CREATED` action.
let adminReportActions = _.values(reportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${adminReportID}`]);
let announceReportActions = _.values(reportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceReportID}`]);
let expenseReportActions = _.values(reportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReportID}`]);
let workspaceReportActions = _.concat(adminReportActions, announceReportActions, expenseReportActions);
_.forEach([adminReportActions, announceReportActions, expenseReportActions], (actions) => {
expect(_.size(actions)).toBe(1);
let adminReportActions: ReportAction[] = Object.values(reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${adminReportID}`] ?? {});
let announceReportActions: ReportAction[] = Object.values(reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceReportID}`] ?? {});
let expenseReportActions: ReportAction[] = Object.values(reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReportID}`] ?? {});
let workspaceReportActions: ReportAction[] = adminReportActions.concat(announceReportActions, expenseReportActions);
[adminReportActions, announceReportActions, expenseReportActions].forEach((actions) => {
expect(actions.length).toBe(1);
});
_.forEach([...adminReportActions, ...announceReportActions, ...expenseReportActions], (reportAction) => {
HezekielT marked this conversation as resolved.
Show resolved Hide resolved
[...adminReportActions, ...announceReportActions, ...expenseReportActions].forEach((reportAction) => {
expect(reportAction.actionName).toBe(CONST.REPORT.ACTIONS.TYPE.CREATED);
expect(reportAction.pendingAction).toBe(CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD);
expect(reportAction.actorAccountID).toBe(ESH_ACCOUNT_ID);
});

// Check for success data
// @ts-expect-error TODO: Remove this once TestHelper (https://github.com/Expensify/App/issues/25318) is migrated to TypeScript.
fetch.resume();
await waitForBatchedUpdates();

Expand All @@ -148,7 +152,7 @@ describe('actions/Policy', () => {
});

// Check if the policy pending action was cleared
expect(policy.pendingAction).toBeFalsy();
expect(policy?.pendingAction).toBeFalsy();

allReports = await new Promise((resolve) => {
const connectionID = Onyx.connect({
Expand All @@ -162,9 +166,13 @@ describe('actions/Policy', () => {
});

// Check if the report pending action and fields were cleared
_.forEach(allReports, (report) => {
expect(report.pendingAction).toBeFalsy();
expect(report.pendingFields.addWorkspaceRoom).toBeFalsy();
Object.values(allReports ?? {}).forEach((report) => {
if (typeof report === 'undefined') {
return;
}

expect(report?.pendingAction).toBeFalsy();
HezekielT marked this conversation as resolved.
Show resolved Hide resolved
expect(report?.pendingFields?.addWorkspaceRoom).toBeFalsy();
});
HezekielT marked this conversation as resolved.
Show resolved Hide resolved

reportActions = await new Promise((resolve) => {
Expand All @@ -179,11 +187,11 @@ describe('actions/Policy', () => {
});

// Check if the report action pending action was cleared
adminReportActions = _.values(reportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${adminReportID}`]);
announceReportActions = _.values(reportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceReportID}`]);
expenseReportActions = _.values(reportActions[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReportID}`]);
workspaceReportActions = _.concat(adminReportActions, announceReportActions, expenseReportActions);
_.forEach(workspaceReportActions, (reportAction) => {
adminReportActions = Object.values(reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${adminReportID}`] ?? {});
announceReportActions = Object.values(reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${announceReportID}`] ?? {});
expenseReportActions = Object.values(reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${expenseReportID}`] ?? {});
workspaceReportActions = adminReportActions.concat(announceReportActions, expenseReportActions);
workspaceReportActions.forEach((reportAction) => {
expect(reportAction.pendingAction).toBeFalsy();
});
});
Expand Down
45 changes: 25 additions & 20 deletions tests/perf-test/OptionsSelector.perf-test.js → ...s/perf-test/OptionsSelector.perf-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import {fireEvent} from '@testing-library/react-native';
import type {RenderResult} from '@testing-library/react-native';
import React from 'react';
import type {ComponentType} from 'react';
import {measurePerformance} from 'reassure';
import _ from 'underscore';
import type {WithLocalizeProps} from '@components/withLocalize';
import type {WithNavigationFocusProps} from '@components/withNavigationFocus';
import OptionsSelector from '@src/components/OptionsSelector';
import variables from '@src/styles/variables';

jest.mock('../../src/components/withLocalize', () => (Component) => {
function WrappedComponent(props) {
jest.mock('@src/components/withLocalize', () => (Component: ComponentType<WithLocalizeProps>) => {
function WrappedComponent(props: WithLocalizeProps) {
return (
<Component
// eslint-disable-next-line react/jsx-props-no-spreading
Expand All @@ -19,8 +22,8 @@ jest.mock('../../src/components/withLocalize', () => (Component) => {
return WrappedComponent;
});

jest.mock('../../src/components/withNavigationFocus', () => (Component) => {
function WithNavigationFocus(props) {
jest.mock('@src/components/withNavigationFocus', () => (Component: ComponentType<WithNavigationFocusProps>) => {
function WithNavigationFocus(props: WithNavigationFocusProps) {
return (
<Component
// eslint-disable-next-line react/jsx-props-no-spreading
Expand All @@ -35,25 +38,27 @@ jest.mock('../../src/components/withNavigationFocus', () => (Component) => {
return WithNavigationFocus;
});

const generateSections = (sectionConfigs) =>
_.map(sectionConfigs, ({numItems, indexOffset, shouldShow = true}) => ({
data: Array.from({length: numItems}, (_v, i) => ({
type GenerateSectionsProps = Array<{numberOfItems: number; indexOffset: number; shouldShow?: boolean}>;

const generateSections = (sections: GenerateSectionsProps) =>
sections.map(({numberOfItems, indexOffset, shouldShow = true}) => ({
data: Array.from({length: numberOfItems}, (v, i) => ({
text: `Item ${i + indexOffset}`,
keyForList: `item-${i + indexOffset}`,
})),
indexOffset,
shouldShow,
}));

const singleSectionSConfig = [{numItems: 1000, indexOffset: 0}];
const singleSectionsConfig = [{numberOfItems: 1000, indexOffset: 0}];

const mutlipleSectionsConfig = [
{numItems: 1000, indexOffset: 0},
{numItems: 100, indexOffset: 70},
{numberOfItems: 1000, indexOffset: 0},
{numberOfItems: 100, indexOffset: 70},
];

// @ts-expect-error TODO: Remove this once OptionsSelector is migrated to TypeScript.
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have any issue to track this @cead22 ? If not, should we be creating one?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have this one but it's closed #25125. Maybe we need a new one for src/components/OptionsSelector/index.android.js and src/components/OptionsSelector/index.js

function OptionsSelectorWrapper(args) {
const sections = generateSections(singleSectionSConfig);
const sections = generateSections(singleSectionsConfig);
return (
<OptionsSelector
value="test"
Expand All @@ -65,12 +70,12 @@ function OptionsSelectorWrapper(args) {
}
HezekielT marked this conversation as resolved.
Show resolved Hide resolved

test('[OptionsSelector] should render text input with interactions', () => {
const scenario = (screen) => {
const scenario = ((screen: RenderResult) => {
const textInput = screen.getByTestId('options-selector-input');
fireEvent.changeText(textInput, 'test');
fireEvent.changeText(textInput, 'test2');
fireEvent.changeText(textInput, 'test3');
};
}) as Awaited<(screen: RenderResult) => Promise<void>>;

measurePerformance(<OptionsSelectorWrapper />, {scenario});
});
Expand All @@ -85,22 +90,22 @@ test('[OptionsSelector] should render multiple sections', () => {
});

test('[OptionsSelector] should press a list items', () => {
const scenario = (screen) => {
const scenario = ((screen: RenderResult) => {
fireEvent.press(screen.getByText('Item 1'));
fireEvent.press(screen.getByText('Item 5'));
fireEvent.press(screen.getByText('Item 10'));
};
}) as Awaited<(screen: RenderResult) => Promise<void>>;

measurePerformance(<OptionsSelectorWrapper />, {scenario});
});

test('[OptionsSelector] should scroll and press few items', () => {
const sections = generateSections(mutlipleSectionsConfig);

const generateEventData = (numOptions, optionRowHeight) => ({
const generateEventData = (numberOfOptions: number, optionRowHeight: number) => ({
nativeEvent: {
contentOffset: {
y: optionRowHeight * numOptions,
y: optionRowHeight * numberOfOptions,
},
contentSize: {
height: optionRowHeight * 10,
Expand All @@ -115,7 +120,7 @@ test('[OptionsSelector] should scroll and press few items', () => {

const eventData = generateEventData(100, variables.optionRowHeight);
const eventData2 = generateEventData(200, variables.optionRowHeight);
const scenario = async (screen) => {
const scenario = async (screen: RenderResult) => {
fireEvent.press(screen.getByText('Item 10'));
fireEvent.scroll(screen.getByTestId('options-list'), eventData);
fireEvent.press(await screen.findByText('Item 100'));
Expand Down
Loading
Loading