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

[NOQA] remove redundant performance tests #51461

Merged
merged 4 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ We use Reassure for monitoring performance regression. It helps us check if our
- Reassure builds on the existing React Testing Library setup and adds a performance measurement functionality. It's intended to be used on local machine and on a remote server as part of your continuous integration setup.
- To make sure the results are reliable and consistent, Reassure runs tests twice – once for the current branch and once for the base branch.

## Performance Testing Strategy (`measurePerformance`)
## Performance Testing Strategy (`measureRenders`)

- Before adding new tests, check if the proposed scenario or component is already covered in existing tests. Duplicate tests can slow down the CI suite, making it harder to spot meaningful regressions.
- Review the current test suite to ensure your test scenario or component isn’t already being tested elsewhere.
mountiny marked this conversation as resolved.
Show resolved Hide resolved
- Test only scenarios that cover new or unique interactions. Avoid testing repetitive user actions that could be captured within a single, comprehensive scenario.
- Where applicable, use utility functions and helper methods to consolidate common actions (e.g., data mocking, scenario setup) across tests. This reduces redundancy and allows tests to be more focused and reusable.
- The primary focus is on testing business cases rather than small, reusable parts that typically don't introduce regressions, although some tests in that area are still necessary.
- To achieve this goal, it's recommended to stay relatively high up in the React tree, targeting whole screens to recreate real-life scenarios that users may encounter.
- For example, consider scenarios where an additional `useMemo` call could impact performance negatively.
Expand Down Expand Up @@ -84,7 +88,7 @@ test('Count increments on press', async () => {
await screen.findByText('Count: 2');
};

await measurePerformance(
await measureRenders(
<Counter />,
{ scenario, runs: 20 }
);
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"ios-build": "bundle exec fastlane ios build_unsigned",
"android-build": "bundle exec fastlane android build_local",
"test": "TZ=utc NODE_OPTIONS=--experimental-vm-modules jest",
"perf-test": "NODE_OPTIONS=--experimental-vm-modules npx reassure",
"typecheck": "NODE_OPTIONS=--max_old_space_size=8192 tsc",
"lint": "NODE_OPTIONS=--max_old_space_size=8192 eslint . --max-warnings=0 --cache --cache-location=node_modules/.cache/eslint",
"lint-changed": "NODE_OPTIONS=--max_old_space_size=8192 eslint --max-warnings=0 --config ./.eslintrc.changed.js $(git diff --diff-filter=AM --name-only origin/main HEAD -- \"*.ts\" \"*.tsx\")",
Expand Down
81 changes: 44 additions & 37 deletions src/components/PopoverMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable react/jsx-props-no-spreading */
import lodashIsEqual from 'lodash/isEqual';
import type {RefObject} from 'react';
import React, {Fragment, useLayoutEffect, useState} from 'react';
import type {ReactNode, RefObject} from 'react';
import React, {useLayoutEffect, useState} from 'react';
import {StyleSheet, View} from 'react-native';
import type {StyleProp, TextStyle, ViewStyle} from 'react-native';
import type {ModalProps} from 'react-native-modal';
Expand Down Expand Up @@ -129,6 +129,14 @@ type PopoverMenuProps = Partial<PopoverModalProps> & {
shouldUpdateFocusedIndex?: boolean;
};

const renderWithConditionalWrapper = (shouldUseScrollView: boolean, contentContainerStyle: StyleProp<ViewStyle>, children: ReactNode): React.JSX.Element => {
if (shouldUseScrollView) {
return <ScrollView contentContainerStyle={contentContainerStyle}>{children}</ScrollView>;
}
// eslint-disable-next-line react/jsx-no-useless-fragment
return <>{children}</>;
};

function PopoverMenu({
menuItems,
onItemSelected,
Expand Down Expand Up @@ -171,7 +179,6 @@ function PopoverMenu({
const {windowHeight} = useWindowDimensions();

const [focusedIndex, setFocusedIndex] = useArrowKeyFocusManager({initialFocusedIndex: currentMenuItemsFocusedIndex, maxIndex: currentMenuItems.length - 1, isActive: isVisible});
const WrapComponent = shouldUseScrollView ? ScrollView : Fragment;

const selectItem = (index: number) => {
const selectedItem = currentMenuItems.at(index);
Expand Down Expand Up @@ -235,6 +242,39 @@ function PopoverMenu({
);
};

const renderedMenuItems = currentMenuItems.map((item, menuIndex) => {
const {text, onSelected, subMenuItems, shouldCallAfterModalHide, ...menuItemProps} = item;
return (
<OfflineWithFeedback
// eslint-disable-next-line react/no-array-index-key
key={`${item.text}_${menuIndex}`}
pendingAction={item.pendingAction}
>
<FocusableMenuItem
// eslint-disable-next-line react/no-array-index-key
key={`${item.text}_${menuIndex}`}
title={text}
onPress={() => selectItem(menuIndex)}
focused={focusedIndex === menuIndex}
shouldShowSelectedItemCheck={shouldShowSelectedItemCheck}
shouldCheckActionAllowedOnPress={false}
onFocus={() => {
if (!shouldUpdateFocusedIndex) {
return;
}
setFocusedIndex(menuIndex);
}}
style={{backgroundColor: item.isSelected ? theme.activeComponentBG : undefined}}
wrapperStyle={StyleUtils.getItemBackgroundColorStyle(!!item.isSelected, focusedIndex === menuIndex, theme.activeComponentBG, theme.hoverComponentBG)}
shouldRemoveHoverBackground={item.isSelected}
titleStyle={StyleSheet.flatten([styles.flex1, item.titleStyle])}
// Spread other props dynamically
{...menuItemProps}
/>
</OfflineWithFeedback>
);
});

const renderHeaderText = () => {
if (!headerText || enteredSubMenuIndexes.length !== 0) {
return;
Expand Down Expand Up @@ -298,40 +338,7 @@ function PopoverMenu({
<View style={[isSmallScreenWidth ? {maxHeight: windowHeight - 250} : styles.createMenuContainer, containerStyles]}>
{renderHeaderText()}
{enteredSubMenuIndexes.length > 0 && renderBackButtonItem()}
{/** eslint-disable-next-line react/jsx-props-no-spreading */}
<WrapComponent {...(shouldUseScrollView && {contentContainerStyle: scrollContainerStyle})}>
{currentMenuItems.map((item, menuIndex) => {
const {text, onSelected, subMenuItems, shouldCallAfterModalHide, ...menuItemProps} = item;
return (
<OfflineWithFeedback
// eslint-disable-next-line react/no-array-index-key
key={`${item.text}_${menuIndex}`}
pendingAction={item.pendingAction}
>
<FocusableMenuItem
// eslint-disable-next-line react/no-array-index-key
key={`${item.text}_${menuIndex}`}
title={text}
onPress={() => selectItem(menuIndex)}
focused={focusedIndex === menuIndex}
shouldShowSelectedItemCheck={shouldShowSelectedItemCheck}
shouldCheckActionAllowedOnPress={false}
onFocus={() => {
if (!shouldUpdateFocusedIndex) {
return;
}
setFocusedIndex(menuIndex);
}}
wrapperStyle={StyleUtils.getItemBackgroundColorStyle(!!item.isSelected, focusedIndex === menuIndex, theme.activeComponentBG, theme.hoverComponentBG)}
shouldRemoveHoverBackground={item.isSelected}
titleStyle={StyleSheet.flatten([styles.flex1, item.titleStyle])}
// eslint-disable-next-line react/jsx-props-no-spreading
{...menuItemProps}
/>
</OfflineWithFeedback>
);
})}
</WrapComponent>
{renderWithConditionalWrapper(shouldUseScrollView, scrollContainerStyle, renderedMenuItems)}
</View>
</FocusTrapForModal>
</PopoverWithMeasuredContent>
Expand Down
81 changes: 4 additions & 77 deletions tests/perf-test/ReportActionCompose.perf-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -113,52 +113,13 @@ test('[ReportActionCompose] should render Composer with text input interactions'
await measureRenders(<ReportActionComposeWrapper />, {scenario});
});

test('[ReportActionCompose] should scroll to hide suggestions', async () => {
test('[ReportActionCompose] should press create button', async () => {
const scenario = async () => {
// Query for the composer
const composer = await screen.findByTestId('composer');

// scroll to hide suggestions
fireEvent.scroll(composer);
};

await waitForBatchedUpdates();
await measureRenders(<ReportActionComposeWrapper />, {scenario});
});

test('[ReportActionCompose] should press to block suggestions', async () => {
const scenario = async () => {
// Query for the composer
const composer = await screen.findByTestId('composer');

// press to block suggestions
fireEvent.press(composer);
};

await waitForBatchedUpdates();
await measureRenders(<ReportActionComposeWrapper />, {scenario});
});

test('[ReportActionCompose] should press add attachemnt button', async () => {
const scenario = async () => {
// Query for the attachment button
// Query for the create button
const hintAttachmentButtonText = Localize.translateLocal('common.create');
const attachmentButton = await screen.findByLabelText(hintAttachmentButtonText);

fireEvent.press(attachmentButton, mockEvent);
};

await waitForBatchedUpdates();
await measureRenders(<ReportActionComposeWrapper />, {scenario});
});

test('[ReportActionCompose] should press add emoji button', async () => {
const scenario = async () => {
// Query for the emoji button
const hintEmojiButtonText = Localize.translateLocal('reportActionCompose.emoji');
const emojiButton = await screen.findByLabelText(hintEmojiButtonText);
const createButton = await screen.findByLabelText(hintAttachmentButtonText);

fireEvent.press(emojiButton);
fireEvent.press(createButton, mockEvent);
};

await waitForBatchedUpdates();
Expand All @@ -177,37 +138,3 @@ test('[ReportActionCompose] should press send message button', async () => {
await waitForBatchedUpdates();
await measureRenders(<ReportActionComposeWrapper />, {scenario});
});

test('[ReportActionCompose] press add attachment button', async () => {
const scenario = async () => {
const hintAddAttachmentButtonText = Localize.translateLocal('reportActionCompose.addAttachment');

const addAttachmentButton = await screen.findByLabelText(hintAddAttachmentButtonText);
fireEvent.press(addAttachmentButton, mockEvent);
};

await waitForBatchedUpdates();
await measureRenders(<ReportActionComposeWrapper />, {scenario});
});

test('[ReportActionCompose] should press split bill button', async () => {
const scenario = async () => {
const hintSplitBillButtonText = Localize.translateLocal('iou.splitExpense');
const splitBillButton = await screen.findByLabelText(hintSplitBillButtonText);
fireEvent.press(splitBillButton, mockEvent);
};

await waitForBatchedUpdates();
await measureRenders(<ReportActionComposeWrapper />, {scenario});
});

test('[ReportActionCompose] should press assign task button', async () => {
const scenario = async () => {
const hintAssignTaskButtonText = Localize.translateLocal('newTaskPage.assignTask');
const assignTaskButton = await screen.findByLabelText(hintAssignTaskButtonText);
fireEvent.press(assignTaskButton, mockEvent);
};

await waitForBatchedUpdates();
await measureRenders(<ReportActionComposeWrapper />, {scenario});
});
51 changes: 1 addition & 50 deletions tests/perf-test/ReportActionsList.perf-test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {fireEvent, screen} from '@testing-library/react-native';
import {screen} from '@testing-library/react-native';
import type {ComponentType} from 'react';
import Onyx from 'react-native-onyx';
import {measureRenders} from 'reassure';
Expand All @@ -7,12 +7,10 @@ import type Navigation from '@libs/Navigation/Navigation';
import ComposeProviders from '@src/components/ComposeProviders';
import {LocaleContextProvider} from '@src/components/LocaleContextProvider';
import OnyxProvider from '@src/components/OnyxProvider';
import * as Localize from '@src/libs/Localize';
import ONYXKEYS from '@src/ONYXKEYS';
import ReportActionsList from '@src/pages/home/report/ReportActionsList';
import {ReportAttachmentsProvider} from '@src/pages/home/report/ReportAttachmentsContext';
import {ActionListContext, ReactionListContext} from '@src/pages/home/ReportScreenContext';
import variables from '@src/styles/variables';
import type {PersonalDetailsList} from '@src/types/onyx';
import createRandomReportAction from '../utils/collections/reportActions';
import * as LHNTestUtilsModule from '../utils/LHNTestUtils';
Expand Down Expand Up @@ -117,50 +115,3 @@ test('[ReportActionsList] should render ReportActionsList with 500 reportActions

await measureRenders(<ReportActionsListWrapper />, {scenario});
});

test('[ReportActionsList] should render list items', async () => {
const scenario = async () => {
const hintText = Localize.translateLocal('accessibilityHints.chatMessage');
await screen.findAllByLabelText(hintText);
};

await waitForBatchedUpdates();

Onyx.multiSet({
[ONYXKEYS.PERSONAL_DETAILS_LIST]: LHNTestUtilsModule.fakePersonalDetails,
});

await measureRenders(<ReportActionsListWrapper />, {scenario});
});

test('[ReportActionsList] should scroll through list of items', async () => {
const eventData = {
nativeEvent: {
contentOffset: {
y: variables.optionRowHeight * 5,
},
contentSize: {
// Dimensions of the scrollable content
height: variables.optionRowHeight * 10,
width: 100,
},
layoutMeasurement: {
// Dimensions of the device
height: variables.optionRowHeight * 5,
width: 100,
},
},
};

const scenario = async () => {
const reportActionsList = await screen.findByTestId('report-actions-list');
fireEvent.scroll(reportActionsList, eventData);
};
await waitForBatchedUpdates();

Onyx.multiSet({
[ONYXKEYS.PERSONAL_DETAILS_LIST]: LHNTestUtilsModule.fakePersonalDetails,
});

await measureRenders(<ReportActionsListWrapper />, {scenario});
});
Loading
Loading