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

ArrayBuffer regression in node env #7780

Closed
sirian opened this issue Feb 2, 2019 · 28 comments
Closed

ArrayBuffer regression in node env #7780

sirian opened this issue Feb 2, 2019 · 28 comments
Labels

Comments

@sirian
Copy link

sirian commented Feb 2, 2019

🐛 Bug Report

#7626 introduced regression

expect(new Uint8Array([]).buffer).toBeInstanceOf(ArrayBuffer);

fails with

    Expected constructor: ArrayBuffer
    Received constructor: ArrayBuffer
    Received value: []
@sirian sirian added the 🐛 Bug label Feb 2, 2019
@SimenB
Copy link
Member

SimenB commented Feb 2, 2019

@H1Gdev ideas?

@SimenB
Copy link
Member

SimenB commented Feb 2, 2019

This might be #2549

@H1Gdev
Copy link
Contributor

H1Gdev commented Feb 3, 2019

@SimenB

Oh...:cry:

Seems that there are multiple type of ArrayBuffer in Node.js vm.

There is also a way to add TypedArrays like ArrayBuffer is easy workaround.
https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/TypedArray

@H1Gdev
Copy link
Contributor

H1Gdev commented Feb 3, 2019

@SimenB

Execute the following code in each environment.(--env=node, --env=jsdom, and node).

const bufFromArray = Buffer.from([0x62, 0x75, 0x66, 0x66, 0x65, 0x72]);
bufFromArray instanceof Uint8Array;
const bufFromArrayBuffer = Buffer.from(new ArrayBuffer(6));
bufFromArrayBuffer instanceof Uint8Array;

In --env=node, results are not true.

It seems that Uint8Array also has the same issue as ArrayBuffer.

@sirian
Copy link
Author

sirian commented Feb 3, 2019

@SimenB

Oh...😢

Seems that there are multiple type of ArrayBuffer in Node.js vm.

There is also a way to add TypedArrays like ArrayBuffer is easy workaround.
https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/TypedArray

not only ArrayBuffer. all globals in vm context differs from globals of main process.
But new vm context doesn't have Buffer. So in this line vmGlobal.Buffer = mainGlobal.Buffer explicitly assigned https://github.com/facebook/jest/blob/master/packages/jest-util/src/installCommonGlobals.js#L65

so both contexts (main and vm) has same Buffer from main context. And Buffer.from() called in vmContext returns instance of mainGlobal.ArrayBuffer.

Probably it's better to contextify Buffer constructor, Buffer.prototype and static methods in installCommonGlobals. Like in vm2 https://github.com/patriksimek/vm2

@sirian
Copy link
Author

sirian commented Feb 3, 2019

Moreover since Buffer from different context this both fails:

expect(Buffer instanceof Object).toBe(true);
expect(Buffer instanceof Function).toBe(true);

Upd. Full test case of globals if needed:

const data = [];
for (const key in global) {
    const value = global[key];
    if ("function" === typeof value || null !== value && "object" === typeof value) {
        data.push(key);
    }
}

test.each(data)("global[%p] instanceof Object", (key) => {
    expect(global[key]).toBeInstanceOf(Object);
});

image

@LynxyssCZ
Copy link

This is really painful in our project. Caused a lot of false positives and nagatives in tests involving checks for value types.
Instead of simple value instanceof Object before chucking the value into WeakMap as a key I have to do
value instanceof Object || value instanceof Buffer
And explain to colleagues, that this is there just because Jest enviroment demands it to be.

@valamidev
Copy link

Possible workaround:

Object.prototype.toString.call( new Uint8Array(10)) === '[object Uint8Array]'

@wi-ski
Copy link

wi-ski commented Apr 17, 2020

This forced me to use mocha.

@wi-ski
Copy link

wi-ski commented Apr 18, 2020

I lied. I came back. Workaround:

// __test-utils__/custom-jest-environment.js
// Stolen from: https://github.com/ipfs/jest-environment-aegir/blob/master/src/index.js
// Overcomes error from jest internals.. this thing: https://github.com/facebook/jest/issues/6248
"use strict";

const NodeEnvironment = require("jest-environment-node");

class MyEnvironment extends NodeEnvironment {
  constructor(config) {
    super(
      Object.assign({}, config, {
        globals: Object.assign({}, config.globals, {
          Uint32Array: Uint32Array,
          Uint8Array: Uint8Array,
          ArrayBuffer: ArrayBuffer,
        }),
      }),
    );
  }

  async setup() {}

  async teardown() {}

}

module.exports = MyEnvironment;

Then in package.json:

  "jest": {
    "testEnvironment": "__test-utils__/custom-jest-environment.js",
    ...
  },

@odahcam
Copy link

odahcam commented May 12, 2020

I just had to put a ./ before the testEnvironment path:

  "jest": {
    "testEnvironment": "./__test-utils__/custom-jest-environment.js",
    ...
  },

@wujekbogdan
Copy link

wujekbogdan commented Jun 18, 2020

@odahcam @wi-ski

Thanks for sharing the workaround, but it seems to me that the solution is already out of date. I've just installed jest-environment-node@^26.0.1 and in jest.config.js I set:

module.exports = {
  // ...
  testEnvironment: 'node',
};

That's it. I didn't need to extend the NodeEnvironment and overload the constructor. It just worked.

Correct me if I'm wrong, but it seems that when jest-environment-node is installed then jest uses that module to configure the environment. So it looks like the bug is already fixed in jest-environment-node modlue.

Am I right?

@stevenyap
Copy link

Setting testEnvironment: 'node' in jest.config.js works for me too without using the workaround. I do not need to install jest-environment-node.

