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

Upgrading Jest to v29 and optimizing default configuration #26041

Closed
wants to merge 52 commits into from

Conversation

ymqy
Copy link
Contributor

@ymqy ymqy commented Jan 25, 2023

Summary

I am preparing to split into smaller separate pr's to make review and merge easier, look #26088

How did you test this change?

ci green

@ymqy
Copy link
Contributor Author

ymqy commented Jan 26, 2023

ReactSuspenseWithNoopRenderer-test.js caused OOM when running this test file, and after analysis, I found that these two tests are the cause of the OOM problem, More specific reasons are under further debugging analysis

  • "keeps showing an avoided parent fallback if it is already showing"
  • "do not show placeholder when mounting an avoided boundary with startTransition"

@ymqy
Copy link
Contributor Author

ymqy commented Jan 27, 2023

ReactSuspenseWithNoopRenderer-test.js caused OOM when running this test file, and after analysis, I found that these two tests are the cause of the OOM problem, More specific reasons are under further debugging analysis

  • "keeps showing an avoided parent fallback if it is already showing"
  • "do not show placeholder when mounting an avoided boundary with startTransition"

The most fundamental reason is that after the expect.toEqual method fails to execute the comparison

expect(ReactNoop.getChildren()).toEqual([span('A')]);

jest needs to print the diff information,it will perform a recursive traversal
https://github.com/facebook/jest/blob/bc84c8a15649aaaefdd624dc83824518c17467ed/packages/expect/src/matchers.ts#L638

and after executing deepCyclicCopyReplaceable(replacedReceived)
https://github.com/facebook/jest/blob/bc84c8a15649aaaefdd624dc83824518c17467ed/packages/jest-matcher-utils/src/index.ts#L410-L422

it enters the recursive loop, because the array element passed into expect contains the fiber property

image

The WeakMap in deepCyclicCopyReplaceable takes up a lot of memory until it triggers OOM.

image

The main branch code does not have this problem because the testRunner uses jasmine, an exception is thrown after executing deepCyclicCopyReplaceable(replacedReceived), the test case itself is expecting a failed result and just happens to get the passed result (but is this the expected behavior? @acdlite ). After analyzing the call stack, I found that when the testRunner is jasmine, the recursive path is some fiber node -> baseState -> cache -> controller -> Symbol(impl) -> signal -> _globalObject -> jasmine -> process - > mainModule -> some module, then it generates an exception error

image

After migrating from jasmine to circus there is no more jasmine property, it no longer generates an exception error but enters a long recursive loop until OOM.

@@ -2428,17 +2440,21 @@ describe('ReactSuspenseWithNoopRenderer', () => {
'Loading B...',
]);
// Still suspended.
expect(ReactNoop.getChildren()).toEqual([span('A')]);
expect(removeNonEnumerableProperties(ReactNoop.getChildren())).toEqual([
Copy link
Contributor Author

@ymqy ymqy Jan 27, 2023

Choose a reason for hiding this comment

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

@react-sizebot
Copy link

react-sizebot commented Jan 28, 2023

Comparing: 6b30832...d67200b

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 = 154.83 kB 154.83 kB = 49.11 kB 49.11 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 156.83 kB 156.83 kB = 49.77 kB 49.77 kB
facebook-www/ReactDOM-prod.classic.js = 533.79 kB 533.79 kB = 95.06 kB 95.06 kB
facebook-www/ReactDOM-prod.modern.js = 518.81 kB 518.81 kB = 92.83 kB 92.83 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against d67200b

@ymqy
Copy link
Contributor Author

ymqy commented Jan 28, 2023

🟢 🥳

@ymqy
Copy link
Contributor Author

ymqy commented Jan 28, 2023

@gaearon @acdlite @gnoff @eps1lon @kassens This PR is important because Jest v29 has been released and the new version contains a lot of new features and bug fix which is important for our project. It's ready for review and merge.

@gaearon
Copy link
Collaborator

gaearon commented Jan 31, 2023

Looks like CI is failing

@ymqy
Copy link
Contributor Author

ymqy commented Jan 31, 2023

Looks like CI is failing

@gaearon Because No cache is found, occasionally encountered, but it is not clear why

image

@ymqy
Copy link
Contributor Author

ymqy commented Jan 31, 2023

Looks like CI is failing

@gaearon I reset the commit d67200b and then repushed it in order to trigger the ci rerun, and now the test cases all pass again

@ymqy ymqy closed this Feb 1, 2023
@ymqy ymqy mentioned this pull request Feb 7, 2023
@ymqy ymqy deleted the feat/upgrade-jest branch February 7, 2023 15:42
@ymqy ymqy restored the feat/upgrade-jest branch February 7, 2023 15:42
@ymqy ymqy deleted the feat/upgrade-jest branch February 7, 2023 15:42
@ymqy ymqy restored the feat/upgrade-jest branch February 7, 2023 15:42
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.

None yet

5 participants