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

FALLBACK_THROTTLE_MS slows tests down significantly - could it be configurable? #30408

Open
phryneas opened this issue Jul 19, 2024 · 4 comments

Comments

@phryneas
Copy link
Contributor

Summary

Hi there 👋,

we encountered a massive slowdown in the Apollo Client React hooks test suite - especially on the suspense-related tests - when we started testing for React 19.

One file specifically went up from a 10s execution time to 80s.

Further discussion on twitter and later in an issue in testing library revealed that this slowdown relates to React 19 being more strict with the throttle of the Suspense fallback, and that every suspense fallback shown in a test now slows down the test by an additional 300ms.

While I generally agree with testing close to the original implementation, in our case this massively slows down our local development and significantly increases the CI minutes used (it sums up over all tests to an additional 2 minutes for every CI run).

Changing everything to mocked timers is something that would honestly be very painful (even if only on affected tests), and if it would really need to be done on a global scale as indicated by @eps1lon in that issue, I believe it would just be completely out of the question in many cases - it would require rewriting thousands of tests, even if not suspense-related.

Would you be willing to make FALLBACK_THROTTLE_MS configurable, similar to IS_REACT_ACT_ENVIRONMENT?

I did patch the timeout for our test suite, and could verify that it almost brings execution times back down to the original - I kept the timeout at a for tests more reasonable 10ms, so we go from 10s on React 18 to 15s on React 19 now.

I would happily submit a PR if you would consider this.

Reproduction

You can see this happening in this reproduction (runnable repo here: https://github.com/phryneas/react-19-reproduction-slow-tests):

test("does not timeout", async () => {
  const initialPromise = Promise.resolve("test");
  console.time("executing component render");
  console.time("got past the `use` call");
  console.time("assertion succeeded");

  function Component() {
    console.timeLog("executing component render");
    const renderResult = use(initialPromise);
    console.timeLog("got past the `use` call", renderResult);
    return <div>{renderResult}</div>;
  }

  render(
    <Suspense fallback={<div>loading</div>}>
      <Component />
    </Suspense>
  );

  await waitFor(() => {
    screen.getByText("test");
    console.timeLog("assertion succeeded");
  });
});

Which logs

    executing component render: 4 ms

    executing component render: 51 ms

    got past the `use` call: 53 ms test

    assertion succeeded: 340 ms
@eps1lon eps1lon changed the title [React 19] FALLBACK_THROTTLE_MS slows tests down significantly - could it be configurable? FALLBACK_THROTTLE_MS slows tests down significantly - could it be configurable? Jul 19, 2024
@eps1lon
Copy link
Collaborator

eps1lon commented Jul 19, 2024

For context, this throttling could be experienced in React 18 with a larger value (500ms). React 19 used it in more scenarios which lead to the decision to reduce the value.

@phryneas
Copy link
Contributor Author

phryneas commented Jul 19, 2024

That's very possible, but tbh. we haven't encountered a single situation in our tests where it triggered with React 18 (at least none of our suspense-related test takes over 500ms) - of course that might have been due to the very specific nature of our tests.
Otherwise, you would probably have gotten this request sooner 😉

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 22, 2024

There's no need to make it configurable since the throttle heuristic is already skipped when resolved during act.

You can fix this today by manually resolving e.g.

let resolve: (value: string) => void;
const initialPromise = new Promise<string>((_resolve) => {
  resolve = _resolve;
});
...
await act(async () => {
  resolve("test");
});

We'll likely deprecate non-awaited act at some point so that you can just write

-await act(async () => {
+await act(() => {

Though at that point you wouldn't need to manually resolve anyway since await render would've flushed the revealed content. Work for that is being tracked in testing-library/react-testing-library#1214. Note that await render(...) is not sufficient today.

@phryneas
Copy link
Contributor Author

This kinda goes opposite to the "you probably don't want to use act at all" advice over in #30031 (comment) 😞

We cannot really have both if this is the only way of avoiding that heuristic. But since the heuristic apparently already is in place, would it be possible to introduce a different way to opt out on top?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants