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.

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

[jest] Explicitly separate mocked native modules from mocked JS modules #24809

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented May 10, 2019

Summary

This commit more clearly defines the mocks RN sets up and uses paths instead of Haste names to define the mocks. The Jest setup script defined mocks for native modules (Obj-C, Java) and mocks for JS modules in the same data structure. This meant that some non-native modules (that is, JS modules) were in the mockNativeModules map -- this commit splits them out and mocks them in typical jest.mock fashion.

Additionally, the setup script used to mock the modules using the Haste names. As one of the steps toward migrating to standard path-based imports, the setup script now mocks JS modules using paths (native modules don't need a Haste name nor path since they are just entries in NativeModules). This gets us closer to being able to remove hasteImpl. (Tracking in #24772.)

Also, this commit removes mocks that are not referenced anywhere in the RN and React repositories (grepped for the names and found no entries outside of the Jest setup scripts).

Changelog

[General] [Changed] - Explicitly separate mocked native modules from mocked JS modules

Test Plan

Test plan: Ran all unit tests to ensure they pass. Additionally copied the RN setup script into jest-expo, which tests some more complex mocking setups with view manager and all tests passed without any modification to jest-expo.

@ide ide requested a review from cpojer May 10, 2019 19:42
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Expo Partner: Expo Partner labels May 10, 2019
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thanks! Let's see what happens with this one when I try to ship it at FB, I may have to patch some things up.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

This commit more clearly defines the mocks RN sets up and uses paths instead of Haste names to define the mocks. The Jest setup script defined mocks for native modules (Obj-C, Java) and mocks for JS modules in the same data structure. This meant that some non-native modules (that is, JS modules) were in the `mockNativeModules` map -- this commit splits them out and mocks them in typical `jest.mock` fashion.

Additionally, the setup script used to mock the modules using the Haste names. As one of the steps toward migrating to standard path-based imports, the setup script now mocks JS modules using paths (native modules don't need a Haste name nor path since they are just entries in `NativeModules`). This gets us closer to being able to remove `hasteImpl`.

Also, this commit removes mocks that are not referenced anywhere in the RN and React repositories (grepped for the names and found no entries outside of the Jest setup scripts).

Test plan: Ran all unit tests to ensure they pass. Additionally copied the RN setup script into `jest-expo`, which tests some more complex mocking setups with view manager and all tests passed without any modification to `jest-expo`.
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.


AssetRegistry = require('../AssetRegistry');
resolveAssetSource = require('../resolveAssetSource');
NativeSourceCode = require('../../NativeModules/specs/NativeSourceCode').default;

Choose a reason for hiding this comment

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

prettier/prettier: Insert ⏎······

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @ide in 1ed3525.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 31, 2019
vovkasm pushed a commit to vovkasm/react-native that referenced this pull request Aug 7, 2019
…ebook#24809)

Summary:
This commit more clearly defines the mocks RN sets up and uses paths instead of Haste names to define the mocks. The Jest setup script defined mocks for native modules (Obj-C, Java) and mocks for JS modules in the same data structure. This meant that some non-native modules (that is, JS modules) were in the `mockNativeModules` map -- this commit splits them out and mocks them in typical `jest.mock` fashion.

Additionally, the setup script used to mock the modules using the Haste names. As one of the steps toward migrating to standard path-based imports, the setup script now mocks JS modules using paths (native modules don't need a Haste name nor path since they are just entries in `NativeModules`). This gets us closer to being able to remove `hasteImpl`. (Tracking in facebook#24772.)

Also, this commit removes mocks that are not referenced anywhere in the RN and React repositories (grepped for the names and found no entries outside of the Jest setup scripts).

[General] [Changed] - Explicitly separate mocked native modules from mocked JS modules
Pull Request resolved: facebook#24809

Differential Revision: D15316882

Pulled By: cpojer

fbshipit-source-id: 039e4e320121bea9580196fe0a091b8b1e8b41bf
(cherry picked from commit 1ed3525)
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
…ebook#24809)

Summary:
This commit more clearly defines the mocks RN sets up and uses paths instead of Haste names to define the mocks. The Jest setup script defined mocks for native modules (Obj-C, Java) and mocks for JS modules in the same data structure. This meant that some non-native modules (that is, JS modules) were in the `mockNativeModules` map -- this commit splits them out and mocks them in typical `jest.mock` fashion.

Additionally, the setup script used to mock the modules using the Haste names. As one of the steps toward migrating to standard path-based imports, the setup script now mocks JS modules using paths (native modules don't need a Haste name nor path since they are just entries in `NativeModules`). This gets us closer to being able to remove `hasteImpl`. (Tracking in facebook#24772.)

Also, this commit removes mocks that are not referenced anywhere in the RN and React repositories (grepped for the names and found no entries outside of the Jest setup scripts).

## Changelog

[General] [Changed] - Explicitly separate mocked native modules from mocked JS modules
Pull Request resolved: facebook#24809

Differential Revision: D15316882

Pulled By: cpojer

fbshipit-source-id: 039e4e320121bea9580196fe0a091b8b1e8b41bf
@ide ide deleted the jest-mocks branch April 25, 2020 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Expo Partner: Expo Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants