Skip to content

Add tests for out-of-tree platform support#20932

Closed
empyrical wants to merge 4 commits intofacebook:masterfrom
empyrical:haste-tests
Closed

Add tests for out-of-tree platform support#20932
empyrical wants to merge 4 commits intofacebook:masterfrom
empyrical:haste-tests

Conversation

@empyrical
Copy link
Contributor

@empyrical empyrical commented Aug 31, 2018

This PR adds tests for out-of-tree platform support.

A new test has been added to hasteImpl-test.js which both verifies the ability of findPlugins to find out of tree platforms, and the ability of hasteImpl to add relevant whitelists and name reducers for out of tree platforms.

Test Plan:

A test has been added to hasteImpl.js's test, so you can try it out by running yarn test. I've added an e2e test for running the bundler on this "dummy" platform.

Release Notes:

[CLI] [ENHANCEMENT] [jest/hasteImpl.js] The hasteImpl is now aware of whether it's in React Native CI, and will change where it looks for "plugins" accordingly
[CLI] [ENHANCEMENT] [jest/tests/hasteImpl-test.js] hasteImpl.js' plugin support is tested
[INTERNAL] [ENHANCEMENT] [ContainerShip/scripts/run-ci-e2e-tests.sh] Bundling the "dummy" platform is now part of the E2E test
[INTERNAL] [ENHANCEMENT] [package.json] The "dummy" platform has been added as a devDependency

@empyrical empyrical requested a review from hramos as a code owner August 31, 2018 02:02
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 31, 2018
@pull-bot
Copy link

pull-bot commented Aug 31, 2018

Warnings
⚠️

🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

⚠️

📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

@react-native-bot react-native-bot added ✅Test Plan Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Aug 31, 2018
@empyrical
Copy link
Contributor Author

Yep, still the same issue: (test_objc)

error: bundling failed: Error: jest-haste-map: @providesModule naming collision:
  Duplicate module name: react-native-dummy
  Paths: /Users/distiller/react-native/RNTester/js/OutOfTreeTestPlatform/package.json collides with /Users/distiller/react-native/node_modules/react-native-dummy/package.json

This error is caused by a @providesModule declaration with the same name across two different files.
    at setModule (/Users/distiller/react-native/node_modules/jest-haste-map/build/index.js:462:17)
    at workerReply (/Users/distiller/react-native/node_modules/jest-haste-map/build/index.js:512:9)
    at process._tickCallback (internal/process/next_tick.js:68:7)
::1 - - [31/Aug/2018:02:11:31 +0000] "GET /IntegrationTests/IntegrationTestsApp.bundle?platform=ios&dev=true HTTP/1.1" 200 - "-" "RNTester/1 CFNetwork/901.1 Darwin/17.4.0"
error: bundling failed: Error: jest-haste-map: @providesModule naming collision:
  Duplicate module name: react-native-dummy
  Paths: /Users/distiller/react-native/RNTester/js/OutOfTreeTestPlatform/package.json collides with /Users/distiller/react-native/node_modules/react-native-dummy/package.json

This error is caused by a @providesModule declaration with the same name across two different files.
    at setModule (/Users/distiller/react-native/node_modules/jest-haste-map/build/index.js:462:17)
    at workerReply (/Users/distiller/react-native/node_modules/jest-haste-map/build/index.js:512:9)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Will have to come up with a way to fix this.

@empyrical
Copy link
Contributor Author

The dummy platform has been moved to its own module/repo, and tests look much better now!

https://github.com/react-native-community/react-native-dummy
https://www.npmjs.com/package/react-native-dummy

@empyrical empyrical changed the title [WIP] Add tests for out-of-tree platform support Add tests for out-of-tree platform support Sep 11, 2018
@matthargett
Copy link
Contributor

test_android failure is unrelated. again.

No build file at ReactAndroid/src/main/java/com/facebook/thecount/BUCK when resolving target //ReactAndroid/src/main/java/com/facebook/thecount:thecount.

This error happened while trying to get dependency '//ReactAndroid/src/main/java/com/facebook/thecount:thecount' of target '//ReactAndroid/src/main/java/com/facebook/react/bridge:bridge'

@empyrical
Copy link
Contributor Author

Rebased against master and things look better 👌

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 13, 2018
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.

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

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Sep 14, 2018
@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@empyrical
Copy link
Contributor Author

Not sure what it got caught on

@hramos
Copy link
Contributor

hramos commented Sep 18, 2018

Re-trying.

@hramos
Copy link
Contributor

hramos commented Sep 19, 2018

@empyrical the internal diff is failing JavaScript tests:

 FAIL  jest/__tests__/hasteImpl-test.js
  ✕ returns the correct haste name for a file with an out-of-tree platform suffix (34ms)
  ○ skipped 6 tests

  ● returns the correct haste name for a file with an out-of-tree platform suffix

    expect(received).toEqual(expected)

    Expected value to equal:
      "AccessibilityInfo"
    Received:
      "AccessibilityInfo.dummy"

      58 |         ),
      59 |       ),
    > 60 |     ).toEqual('AccessibilityInfo');
         |     ^
      61 |   }
      62 | });
      63 |

      at Object.<anonymous> (jest/__tests__/hasteImpl-test.js:60:5)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 6 skipped, 7 total
