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

waitFor introduces unexpected error with rejected Promise #1210

Open
Dru89 opened this issue Feb 3, 2023 · 3 comments
Open

waitFor introduces unexpected error with rejected Promise #1210

Dru89 opened this issue Feb 3, 2023 · 3 comments
Labels
needs discussion We need to discuss this to come up with a good solution

Comments

@Dru89
Copy link

Dru89 commented Feb 3, 2023

  • @testing-library/dom version: 8.20.0
  • Testing Framework and version: jest@29.4.1
  • DOM Environment: jest-environment-jsdom@29.4.1

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(<SplineReticulator reticulate={reticulate} />);  
      46 |   userEvent.click(getByText("Click me!"));

      at src/repro.test.tsx:43:50
      at src/repro.test.tsx:31:71
      at Object.<anonymous>.__awaiter (src/repro.test.tsx:27:12)
      at Object.<anonymous> (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
Copy link
Author

Dru89 commented Feb 3, 2023

Hmm, this might actually be a duplicate of jestjs/jest#5311.

@eps1lon
Copy link
Member

eps1lon commented Feb 15, 2023

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 jestjs/jest#5311 since jestjs/jest#5311 is about unhandled promise rejections. The warning you're seeing is about handling rejections asynchronously. Those are different things.

@eps1lon eps1lon added the needs discussion We need to discuss this to come up with a good solution label Feb 15, 2023
@eps1lon
Copy link
Member

eps1lon commented Feb 15, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion We need to discuss this to come up with a good solution
Projects
None yet
Development

No branches or pull requests

2 participants