-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Fix lazy import error from jest and Appearance.js #47629
Conversation
we can either mock https://github.com/facebook/react-native/blob/9a60038a40e16925ea1adeb3e3c937c22a615485/packages/react-native/Libraries/Utilities/Appearance.js. but that feels more complex to mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks :)
@huntie has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thanks for the fix. Can you also add a Jest unit test that would fail without this fix? |
This pull request was successfully merged by @Kudo in ce838a4 When will my fix make it into a release? | How to file a pick request? |
@yungsters will try that in a separate pr. thanks! |
Summary: add a jest test to test when `useColorScheme` is not mocked. following up #47629 (comment) ## Changelog: [GENERAL] [ADDED] - Add useColorScheme mock test Pull Request resolved: #47988 Test Plan: ci passed Reviewed By: javache Differential Revision: D66573172 Pulled By: blakef fbshipit-source-id: 820227f6fc4e18a968b3181fad8f534a716f1e9e
Summary:
currently running jest test, it shows an error:
it is a regression from #46123 that to have a lazy require.
this pr tries to mock
useColorScheme
to returnlight
. i think we don't necessarily test the color scheme changes in jest runtime. originallyuseColorScheme
also returnslight
because of this statementChangelog:
[GENERAL] [FIXED] - Fixed jest error from Appearance.js
Test Plan: