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

Jest globals differ from Node globals #2549

Open
thomashuston opened this issue Jan 10, 2017 · 115 comments
Open

Jest globals differ from Node globals #2549

thomashuston opened this issue Jan 10, 2017 · 115 comments
Labels
🐛 Bug Discussion Has Bounty There is a bounty on the open collective for this issue Pinned

Comments

@thomashuston
Copy link
Contributor

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
After making a request with Node's http package, checking if one of the response headers is an instanceof Array fails because the Array class used inside http seems to differ from the one available in Jest's VM.

I specifically came across this when trying to use node-fetch in Jest to verify that cookies are set on particular HTTP responses. The set-cookie header hits this condition and fails to pass in Jest https://github.com/bitinn/node-fetch/blob/master/lib/headers.js#L38

This sounds like the same behavior reported in #2048; re-opening per our discussion there.

If the current behavior is a bug, please provide the steps to reproduce and either a repl.it demo through https://repl.it/languages/jest or a minimal repository on GitHub that we can yarn install and yarn test.
https://github.com/thomas-huston-zocdoc/jest-fetch-array-bug

What is the expected behavior?
The global Array class instance in Jest should match that of Node's packages so type checks behave as expected.

I've submitted a PR to node-fetch switching from instanceof Array to Array.isArray to address the immediate issue, but the Jest behavior still seems unexpected and it took quite a while to track down.

Please provide your exact Jest configuration and mention your Jest, node, yarn/npm version and operating system.
I am using the default Jest configuration (I have not changed any settings in my package.json).
Jest - 18.1.0
Node - 6.9.1 (also tested in 4.7.0 and saw the same error)
npm - 3.10.8
OS - Mac OS X 10.11.6

@suchipi
Copy link
Contributor

suchipi commented Jan 10, 2017

This is likely due to the behavior of vm; see nodejs/node-v0.x-archive#1277

Does Jest do anything to try to avoid this right now?

@PlasmaPower
Copy link

I came across the exact scenario. It's very hard to diagnose. I also came across it with Error objects. Writing wrapper workarounds for this is getting annoying.

From the linked nodejs issue:

Yes, Array.isArray() is the best way to test if something is an array.

However, Error.isError is not a function.

@suchipi
Copy link
Contributor

suchipi commented Mar 1, 2017

Jest team- should the node (and maybe jsdom) environment(s) be changed to put things like Error, Array, etc from the running context into the vm context? I believe that would solve this issue.

Alternatively, maybe babel-jest could transform instanceof calls against global bindings such that they work across contexts.

@PlasmaPower
Copy link

I don't like the babel-jest idea, if something like that is implemented it should be its own plugin. Other than that, I agree.

@cpojer
Copy link
Member

cpojer commented Mar 2, 2017

We can't pull in the data structures from the parent context because we want to sandbox every test. If you guys could enumerate the places where these foreign objects are coming from, we can wrap those places and emit the correct instances. For example, if setTimeout throws an error, then we can wrap that and re-throw with an Error from the vm context.

@suchipi
Copy link
Contributor

suchipi commented Mar 2, 2017

Is there any risk to the sandboxing added other than "if someone messes with these objects directly, it will affect other tests"? Or is there something inherent in the way the contexts are set up that would make this dangerous passively? Just trying to understand. I'd guess that instanceof Error checks are more likely than Error.foo = "bar" type stuff.

@cpojer
Copy link
Member

cpojer commented Mar 2, 2017

It's one of the guarantees of Jest that two tests cannot conflict with each other, so we cannot change it. The question is where you are getting your Error and Arrays from that are causing trouble.

@PlasmaPower
Copy link

They come from node native libraries like fs or http.

@cpojer
Copy link
Member

cpojer commented Mar 2, 2017

Ah, hmm, that's a good point. It works for primitives but not that well for errors or arrays :(

@suchipi
Copy link
Contributor

suchipi commented Mar 2, 2017

What if jest transformed instanceof Array and instanceof Error specifically into something like instanceof jest.__parentContextArray and instanceof jest.__parentContextError?

@cpojer
Copy link
Member

cpojer commented Mar 2, 2017

meh, I'm not sure I love that :(

@suchipi
Copy link
Contributor

suchipi commented Mar 2, 2017

We could override Symbol.hasInstance on the globals in the child context to also check their parent context if the first check fails... But Symbol.hasInstance only works in node 6.10.0+ or babel. Can't remember; does jest use babel everywhere by default?

@cpojer
Copy link
Member

cpojer commented Mar 2, 2017

I'm ok if this feature only works in newer versions of node. It seems much cleaner to me; assuming it doesn't have negative performance implications.

@suchipi
Copy link
Contributor

suchipi commented Mar 2, 2017

Assuming performance seems fine, which globals should it be applied to? Error and Array... Buffer maybe, too?

@cpojer
Copy link
Member

cpojer commented Mar 2, 2017

Yeah, that sounds like a good start.

@suchipi
Copy link
Contributor

suchipi commented Mar 2, 2017

I may be able to tackle a PR for this this weekend. I'm assuming we want it in both the node and jsdom environments?

@suchipi
Copy link
Contributor

suchipi commented Mar 6, 2017

I've started work on this in https://github.com/suchipi/jest/tree/instanceof_overrides, but am having difficulty reproducing the original issue. @PlasmaPower or @thomashuston do you have a minimal repro I could test against?

@PlasmaPower
Copy link

@joedynamite
Copy link

Not sure if it is 100% related or not but I have issues with exports not being considered Objects. For example the test in this gist will fail but if I run node index and log I get true: https://gist.github.com/joedynamite/b98494be21cd6d8ed0e328535c7df9d0

@PlasmaPower
Copy link

@joedynamite sounds like the same issue

@PlasmaPower
Copy link

Assuming performance seems fine, which globals should it be applied to? Error and Array... Buffer maybe, too?

Why not everything? I'm assuming performance won't be an issue as instanceof shouldn't be called often.

@jakeorr
Copy link

jakeorr commented May 18, 2017

I ran into a related issue with Express+Supertest+Jest. The 'set-cookie' header comes in with all cookies in a single string rather than a string for each cookie. Here is a reproduction case with the output I'm seeing with Jest and with Mocha (it works with mocha): #3547 (comment)

@rexxars
Copy link

rexxars commented Jul 25, 2017

Just spent a couple of hours trying to figure out what happened when an app failed in weird ways because of an instanceof Error check.

Basically, http errors seem to not be instances of Error, which is very frustrating.

Very simple, reproducible test case here.

Durisvk pushed a commit to Durisvk/langchainjs that referenced this issue Sep 3, 2023
…shared in jest context isolation

The way chromadb imports @xenova/transformers package in file
chromadb/src/embeddings/TransformersEmbeddingFunction.ts:33 makes it result in random segment fault errors terminating the tests prematurely.
This fix contains a code that bypasses chromadb package and directly uses the @xenova/transformers package

Due to how jest isolates the context of each running test (xenova/transformers.js#57, https://github.com/kayahr/jest-environment-node-single-context,
jestjs/jest#2549) - it makes it impossible for onnxruntime-node package to validate the array passed as an input to it is actually an `instanceof Float32Array`
type. The `instanceof` results in false because the globals are different between context. This commit shares the Float32Array global between each context.
Durisvk pushed a commit to Durisvk/langchainjs that referenced this issue Sep 3, 2023
…shared in jest context isolation

The way chromadb imports @xenova/transformers package in file
chromadb/src/embeddings/TransformersEmbeddingFunction.ts:33 makes it result in random segment fault errors terminating the tests prematurely.
This fix contains a code that bypasses chromadb package and directly uses the @xenova/transformers package

Due to how jest isolates the context of each running test (xenova/transformers.js#57, https://github.com/kayahr/jest-environment-node-single-context,
jestjs/jest#2549) - it makes it impossible for onnxruntime-node package to validate the array passed as an input to it is actually an `instanceof Float32Array`
type. The `instanceof` results in false because the globals are different between context. This commit shares the Float32Array global between each context.
Durisvk pushed a commit to Durisvk/langchainjs that referenced this issue Sep 3, 2023
…shared in jest context isolation

The way chromadb imports @xenova/transformers package in file
chromadb/src/embeddings/TransformersEmbeddingFunction.ts:33 makes it result in random segment fault errors terminating the tests prematurely.
This fix contains a code that bypasses chromadb package and directly uses the @xenova/transformers package

Due to how jest isolates the context of each running test (xenova/transformers.js#57, https://github.com/kayahr/jest-environment-node-single-context,
jestjs/jest#2549) - it makes it impossible for onnxruntime-node package to validate the array passed as an input to it is actually an `instanceof Float32Array`
type. The `instanceof` results in false because the globals are different between context. This commit shares the Float32Array global between each context.
jacoblee93 added a commit to langchain-ai/langchainjs that referenced this issue Sep 5, 2023
…shared in jest context isolation (#2487)

* Local embeddings function with Transformer.js API

* added packages

* Adds integration test, update naming and deps

* Formatting

* Fix random test fails due to segfault by chromadb & Float32Array not shared in jest context isolation

The way chromadb imports @xenova/transformers package in file
chromadb/src/embeddings/TransformersEmbeddingFunction.ts:33 makes it result in random segment fault errors terminating the tests prematurely.
This fix contains a code that bypasses chromadb package and directly uses the @xenova/transformers package

Due to how jest isolates the context of each running test (xenova/transformers.js#57, https://github.com/kayahr/jest-environment-node-single-context,
jestjs/jest#2549) - it makes it impossible for onnxruntime-node package to validate the array passed as an input to it is actually an `instanceof Float32Array`
type. The `instanceof` results in false because the globals are different between context. This commit shares the Float32Array global between each context.

* Update deps

* Docs change

* Add entrypoint

* Fix concurrency

* Docs update

* Fix docs typo

---------

Co-authored-by: l4b4r4b4b4 <lucas.cansino@mail.de>
Co-authored-by: Lucas Hänke de Cansino <lhc@next-boss.eu>
Co-authored-by: jacoblee93 <jacoblee93@gmail.com>
Co-authored-by: Juraj Carnogursky <durivk2@gmail.com>
@CMCDragonkai
Copy link

So does this issue not have an official workaround or solution atm?

I had to switch tests to doing this:

    // This should throw a range error, cannot test against `RangeError` due to jest
    expect(
      socket.send(msg, 0, '::1')
    ).rejects.toHaveProperty('name', 'RangeError');

@shunkica
Copy link

shunkica commented Dec 13, 2023

Well this just rendered jest unusable for me.
Totally breaks libxmljs functionality when running under jest. The Document.get() function returns an array with one libxmljs.Element instead of just the libxmljs.Element as it should.
It's not like I spent two hours trying to figure out what the heck was happening or anything....

Edit:
I cba to switch to a different framework so I put this in my jest setup file:

import * as libxmljs from "libxmljs";

libxmljs.Document.prototype.get = function () {
    const res = this.find.apply(this, arguments as any);
    if (Array.isArray(res)) {
        return res[0];
    } else {
        return res;
    }
};

libxmljs.Element.prototype.get = function () {
    const res = this.find.apply(this, arguments as any);
    if (Array.isArray(res)) {
        return res[0] instanceof libxmljs.Element ? res[0] : null;
    } else {
        return res;
    }
};

@tobiashochguertel
Copy link

In the mswjs project we faced similar issues and have the following workaround / solution developed which might be helpful for jest to integrate?

File: jest.config.js

/** @type {import('ts-jest').JestConfigWithTsJest} */
module.exports = {
    verbose: true,
    preset: 'ts-jest',
    testEnvironment: "<rootDir>/jsdom-extended.js",
    // Next Line(s) is important! https://github.com/mswjs/msw/issues/1786#issuecomment-1782559851
    testEnvironmentOptions: {
        customExportConditions: [''],
    },

File: jsdom-extended.js

const JSDOMEnvironment = require("jest-environment-jsdom").default; // or import JSDOMEnvironment from 'jest-environment-jsdom' if you are using ESM modules

class JSDOMEnvironmentExtended extends JSDOMEnvironment {
    constructor(...args) {
        super(...args);

        this.global.ReadableStream = ReadableStream;
        this.global.TextDecoder = TextDecoder;
        this.global.TextEncoder = TextEncoder;

        this.global.Blob = Blob;
        this.global.File = File;
        this.global.Headers = Headers;
        this.global.FormData = FormData;
        this.global.Request = Request;
        this.global.Response = Response;
        this.global.Request = Request;
        this.global.Response = Response;
        this.global.fetch = fetch;
        this.global.structuredClone = structuredClone;
    }
}

module.exports = JSDOMEnvironmentExtended;

@kettanaito
Copy link
Contributor

kettanaito commented Feb 1, 2024

Hi, folks 👋

From my experience, the main pain point when testing in Jest + JSDOM comes from jest-environment-jsdom that doesn't extend the jest-environment-node. Not only that strips the end developer off all the Node.js globals, but it also doesn't (or incorrectly) polyfill the global browser APIs (see mswjs/msw#1916 (comment)). fetch doesn't exist, neither does ReadableStream, or Headers, or structuredClone. Those are standard, globally available APIs in both browser and Node.js, but not if you decide to use jest-environment-jsdom.

This looks like a fundamental design issue to me. I propose you change jest-environment-jsdom to extend jest-environment-node and have those issues solved automatically.

While JSDOM is meant to emulate a browser environment, there's no lie that the tests are still running in Node.js. Respecting the actual environment in which the tests are run is crucial. I hope you find this important and we can have a productive discussion on how to resolve this for everyone.

@kayahr
Copy link

kayahr commented Feb 1, 2024

@kettanaito Are you sure you are in the correct issue? This issue is not about jsdom environment and missing globals. It is about node environment and WRONG globals because of context isolation. Or was that a direct answer to the previous post which also mentions jsdom?

@kettanaito
Copy link
Contributor

@kayahr, sorry for the confusion. Yes, my comment was an elaboration to the previously posted comment.

Afaik, Jest has moved away from meddling with globals in Node, so this original issue shouldn't be reproducible anymore (Jest remaps all present Node globals to the jest-environment-node automatically, nothing should be missing). jest-environment-jsdom does, however, still miss Node globals, to which I've commented.

favoyang pushed a commit to openupm/openupm-cli that referenced this issue Mar 10, 2024
* refactor: remove explicit mocha imports

* deps: add ts-jest

* refactor: rename test files

By default uses the convention of test-files being names *.test.ts. Renamed test files accordingly

* conf: create jest config

Uses ts-jest

* refactor: use body syntax in tests

* refactor: error assertion

We cannot use `instanceOf Error` in jest because of jestjs/jest#2549

* fix: console log issue

Jest automatically consumed and cleared calls to `console.log`. Fixed according to
https://stackoverflow.com/questions/54190596/no-console-log-to-stdout-when-running-npm-test-jest

* refactor: transition to jest

* deps: remove mocha
@davidjb
Copy link
Contributor

davidjb commented Mar 13, 2024

@kettanaito This issue is still present in the latest versions of Jest v29.7.0 and v30.0.0-alpha.3. https://github.com/thomas-huston-zocdoc/jest-fetch-array-bug is still reproducing the issue for Array (with Jest suitably upgraded). In my case, instanceof Error returns true under Node but still returns false under Jest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Discussion Has Bounty There is a bounty on the open collective for this issue Pinned
Projects
None yet
Development

Successfully merging a pull request may close this issue.