Fix `instanceof` checks #5995

SimenB · 2018-04-15T16:24:55Z

Summary

This is a rebase of @suchipi's https://github.com/suchipi/jest/tree/instanceof_overrides with added tests.

I added the two small reproductions from #2549 for Error, but I'd love to get small examples of other globals breaking (Object, Array, Buffer, Symbol comes to mind). Can anyone provide those?

Fixes #2549.

Test plan

Tests added (but I want more of them).

SimenB · 2018-04-15T16:28:28Z

packages/jest-util/src/install_common_globals.js

@@ -33,5 +34,7 @@ export default function(globalObject: Global, globals: ConfigGlobals) {
  globalObject.setImmediate = global.setImmediate;
  globalObject.clearImmediate = global.clearImmediate;

+  addInstanceOfAlias(globalObject.Error, Error);


We should probably have some sort of blacklist and just loop all globals, rather than patching them in as people report bugs.

Yes, please.

SimenB · 2018-04-15T16:29:45Z

integration-tests/instanceof/__tests__/test.js

+});
+
+test('Array', () => {
+  expect([]).toBeInstanceOf(Array);


this assertion passes without change in jest...

Does it make sense to test it in jsdom environment too?

Too bad it's not supported as a flag, but yeah, I can run the tests in both envs

SimenB · 2018-04-15T16:54:26Z

This seems to have broken node assert support... I'll see if I can take a look at it later this coming week. Help appreciated!

cpojer · 2018-04-15T17:39:55Z

packages/jest-util/src/add_instance_of_alias.js

+ */
+'use strict';
+
+export default function addInstanceOfAlias(target: Object, alias: Object) {


I don't know if alias is a good name. I only know alias in terms of giving things a different name but this is setting a different constructor. How about addInstanceOfObject or addInstanceOfConstructor or addInstanceParent or similar?

cpojer · 2018-04-15T17:42:58Z

This is really super cool. Didn't know this feature in JS. TIL.

suchipi · 2018-04-15T18:04:03Z

@SimenB thanks for picking this up, sorry I never got around to finishing it

SimenB · 2018-04-15T18:48:23Z

@suchipi Thanks for pointing Symbol.hasInstance!

Btw, do you have any ideas about why this instanceof fails with the changes in this PR: https://github.com/facebook/jest/blob/65d3d8d37e471ae9a02150933168470d9966c021/packages/jest-jasmine2/src/jasmine/Spec.js#L135?

cpojer · 2018-04-15T18:50:15Z

Can we use duck-typing in that assertion?

SimenB · 2018-04-15T18:52:34Z

I had a hope that instanceof would just work:tm: with this change. But as long as it works in user's test code while we have to fake it in Jest internals, that should be fine

suchipi · 2018-04-15T20:39:45Z

I'm not sure why that's failing; before merging, it'd be good to verify the same issue doesn't occur in user code when using AssertionError

SimenB · 2018-05-21T05:20:06Z

Tried a rebase. Seems like the issue here is that all someError instanceof AnyError evaluate to true with this diff

Bradcomp · 2018-07-16T18:06:43Z

Is there any progress on this? I just ran into the same issue and it's really annoying.

edevil · 2018-10-08T14:29:22Z

Is there anything that I can do to help move this along? I just spent 2 hours trying to track down why parsing the "Set-Cookie" header was not working only on tests.

SimenB · 2018-10-08T14:37:43Z

You can play with it and see if you can get the tests to pass. I can give it a quick rebase first, though 🙂

All help welcome! This is probably the biggest pain point I have with Jest, and a huge gotcha

SimenB · 2018-10-08T14:48:47Z

packages/jest-jasmine2/src/jasmine/Spec.js

@@ -148,12 +146,12 @@ Spec.prototype.onException = function onException(error) {
    return;
  }

-  if (error instanceof ExpectationFailed) {
-    return;
+  if (error && error.code === 'ERR_ASSERTION') {


not happy with this. The conditional itself is fine (probably better than instanceof), but this shouldn't be a necessary change

edevil · 2018-10-08T16:21:42Z

@SimenB FWIW, I tried Jest with this PR and it did not solve the bug.

I'm using superagent, and the multiple set-cookie headers on a response are still returned concatenated instead of as separate elements in an array, as it does outside Jest.

I linked jest-cli and jest-util, is this not enough to exercise the new code?

SimenB · 2018-10-08T18:47:31Z

Can you share your reproduction? I can add it as a test case 🙂

edevil · 2018-10-08T22:01:36Z

@SimenB This is one from the other ticket:

    let url = require('url');
    let query = url.parse('https://brpx.com/?f=1&f=2&f=3', true).query;
    console.log(query.f instanceof Array);

This is fixed by addInstanceOfAlias(globalObject.Array, Array);

However, this also does not fix my problem. I'm trying to find a small repro script but I've just noticed that in node 10 the problem does not happen, only in 8, so I'm less motivated now. :)

edevil · 2018-10-08T22:32:54Z

I've noticed that in Node 10 several instanceof Array were changed to Array.isArray (https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L398). This may explain why it now works. But why can't this Array alias work in Node 8? Is it because it's internal Node code?

edevil · 2018-10-09T09:48:02Z

I just confirmed that the problem is on https://github.com/nodejs/node/blob/v8.x/lib/_http_outgoing.js#L313.

if (value instanceof Array) {

This check is not fixed even when aliasing Array with this PR.

SimenB · 2019-01-26T15:30:37Z

@edevil that's fixed in newer versions of Node, thankfully 🙂 (as you mention)

nodejs/node#20250

If we can get this working for node 10, I'm perfectly fine with that (although it would be great if we could find a solution that works for all supported versions of node)

mcheshkov · 2019-04-13T17:05:32Z

I was trying to extract this to local jest environment, and found a bug.

[Symbol.hasInstance] implementation in this PR does not validate which constructor was instanceof originally called for. So, for obj instanceof Subclass it will try to call Subclass[Symbol.hasInstance], and, if it's not defined it will fall through to Class[Symbol.hasInstance], which resolves to true, which is may be correct result for Class, but incorrect for Subclass

Try running this:

class Class {
  static [Symbol.hasInstance](){ console.log('Class hasInstance'); return true; }
}
class Subclass extends Class {}
const obj = new Class(); // or even {} for this example
console.log(obj instanceof Subclass);

I've fixed this locally using this === target in hasInstance implementation. this resolves to actual constructor in RHS of instanceof, and target is one we are patching.

rodush · 2020-02-22T06:55:07Z

Guys, this seems to be abandoned, but the fix is really needed to be able to (integration-) test rest apps, with e.g. restify, and the supertest.
Currently, the status code of the original error (one of restify-errors, e.g.: 503) passed to the next() callback, is swallowed and lost.
It results in supertest receives the incorrect status code (500 instead of 503 or any other custom code).

julienw · 2020-06-02T12:23:07Z

FYI I found an instance of (pun not intended) this problem in koajs/koa#1466.

kayahr · 2021-10-01T06:38:40Z

You were asking for more tests. I collected a bunch here: SingleContextNodeEnvironment.test.ts

Most of these tests fail with standard Jest but all work when using jest-environment-node-single-context

Some of the tests may look very constructed but they simulate problems happening for real when working with types returned by Node APIs. I just had to be creative for these tests to get the various datatypes directly from Node.

SimenB · 2021-10-01T08:39:50Z

Hah wow, almost 3.5 years old this PR 🙈

Thanks a bunch @kayahr! I'll rebase this and add those. I have zero recollection of the state of this PR, but let's see where it's at.

I need a screenshot to remember the times we supported node 6, tho! 😀

SimenB · 2021-10-01T09:14:39Z

@mcheshkov I don't follow your example. Running your code in plain Node logs out true - and the same works in Jest without any changes at all (and in this PR)

github-actions · 2022-10-01T09:32:03Z

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions · 2022-10-31T09:44:42Z

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

github-actions · 2022-12-01T00:09:59Z

This pull request 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.

SimenB requested review from cpojer, mjesun, aaronabramov, rickhanlonii and thymikee April 15, 2018 16:24

facebook-github-bot added the cla signed label Apr 15, 2018

SimenB mentioned this pull request Apr 15, 2018

Jest globals differ from Node globals #2549

Open

SimenB commented Apr 15, 2018

View reviewed changes

cpojer reviewed Apr 15, 2018

View reviewed changes

SimenB force-pushed the instanceof_overrides branch from c9a7b57 to 5cae4b3 Compare May 21, 2018 05:08

SimenB mentioned this pull request Aug 23, 2018

Date or Promise instantiated in test environment will not pass expect.any(Date|Promise) #6877

Closed

SimenB force-pushed the instanceof_overrides branch 2 times, most recently from e4db645 to 8c16548 Compare October 8, 2018 14:46

SimenB commented Oct 8, 2018

View reviewed changes

SimenB force-pushed the instanceof_overrides branch from ce6b56d to 4309d7a Compare January 26, 2019 15:25

This was referenced Feb 12, 2019

nodejs instanceof array e2e test and fix SimenB/jest#155

Closed

nodejs instanceof array e2e test and fix #7903

Closed

ljharb mentioned this pull request Mar 11, 2019

Final 6.x release before EOL? [Discussion] nodejs/Release#414

Closed

gkubisa mentioned this pull request Mar 26, 2019

Fix "instanceof Error" #8220

Closed

mxschmitt mentioned this pull request May 4, 2020

Timeout errors microsoft/playwright-test#46

Closed

suchipi and others added 5 commits October 1, 2021 10:42

Add addInstanceOfAlias

afea495

setup instanceof for Error, and add test

4f2731b

update changelog

d3a9ea7

convert to TS

4f046c5

update copyright

ca75368

SimenB force-pushed the instanceof_overrides branch from 4309d7a to ca75368 Compare October 1, 2021 09:00

SimenB added 2 commits October 1, 2021 11:22

add more tests and fix some of them

de079dc

do not export it

edbab88

github-actions bot added the Stale label Oct 1, 2022

github-actions bot closed this Oct 31, 2022

github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `instanceof` checks #5995

Fix `instanceof` checks #5995

SimenB commented Apr 15, 2018

SimenB Apr 15, 2018

edevil Oct 9, 2018

SimenB Apr 15, 2018

thymikee Apr 15, 2018

SimenB Apr 15, 2018

SimenB commented Apr 15, 2018 •

edited

Loading

cpojer Apr 15, 2018

cpojer commented Apr 15, 2018

suchipi commented Apr 15, 2018

SimenB commented Apr 15, 2018

cpojer commented Apr 15, 2018

SimenB commented Apr 15, 2018

suchipi commented Apr 15, 2018

SimenB commented May 21, 2018

Bradcomp commented Jul 16, 2018

edevil commented Oct 8, 2018

SimenB commented Oct 8, 2018

SimenB Oct 8, 2018 •

edited

Loading

edevil commented Oct 8, 2018

SimenB commented Oct 8, 2018

edevil commented Oct 8, 2018 •

edited

Loading

edevil commented Oct 8, 2018

edevil commented Oct 9, 2018

SimenB commented Jan 26, 2019 •

edited

Loading

mcheshkov commented Apr 13, 2019 •

edited

Loading

rodush commented Feb 22, 2020

julienw commented Jun 2, 2020

kayahr commented Oct 1, 2021

SimenB commented Oct 1, 2021 •

edited

Loading

SimenB commented Oct 1, 2021

github-actions bot commented Oct 1, 2022

github-actions bot commented Oct 31, 2022

github-actions bot commented Dec 1, 2022

Fix instanceof checks #5995

Fix instanceof checks #5995

Conversation

SimenB commented Apr 15, 2018

Summary

Test plan

SimenB Apr 15, 2018

Choose a reason for hiding this comment

edevil Oct 9, 2018

Choose a reason for hiding this comment

SimenB Apr 15, 2018

Choose a reason for hiding this comment

thymikee Apr 15, 2018

Choose a reason for hiding this comment

SimenB Apr 15, 2018

Choose a reason for hiding this comment

SimenB commented Apr 15, 2018 • edited Loading

cpojer Apr 15, 2018

Choose a reason for hiding this comment

cpojer commented Apr 15, 2018

suchipi commented Apr 15, 2018

SimenB commented Apr 15, 2018

cpojer commented Apr 15, 2018

SimenB commented Apr 15, 2018

suchipi commented Apr 15, 2018

SimenB commented May 21, 2018

Bradcomp commented Jul 16, 2018

edevil commented Oct 8, 2018

SimenB commented Oct 8, 2018

SimenB Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

edevil commented Oct 8, 2018

SimenB commented Oct 8, 2018

edevil commented Oct 8, 2018 • edited Loading

edevil commented Oct 8, 2018

edevil commented Oct 9, 2018

SimenB commented Jan 26, 2019 • edited Loading

mcheshkov commented Apr 13, 2019 • edited Loading

rodush commented Feb 22, 2020

julienw commented Jun 2, 2020

kayahr commented Oct 1, 2021

SimenB commented Oct 1, 2021 • edited Loading

SimenB commented Oct 1, 2021

github-actions bot commented Oct 1, 2022

github-actions bot commented Oct 31, 2022

github-actions bot commented Dec 1, 2022

Fix `instanceof` checks #5995

Fix `instanceof` checks #5995

SimenB commented Apr 15, 2018 •

edited

Loading

SimenB Oct 8, 2018 •

edited

Loading

edevil commented Oct 8, 2018 •

edited

Loading

SimenB commented Jan 26, 2019 •

edited

Loading

mcheshkov commented Apr 13, 2019 •

edited

Loading

SimenB commented Oct 1, 2021 •

edited

Loading