Skip to content

Test with rejected Promise fails in Node, not in browser #468

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
ghengeveld opened this issue Aug 28, 2019 · 17 comments
Closed

Test with rejected Promise fails in Node, not in browser #468

ghengeveld opened this issue Aug 28, 2019 · 17 comments
Labels
help wanted Extra attention is needed needs investigation

Comments

@ghengeveld
Copy link

ghengeveld commented Aug 28, 2019

Relevant code

import React from "react"
import { render } from "@testing-library/react"

const Hello = () => {
  const promise = Promise.reject("Oh no!")
  console.log(promise)
  return <div>Hello RTL</div>
}

test("HelloRTL", async () => {
  const { findByText } = render(<Hello />)
  await findByText("Hello RTL")
})

The problem

Ran from the command line with yarn test --watchAll, this fails with the following output:

 FAIL  src/hello.spec.js
  ✕ HelloRTL (22ms)

  ● HelloRTL

    Oh no!



  console.log src/hello.spec.js:6
    Promise { <rejected> 'Oh no!' }

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        0.774s, estimated 1s
Ran all test suites.

Reproduction

  • Naturally I've setup a CodeSandbox.
  • As you can see the test actually passes in CodeSandbox, I suppose this is because it's run in the browser?
  • I've setup a repository here.
  • I've added the repo to Travis, where you can see it fail.
  • I've tried using plain Jest, where this issue doesn't appear.
  • I've tried without react-scripts, but the issue persists.
  • I've tried jest-environment-jsdom-fourteen, but the issue persists.
  • I was able to pin this down to v8.0.6. In v8.0.5 it works fine.

Expected outcome

I would expect the test to pass in both environments. There is nothing wrong with assigning a rejected Promise to a variable, as long as it doesn't get rendered. The React component works fine.

@kentcdodds
Copy link
Member

My guess is this is a codesandbox bug. Is there a real world scenario that you can point us to for how this is issue impacting you?

@kentcdodds
Copy link
Member

Also, this seems unrelated to react testing library. Could you verify that it's an issue in RTL?

@ghengeveld
Copy link
Author

ghengeveld commented Aug 28, 2019

It was introduced in v8.0.6. My use case is React Async, which keeps a promise in local state (via useReducer). If this is a rejected promise (which naturally is something I want to test against), the test blows up.

@kentcdodds
Copy link
Member

That's pretty strange. Would you mind doing a little digging to see if you can figure out why this is happening?

@kentcdodds kentcdodds added help wanted Extra attention is needed needs investigation labels Aug 28, 2019
@ghengeveld
Copy link
Author

You should know this whole thing was actually triggered by the promise prop I recently introduced. It's becoming a bit problematic. Any help would be appreciated.

Anyway, I'll see what I can do to further pin down the issue.

@ghengeveld
Copy link
Author

Interestingly, I was able to bring down act-compat.js to just a few lines of code to make it work:

import {act} from 'react-dom/test-utils'

act(() => ({
  then: () => {},
}))

export const asyncAct = act
export default act

Turns out this little gem was removed in the 8.0.6 release.

Obviously this completely breaks the actual act compatibility, but it shows the gist of it.

@ghengeveld
Copy link
Author

ghengeveld commented Aug 28, 2019

As expected, the test passes once I call act like above:

import React from "react"
import { render } from "@testing-library/react"
import { act } from "react-dom/test-utils"

act(() => ({ then: () => {} })) // this fixes the test
  .then(() => {})               // this suppresses the "act without await" warning

const Hello = () => {
  const promise = Promise.reject("Oh no!")
  console.log(promise)
  return <div>Hello RTL</div>
}

test("HelloRTL", async () => {
  const { findByText } = render(<Hello />)
  await findByText("Hello RTL")
})
PASS src/hello.spec.js
  ✓ HelloRTL (21ms)

  console.log src/hello.spec.js:11
    Promise { <rejected> 'Oh no!' }

@kentcdodds
Copy link
Member

The problem is that we need to support 16.8 for the foreseeable future. That's why act-compat exists 😬

@ghengeveld
Copy link
Author

Yes, but I'm sure we can just put that little snippet back in at the right place to fix this issue, while keeping the whole compat thing. I only brought it down to the bare minimum for debugging purposes.

@ghengeveld
Copy link
Author

By the way relative-deps has been indispensable to get my local fork of react-testing-library into the example repo. Symlinking doesn't work due to both depending on different instances of react and react-dom.

@ghengeveld
Copy link
Author

ghengeveld commented Aug 28, 2019

I've pushed the workaround to the demo repo and confirmed that this fixes the test.

@gnbaron
Copy link

gnbaron commented Aug 29, 2019

I'm experiencing a similar issue I guess, but I'm pretty sure it has something to do with 16.9.0. You can confirm that downgrading react and react-dom to 16.8.0 in your demo repo, tests will pass even without your workaround.

@eps1lon
Copy link
Member

eps1lon commented Aug 29, 2019

This may sound naive but isn't Promise.reject('foo') an uncaught promise? Some environments might treat them differently since they're only deprecated in node with the notice that they will throw later on.

Could you check if the behavior is different without using @testing-library/react?

@ghengeveld
Copy link
Author

@gnbaron Thanks, I will give that a try.

@eps1lon I've tested this with plain Jest, following the testing guide on the React website. The problem does not appear there.

Indeed this is an uncaught exception in Node. Perhaps I should change from a Promise to a Thenable to avoid that oddity. In my mind a rejected promise is just a value like any other, but this might not be true in certain environments. My goal is to expose this promise as a render prop in React Async, so it may or may not be used by library users. This works fine but blows up in RTL.

@ghengeveld
Copy link
Author

ghengeveld commented Aug 29, 2019

@gnbaron So downgrading to 16.8.0 makes the test pass, but RTL will tell you that you need to upgrade to 16.9 because you need an async version of act.

This also works by the way (with 16.9 and RTL):

import React from "react"
import { render } from "@testing-library/react"

const Hello = () => {
  const promise = Promise.reject("Oh no!")
  promise.then(m => console.log(m), e => console.log(e)) // catch the exception
  return <div>Hello RTL</div>
}

test("HelloRTL", async () => {
  const { findByText } = render(<Hello />)
  await findByText("Hello RTL")
})

So it seems @eps1lon is correct.

@eps1lon
Copy link
Member

eps1lon commented Aug 29, 2019

Just tested it with plain react-dom and if you have a sync test it passes (since the promise is rejected after the test finishes. Though you will get the usual unhandled promise rejection warning. Only if the promise is rejected during the test will the test also fail since it is uncaught during the test.

I don't think this is an issue rtl should solve. The environment should decide what it does with unhandled rejections.

diff --git a/src/hello.spec.js b/src/hello.spec.js
index 9aa3707..4f00861 100644
--- a/src/hello.spec.js
+++ b/src/hello.spec.js
@@ -1,5 +1,5 @@
 import React from "react";
-import { render } from "@testing-library/react";
+import { render } from "react-dom";
 
 const Hello = () => {
   const promise = Promise.reject("Oh no!");
@@ -7,7 +7,17 @@ const Hello = () => {
   return <div>Hello RTL</div>;
 };
 
-test("HelloRTL", async () => {
-  const { findByText } = render(<Hello />);
-  await findByText("Hello RTL");
+let container;
+beforeEach(() => {
+  container = document.createElement("div");
+});
+
+afterEach(() => {
+  container = null;
+});
+
+test("HelloRTL", done => {
+  render(<Hello />, container);
+
+  setTimeout(() => done(), 100);
 });

@kentcdodds
Copy link
Member

Considering this is unrelated to RTL specifically, I think we'll close this one. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed needs investigation
Projects
None yet
Development

No branches or pull requests

4 participants