testing-library / dom-testing-library

🐙 Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.26k stars 466 forks source link

`waitFor` introduces unexpected error with rejected Promise #1210

Open Dru89 opened 1 year ago

Dru89 commented 1 year ago

What you did:

Here's a minimal test to reproduce the issue

import * as React from "react";
import "@testing-library/jest-dom"

import { render, waitFor } from "@testing-library/react"
import userEvent from "@testing-library/user-event";

type Props = {reticulate: () => Promise<number>;}
function SplineReticulator({reticulate}: Props) {
  const openWindow = async () => {
    return new Promise<Window>((res) => {
      setTimeout(() => res(window.open('https://www.example.com')!), 250)
    })
  }

  const handler = () => {
    const promise = new Promise<number>(async (res, rej) => {
      try {
        const result = reticulate();
        // Uncommenting this line makes the tests pass.
        // result.catch(() => {});

        await openWindow();
        const answer = await result;
        return res(answer);
      } catch (err) {
        return rej(err);
      }
    });

    promise.then(
      (result) => { console.log('good!', result) }, 
      (reason) => { console.error('bad!', reason.message) }
    );
  }

  return (
    <button onClick={() => handler()}>Click me!</button>
  )
}

it('reticulates splines', async () => {
  window.open = jest.fn().mockReturnValue({ close: jest.fn(), closed: false });
  const reticulate = jest.fn().mockRejectedValue(new Error('oops!'));

  const {getByText} = render(<SplineReticulator reticulate={reticulate} />);  
  userEvent.click(getByText("Click me!"));
  await waitFor(() => expect(window.open).toHaveBeenCalled());
});

What happened:

Running this change as-is produces the following error:

Console output ``` at 09:45:09 PM $ npx jest repro.test.tsx console.error bad! oops! 30 | promise.then( 31 | (result) => { console.log('good!', result) }, > 32 | (reason) => { console.error('bad!', reason.message) } | ^ 33 | ); 34 | } 35 | at src/repro.test.tsx:32:29 (node:97479) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1) (Use `node --trace-warnings ...` to show where the warning was created) FAIL src/repro.test.tsx ✕ reticulates splines (322 ms) ● reticulates splines oops! 41 | it('reticulates splines', async () => { 42 | window.open = jest.fn().mockReturnValue({ close: jest.fn(), closed: false }); > 43 | const reticulate = jest.fn().mockRejectedValue(new Error('oops!')); | ^ 44 | 45 | const {getByText} = render(); 46 | userEvent.click(getByText("Click me!")); at src/repro.test.tsx:43:50 at src/repro.test.tsx:31:71 at Object..__awaiter (src/repro.test.tsx:27:12) at Object. (src/repro.test.tsx:41:38) Test Suites: 1 failed, 1 total Tests: 1 failed, 1 total Snapshots: 0 total Time: 2.002 s Ran all test suites matching /repro.test.tsx/i. ```

However, if I uncomment Line 20, that section of code now looks like:

const promise = new Promise<number>(async (res, rej) => {
  try {
    const result = reticulate();
    result.catch(() => {});

    await openWindow();
    const answer = await result;
    return res(answer);
  } catch (err) {
    return rej(err);
  }
});

and the tests pass as expected:

Console output ``` at 09:53:13 PM $ npx jest repro.test.tsx console.error bad! oops! 28 | promise.then( 29 | (result) => { console.log('good!', result) }, > 30 | (reason) => { console.error('bad!', reason.message) } | ^ 31 | ); 32 | } 33 | at src/repro.test.tsx:30:29 PASS src/repro.test.tsx ✓ reticulates splines (323 ms) Test Suites: 1 passed, 1 total Tests: 1 passed, 1 total Snapshots: 0 total Time: 1.992 s, estimated 2 s Ran all test suites matching /repro.test.tsx/i. ```

Reproduction:

You can also find this test file available at https://gist.github.com/Dru89/8c37aa36b8fe7093b97761283e9feece

The jest config is basically just the default configuration, with added support for React. I have tried this using both "fake timers" and "real timers" in jest, and it produces the same result.

Problem description:

As you can see, I'm not actually doing anything with that call to result.catch(() => {}), and the promise does get handled, albeit after the call to await openWindow().

My best guess about why this happens comes from a hint in the warnings from Node:

(node:3233) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)

As I mentioned, the promise does actually get handled with the call to await result; and then gets caught by the surrounding try/catch blocks, but I think maybe something in the waitFor might be causing fulfilled promises to settle, which triggers some "unhandled promise rejections" in jest/node or similar?

That theory is buoyed by the fact that if I change the reticulate mock to reject the promise after a delay, that also makes the tests pass. Basically, take the above reproduction, but change reticulate to be:

const reticulate = jest.fn(() => {
  return new Promise<number>((res, rej) => {
    setTimeout(() => {
      rej(new Error('oops!'));
    }, 1000);
  });
});

Suggested solution:

I don't have any suggestions for possible solutions here. And I'm not even really sure if there's anything that could be done, if the problem is basically that waitFor is "running out" any remaining promises, and jest is maybe just listening for those errors somewhere.

But it was a weird bug that I couldn't find any existing examples for after searching, and wanted to at least document this in case it helped anyone else. (And maybe it is an issue that could be fixed in how waitFor is implemented? 🤷)

Dru89 commented 1 year ago

Hmm, this might actually be a duplicate of https://github.com/facebook/jest/issues/5311.

eps1lon commented 1 year ago

What Node.js version are you using?

As I mentioned, the promise does actually get handled with the call to await result;

That's not where the error message is pointing at nor what it's about. It's specifically warning about actually handling the rejecting (but asynchronosuly): promise.then(() => {}, reason => {}). Which I don't really understand why Node.js would care. If I want to fire a Promise but don't care about when/if/how it settles, why would that not be supported?

I don't think we can do anything here as far as I can tell. It would be up to your implementation to cleanup the Promise but as far as I know, this isn't possible with React and Node.js

I don't think it's necessarily duplicate of https://github.com/facebook/jest/issues/5311 since https://github.com/facebook/jest/issues/5311 is about unhandled promise rejections. The warning you're seeing is about handling rejections asynchronously. Those are different things.

eps1lon commented 1 year ago

Can you create a repro that's as minimal as possible? Preferably no TypeScript and no user-event (can use fireEvent.click instead).