[web] Fix missing WebCrypto mock in tests
ClosedPublic
Actions

Authored by bartek on Mar 9 2023, 9:15 AM.

Details

Reviewers

ashoat

Commits

rCOMM961052269b9f: [web] Fix missing WebCrypto mock in tests

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

Branch

barthap/aes-web

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Mar 9 2023, 9:15 AM

Herald added subscribers: atul, tomek. · View Herald TranscriptMar 9 2023, 9:15 AM

Harbormaster completed remote builds in B17211: Diff 23575.Mar 9 2023, 9:31 AM

bartek requested review of this revision.Mar 9 2023, 9:31 AM

Seems reasonable

This revision is now accepted and ready to land.Mar 9 2023, 9:55 AM

ashoat added inline comments.Mar 9 2023, 9:56 AM

web/jest-setup.js
6	`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:

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

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	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

bartek added a child revision: D6558: Added web tests to .lintstagedrc and CI.Mar 9 2023, 10:38 AM

bartek added a child revision: D7025: [web] Add unit tests for AES functions.Mar 9 2023, 11:23 AM

Changed flow-typed -> Flow as suggested

This revision was landed with ongoing or failed builds.Mar 9 2023, 11:07 PM

Closed by commit rCOMM961052269b9f: [web] Fix missing WebCrypto mock in tests (authored by bartek). · Explain Why

This revision was automatically updated to reflect the committed changes.

bartek added a commit: rCOMM961052269b9f: [web] Fix missing WebCrypto mock in tests.

Harbormaster completed remote builds in B17230: Diff 23598.Mar 9 2023, 11:18 PM

ashoat mentioned this in D7090: Unit tests for Olm's new X3DH methods.Mar 16 2023, 6:57 PM