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

Fix test-build-devtools if build was generated by build-for-devtools #24088

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 13, 2022

Summary

I couldn't get yarn test-build-devtools to run for me locally even though CI passed. "Store › StrictMode compliance › should mark non strict root elements as not strict" and "Store › StrictMode compliance › should mark non strict root elements as not strict" were failing.
First I thought the difference in build variants was the issue (first commit) but this didn't fix tests locally.

The issue was that I was using yarn build-for-devtools while CI was using yarn build-combined. yarn build-combined updates ReactVersion.js which is used by Devtools to determine what bits are set in the Fiber mode if StrictMode is enabled. Since yarn build-for-devtools resulted in 17.0.3, Devtools didn't determine the correct StrictMode bits.

How did you test this change?

  • git clean -fdx; yarn; yarn build-for-devtools; yarn test-build-devtools
  • CI

@@ -131,7 +131,7 @@
"test-prod-build": "yarn test --deprecated 'yarn test --prod --build'",
"test-build": "yarn test --deprecated 'yarn test --build'",
"test-build-prod": "yarn test --deprecated 'yarn test --build --prod'",
"test-build-devtools": "yarn test --build --project devtools",
"test-build-devtools": "node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CI is using experimental

Previous command would use www-modern

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also @rickhanlonii might want to chime in since he was working on these test commands recently.

@sizebot
Copy link

sizebot commented Mar 13, 2022

Comparing: 061ac27...6f99d07

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 130.94 kB 130.94 kB = 41.92 kB 41.92 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.01 kB 136.01 kB = 43.40 kB 43.40 kB
facebook-www/ReactDOM-prod.classic.js = 435.49 kB 435.49 kB = 79.78 kB 79.78 kB
facebook-www/ReactDOM-prod.modern.js = 421.91 kB 421.91 kB = 77.76 kB 77.76 kB
facebook-www/ReactDOMForked-prod.classic.js = 435.49 kB 435.49 kB = 79.78 kB 79.78 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 6f99d07

@eps1lon eps1lon marked this pull request as ready for review March 13, 2022 09:13
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Let's let Andrew weigh in on the shared version bump just to make sure.

// TODO: This module is used both by the release scripts and to expose a version
// at runtime. We should instead inject the version number as part of the build
// process, and use the ReactVersions.js module as the single source of truth.
export default '17.0.3';
export default '18.0.0';
Copy link
Contributor

@bvaughn bvaughn Mar 15, 2022

Choose a reason for hiding this comment

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

cc @acdlite How do you feel about this change? I don't remember if this would mess up/confuse our release scripts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this should be fine