@shrpne
Copy link

shrpne commented Aug 6, 2020

let t = Buffer.alloc(0)
t instanceof Uint8Array

true in jest-environment-jsdom@25
false in jest-environment-jsdom@26

@arnotixe
Copy link

this bug is over a year old - is it an intended side-effect in v26.1.0 or can I open a PR that addresses this

still bit me on jest 26.4.2… but wujekbogdan's solution above adding to jest.config.js did help me!

@mogarick
Copy link

mogarick commented Sep 2, 2020

I'm running Jest v26.4.0 and the problem occurs even though I added testEnvironment="node" in jest.config.js. I used the code posted by @wi-ski and that solved the problem.
In my case I'm using Amplify and for Auth it uses its own crypto-js module version and it was failing when comparing aVariable instanceof Uint8Array presumably due the the same problem reported here.
Happy to have found this issue and how to cope with it.

dlech added a commit to pybricks/pybricks-code that referenced this issue Nov 21, 2020
- had to update eslint deps
- ran into palantir/blueprint#4112
- had to move beta.svg out of static/
- had to fix prettier formatting changes
- ran into jestjs/jest#7780
dlech added a commit to pybricks/pybricks-code that referenced this issue Nov 21, 2020
- had to update eslint deps
- ran into palantir/blueprint#4112
- had to move beta.svg out of static/
- had to fix prettier formatting changes
- ran into jestjs/jest#7780
adrianclay added a commit to adrianclay/shopping-list that referenced this issue Jan 2, 2021
This reverts commit f825ea1.

The upgrade to react-scripts includes an upgrade to jest-environment-jsdom which
introduces a regression with firebase related tests detailed here:
  https://stackoverflow.com/q/61914567
  jestjs/jest#7780
  firebase/firebase-js-sdk#3096

It's easier to hold back on the react-scripts upgrade for now.
Sajjon added a commit to radixdlt/radixdlt-javascript that referenced this issue Feb 19, 2021
yqrashawn added a commit to Conflux-Chain/helios that referenced this issue Apr 15, 2021
@gindyo
Copy link

gindyo commented May 29, 2021

I am still experiencing this issue with jest-environment-jsdom@27. I tried the solution of adding a custom environment, but then the behavior of VSCode's jest extension becomes all wonky. It can't figure out the status of the tests. Ughh

@RobertLowe
Copy link

@shrpne
Copy link

shrpne commented Jun 11, 2021

i worked around this by adding jest-environment-jsdom@25 as a dev dep

jest-environment-jsdom@25 doesn't work with jest@27 anymore

@odbol
Copy link

odbol commented Jun 18, 2021

So, create-react-app doesn't allow you to override testEnvironment. Does anyone know a way to apply the workaround when using create-react-app?

I tried using jest { setupGlobals } but that wouldn't let me modify the globals config.

npm test

> my-app@0.1.0 test ***
> react-scripts test


Out of the box, Create React App only supports overriding these Jest options:

  • clearMocks
  • collectCoverageFrom
  • coveragePathIgnorePatterns
  • coverageReporters
  • coverageThreshold
  • displayName
  • extraGlobals
  • globalSetup
  • globalTeardown
  • moduleNameMapper
  • resetMocks
  • resetModules
  • restoreMocks
  • snapshotSerializers
  • testMatch
  • transform
  • transformIgnorePatterns
  • watchPathIgnorePatterns.

These options in your package.json Jest configuration are not currently supported by Create React App:

  • testEnvironment

If you wish to override other Jest options, you need to eject from the default setup. You can do so by running npm run eject but remember that this is a one-way operation. You may also file an issue with Create React App to discuss supporting more options out of the box.

@chrisfosterelli
Copy link

@odbol We did this on the command line by replacing the test script in package.json with react-scripts test --env <path-to-file>

@nlinx
Copy link

nlinx commented Jun 21, 2021

@odbol You can also just add this

/**
 * @jest-environment node
 */

to the top of the file having this issue

@odbol
Copy link

odbol commented Jun 21, 2021

Wow, @nlinx, that worked perfectly. Who knew it was so easy? Thanks!

adrianclay added a commit to adrianclay/shopping-list that referenced this issue Jun 27, 2021
adrianclay added a commit to adrianclay/shopping-list that referenced this issue Jun 27, 2021
@johnnybenson
Copy link

This seems like it is still an issue for Int32Array

/**
 * @jest-environment node
 */

    test('Int32Array Buffer', () => {
        expect(new Int32Array([]).buffer).toBeInstanceOf(ArrayBuffer);
    });
    expect(received).toBeInstanceOf(expected)

    Expected constructor: ArrayBuffer
    Received constructor: ArrayBuffer

      61 |     test('Int32Array Buffer', () => {
    > 62 |         expect(new Int32Array([]).buffer).toBeInstanceOf(ArrayBuffer);
         |                                           ^
      63 |     });

wojtekmaj added a commit to wojtekmaj/react-pdf that referenced this issue Sep 21, 2021
Includes workaround for jestjs/jest#7780
josueetcom added a commit to beyondhb1079/s4us that referenced this issue Jan 9, 2022
This doesn't work tho because Jest v27 seems to be required.

The workarounds I see in jestjs/jest#7780 and firebase/firebase-js-sdk#3096 don't seem to work
@SimenB
Copy link
Member

SimenB commented Feb 21, 2022

Example in OP passes in newer versions of Jest. Please open new issues with reproductions for other environments.

(the last example probably still fails due to https://github.com/facebook/jest/blob/b372332a8ff25aca23d36c5297db94581b6d439e/packages/jest-environment-node/src/index.ts#L42, but nevertheless it's a different issue)

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests