Page MenuHomePhabricator

[web] Fix missing WebCrypto mock in tests
ClosedPublic

Authored by bartek on Mar 9 2023, 9:15 AM.
Tags
None
Referenced Files
F2293305: D7019.diff
Wed, Jul 17, 3:05 AM
Unknown Object (File)
Mon, Jul 1, 12:43 AM
Unknown Object (File)
Mon, Jul 1, 12:43 AM
Unknown Object (File)
Mon, Jul 1, 12:43 AM
Unknown Object (File)
Mon, Jul 1, 12:40 AM
Unknown Object (File)
Sun, Jun 30, 11:37 PM
Unknown Object (File)
Fri, Jun 21, 2:02 PM
Unknown Object (File)
Fri, Jun 21, 11:58 AM
Subscribers

Details

Summary

Fixes https://linear.app/comm/issue/ENG-3276/web-unit-tests-mock-missing-webcrypto-apis

There are a few solutions, I decided to go with Jest setupFiles.

Some other solutions on StackOverflow: https://stackoverflow.com/questions/52612122/how-to-use-jest-to-test-functions-using-crypto-or-window-mscrypto - they use Object.defineProperty, but simply assigning the variable does the trick as well.

The crypto.webcrypto is a polyfill introduced in Node 15 that provides compatibility with browser APIs.

Test Plan
  • Run yarn test in web without this change. Tests for text-utils and device-id should fail.
  • Apply this diff and re-run tests. All tests should pass.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek requested review of this revision.Mar 9 2023, 9:31 AM
This revision is now accepted and ready to land.Mar 9 2023, 9:55 AM
web/jest-setup.js
6 ↗(On Diff #23575)

flow-typed doesn't handle Node types... those are in base Flow

Thanks for fixing these tests. Now that they're working, can we update this diff to include them in CI?

For Buildkite we use the package.json:jest-all command, so we should update that to include web:

040888.png (768×2 px, 357 KB)

For GitHub, we can just copy/paste one of the existing test steps for web:

523dcd.png (694×1 px, 171 KB)

Now that they're working, can we update this diff to include them in CI?

This is more related to this task: https://linear.app/comm/issue/ENG-2712/we-dont-run-tests-in-web-and-native-in-precommit-hook
So I'd rather do that when adding the hook

web/jest-setup.js
6 ↗(On Diff #23575)

Good catch, I'll change this

This is more related to this task: https://linear.app/comm/issue/ENG-2712/we-dont-run-tests-in-web-and-native-in-precommit-hook
So I'd rather do that when adding the hook

Yeah makes sense to couple those

Changed flow-typed -> Flow as suggested

This revision was landed with ongoing or failed builds.Mar 9 2023, 11:07 PM
This revision was automatically updated to reflect the committed changes.