[No QA] Add expo-location mock to jest setup#85257
Conversation
Co-authored-by: Tim Golen <tgolen@users.noreply.github.com>
melvin-bot
bot
requested review from
ShridharGoel
and removed request for
a team
|
@ShridharGoel Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a020baa2ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| requestForegroundPermissionsAsync: jest.fn(() => Promise.resolve({status: 'granted', granted: true, canAskAgain: true, expires: 0})), | ||
| requestBackgroundPermissionsAsync: jest.fn(() => Promise.resolve({status: 'granted', granted: true, canAskAgain: true, expires: 0})), | ||
| getForegroundPermissionsAsync: jest.fn(() => Promise.resolve({status: 'granted', granted: true, canAskAgain: true, expires: 0})), | ||
| getBackgroundPermissionsAsync: jest.fn(() => Promise.resolve({status: 'granted', granted: true, canAskAgain: true, expires: 0})), |
There was a problem hiding this comment.
The mocked expo-location permission responses are missing the Android accuracy field, so Android code paths that require precise location are treated as not granted even when status: 'granted'. In src/pages/iou/request/step/IOURequestStepDistanceGPS/BackgroundLocationPermissionsFlow/index.android.tsx, both checkPermissions() and requestForegroundPermissions() require android?.accuracy === 'fine'; with this mock they always take the non-granted branch, which can misroute Android Jest tests into deny/ask-for-permissions flows and hide regressions in the true granted path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@tgolen NAB but thoughts on why this shouldn't be done? We can have a global value for android.accuracy and specific tests can override it if they need to change it.
There was a problem hiding this comment.
I don't think the suggestion makes any sense. Tests don't run in android, so supporting the accuracy field doesn't make sense to me. Am I thinking about that wrong?
There was a problem hiding this comment.
accuracy field is used here: https://github.com/Expensify/App/blob/main/src/pages/iou/request/step/IOURequestStepDistanceGPS/BackgroundLocationPermissionsFlow/index.android.tsx
Though it seems there's no corresponding tests for the above, as of now.
There was a problem hiding this comment.
Ok, I get what you mean. The UI tests that we have don't run on Android.
So it is fine then, we can ignore it.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
ShridharGoel
left a comment
ShridharGoel
left a comment
There was a problem hiding this comment.
Just this comment to be discussed
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #85243 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.3.39-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.39-3 🚀
|
5 participants
Explanation of Change
The
jest-expopreset replaces all native module methods withjest.fn(async () => {}), which resolves toundefinedinstead of a properPermissionResponseobject. WhengetCurrentPosition/index.tscallsrequestForegroundPermissionsAsync()and then reads.statusfrom the result, it crashes withTypeError: Cannot read properties of undefined (reading 'status').This adds a project-level mock for
expo-locationinjest/setup.tsthat provides proper return values for all permission and location functions used in the codebase. This is the same pattern already used for other expo modules likeexpo-task-manager.Fixed Issues
$ #85243
Tests
npx jest tests/ui/components/IOURequestStepConfirmationPageTest.tsx --no-cacheTypeError: Cannot read properties of undefined (reading 'status')npx jest tests/ui/IOURequestStepScanTest.tsx --no-cacheOffline tests
N/A — this change only affects test mocks, not runtime behavior.
QA Steps
[No QA] — this is a test infrastructure fix only. No production code is changed.
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))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
N/A — test mock change only
Android: mWeb Chrome
N/A — test mock change only
iOS: Native
N/A — test mock change only
iOS: mWeb Safari
N/A — test mock change only
MacOS: Chrome / Safari
N/A — test mock change only