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 · 41 comments

Comments

Projects
None yet
@thomashuston
Copy link
Contributor

thomashuston commented Jan 10, 2017

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

PlasmaPower commented Mar 1, 2017

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

PlasmaPower commented Mar 1, 2017

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

PlasmaPower commented Mar 2, 2017

They come from node native libraries like fs or http.

@cpojer

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

cpojer commented Mar 2, 2017

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

@suchipi

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

cpojer commented Mar 2, 2017

Yeah, that sounds like a good start.

@suchipi

This comment has been minimized.

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

This comment has been minimized.

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?

@joedynamite

This comment has been minimized.

Copy link

joedynamite commented Mar 7, 2017

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

This comment has been minimized.

Copy link

PlasmaPower commented Mar 7, 2017

@joedynamite sounds like the same issue

@PlasmaPower

This comment has been minimized.

Copy link

PlasmaPower commented Mar 7, 2017

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

This comment has been minimized.

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

This comment has been minimized.

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.

@L0stSoul

This comment has been minimized.

Copy link

L0stSoul commented May 21, 2018

@SimenB small example with Array(simplified one):

 it.only('multiple parameters', function () {
     let url = require('url');
     let query = url.parse('https://www.rakuten.co.jp/?f=1&f=2&f=3', true).query;
     console.log(query.f); // [ '1', '2', '3' ]
     console.log(query.f instanceof Array); //false
});
@L0stSoul

This comment has been minimized.

Copy link

L0stSoul commented May 21, 2018

btw i'm not fully understand why this labeled as "enchantment" instead of "bug" - Changing behaviour of instanceof for specific cases in the whole project that jest supposed to test, not looks like normal behaviour.

@Bradcomp

This comment has been minimized.

Copy link

Bradcomp commented Jul 18, 2018

Is there any progress on this? This is leading to a very frustrating situation in one of my projects right now where a third party library is doing an instanceof check, and failing to recognize an array...

@Morikko

This comment has been minimized.

Copy link

Morikko commented Aug 17, 2018

For the one willing to parse cookies with the extension cookie-session, a guy wrote a nice and easy solution on Medium.

@ssetem

This comment has been minimized.

Copy link

ssetem commented Sep 20, 2018

Found a solution, we were having issues testing multiple set-cookie headers in node.js tests, this worked for us

Step 1
Create a testEnvironment file, we put it in utils/ArrayFixNodeEnvironment.js

Object.defineProperty(Array, Symbol.hasInstance, {
    value(target) {
        return Array.isArray(target)
    }
});

module.exports = require('jest-environment-node')

Step 2
Run jest with --testEnvironment flag

jest --testEnvironment ./utils/ArrayFixNodeEnvironment.js
@meltedspark

This comment has been minimized.

Copy link

meltedspark commented Oct 27, 2018

@ssetem You're a lifesaver, thank you!

@orta orta added the Has Bounty label Dec 29, 2018

@orta

This comment has been minimized.

Copy link
Collaborator

orta commented Dec 29, 2018

Hey folks, to get this moving forwards - we've decided to put a $599 bounty on this issue from our OpenCollective account.

If you're interested in doing some Open Source good-will and get paid for it, we're looking to have this shipped to production. There are a few ideas already e.g. #5995 which you could take over, or you could start from fresh too

To get the bounty you would need to submit an expense via OpenCollective, here's their FAQ and I'll help that part of the process

@jamesgeorge007

This comment has been minimized.

Copy link
Contributor

jamesgeorge007 commented Jan 26, 2019

@orta @SimenB I would love to work on what is left further. It would be great if you could give me the proper guidance as I'm not that aware of the context here 👍

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 26, 2019

@jamesgeorge007 that's awesome! Not sure what information you're after - there's a bunch of reproducing examples in this issue (and linked) ones. You can also see my PR #5995 and take a look at fixing the errors from its CI run. I can rebase that PR now so it's fresh, though 🙂

Any questions in particular?

@dhans

This comment has been minimized.

Copy link

dhans commented Feb 6, 2019

Unclear if we were hitting exactly the same bug - but upgrading the test environment to Node.js 10 resolved an issue for us where tests running under jest using supertest were failing to find the existing session, using multiple cookies.

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Feb 6, 2019

That's expected, Node fixed their code to use Array.isArray. See #5995 (comment)

That's just a symptom though, the underlying issue is not solved

@RLovelett

This comment has been minimized.

Copy link

RLovelett commented Feb 6, 2019

Found a solution, we were having issues testing multiple set-cookie headers in node.js tests, this worked for us

Step 1
Create a testEnvironment file, we put it in utils/ArrayFixNodeEnvironment.js

Object.defineProperty(Array, Symbol.hasInstance, {
    value(target) {
        return Array.isArray(target)
    }
});

module.exports = require('jest-environment-node')

Step 2
Run jest with --testEnvironment flag

jest --testEnvironment ./utils/ArrayFixNodeEnvironment.js

Unfortunately @ssetem's work-around does not seem to be working for ArrayBuffer on Node v8.x or Node v10.x

console.log('ArrayBufferFixNodeEnvironment');

// Fix for Jest in node v8.x and v10.x
Object.defineProperty(ArrayBuffer, Symbol.hasInstance, {
    value(inst) {
        return inst && inst.constructor && inst.constructor.name === 'ArrayBuffer';
    }
});

module.exports = require('jest-environment-node')

AFAICT it never calls the value function at all. I see it call print ArrayBufferFixNodeEnvironment when running so I'm fairly confident that the environment is being loaded.

jsdom here I come 🤷‍♂️

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