fix: mWeb - Selector - User lands on the same WS when double tapping on different one on selector. #52438
Conversation
…on different one on selector. Signed-off-by: krishna2323 <[email protected]>
|
I will work on unit test tomorrow. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
Signed-off-by: krishna2323 <[email protected]>
|
@s77rt, added the unit tests. |
|
The test unit should fail if we remove the |
Signed-off-by: krishna2323 <[email protected]>
@s77rt added a test for this case. |
|
I don't see how the latest change will help. You should render the App (and the Workspace Switcher Page) from the app source and not by defining a function in the test. The function in the test will always pass since it will always has the solution in place. |
|
Not a problem! Please use PaginationTest as a reference instead. We render the |
|
I'm working on writing the tests and will probably update them today. |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
tests/ui/WorkspaceSwitcherTest.tsx
Outdated
|
|
||
| async function navigateToWorkspaceSwitcher(): Promise<void> { | ||
| const hintText = Localize.translateLocal('workspace.switcher.headerTitle'); | ||
| const optionRow = await screen.findByTestId(hintText); |
There was a problem hiding this comment.
@s77rt, if you have a moment, could you please check why I'm getting Unable to find an element with testID: Choose a workspace here?
There was a problem hiding this comment.
The MockedSidebarLinks is missing the <TopBar />
tests/ui/WorkspaceSwitcherTest.tsx
Outdated
|
|
||
| async function navigateToWorkspaceSwitcher(): Promise<void> { | ||
| const hintText = Localize.translateLocal('workspace.switcher.headerTitle'); | ||
| const optionRow = await screen.findByTestId(hintText); |
There was a problem hiding this comment.
The MockedSidebarLinks is missing the <TopBar />
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
|
@Krishna2323 Can you please let me know when this is ready for review |
@s77rt, will try to finish this today, I added
|
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
|
I'm sorry for the delay 🙏🏻. I'm currently stuck in a loop of errors while writing tests, and it's taking longer than expected. This is the first time I'm writing tests for the UI, and I'm feeling a bit clueless about it. I've already spent many hours trying to resolve the issues. I will try to finish this within the next 3 days, or I may ask for reassignment if needed. |
import * as NativeNavigation from '@react-navigation/native';
import {act, fireEvent, render, screen} from '@testing-library/react-native';
import Onyx from 'react-native-onyx';
import * as Localize from '@libs/Localize';
import * as AppActions from '@userActions/App';
import * as User from '@userActions/User';
import App from '@src/App';
import ONYXKEYS, {OnyxKey} from '@src/ONYXKEYS';
import type {NativeNavigationMock} from '../../__mocks__/@react-navigation/native';
import * as LHNTestUtils from '../utils/LHNTestUtils';
import PusherHelper from '../utils/PusherHelper';
import * as TestHelper from '../utils/TestHelper';
import waitForBatchedUpdates from '../utils/waitForBatchedUpdates';
import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct';
// We need a large timeout here as we are lazy loading React Navigation screens and this test is running against the entire mounted App
jest.setTimeout(60000);
jest.mock('@react-navigation/native');
TestHelper.setupApp();
async function navigateToWorkspaceSwitcher(): Promise<void> {
const workspaceSwitcherButton = await screen.findByTestId('WorkspaceSwitcherButton');
fireEvent(workspaceSwitcherButton, 'press');
await act(() => {
(NativeNavigation as NativeNavigationMock).triggerTransitionEnd();
});
await waitForBatchedUpdatesWithAct();
}
async function signInAndGetApp(): Promise<void> {
// Render the App and sign in as a test user.
render(<App />);
await waitForBatchedUpdatesWithAct();
const hintText = Localize.translateLocal('loginForm.loginForm');
const loginForm = await screen.findAllByLabelText(hintText);
expect(loginForm).toHaveLength(1);
await act(async () => {
await TestHelper.signInWithTestUser();
});
await waitForBatchedUpdatesWithAct();
User.subscribeToUserEvents();
await waitForBatchedUpdates();
AppActions.setSidebarLoaded();
await waitForBatchedUpdatesWithAct();
}
describe('WorkspaceSwitcherPage', () => {
beforeEach(() => {
jest.clearAllMocks();
Onyx.clear();
// Unsubscribe to pusher channels
PusherHelper.teardown();
});
it('triggers press on workspaces list item only once', async () => {
await signInAndGetApp();
await Onyx.mergeCollection(ONYXKEYS.COLLECTION.POLICY, {
[`${ONYXKEYS.COLLECTION.POLICY}1` as const]: LHNTestUtils.getFakePolicy('1', 'Workspace A'),
[`${ONYXKEYS.COLLECTION.POLICY}2` as const]: LHNTestUtils.getFakePolicy('2', 'Workspace B'),
[`${ONYXKEYS.COLLECTION.POLICY}3` as const]: LHNTestUtils.getFakePolicy('3', 'Workspace C'),
});
await navigateToWorkspaceSwitcher();
const workspaceRow = screen.getByLabelText('Workspace A');
expect(workspaceRow).toBeOnTheScreen();
});
});Here a simplified test that you can work on top of. Basically now we need to press on Workspace A once and verify it's selected. Then press on workspace B twice and verify that B is selected. Let me know if you have any questions |
|
Will start woking on this again in a hour. Thanks @s77rt for the help 🙇🏻❤️ |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
|
@s77rt, when I try to simulate multiple presses in a quick session, the tests throw this error:
|
|
@Krishna2323 Wrap the calls in |
|
@s77rt, testing Library utilities are already wrapped in act and I get a warning when trying to do so:
|
|
@Krishna2323 Instead of trying to fire the press event twice, mock |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
|
Thanks again for the help, @s77rt 🙇🏻. The tests have been updated, and I also verified the test by removing the |
| jest.mock('@react-navigation/native', () => { | ||
| const actualNav = jest.requireActual<typeof Navigation>('@react-navigation/native'); | ||
| return { | ||
| ...actualNav, | ||
| useIsFocused: jest.fn(), | ||
| triggerTransitionEnd: jest.fn(), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Can we not use the actualNav and just return useIsFocused
There was a problem hiding this comment.
I tried removing the actualNav and we can't remove it as causes issues for other missing methods in nav.
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.74-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.74-8 🚀
|
Explanation of Change
isFocusedstate to prevent multiple clicks on an option in workspace switcher page which was leading to multiple navigation issue.Fixed Issues
$ #51402
PROPOSAL: #51402 (comment)
Tests
Expensifyis selected.Offline tests
Expensifyis selected.QA Steps
Expensifyis selected.// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4