@@ -220,7 +220,7 @@ export function getInternalReactConstants(
// Keep it in sync, and add version guards if it changes.
//
// TODO Update the gt() check below to be gte() whichever the next version number is.
// Currently the version in Git is 17.0.2 (but that version has not been/may not end up being released).
// Currently the version in Git is 18.0.0 (but that version has not been/may not end up being released).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the old comment was probably fine. The main point wasn't the current version so much as "why 17.0.1 vs 17.0.0"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
// Currently the version in Git is 18.0.0 (but that version has not been/may not end up being released).
// Currently the version in Git is 17.0.2 (but that version has not been/may not end up being released).

Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
@bvaughn bvaughn merged commit 3f89908 into facebook:main Mar 23, 2022
@bvaughn bvaughn deleted the test/build-for-devtools-test branch March 23, 2022 13:52
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 24, 2022
Summary:
This sync includes the following changes:
- **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([#24088](facebook/react#24088)) //<Sebastian Silbermann>//
- **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([#24131](facebook/react#24131)) //<David McCabe>//
- **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([#24082](facebook/react#24082)) //<Andrew Clark>//
- **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([#24126](facebook/react#24126)) //<Sebastian Markbåge>//
- **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([#24109](facebook/react#24109)) //<Luna>//
- **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([#24107](facebook/react#24107)) //<salazarm>//
- **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([#24026](facebook/react#24026)) //<Luna Ruan>//
- **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([#23366](facebook/react#23366)) //<Luna>//
- **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([#24081](facebook/react#24081)) //<Andrew Clark>//
- **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>//
- **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>//
- **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>//
- **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([#24068](facebook/react#24068)) //<Josh Story>//
- **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([#24047](facebook/react#24047)) //<Sebastian Markbåge>//
- **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([#24055](facebook/react#24055)) //<Sebastian Markbåge>//
- **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([#24054](facebook/react#24054)) //<Sebastian Markbåge>//
- **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([#23244](facebook/react#23244)) //<salazarm>//
- **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([#24038](facebook/react#24038)) //<Sebastian Markbåge>//
- **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([#24037](facebook/react#24037)) //<Sebastian Markbåge>//
- **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([#24034](facebook/react#24034)) //<Josh Story>//
- **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([#24030](facebook/react#24030)) //<Sebastian Markbåge>//
- **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([#24025](facebook/react#24025)) //<Sebastian Markbåge>//
- **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([#24024](facebook/react#24024)) //<salazarm>//
- **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([#23386](facebook/react#23386)) //<Joshua Gross>//
- **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([#23195](facebook/react#23195)) //<BIKI DAS>//
- **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([#23393](facebook/react#23393)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 1780659...1159ff6

jest_e2e[run_all_tests]

Reviewed By: lunaleaps

Differential Revision: D34928167

fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
rickhanlonii pushed a commit that referenced this pull request Apr 13, 2022
…24088)

Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This sync includes the following changes:
- **[3f8990898](facebook/react@3f8990898 )**: Fix test-build-devtools if build was generated by build-for-devtools ([facebook#24088](facebook/react#24088)) //<Sebastian Silbermann>//
- **[577f2de46](facebook/react@577f2de46 )**: enableCacheElement flag ([facebook#24131](facebook/react#24131)) //<David McCabe>//
- **[2e0d86d22](facebook/react@2e0d86d22 )**: Allow updating dehydrated root at lower priority without forcing client render ([facebook#24082](facebook/react#24082)) //<Andrew Clark>//
- **[dbe9e732a](facebook/react@dbe9e732a )**: Avoid conditions where control flow is sufficient ([facebook#24126](facebook/react#24126)) //<Sebastian Markbåge>//
- **[b075f9742](facebook/react@b075f9742 )**: Fix dispatch config type for skipBubbling ([facebook#24109](facebook/react#24109)) //<Luna>//
- **[ef23a9ee8](facebook/react@ef23a9ee8 )**: Flag for text hydration mismatch ([facebook#24107](facebook/react#24107)) //<salazarm>//
- **[0412f0c1a](facebook/react@0412f0c1a )**: add offscreen state node ([facebook#24026](facebook/react#24026)) //<Luna Ruan>//
- **[43eb28339](facebook/react@43eb28339 )**: Add skipBubbling property to dispatch config ([facebook#23366](facebook/react#23366)) //<Luna>//
- **[832e2987e](facebook/react@832e2987e )**: Revert accdientally merged PR ([facebook#24081](facebook/react#24081)) //<Andrew Clark>//
- **[02b65fd8c](facebook/react@02b65fd8c )**: Allow updates at lower pri without forcing client render //<Andrew Clark>//
- **[83b941a51](facebook/react@83b941a51 )**: Add isRootDehydrated function //<Andrew Clark>//
- **[c8e4789e2](facebook/react@c8e4789e2 )**: Pass children to hydration root constructor //<Andrew Clark>//
- **[581f0c42e](facebook/react@581f0c42e )**: [Flight] add support for Lazy components in Flight server ([facebook#24068](facebook/react#24068)) //<Josh Story>//
- **[72a933d28](facebook/react@72a933d28 )**: Gate legacy hidden ([facebook#24047](facebook/react#24047)) //<Sebastian Markbåge>//
- **[b9de50d2f](facebook/react@b9de50d2f )**: Update test to reset modules instead of using private state ([facebook#24055](facebook/react#24055)) //<Sebastian Markbåge>//
- **[c91892ec3](facebook/react@c91892ec3 )**: [Fizz] Don't flush empty segments ([facebook#24054](facebook/react#24054)) //<Sebastian Markbåge>//
- **[d5f1b067c](facebook/react@d5f1b067c )**: [ServerContext] Flight support for ServerContext ([facebook#23244](facebook/react#23244)) //<salazarm>//
- **[6edd55a3f](facebook/react@6edd55a3f )**: Gate unstable_expectedLoadTime on enableCPUSuspense ([facebook#24038](facebook/react#24038)) //<Sebastian Markbåge>//
- **[57799b912](facebook/react@57799b912 )**: Add more feature flag checks ([facebook#24037](facebook/react#24037)) //<Sebastian Markbåge>//
- **[e09518e5b](facebook/react@e09518e5b )**: [Fizz] write chunks to a buffer with no re-use ([facebook#24034](facebook/react#24034)) //<Josh Story>//
- **[14c2be8da](facebook/react@14c2be8da )**: Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes ([facebook#24030](facebook/react#24030)) //<Sebastian Markbåge>//
- **[cb1e7b1c6](facebook/react@cb1e7b1c6 )**: Move onCompleteAll to .allReady Promise ([facebook#24025](facebook/react#24025)) //<Sebastian Markbåge>//
- **[566285761](facebook/react@566285761 )**: [Fizz] Export debug function for FB ([facebook#24024](facebook/react#24024)) //<salazarm>//
- **[05c283c3c](facebook/react@05c283c3c )**: Fabric HostComponent as EventEmitter: support add/removeEventListener (unstable only) ([facebook#23386](facebook/react#23386)) //<Joshua Gross>//
- **[08644348b](facebook/react@08644348b )**: Added unit Tests in the ReactART, increasing the code coverage ([facebook#23195](facebook/react#23195)) //<BIKI DAS>//
- **[feefe437f](facebook/react@feefe437f )**: Refactor Cache Code ([facebook#23393](facebook/react#23393)) //<Luna Ruan>//

Changelog:
[General][Changed] - React Native sync for revisions 1780659...1159ff6

jest_e2e[run_all_tests]

Reviewed By: lunaleaps

Differential Revision: D34928167

fbshipit-source-id: 8c386f2be5871981d217ab9a514892ed88eafcfb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants