Skip to content

waitFor based helper failing randomly with multiple jest workers #848

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

Closed
mlarcher opened this issue Dec 8, 2020 · 7 comments
Closed

waitFor based helper failing randomly with multiple jest workers #848

mlarcher opened this issue Dec 8, 2020 · 7 comments
Labels
question Further information is requested

Comments

@mlarcher
Copy link

mlarcher commented Dec 8, 2020

  • @testing-library/react version: 5.11.6
  • Testing Framework and version: Jest 26.6.0 (react-scripts 4.0.1)
  • DOM Environment: jsdom 16.4.0
  • node 14.2.0
  • react: 17.0.1

Considering a Component can make an API call, then instantiate a form with various widgets, some of which might need data from the server and make API calls too, we need a way to know when the page is "stable" and has resolved all its server side dependencies before trying to interact with the page (mostly forms).

Relevant code or config:

In our codebase, all calls to the server trigger the presence of a Loader element with data-testid="loader", and we wrote the following helper to wait until everything is ready :

export async function waitForPageLoaded() {
  let ticksWithoutLoader = 0;
  let loadersCount = 0;
  await waitFor(
    () => {
      loadersCount = screen.queryAllByTestId('loader').length;
      if (loadersCount === 0) {
        ticksWithoutLoader += 1;
      } else {
        ticksWithoutLoader = 0;
      }
      if (loadersCount) {
        throw new Error('Still waiting for Loaders to disappear...');
      }
      if (ticksWithoutLoader <= 3) {
        throw new Error('Still waiting in case a Loader (re-)appears...');
      }
    },
    {
      onTimeout: (e) => {
        // eslint-disable-next-line testing-library/no-debug
        screen.debug();
        throw e;
      },
    },
  );
}

What you did:

The idea is to wait until there are no loader present and then let the waitFor be called again 2 more times in case a component that has just been instantiated triggers a new server call. When we see no loader for 3 waitFor calls in a row, we consider our page to be ready to play with.

Problem description:

We've been using that for a while, but noticed random test fails with it as tests number increased.
When using --runInBand with jest to force a single worker, it seems to be working fine every time.
But when we use workers (the default), we have random test fails more and more frequently.
We started monitoring the failing tests to find a pattern, but so far we saw about 25 different tests failing, out of 660.

Also, it seems that increasing the timeout option of the waitFor call helps, but that doesn't seem like a scalable solution...

Suggested solution:

Is there something fundamentally wrong with out approach ? Another way to achieve the same result in a more reliable way ? Is there a bug in the waitFor implementation that makes the jest workers affect each other's behavior ? We tried many things to fix the issue, and searched a lot on the matter, but can't get rid of it or find an alternative.

We love the react-testing-library's approach, but seem to be stuck in an uncomfortable situation and we're not sure if it's a bug or if we're doing things the wrong way.

Any hint would be appreciated :)

@mathiassoeholm
Copy link

You mention that increasing the timeout helps, so to me it sounds like a performance problem.

I have seen something similar, where a test took 1.8 seconds when running everything concurrently, but the default timeout of waitFor is 1 second.

I would start digging into those 25 tests to see if there's a bottleneck that makes them run slow. In my case I was parsing and serialising some XML, which I ended up having to mock to make the test run faster.

As a final option, you can consider changing the timeout to be 5 seconds, which is the same as Jests' default timeout for the entire test.

@mlarcher
Copy link
Author

mlarcher commented Jan 8, 2021

That sounds reasonable. Indeed it looks like the issue is triggered by a performance issue. The rare times it happens locally, my laptop fan is always noisy.

We don't have any obvious low performing functions in our tests, though, so I guess I'll have to try and profile the cpu during test runs to figure out where the slowdown happens 🧐

@mlarcher
Copy link
Author

mlarcher commented Jan 12, 2021

It was a hassle, but I eventually managed to peek into the jest workers cpu profiles. I didn't find anything that caught my eyes in there, everything seems to run slow, not a particular call 🤷

I also confirmed that there is no memory leaked between consecutive test files.

I suppose we'll have to increase the timeout and hope for a package upgrade to fix the problem (jest ? jsdom ? ...)

Anyhow it seems the issue does not reside in react-testing-library, so I'll close this ticket.

PS: here are a few interesting links I found while digging the topic:
https://medium.com/@paul_irish/debugging-node-js-nightlies-with-chrome-devtools-7c4a1b95ae27
https://indepth.dev/posts/1186/how-to-debug-a-child-process-in-node-and-gatsby-js-with-chrome#patching-jest-workers-to-debug-child-process-in-node
https://nolanlawson.com/2020/02/19/fixing-memory-leaks-in-web-applications/
https://media-codings.com/articles/automatically-detect-memory-leaks-with-puppeteer
https://nodesource.com/blog/diagnostics-in-NodeJS-2
https://chanind.github.io/javascript/2019/10/12/jest-tests-memory-leak.html

@mlarcher
Copy link
Author

mlarcher commented Jan 19, 2021

After seeing some issues on the https://github.com/testing-library/dom-testing-library side about getByRole and it's poor performance (I'm looking at you testing-library/dom-testing-library#698 and testing-library/dom-testing-library#820 😉 ) I think it might be worth re-opening this issue.

Perhaps there is something to be found on the react-testing-library side after all.

If nothing else, I would love to know what the recommended approach to the described problem is.

After all, if we want our test to "resemble the way [our] software is used", we need a way to wait for all loaders to disappear, as our users actually do before clicking on a form...

@mlarcher mlarcher reopened this Jan 19, 2021
@paxarpp
Copy link

paxarpp commented Jan 24, 2021

Hi,
I observe similar behavior
noticed that tests are unstable most often if there are several tests in one file,
if you make 1 test 1 file, then sometimes it solves the problem of instability

@eps1lon
Copy link
Member

eps1lon commented Jun 2, 2021

It doesn't look like this bug report has enough info for one of us to reproduce it.

Please provide a CodeSandbox (https://react.new), a link to a repository on GitHub, or provide a minimal code example that reproduces the problem.

Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve

Note that this issue will be closed if no reproduction is provided. Config, source, and test are neccessary for us to be able to debug these issues.

@eps1lon eps1lon added the question Further information is requested label Jun 2, 2021
@eps1lon
Copy link
Member

eps1lon commented Sep 13, 2021

Note that this issue will be closed if no reproduction is provided. Config, source, and test are neccessary for us to be able to debug these issues.

Closing since over 2 months have passed. testing-library/dom-testing-library#698 probably covers this particular issue already.

@eps1lon eps1lon closed this as completed Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants