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

fix: mock animation completion callback to trigger setState #17956

Merged
merged 3 commits into from
Apr 26, 2023
Merged

fix: mock animation completion callback to trigger setState #17956

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2807556
fix: mock animation completion callback to trigger setState
WoLewicki Apr 25, 2023
8be6a05
fix: move InteractionManager mock to dedicated file
WoLewicki Apr 26, 2023
bcbfccb
fix: restore old prop and add comment
WoLewicki Apr 26, 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 src/pages/home/sidebar/SidebarLinks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class SidebarLinks extends React.Component {

return (
<View
accessibilityElementsHidden={!this.props.isFocused}
isFocused={this.props.isFocused}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is all this necessary for mocking jest? I can't imagine it is, so it's just strange to see these changes in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accessibilityElementsHidden was used before both for checking the value in tests and probably to mock the navigation behavior of accessibility (see: https://github.com/react-navigation/react-navigation/blob/9fe34b445fcb86e5666f61e144007d7540f014fa/packages/stack/src/views/Stack/CardContainer.tsx#L242). If we still want to check if this route is focused or not, we need to keep a way of doing it. If you know a better way to do it, it would be great 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that explains why accessibilityElementsHidden is there in the first place, but I guess I am still not quite understanding why it needs to change from accessibilityElementsHidden to isFocused. We can't keep the original way, or it's not desired for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm accessibilityElementsHidden should not be needed anymore, but we can keep it. I just wanted a prop that does not do anything so I didn't destroy any of the behaviors, but since the screen is not focused when this prop is set, it should all work just fine.

accessibilityLabel={this.props.translate('sidebarScreen.listOfChats')}
style={[styles.flex1, styles.h100]}
>
Expand Down
33 changes: 19 additions & 14 deletions tests/ui/UnreadIndicatorsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ jest.setTimeout(30000);

jest.mock('../../src/libs/Notification/LocalNotification');

// we need to mock it for the ReportScreen to update its state immediately for tests to pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be sure to capitalize the first word in every comment. Also, this comment is only a partial explanation of why the mocking is necessary. Can you please improve it so that it also says why the state isn't updating immediately in the first place and why the tests fail when it doesn't update immediately?

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 just saw that we have a special file for adding the mocks to react-native, so I'll move this mock there. Should I add a comment there or maybe just in the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for moving it to the __mocks__ folder! Can you please add the comment there in the file?

jest.mock('react-native/libraries/Interaction/InteractionManager', () => {
const IM = jest.requireActual('react-native/libraries/Interaction/InteractionManager');
IM.runAfterInteractions = jest.fn(fn => fn());

return IM;
});

beforeAll(() => {
// In this test, we are generically mocking the responses of all API requests by mocking fetch() and having it
// return 200. In other tests, we might mock HttpUtils.xhr() with a more specific mock data response (which means
Expand All @@ -39,7 +47,7 @@ beforeAll(() => {
// simulate data arriving we will just set it into Onyx directly with Onyx.merge() or Onyx.set() etc.
global.fetch = TestHelper.getGlobalFetchMock();

Linking.setInitialURL('https://new.expensify.com/r');
Linking.setInitialURL('https://new.expensify.com/');
appSetup();

// Connect to Pusher
Expand Down Expand Up @@ -105,10 +113,10 @@ function navigateToSidebarOption(index) {
/**
* @return {Boolean}
*/
function isDrawerOpen() {
function areYouOnChatListScreen() {
const hintText = Localize.translateLocal('sidebarScreen.listOfChats');
const sidebarLinks = screen.queryAllByLabelText(hintText);
return !lodashGet(sidebarLinks, [0, 'props', 'accessibilityElementsHidden']);
return lodashGet(sidebarLinks, [0, 'props', 'isFocused']);
}

const REPORT_ID = '1';
Expand Down Expand Up @@ -196,7 +204,7 @@ function signInAndGetAppWithUnreadChat() {
});
}

xdescribe('Unread Indicators', () => {
describe('Unread Indicators', () => {
afterEach(() => {
jest.clearAllMocks();
Onyx.clear();
Expand All @@ -211,7 +219,7 @@ xdescribe('Unread Indicators', () => {
const sidebarLinksHintText = Localize.translateLocal('sidebarScreen.listOfChats');
const sidebarLinks = screen.queryAllByLabelText(sidebarLinksHintText);
expect(sidebarLinks).toHaveLength(1);
expect(isDrawerOpen()).toBe(true);
expect(areYouOnChatListScreen()).toBe(true);

// Verify there is only one option in the sidebar
const optionRowsHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
Expand All @@ -227,7 +235,7 @@ xdescribe('Unread Indicators', () => {
})
.then(() => {
// Verify that the report screen is rendered and the drawer is closed
expect(isDrawerOpen()).toBe(false);
expect(areYouOnChatListScreen()).toBe(false);

// That the report actions are visible along with the created action
const welcomeMessageHintText = Localize.translateLocal('accessibilityHints.chatWelcomeMessage');
Expand Down Expand Up @@ -255,14 +263,14 @@ xdescribe('Unread Indicators', () => {
// Navigate to the unread chat from the sidebar
.then(() => navigateToSidebarOption(0))
.then(() => {
expect(isDrawerOpen()).toBe(false);
expect(areYouOnChatListScreen()).toBe(false);

// Then navigate back to the sidebar
return navigateToSidebar();
})
.then(() => {
// Verify the LHN is now open
expect(isDrawerOpen()).toBe(true);
expect(areYouOnChatListScreen()).toBe(true);

// Verify that the option row in the LHN is no longer bold (since OpenReport marked it as read)
const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
Expand All @@ -277,7 +285,7 @@ xdescribe('Unread Indicators', () => {
const newMessageLineIndicatorHintText = Localize.translateLocal('accessibilityHints.newMessageLineIndicator');
const unreadIndicator = screen.queryAllByLabelText(newMessageLineIndicatorHintText);
expect(unreadIndicator).toHaveLength(0);
expect(isDrawerOpen()).toBe(false);
expect(areYouOnChatListScreen()).toBe(false);

// Scroll and verify that the new messages badge is also hidden
scrollUpToRevealNewMessagesBadge();
Expand Down Expand Up @@ -343,9 +351,6 @@ xdescribe('Unread Indicators', () => {
.then(() => {
// Verify notification was created
expect(LocalNotification.showCommentNotification).toBeCalled();

// // Navigate back to the sidebar
return navigateToSidebar();
})
.then(() => {
// // Verify the new report option appears in the LHN
Expand Down Expand Up @@ -440,7 +445,7 @@ xdescribe('Unread Indicators', () => {
it('Removes the new line indicator when a new message is created by the current user', () => signInAndGetAppWithUnreadChat()
.then(() => {
// Verify we are on the LHN and that the chat shows as unread in the LHN
expect(isDrawerOpen()).toBe(true);
expect(areYouOnChatListScreen()).toBe(true);

const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNameTexts = screen.queryAllByLabelText(hintText);
Expand Down Expand Up @@ -469,7 +474,7 @@ xdescribe('Unread Indicators', () => {
it('Keeps the new line indicator when the user moves the App to the background', () => signInAndGetAppWithUnreadChat()
.then(() => {
// Verify we are on the LHN and that the chat shows as unread in the LHN
expect(isDrawerOpen()).toBe(true);
expect(areYouOnChatListScreen()).toBe(true);

const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNameTexts = screen.queryAllByLabelText(hintText);
Expand Down