Snapshots:   0 total
Time:        0.518s, estimated 1s
Ran all test suites related to changed files.

@empyrical
Copy link
Contributor Author

Very weird that it is working in OSS but not internally. Since react-native-dummy is a new dependency, do you have internal checks/approvals that need to be done to allow new dependencies through?

@empyrical
Copy link
Contributor Author

Or -- would adding the changed yarn.lock to this PR help?

@hramos
Copy link
Contributor

hramos commented Sep 19, 2018

I did have to take care of that internally, since we have an offline yarn mirror.

Let me re-import, as the internal diff might be out of date.

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.

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

@hramos
Copy link
Contributor

hramos commented Sep 20, 2018

I am still working on this. Will update soon.

@hramos hramos added ✅Release Notes and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Sep 20, 2018
@hramos
Copy link
Contributor

hramos commented Sep 20, 2018

I'm at a loss. Jest is still failing the new test case when run internally:

 ● returns the correct haste name for a file with an out-of-tree platform suffix

    expect(received).toEqual(expected)

    Expected value to equal:
      "AccessibilityInfo"
    Received:
      "AccessibilityInfo.dummy"

      58 |         ),
      59 |       ),
    > 60 |     ).toEqual('AccessibilityInfo');
         |     ^
      61 |   }
      62 | });
      63 | 

      at Object.<anonymous> (jest/__tests__/hasteImpl-test.js:60:5)

@empyrical
Copy link
Contributor Author

So, the reason that test would fail is because findPlugins isn't able to see the react-native-dummy platform for some reason.

const REACT_NATIVE_CI = process.cwd() === path.resolve(__dirname, '..');
let pluginsPath;
if (REACT_NATIVE_CI) {
pluginsPath = '..';
} else {
pluginsPath = '../../../';
}
const plugins = findPlugins([path.resolve(__dirname, pluginsPath)]);

In hasteImpl, I've set it to look in ../node_modules if it detects that it is being ran as part of React Native's tests, rather than ../../../node_modules where it's inside of a react native project. It does this detection by checking if the cwd() is the parent directory. It is possible that cwd() may be different somehow for the internal tests.

If you were to have a console.log or something there to print the value of the REACT_NATIVE_CI variable, does it show up as true or false? If it's false that may be the issue and I'd have to come up with a different way to detect if it's in CI.

@hramos
Copy link
Contributor

hramos commented Sep 20, 2018

A-ha! We're using Yarn Plug-n-Play, so node_modules is not a thing in the environment these tests are running in.

I'm aware of an alternate way to detect if a test is running within our internal CI. I am not sure if it's something that would be OK to include in open source, but regardless, it looks like we'd need to solve not having node_modules available in the internal CI.

@empyrical
Copy link
Contributor Author

The way that findPlugins currently works is it doesn't use require.resolve or anything like that - it concats the input directory + node_modules + {module_name} + package.json.

if (isReactNativePlugin(pkg)) {
const pkgJson = readPackage(path.join(folder, 'node_modules', pkg));
if (pkgJson) {
commands = commands.concat(findPluginsInReactNativePackage(pkgJson));
platforms = platforms.concat(findPlatformsInPackage(pkgJson));
findHasteConfigInPackageAndConcat(pkgJson, haste);
}
}

It looks like RNPM will need to be adapted to work with Yarn Plug 'n Play. For now, perhaps this test could somehow be skipped on the internal test suite until YPNP is made public?

@empyrical
Copy link
Contributor Author

Actually - perhaps I could modify the test to skip itself if the node_modules folder isn't detected.

@empyrical
Copy link
Contributor Author

@hramos I modified the hasteImpl test to skip if node_modules/react-native-dummy isn't present. Will this pass in the internal test suite?

@hramos
Copy link
Contributor

hramos commented Sep 26, 2018

Sorry for the delay. I just incorporated your updates into the internal diff. Will merge if tests pass. Thanks!

@react-native-bot
Copy link
Collaborator

@empyrical merged commit 57041ee into facebook:master.


Once this commit is added to a release, you will see the corresponding version tag below the description at 57041ee. If the commit has a single master tag, it is not yet part of a release.

@facebook facebook locked as resolved and limited conversation to collaborators Sep 27, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 27, 2018
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
This PR is a WIP for adding tests for out-of-tree platform support. [I originally had issues](facebook#20825 (comment)) with this, so I want to give it a try in a separate pull request. None of these issues appear on my machine while running these tests as of this rebase - if everything seems okay on CircleCI after this rebase against `master`, I will ditch the [WIP] tag. Otherwise, I will see if I can find a way to make this work.

The bunch of JS files that will give this a "Large PR" tag are in `RNTester/js/OutOfTreeTestPlatform` - they are only used by the bundler and not executed at any point in time. So if another file needs to be added when React Native's module structure changes, you do not need to have a functional JS file in there as a stub. `module.exports` could be `null` if you wanted. I just had copied over stubs from `Libraries` because I wanted a non-trivial haste module map to be in the test.
Pull Request resolved: facebook#20932

Reviewed By: axe-fb

Differential Revision: D9818112

Pulled By: hramos

fbshipit-source-id: 0b53359b84430fdefb972587c95d19f85773c5fa
@facebook-github-bot facebook-github-bot added the Contributor A React Native contributor. label Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants