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 vs Bazel: Something is broken #5367

Closed
wants to merge 1 commit into from
Closed

Jest vs Bazel: Something is broken #5367

wants to merge 1 commit into from

Conversation

hurryabit
Copy link
Contributor

@hurryabit hurryabit commented Apr 2, 2020

Running the test in the new project via yarn test works just fine.
However, running the same tests via

bazel test //language-support/ts/jest_vs_bazel:test

doesn't work. The test fails because it thinks the ws.server object
is not an instance of class Server. This clearly contradicts the
implementation of the WS class at
https://github.com/romgain/jest-websocket-mock/blob/eb8324ff0d4fd1a09effd994cba142f240fb8f7a/src/websocket.ts#L48

My suspicion is that bazel somehow brings multiple versions of the
Server class from mock-socket into scope. If you drill into
node_modules/mock-socket, you'll see that package.json lists
"main": "./dist/mock-socket" and there are five files in dist:

  • mock-socket.amd.js
  • mock-socket.cjs.js
  • mock-socket.es.js
  • mock-socket.es.mjs
  • mock-socket.js
    The is also src/index.js which exports a Server class. So, there's
    plenty of ooportunity for bazel to get confused.

Despite that, I wonder how jest gets it right on its own but bazel
manages to confuse jest to get it wrong under bazel's guidance.

CHANGELOG_BEGIN
CHANGELOG_END

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.


This change is Reviewable

Running the test in the new project via `yarn test` works just fine.
However, running the same tests via
```
bazel test //language-support/ts/jest_vs_bazel:test
```
doesn't work. The test fails because it thinks the `ws.server` object
is not an instance of class `Server`. This clearly contradicts the
implementation of the `WS` class at
https://github.com/romgain/jest-websocket-mock/blob/eb8324ff0d4fd1a09effd994cba142f240fb8f7a/src/websocket.ts#L48

My suspicion is that bazel somehow brings multiple versions of the
`Server` class from `mock-socket` into scope. If you drill into
`node_modules/mock-socket`, you'll see that `package.json` lists
`"main": "./dist/mock-socket"` and there are five files in `dist`:
- `mock-socket.amd.js`
- `mock-socket.cjs.js`
- `mock-socket.es.js`
- `mock-socket.es.mjs`
- `mock-socket.js`
The is also `src/index.js` which exports a `Server` class. So, there's
plenty of ooportunity for `bazel` to get confused.

Despite that, I wonder how `jest` gets it right on its own but `bazel`
manages to confuse `jest` to get it wrong under `bazel`'s guidance.

CHANGELOG_BEGIN
CHANGELOG_END
@hurryabit hurryabit requested review from a user and aherrmann-da April 2, 2020 09:20
@aherrmann-da
Copy link
Contributor

I've been able to reproduce this issue in isolation https://github.com/aherrmann/mock_class_mismatch using vanilla rules_nodejs-1.1.0. So, this should not be related to any specifics of the daml repository.

I've used the following --node_options to try and trace module loading.

        "--node_options=--trace-event-categories=node",
        "--node_options=--trace-event-file-pattern=/tmp/node-trace-$${pid}-$${rotation}.log",

According to the output we are consistently loading the common-js versions under yarn

node_modules/jest-websocket-mock/lib/jest-websocket-mock.cjs.js
node_modules/mock-socket/dist/mock-socket.cjs.js

while loading mixed versions under Bazel

node_modules/jest-websocket-mock/lib/jest-websocket-mock.cjs.js
node_modules/mock-socket/dist/mock-socket.js

However, this does not seem to be the cause of the issue. I've patched the node_modules such that only the common-js versions are available and the same trace flags as above confirm that then also under Bazel only common-js versions are loaded. Nonetheless, the issue persists.

According to jestjs/jest#2549 using instanceof is problematic under Jest, because Jest runs tests under a sandbox context where classes can differ from their versions under the global Node context. The issue mentions differences between node versions. I have confirmed that the tests pass using yarn with node version 13.7.0 but fail under bazel with the same node version. I've also tried the SingleContextNodeEnvironment workaround described in that issue, sadly without success. It is still unclear why the tests pass under yarn but fail under bazel.

@hurryabit
Copy link
Contributor Author

@aherrmann Thanks for the progress report. Great work! This seems to be a very dire problem. 😿

Regarding instanceof in jest, we should not be affected by this issue since the Server class is not coming from node but from one of the packages.

cocreature added a commit that referenced this pull request Apr 13, 2020
closes #5367

This includes the fixes for the issues in jest that we’ve been seeing.

changelog_begin
changelog_end
aherrmann-da pushed a commit that referenced this pull request Apr 17, 2020
closes #5367

This includes the fixes for the issues in jest that we’ve been seeing.

changelog_begin
changelog_end
@mergify mergify bot closed this in #5539 Apr 17, 2020
mergify bot pushed a commit that referenced this pull request Apr 17, 2020
* Upgrade rules_nodejs to version 1.6.0

closes #5367

This includes the fixes for the issues in jest that we’ve been seeing.

changelog_begin
changelog_end

* Fix eslint rules

* A bit of progress

* Try to add LinkablePackageInfo (doesn’t seem to work yet)

* Add rootDirs

* revert da_ts_library

* da_ts_library: add LinkablePackageInfo info

* Remove react hook workaround

Since rules_nodejs 1.6.0 this fails with the following error:
```
  ● Test suite failed to run

    Configuration error:

    Could not locate module react mapped as:
    /.../execroot/com_github_digital_asset_daml/bazel-out/k8-opt/bin/language-support/ts/daml-react/test.sh.runfiles/com_github_digital_asset_daml/node_modules/react/umd/react.development.js.

    Please check your configuration for these entries:
    {
      "moduleNameMapper": {
        "/^react$/": "/.../execroot/com_github_digital_asset_daml/bazel-out/k8-opt/bin/language-support/ts/daml-react/test.sh.runfiles/com_github_digital_asset_daml/node_modules/react/umd/react.development.js"
      },
      "resolver": null
    }

      49 | // like a promis without being one.
      50 | /* eslint-disable @typescript-eslint/no-floating-promises */
    > 51 | var react_1 = __importStar(require("react"));
         |                            ^
      52 | var react_hooks_1 = require("@testing-library/react-hooks");
      53 | var index_1 = __importStar(require("./index"));
      54 | var events_1 = require("events");

      at createNoMappedModuleFoundError (../../../../../../../../../../../node_modules/jest-resolve/build/index.js:501:17)
      at Object.<anonymous> (index.test.js:51:28)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        1.88s
Ran all test suites within paths "language-support/ts/daml-react/DamlLedger.d.ts", "language-support/ts/daml-react/DamlLedger.js", "language-support/ts/daml-react/context.d.ts", "language-support/ts/daml-react/context.js", "language-support/ts/daml-react/hooks.d.ts", "language-support/ts/daml-react/hooks.js", "language-support/ts/daml-react/index.d.ts", "language-support/ts/daml-react/index.js", "language-support/ts/daml-react/index.test.d.ts", "language-support/ts/daml-react/index.test.js".
=
```

* rootDirs is not needed for tsc

This is only required for ts_project

* Update yarn Bazel packages

* docs/theme add missing dependencies

* Remove unused attribute module_root

Co-authored-by: Andreas Herrmann <andreas.herrmann@tweag.io>
@hurryabit hurryabit deleted the jest-vs-bazel branch July 28, 2020 09:09
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

Successfully merging this pull request may close these issues.

2 participants