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's expect toBeInstanceOf fails in rules_nodejs #1813

Closed
aherrmann opened this issue Apr 9, 2020 · 5 comments
Closed

Jest's expect toBeInstanceOf fails in rules_nodejs #1813

aherrmann opened this issue Apr 9, 2020 · 5 comments

Comments

@aherrmann
Copy link
Member

🐞 bug report

Affected Rule

The issue is caused by the rule: jest_test from @npm//jest-cli:index.bzl

Is this a regression?

Not that I'm aware of.

Description

A jest test making use of expect(...).toBeInstanceOf(...) fails when it should succeed, the same test succeeds when run using yarn test.

🔬 Minimal Reproduction

  • Checkout https://github.com/aherrmann/mock_class_mismatch (master fec40bbe7159854dbcb6c6f9ab81e28aa624e5d8 at the time of writing).
  • Execute
    $ bazel test //:repro --test_output=streamed
    
  • Observe the following test failure:
      ...
      expect(received).toBeInstanceOf(expected)
    
      Expected constructor: Server
      Received constructor: Server
    
        10 |   console.log(ws.server.constructor);  // [Function: Server] { of: [Function: of] }
        11 |   console.log(Server);  // [Function: Server] { of: [Function: of] }
      > 12 |   expect(ws.server).toBeInstanceOf(Server);  // FAILS
           |                     ^
        13 | });
        14 |
      ...
    
  • For comparison, run the test with yarn and see it succeed:
    $ yarn test
    

🔥 Exception or Error

  ...
  expect(received).toBeInstanceOf(expected)

  Expected constructor: Server
  Received constructor: Server

    10 |   console.log(ws.server.constructor);  // [Function: Server] { of: [Function: of] }
    11 |   console.log(Server);  // [Function: Server] { of: [Function: of] }
  > 12 |   expect(ws.server).toBeInstanceOf(Server);  // FAILS
       |                     ^
    13 | });
    14 |
  ...

🌍 Your Environment

Operating System:

  
Ubuntu 19.10
  

Output of bazel version:

  
2.0.0
  

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
1.1.0
  

Anything else relevant?

  • yarn version 1.21.11

  • node version 13.7.0 under yarn, tried both 12.13.0 and 13.7.0 under bazel, issue observed in both cases.

  • node_modules contains multiple versions (.cjs.js, .es.js, ...) for the packages jest-websocket-mock and mock-socket.

    $ ls node_modules/jest-websocket-mock/lib/
    index.d.ts                  jest-websocket-mock.es.js  queue.d.ts
    jest-websocket-mock.cjs.js  matchers.d.ts              websocket.d.ts
    $ ls node_modules/mock-socket/dist/
    mock-socket.amd.js  mock-socket.es.js   mock-socket.js
    mock-socket.cjs.js  mock-socket.es.mjs
    

    It may be that the class Server is loaded from different versions between jest-websocket-mock and mock-socket and that this causes the discrepancy.

    Using --node-options=--trace-event-categories=node I've tried tracing which formats are loaded. Indeed, under yarn I see that both modules are loaded from .cjs.js files, while under Bazel jest-websocket-mock is loaded from .cjs.js but mock-socket from plain .js.

    However, I tried patching both packages to drop all but .cjs.js files and the issue persists. I've verified with --trace-event-categories that in this case only .cjs.js versions are loaded and the node_modules folder in the sandbox only contains .cjs.js files.

  • There are cases under which isinstanceof is problematic in jest (Jest globals differ from Node globals jestjs/jest#2549), however, to my understanding we shouldn't be affected by this issue in this case. If, we should be hitting it in yarn as well, and I've tried the suggested SingleContextNodeEnvironment workaround without success.

  • I tried using Babel as described in the jest example in the rules_nodejs repository but also without success.

  • Test with Jest and Enzyme Internal Error #1651 could be a related issue.

  • Further context in Jest vs Bazel: Something is broken digital-asset/daml#5367

@gregmagolan
Copy link
Collaborator

Thanks for the repo! I was just about to work on a repro for this issue I'm fixing the underlying cause so this is very helpful.

It is fixed in different ways by both #1800 and #1805. #1800 has landed and #1805 is going to land today so the issue should be resolved on the next release which should come later today or tomorrow.

Marking as fixed since #1800 was merged yesterday.

gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Apr 9, 2020
Issue bazel-contrib#1813 was fixed by bazel-contrib#1800 which is already landed. bazel-contrib#1805 also fixes via a different mechanism and will also land soon.
@gregmagolan
Copy link
Collaborator

Added your repo to rules_nodejs for coverage. #1814. Should be green since #1800 has already landed.

gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Apr 9, 2020
Issue bazel-contrib#1813 was fixed by bazel-contrib#1800 which is already landed. bazel-contrib#1805 also fixes via a different mechanism and will also land soon.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Apr 9, 2020
Issue bazel-contrib#1813 was fixed by bazel-contrib#1800 which is already landed. bazel-contrib#1805 also fixes via a different mechanism and will also land soon.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Apr 9, 2020
Issue bazel-contrib#1813 was fixed by bazel-contrib#1800 which is already landed. bazel-contrib#1805 also fixes via a different mechanism and will also land soon.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Apr 9, 2020
Issue bazel-contrib#1813 was fixed by bazel-contrib#1800 which is already landed. bazel-contrib#1805 also fixes via a different mechanism and will also land soon.
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Apr 9, 2020
Issue bazel-contrib#1813 was fixed by bazel-contrib#1800 which is already landed. bazel-contrib#1805 also fixes via a different mechanism and will also land soon.
gregmagolan added a commit that referenced this issue Apr 9, 2020
Issue #1813 was fixed by #1800 which is already landed. #1805 also fixes via a different mechanism and will also land soon.
@aherrmann
Copy link
Member Author

@gregmagolan Thanks a lot, that's great news. I can confirm that the issue is resolved with rules_nodejs 1.6.0.

@whilp
Copy link

whilp commented Apr 14, 2020

I see that the jest-serializer pin is still present in package.json as of 1.6.0:

https://github.com/bazelbuild/rules_nodejs/blob/bd83907954d1b3cd53dff01c37e3c4964890ef5c/package.json#L93-L96

Is there another bug that needs to be resolved before that pin can be removed?

@whilp
Copy link

whilp commented Jun 2, 2020

Hi! the jest-serializer pin is still present:

https://github.com/bazelbuild/rules_nodejs/blob/58fe08b6e4944be5b32261915b91c57bc9a89426/package.json#L100-L103

Is it still needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants