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

Re-enable InspectorProxy* Jest tests #42682

Closed
wants to merge 3 commits into from

Conversation

robhogan
Copy link
Contributor

Summary:
Since D53125777, these should be safe to re-enable

Changelog:
[Internal] Re-enable dev-middleware InspectorProxy* tests

Differential Revision: D53125778

@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 Jan 26, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D53125778

…hers (facebook#42681)

Summary:

CI failures in Windows JS tests recently (facebook#41463) were caused by the triggering of Babel registration during tests, due to an import of `packages/dev-middleware` (index), breaking subsequent transformation of other tests.

## Root cause
Example of a problematic import:
https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/__tests__/ServerUtils.js#L15

..which triggers a Babel registration:
https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/index.js#L16-L18

That registration behaves differently on Windows due to the `ignore: [/\/node_modules\/\]`, which doesn't match against Windows path separators - Babel matches against system separators.

In particular, this changed whether `node_modules/flow-parser` was transformed when loading the RN Babel transformer. Transforming this file causes a `console.warn` from Babel due to its size:
> [BABEL] Note: The code generator has deoptimised the styling of /Users/robhogan/workspace/react-native/node_modules/flow-parser/flow_parser.js as it exceeds the max of 500KB.

This throws due to our setup:
https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/react-native/jest/local-setup.js#L27

This all manifests as the first test following a Babel registration (within the same Jest worker) that requires the RN Babel transformer throwing during script transformation.

## This change
This is the minimally disruptive change that makes Babel registration behaviour consistent between Windows and other platforms. The more durable solution here would be *not* to rely on any Babel registration for Jest, which has its own `ScriptTransformer` mechanism for running code from source. Given the fragile way our internal+OSS Babel set up hangs together that's a higher-risk change, so I'll follow up separately.

Changelog: [Internal]

Reviewed By: huntie

Differential Revision: D53124578
…cebook#42690)

Summary:

Issues triggered by `InspectorProxy*` tests under `packages/dev-middleware` (T169943794) can be root-caused to `dev-middleware` performing Babel registration within a test run, after Jest has hooked its own transformer.

Babel registration is only required when running this code (`dev-middleware`, etc) directly from source - we already have the `BUILD_EXCLUDE_BABEL_REGISTER` mechanism to strip it out from production builds, but we currently don't prevent registration under tests, where Jest's transformer should be allowed to do its work.

This adds the same `babel-plugin-transform-define` mechanism that we use for production builds to the Jest transformer.

Changelog:
[Internal] Prevent inadvertent Babel registration during running of repo tests

Reviewed By: huntie

Differential Revision: D53125777
Summary:

Since D53125777, these should be safe to re-enable

Changelog:
[Internal] Re-enable dev-middleware InspectorProxy* tests

Reviewed By: huntie

Differential Revision: D53125778
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D53125778

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jan 27, 2024
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in cfc0ba0.

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. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants