[NoQA] Optimise isActiveRouteTests#75604
Conversation
melvin-bot
Bot
requested review from
situchan
and removed request for
a team
melvin-bot
Bot
requested review from
trjExpensify
and removed request for
a team
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
Please fix failing checks |
|
Updated |
Codecov Report✅ All modified and coverable lines are covered by tests. |
| * Removes redundant slashes and query params from a route path. | ||
| */ | ||
| function cleanRoutePath(routePath: string): string { | ||
| return routePath.replace(CONST.REGEX.ROUTES.REDUNDANT_SLASHES, (match, p1) => (p1 ? '/' : '')).replace(/\?.*/, ''); |
There was a problem hiding this comment.
replaceAll is changed to replace.
Can you explain this change?
There was a problem hiding this comment.
I'd like to keep original logic and regex to avoid any unexpected regressions.
There was a problem hiding this comment.
CONST.REGEX.ROUTES.REDUNDANT_SLASHES already has the g flag, so replace still checks every match, and the query remover (replace(/\?.*/, '')) only needs to drop the first ? since everything after it is query params (there's only going to be a single ?).
If we use replaceAll, the tests give this: TypeError: String.prototype.replaceAll called with a non-global RegExp argument because Jest runs in Node 20 without polyfills. replace supports ES5 so using it we can solve this issue.
There was a problem hiding this comment.
If we use replaceAll, the tests give this: TypeError: String.prototype.replaceAll called with a non-global RegExp argument
Have you ever seen failing tests because of this? If so, can you share link?
There was a problem hiding this comment.
Please check #67425. We're going to deprecate replace in favor of replaceAll
There was a problem hiding this comment.
The purpose of this PR is just to fix flaky test. Let's not change original logic in non-test files.
There was a problem hiding this comment.
Have you ever seen failing tests because of this? If so, can you share link?
It can be checked locally. With the changes done here, if you change replace to replaceAll, the tests would fail.
The purpose of this PR is just to fix flaky test. Let's not change original logic in non-test files.
Makes sense, but the proposal was to extract the logic from Navigation.isActiveRoute into a helper (src/libs/Navigation/helpers/isRouteActive.ts). That lets us exercise the logic without using React Navigation or rendering components.
Before we extracted cleanRoutePath into helpers/isRouteActive.ts, the only way to exercise that logic in tests was through the full Navigation integration test (older code in tests/navigation/isActiveRouteTests.tsx). That test rendered TestNavigationContainer, used actual React Navigation state, and then called Navigation.isActiveRoute. Because everything flowed through Navigation.ts, replaceAll was polyfilled. But that would not work for the test if we are trying to skip the rendering of TestNavigationContainer.
Do you have any suggestion on how this should be handled?
CC: @mountiny
There was a problem hiding this comment.
I think we should not be changing the logic here unless its wrong, only tests should be changed
| const ref = navigationRef as typeof navigationRef & {current: {getCurrentRoute: () => {name: string}} | null}; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-assignment | ||
| ref.current = {getCurrentRoute: jest.fn().mockReturnValue({name: 'test'})} as any; |
There was a problem hiding this comment.
Can't this be set directly to navigationRef.current without introducing ref variable?
I don't like this typecast - as any
| jest.spyOn(navigationRef, 'getRootState').mockReturnValue({} as unknown as ReturnType<typeof navigationRef.getRootState>); | ||
| jest.spyOn(navigationRef, 'isReady').mockReturnValue(true); |
There was a problem hiding this comment.
I think we can remove these in favor of jest.mock('@libs/Navigation/navigationRef');
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
job 8 failure is not related |
situchan
left a comment
situchan
left a comment
There was a problem hiding this comment.
run 20 times locally and all pass less than 3s
|
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
That jest test should be unrelated as @situchan mentioned |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.67-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.70-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.70-0 🚀
|
5 participants
Explanation of Change
Optimise isActiveRouteTests
Fixed Issues
$ #74380
PROPOSAL: #74380 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop