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

test: Don't retry flushActWork if flushUntilNextPaint threw #26121

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Feb 7, 2023

Summary

Fixes "ReferenceError: You are trying to access a property or method of the Jest environment after it has been torn down." in ReactIncrementalErrorHandling-test.internal.js

Alternatives:

  1. Additional await act(cb) call where cb makes sure we can flush until next paint without throwing
    // Ensure test isn't exited with pending work
    await act(async () => {
      root.render(<App shouldThrow={false} />);
    });
  2. Use toFlushAndThrow
    -    let error;
    -    try {
    -      await act(async () => {
    -        root.render(<App shouldThrow={true} />);
    -      });
    -    } catch (e) {
    -      error = e;
    -    }
    +    root.render(<App shouldThrow={true} />);
     
    -    expect(error.message).toBe('Oops!');
    +    expect(Scheduler).toFlushAndThrow('Oops!');
         expect(numberOfThrows < 100).toBe(true);

But then it still wouldn't make sense to pass resolve and reject to the next flushActWork. Even if the next flushActWork would flush until next paint without throwing, we couldn't resolve or reject because we already did reject.

How did you test this change?

  • yarn test --watch packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js produces no more errors after the test finishes.

@eps1lon eps1lon requested a review from acdlite February 7, 2023 16:29
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 7, 2023
@react-sizebot
Copy link

react-sizebot commented Feb 7, 2023

Comparing: 1445acf...d3668f4

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.84 kB 154.84 kB = 49.12 kB 49.12 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 156.85 kB 156.85 kB = 49.78 kB 49.78 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.82 kB 92.82 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/jest-react/cjs/jest-react.production.min.js +0.29% 2.42 kB 2.43 kB +0.34% 1.17 kB 1.18 kB
oss-stable-semver/jest-react/cjs/jest-react.production.min.js +0.29% 2.42 kB 2.43 kB +0.34% 1.17 kB 1.18 kB
oss-stable/jest-react/cjs/jest-react.production.min.js +0.29% 2.42 kB 2.43 kB +0.34% 1.17 kB 1.18 kB

Generated by 🚫 dangerJS against d3668f4

Fixes "ReferenceError: You are trying to access a property or method of the Jest environment after it has been torn down."
in `ReactIncrementalErrorHandling-test.internal.js`
@eps1lon eps1lon merged commit 86c8c8d into facebook:main Feb 13, 2023
@eps1lon eps1lon deleted the test/internalAct-no-retry branch February 13, 2023 20:46
github-actions bot pushed a commit that referenced this pull request Feb 13, 2023
## Summary

Fixes "ReferenceError: You are trying to access a property or method of
the Jest environment after it has been torn down." in
`ReactIncrementalErrorHandling-test.internal.js`

Alternatives:

1. Additional `await act(cb)` call where `cb` makes sure we can flush
until next paint without throwing
    ```js
    // Ensure test isn't exited with pending work
    await act(async () => {
      root.render(<App shouldThrow={false} />);
    });
    ```
1. Use `toFlushAndThrow`
    ```diff
    -    let error;
    -    try {
    -      await act(async () => {
    -        root.render(<App shouldThrow={true} />);
    -      });
    -    } catch (e) {
    -      error = e;
    -    }
    +    root.render(<App shouldThrow={true} />);

    -    expect(error.message).toBe('Oops!');
    +    expect(Scheduler).toFlushAndThrow('Oops!');
         expect(numberOfThrows < 100).toBe(true);
    ```

But then it still wouldn't make sense to pass `resolve` and `reject` to
the next `flushActWork`. Even if the next `flushActWork` would flush
until next paint without throwing, we couldn't resolve or reject because
we already did reject.

## How did you test this change?

- `yarn test --watch
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js`
produces no more errors after the test finishes.

DiffTrain build for [86c8c8d](86c8c8d)
[View git log for this commit](https://github.com/facebook/react/commits/86c8c8db7938319027132eeea81c9b7e28938f9d)
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 15, 2023
Summary:
This sync includes the following changes:
- **[86c8c8db7](facebook/react@86c8c8db7 )**: test: Don't retry flushActWork if flushUntilNextPaint threw ([#26121](facebook/react#26121)) //<Sebastian Silbermann>//
- **[64acd3918](facebook/react@64acd3918 )**: remove unguarded getRootNode call ([#26152](facebook/react#26152)) //<Josh Story>//
- **[71cace4d3](facebook/react@71cace4d3 )**: Migrate testRunner from jasmine2 to jest-circus ([#26144](facebook/react#26144)) //<Ming Ye>//
- **[c8510227c](facebook/react@c8510227c )**: Treat displayName as undefined ([#26148](facebook/react#26148)) //<Sebastian Markbåge>//
- **[55542bc73](facebook/react@55542bc73 )**: Update jest printBasicPrototype config ([#26142](facebook/react#26142)) //<Ming Ye>//
- **[6396b6641](facebook/react@6396b6641 )**: Model Float on Hoistables semantics ([#26106](facebook/react#26106)) //<Josh Story>//
- **[ef9f6e77b](facebook/react@ef9f6e77b )**: Enable passing Server References from Server to Client ([#26124](facebook/react#26124)) //<Sebastian Markbåge>//
- **[35698311d](facebook/react@35698311d )**: Update jest escapeString config ([#26140](facebook/react#26140)) //<Ming Ye>//
- **[6ddcbd4f9](facebook/react@6ddcbd4f9 )**: [flow] enable LTI inference mode ([#26104](facebook/react#26104)) //<Jan Kassens>//
- **[53b1f69ba](facebook/react@53b1f69ba )**: Implement unstable_getBoundingClientRect in RN Fabric refs ([#26137](facebook/react#26137)) //<Rubén Norte>//
- **[594093496](facebook/react@594093496 )**: Update to Jest 29 ([#26088](facebook/react#26088)) //<Ming Ye>//
- **[28fcae062](facebook/react@28fcae062 )**: Add support for SVG `transformOrigin` prop ([#26130](facebook/react#26130)) //<Aravind D>//
- **[3ff1540e9](facebook/react@3ff1540e9 )**: Prefer JSX in ReactNoop assertions (to combat out-of-memory test runs) ([#26127](facebook/react#26127)) //<Sebastian Silbermann>//
- **[01a0c4e12](facebook/react@01a0c4e12 )**: Add Edge Server Builds for workerd / edge-light ([#26116](facebook/react#26116)) //<Sebastian Markbåge>//
- **[f0cf832e1](facebook/react@f0cf832e1 )**: Update Flight Fixture to "use client" instead of .client.js ([#26118](facebook/react#26118)) //<Sebastian Markbåge>//
- **[03a216070](facebook/react@03a216070 )**: Rename "dom" fork to "dom-node" and "bun" fork to "dom-bun" ([#26117](facebook/react#26117)) //<Sebastian Markbåge>//
- **[4bf2113a1](facebook/react@4bf2113a1 )**: Revert "Move the Webpack manifest config to one level deeper ([#26083](facebook/react#26083))"  ([#26111](facebook/react#26111)) //<Sebastian Markbåge>//
- **[2ef24145e](facebook/react@2ef24145e )**: [flow] upgrade to 0.199.0 ([#26096](facebook/react#26096)) //<Jan Kassens>//
- **[922dd7ba5](facebook/react@922dd7ba5 )**: Revert the outer module object to an object ([#26093](facebook/react#26093)) //<Sebastian Markbåge>//
- **[9d111ffdf](facebook/react@9d111ffdf )**: Serialize Promises through Flight ([#26086](facebook/react#26086)) //<Sebastian Markbåge>//
- **[0ba4698c7](facebook/react@0ba4698c7 )**: Fix async test in React reconciler ([#26087](facebook/react#26087)) //<Ming Ye>//
- **[8c234c0de](facebook/react@8c234c0de )**: Move the Webpack manifest config to one level deeper ([#26083](facebook/react#26083)) //<Sebastian Markbåge>//
- **[977bccd24](facebook/react@977bccd24 )**: Refactor Flight Encoding ([#26082](facebook/react#26082)) //<Sebastian Markbåge>//
- **[d7bb524ad](facebook/react@d7bb524ad )**: [cleanup] Remove unused package jest-mock-scheduler ([#26084](facebook/react#26084)) //<Ming Ye>//
- **[6b3083266](facebook/react@6b3083266 )**: Upgrade prettier ([#26081](facebook/react#26081)) //<Jan Kassens>//
- **[1f5ce59dd](facebook/react@1f5ce59dd )**: [cleanup] fully roll out warnAboutSpreadingKeyToJSX ([#26080](facebook/react#26080)) //<Jan Kassens>//

Changelog:
[General][Changed] - React Native sync for revisions 48b687f...fccf3a9

jest_e2e[run_all_tests]

Reviewed By: rubennorte

Differential Revision: D43305607

fbshipit-source-id: 8da7567ca2a182f4be27788935c2da30a731f83b
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[86c8c8db7](facebook/react@86c8c8db7 )**: test: Don't retry flushActWork if flushUntilNextPaint threw ([facebook#26121](facebook/react#26121)) //<Sebastian Silbermann>//
- **[64acd3918](facebook/react@64acd3918 )**: remove unguarded getRootNode call ([facebook#26152](facebook/react#26152)) //<Josh Story>//
- **[71cace4d3](facebook/react@71cace4d3 )**: Migrate testRunner from jasmine2 to jest-circus ([facebook#26144](facebook/react#26144)) //<Ming Ye>//
- **[c8510227c](facebook/react@c8510227c )**: Treat displayName as undefined ([facebook#26148](facebook/react#26148)) //<Sebastian Markbåge>//
- **[55542bc73](facebook/react@55542bc73 )**: Update jest printBasicPrototype config ([facebook#26142](facebook/react#26142)) //<Ming Ye>//
- **[6396b6641](facebook/react@6396b6641 )**: Model Float on Hoistables semantics ([facebook#26106](facebook/react#26106)) //<Josh Story>//
- **[ef9f6e77b](facebook/react@ef9f6e77b )**: Enable passing Server References from Server to Client ([facebook#26124](facebook/react#26124)) //<Sebastian Markbåge>//
- **[35698311d](facebook/react@35698311d )**: Update jest escapeString config ([facebook#26140](facebook/react#26140)) //<Ming Ye>//
- **[6ddcbd4f9](facebook/react@6ddcbd4f9 )**: [flow] enable LTI inference mode ([facebook#26104](facebook/react#26104)) //<Jan Kassens>//
- **[53b1f69ba](facebook/react@53b1f69ba )**: Implement unstable_getBoundingClientRect in RN Fabric refs ([facebook#26137](facebook/react#26137)) //<Rubén Norte>//
- **[594093496](facebook/react@594093496 )**: Update to Jest 29 ([facebook#26088](facebook/react#26088)) //<Ming Ye>//
- **[28fcae062](facebook/react@28fcae062 )**: Add support for SVG `transformOrigin` prop ([facebook#26130](facebook/react#26130)) //<Aravind D>//
- **[3ff1540e9](facebook/react@3ff1540e9 )**: Prefer JSX in ReactNoop assertions (to combat out-of-memory test runs) ([facebook#26127](facebook/react#26127)) //<Sebastian Silbermann>//
- **[01a0c4e12](facebook/react@01a0c4e12 )**: Add Edge Server Builds for workerd / edge-light ([facebook#26116](facebook/react#26116)) //<Sebastian Markbåge>//
- **[f0cf832e1](facebook/react@f0cf832e1 )**: Update Flight Fixture to "use client" instead of .client.js ([facebook#26118](facebook/react#26118)) //<Sebastian Markbåge>//
- **[03a216070](facebook/react@03a216070 )**: Rename "dom" fork to "dom-node" and "bun" fork to "dom-bun" ([facebook#26117](facebook/react#26117)) //<Sebastian Markbåge>//
- **[4bf2113a1](facebook/react@4bf2113a1 )**: Revert "Move the Webpack manifest config to one level deeper ([facebook#26083](facebook/react#26083))"  ([facebook#26111](facebook/react#26111)) //<Sebastian Markbåge>//
- **[2ef24145e](facebook/react@2ef24145e )**: [flow] upgrade to 0.199.0 ([facebook#26096](facebook/react#26096)) //<Jan Kassens>//
- **[922dd7ba5](facebook/react@922dd7ba5 )**: Revert the outer module object to an object ([facebook#26093](facebook/react#26093)) //<Sebastian Markbåge>//
- **[9d111ffdf](facebook/react@9d111ffdf )**: Serialize Promises through Flight ([facebook#26086](facebook/react#26086)) //<Sebastian Markbåge>//
- **[0ba4698c7](facebook/react@0ba4698c7 )**: Fix async test in React reconciler ([facebook#26087](facebook/react#26087)) //<Ming Ye>//
- **[8c234c0de](facebook/react@8c234c0de )**: Move the Webpack manifest config to one level deeper ([facebook#26083](facebook/react#26083)) //<Sebastian Markbåge>//
- **[977bccd24](facebook/react@977bccd24 )**: Refactor Flight Encoding ([facebook#26082](facebook/react#26082)) //<Sebastian Markbåge>//
- **[d7bb524ad](facebook/react@d7bb524ad )**: [cleanup] Remove unused package jest-mock-scheduler ([facebook#26084](facebook/react#26084)) //<Ming Ye>//
- **[6b3083266](facebook/react@6b3083266 )**: Upgrade prettier ([facebook#26081](facebook/react#26081)) //<Jan Kassens>//
- **[1f5ce59dd](facebook/react@1f5ce59dd )**: [cleanup] fully roll out warnAboutSpreadingKeyToJSX ([facebook#26080](facebook/react#26080)) //<Jan Kassens>//

Changelog:
[General][Changed] - React Native sync for revisions 48b687f...fccf3a9

jest_e2e[run_all_tests]

Reviewed By: rubennorte

Differential Revision: D43305607

fbshipit-source-id: 8da7567ca2a182f4be27788935c2da30a731f83b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants