jest
jest copied to clipboard
jestjs
Jest globals differ from Node globals
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 https://github.com/facebook/jest/issues/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
This is likely due to the behavior of vm; see https://github.com/nodejs/node-v0.x-archive/issues/1277
Does Jest do anything to try to avoid this right now?
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.
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.
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.
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.
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.
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.
Ah, hmm, that's a good point. It works for primitives but not that well for errors or arrays :(
What if jest transformed instanceof Array and instanceof Error specifically into something like instanceof jest.__parentContextArray and instanceof jest.__parentContextError?
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?
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.
Assuming performance seems fine, which globals should it be applied to? Error and Array... Buffer maybe, too?
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?
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?
@suchipi https://gist.github.com/PlasmaPower/4a59544f4e631e32515f927eef8a08a3
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
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.
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): https://github.com/facebook/jest/issues/3547#issuecomment-302541653
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.
I'm having trouble with http headers: The following nodejs(8.9.1) code doesn't work in jest, I assume it has to do with an Array check?
const http = require('http');
const COOKIE = [ 'sess=fo; path=/; expires=Thu, 25 Jan 2018 02:09:07 GMT; httponly',
'sess.sig=bar; path=/; expires=Thu, 25 Jan 2018 02:09:07 GMT; httponly' ]
const server = http.createServer((req, res) => {
res.setHeader('Set-Cookie', COOKIE);
res.end();
});
server.listen(8000);
@suchipi have you found any more time to be able to work on this? Would be amazing to solve this, and your idea of patching the instanceOf checks (which I didn't even know was possible, gotta ❤️ JS) seems like a really good solution.
I haven't looked at it in ages, but I still have a branch somewhere. I might be able to take a look this weekend.
PR: #5995.
Please provide small reproductions of cases with different failing globals. The PR for now just handles Error (with 2 tests provided as reproductions in this issue), would love to have tests for Function, Symbol, Array etc as well
@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
});
