Skip to content

chore: improve test runtime by replacing ts-jest with swc #4336

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

Merged
merged 19 commits into from
Jun 4, 2021

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented Jun 4, 2021

Details

Profiling our unit tests showed that the overwhelming majority of time spent was in ts-jest parsing typescript files. This PR is an experiment in replacing ts-jest with an alternative typescript transpiler, swc, which is less mature but much faster.

Motivation

On my local machine (a Surface Laptop 3 running Windows 21H1 OS Build 19043.985), comparative timings are:

scenario main (ts-jest) this PR (swc)
yarn test -- -- --coverage=false -- footer-section.test.tsx 12s ±1s 4.3s ±0.2s
yarn test -- -- -- footer-section.test.tsx 45s ±3s 6.2s ±0.0s
yarn test 11m54s 3m ±14s

On our CI build agents:

scenario main (ts-jest) this PR (swc)
unit tests 15m1s 5m9s
web-e2e tests 10m10s 8m6s
unified-e2e tests 10m39s 10m33s
total PR build time ~22m ~14m
Context

This is not as clear cut a transition as the timings above suggest. swc is a relatively young project; it's not quite as firmly single-maintainer as esbuild, but it's still got a fairly concerning bus factor compared to tsc or babel (what ts-jest hands off to). Still, the timings speak for themselves and it's easy enough to transition back if that ever becomes necessary.

If this experiment goes well, we might consider a similar change to update our webpack config to use swc-loader instead of ts-loader.

One of the reasons swc is so much faster is that it does not perform type-checking. Right now we do type-checking as part of build, but if we were to switch webpack to use swc, we'd then need a separate build step for type-checking (comparable to the current yarn null:check step).

Implementation notes:

  • There are a few subtly important things in the .swcrc file, which unfortunately doesn't support comments:
    • The noInterop: true flag is the equivalent of TypeScript's default esModuleInterop: false behavior. It is necessary; we use several modules which cannot be used correctly with interop behavior in play. See commonjs wildcard imports only expose enumerable properties of requirees' exports swc-project/swc#1778 for details.
    • The empty "env": {} property is necessary to trigger swc to respect the browserslist entry in package.json
    • Directing swc to target a reasonably recent set of browsers (via the new browserslist entry in package.json) is required; there are several tests with snapshots that change under default targeting, and there are a few components that break outright.
    • tsx: true is required (it tells swc to enable support for tsx files; it doesn't prevent non-tsx files from parsing)
  • For now, this is using a custom Jest transformer to call @swc/core. The swc project maintains a @swc/jest transformer, but No way to override jsc or module swc configuration? swc-project/jest#8 prevents it from being able to use the noInterop: true setting we require, and I can't submit a PR to patch the project until legal rubber stamps their CLA
  • collapsible-script-provider.tsx required a hacky workaround for swc incorrectly discards export statements and (most) comments  swc-project/swc#1165
    • even after applying the workaround, running jest with and without --coverage=false emitted javascript with different whitespace. collapsible-script-provider.test.ts needed to be updated to format the resulting js before snapshotting it.
  • src/scanner/processor.ts used a mutable global in a module to enable its tests to inject test data. This confused swc; I replaced it with a more typical ctor injection pattern.
  • browser-adapter-factory.test.tsx did a dance around an import 'webextension-polyfill-ts'; statement to trick the module into thinking window.chrome.runtime.id is defined (it checks for this during import and refuses to load if it is not set). The dance we used is not compliant with how ES6 imports are defined to work (they are defined as hoisting import statements to the top of the file, ie, before we stub the window property). TypeScript has an outstanding PR/issue to stop supporting what we were previously doing (Hoist import declarations when transforming to commonjs/amd/umd module TypeScript#39764) and swc intentionally doesn't support it (Import statements are hoisted when target is CommonJs swc-project/swc#1686) (babel doesn't either), so I had to modify how our dance worked by moving it to a separate file and having it use a raw require() rather than an import.

Pull request checklist

  • [n/a] Addresses an existing issue: #0000
  • Ran yarn fastpass
  • Added/updated relevant unit test(s) (and ran yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
  • [n/a] (UI changes only) Added screenshots/GIFs to description above
  • [n/a] (UI changes only) Verified usability with NVDA/JAWS

@dbjorge dbjorge marked this pull request as ready for review June 4, 2021 01:23
@dbjorge dbjorge requested a review from a team as a code owner June 4, 2021 01:23
@dbjorge dbjorge changed the title chore: experiment with replacing ts-jest with swc chore: improve test runtime by replacing ts-jest with swc Jun 4, 2021
const baseTransformOptions = merge({}, swcrc, jestTransformOverrides);

module.exports = {
process: (src, path) => {
Copy link
Contributor

@karanbirsingh karanbirsingh Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like Jest docs recommend implementing getCacheKey as well, although swc-jest transformer also doesn't seem to have it. Do you know if this would provide us any benefits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent question; I didn't look at the jest transformer docs at all to be honest, I was mostly adapting from swc-jest's transformer (it would be better to contribute .swcrc support back to them, but I'm blocked on legal approval to sign their CLA before I can do that). I'll investigate this after standup!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline; will investigate improvements to the transformer in a separate PR (ideally against the swc-project/jest repo directly)

@dbjorge dbjorge merged commit 8380b72 into microsoft:main Jun 4, 2021
@dbjorge dbjorge deleted the swc branch June 4, 2021 20:34
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