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

Several tests fail on main with Node v16 #22441

Closed
josephsavona opened this issue Sep 27, 2021 · 8 comments · Fixed by #22477
Closed

Several tests fail on main with Node v16 #22441

josephsavona opened this issue Sep 27, 2021 · 8 comments · Fixed by #22477

Comments

@josephsavona
Copy link
Contributor

I forked and cloned the repo this morning (at c88fb49), ran yarn and yarn test, and several tests failed.

React version: main (c88fb49)

Steps To Reproduce

  1. Clone repo
  2. yarn and yarn test

I was on the latest NodeJS version (16.10.0). Note that tests pass for me when switching to the 14.x LTS release of Node.

The current behavior

13 tests fail across 5 suites. Output at https://gist.github.com/josephsavona/91a3d48add8a1c47c71178522583281b

The expected behavior

All tests pass

@josephsavona josephsavona added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Sep 27, 2021
@sebmarkbage
Copy link
Collaborator

At least a couple of these are just that V8 changed format of the error message for undefined values.

We know that stopping to respect displayName will mess with our component stack traces.

Some of the ART ones seems to be that displayName doesn't show up in stacks (at least create-react-class).

But this seems like it should fail too: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/__tests__/ReactMemo-test.js#L572

@bvaughn bvaughn added Component: Build Infrastructure Type: Discussion and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Sep 27, 2021
@bvaughn
Copy link
Contributor

bvaughn commented Sep 27, 2021

I was on the latest NodeJS version (16.10.0). Note that tests pass for me when switching to the 14.x LTS release of Node.

Our devEngines block should really not include 15 or 16, so long as 14 is the latest that works.

react/package.json

Lines 102 to 104 in 8131de1

"devEngines": {
"node": "^12.17.0 || 13.x || 14.x || 15.x || 16.x"
},

@bvaughn bvaughn changed the title Bug: Several tests fail on main with Node v16 Sep 27, 2021
@Aditya-Bhate
Copy link

A couple of these are just that V8 changed format of the error message for undefined values.!

@akgupta0777
Copy link
Contributor

akgupta0777 commented Sep 30, 2021

so should we need to improve test support for v16? Or it's good to stick to the LTS version.

@bvaughn
Copy link
Contributor

bvaughn commented Sep 30, 2021

Seems like it would be nice if we could feature detect and support both. (Have not thought about how this would be done.)

@bvaughn bvaughn self-assigned this Sep 30, 2021
@bvaughn
Copy link
Contributor

bvaughn commented Sep 30, 2021

I will take a pass at this.

@bvaughn
Copy link
Contributor

bvaughn commented Sep 30, 2021

Here we go: #22477

@0xdevalias
Copy link
Contributor

0xdevalias commented Jul 13, 2022

It looks like there are a number of variations of ways that are currently being used to test that aren't captured by the changes made in #22477

I'll just include one example for each pattern variation I saw in my failed test runs:

packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
    expect(received).toEqual(expected) // deep equality

    Expected: "Cannot read property 'toUpperCase' of undefined"
    Received: "Cannot read properties of undefined (reading 'toUpperCase')"
 packages/react-debug-tools/src/__tests__/ReactHooksInspection-test.js
    expect(received).toBe(expected) // Object.is equality

    Expected: "Cannot read property 'useState' of null"
    Received: "Cannot read properties of null (reading 'useState')"

So it looks like the other methods being used to test for this are .toEqual and .toBe


Edit: Opened a new issue for this:

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

Successfully merging a pull request may close this issue.

6 participants