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

use Babel instead of ts-jest to speed up and simplify tests #3959

Merged
merged 2 commits into from Jul 25, 2019

Conversation

sqs
Copy link
Member

@sqs sqs commented May 12, 2019

Removed typechecking from jest tests (by removing ts-jest and just using Babel's TypeScript transpilation, per https://jestjs.io/docs/en/getting-started#using-typescript). This makes tests run faster (~2s vs. ~9s for a single test after making a change to a .ts file) and simplifies their build configuration (by removing an unnecessary tool, ts-jest, from our toolchain). You should instead rely on your editor (and CI) for typechecking.

Babel is a good choice because it is the standard for transpilation, and tslint is being deprecated in favor of Babel + eslint (so we need to switch soon anyway to get the latest lint rules, such as those for React hooks).

@sqs sqs added the webapp label May 12, 2019
@sqs sqs requested a review from felixfbecker May 12, 2019 19:44
@sqs sqs mentioned this pull request May 12, 2019
@sqs sqs force-pushed the use-babel-for-ts-tests branch 2 times, most recently from f2dc1a5 to 2c593cb Compare May 14, 2019 05:42
@felixfbecker
Copy link
Contributor

I don't currently see compile errors from test files in VS Code (without explicitly opening the file). It seems like they are not checked by the tsc tasks that get run. Could you ensure that?

@sqs sqs force-pushed the use-babel-for-ts-tests branch 2 times, most recently from 72ba85b to a8ef0cf Compare May 23, 2019 02:48
@sqs sqs requested a review from beyang as a code owner May 23, 2019 02:48
@sqs sqs changed the base branch from ts-babel-base to master May 23, 2019 02:48
@beyang
Copy link
Member

beyang commented May 31, 2019

@felixfbecker given that @sqs is out on pat leave, is this something that should be merged, or should we close this?

@sqs
Copy link
Member Author

sqs commented Jun 12, 2019

@beyang keep open pls

This can be merged when @babel/plugin-transform-typescript 7.5.0 is released, which should be soon. That version will incorporate the fix in babel/babel#9610, which should solve the problem that causes CI to fail.

@felixfbecker
Copy link
Contributor

felixfbecker commented Jul 4, 2019

@sqs 7.5.0 was just released :)

@beyang beyang removed their request for review July 8, 2019 17:29
@sqs
Copy link
Member Author

sqs commented Jul 25, 2019

@felixfbecker Re: vscode not showing errors from not-yet-opened files, I am indeed seeing that problem, but I am actually not seeing any of the tasks work:

> Executing task: yarn --prefix web run watch <

yarn run v1.16.0
error Command "run" not found.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
The terminal process terminated with exit code: 1

It doesn't seem to be related to this PR, so I'm not going to block this PR.

@sqs sqs force-pushed the use-babel-for-ts-tests branch 2 times, most recently from 15b17eb to 6055ef0 Compare July 25, 2019 07:30
sqs added 2 commits July 25, 2019 01:10
Removed typechecking from jest tests (by removing ts-jest and just using Babel's TypeScript transpilation, per https://jestjs.io/docs/en/getting-started#using-typescript). This makes tests run faster (~2s vs. ~9s for a single test after making a change to a `.ts` file) and simplifies their build configuration (by removing an unnecessary tool, ts-jest, from our toolchain). You should instead rely on your editor (and CI) for typechecking.

Babel is a good choice because it is the standard for transpilation, and tslint is being deprecated in favor of Babel + eslint (so we need to switch soon anyway to get the latest lint rules, such as those for React hooks).
@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

Merging #3959 into master will decrease coverage by 1.62%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3959      +/-   ##
==========================================
- Coverage   48.06%   46.44%   -1.63%     
==========================================
  Files         733      730       -3     
  Lines       44817    44419     -398     
  Branches     1763     2610     +847     
==========================================
- Hits        21541    20630     -911     
- Misses      21300    21813     +513     
  Partials     1976     1976
Impacted Files Coverage Δ
...wser/src/libs/code_intelligence/util/selections.ts 0% <0%> (-75%) ⬇️
browser/src/libs/github/scrape.ts 0% <0%> (-66.67%) ⬇️
web/src/components/ModalContainer.tsx 0% <0%> (-60%) ⬇️
browser/src/libs/phabricator/index.tsx 0% <0%> (-50%) ⬇️
shared/src/api/client/client.ts 0% <0%> (-44.45%) ⬇️
shared/src/components/CodeExcerpt2.tsx 0% <0%> (-41.67%) ⬇️
...rowser/src/libs/code_intelligence/hover_alerts.tsx 0% <0%> (-38.89%) ⬇️
web/src/components/SaveToolbar.tsx 7.69% <0%> (-32.31%) ⬇️
browser/src/shared/backend/diffs.tsx 11.11% <0%> (-30.56%) ⬇️
web/src/site-admin/backend.tsx 0% <0%> (-30.44%) ⬇️
... and 241 more

@sqs sqs merged commit 047ddd3 into master Jul 25, 2019
@sqs sqs deleted the use-babel-for-ts-tests branch July 25, 2019 08:38
@felixfbecker
Copy link
Contributor

🎉

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

Successfully merging this pull request may close these issues.

None yet

3 participants