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

Sourcemaps #3458

Merged
merged 10 commits into from
Oct 9, 2017
Merged

Sourcemaps #3458

merged 10 commits into from
Oct 9, 2017

Conversation

felipeochoa
Copy link
Contributor

Summary

This is an initial implementation of source map support (#336). Source maps were already supported for outputting coverage information when using the mapCoverage option. This PR uses the same machinery to allow any transformer to provide source maps for the runtime to use when generating stack traces (using the source-map-support package). With this change, transformers can provide source maps by simply returning a {code, map} object instead of the transformed string.

Test plan

There are a couple of items tested:

  • Source maps returned by transformers are stored in the Runtime source map cache. See abc1508
  • Stack traces are modified using the given source maps. See ee757f9

Pending tasks

  • Add an end-to-end test with Babel transforming a file [not sure if this slows the suite down too much]
  • Are there other places in the codebase where source maps would be useful?
  • Update yarn files -- source-map-support is already included as a dependency somewhere in the tree, but I need to bring it up to the top

cc @cpojer -- let me know what you think of this!

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@benedyktdryl
Copy link

@cpojer any updates on this one? Can I help somehow?

@felipeochoa
Copy link
Contributor Author

@benedyktdryl I haven't had time to go back to this, but you could investigate why the CircleCI and AppVeyor builds failed. Also the PR needs to be rebased, so not sure how painful that would be.

.install({
handleUncaughtExceptions: false,
retrieveSourceMap: source => {
if (runtime._sourceMapRegistry[source]) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think accessing a private field of runtime should be done here :)

@cpojer
Copy link
Member

cpojer commented Aug 24, 2017

The overall approach looks good, but it needs a rebase and tests like mentioned in the initial PR. source-map-support would need to be added to the jasmine2 package also. I'm a bit worried about performance implications of adding this – does it slow down runtime of individual tests to load
source-map-support for every test?

@GeeWee
Copy link

GeeWee commented Aug 24, 2017

Perspectives on performance: We load source-map support in ts-jest for every test, and after fixing some caching issues we're not that much slower than regular jest.
We've discussed performance in kulshekhar/ts-jest#280 - and it seems like the performance bottleneck in jest is currently resolving files in fs.statSync, and not something like source-map-supprt

@cpojer
Copy link
Member

cpojer commented Aug 24, 2017

Right, but this is a feature that most people don't really need because retainLines works well enough. However, I trade correctness for perf. Can you give me stats from before and after applying this diff?

@ljharb
Copy link
Contributor

ljharb commented Aug 24, 2017

fwiw I strongly dispute "most people don't really need"; not every babel plugin supports that setting nor is capable of retaining line numbers, and correct source maps are something that people generally seem to universally need.

@kulshekhar
Copy link

@cpojer

Can you give me stats from before and after applying this diff

Case 1

ts-jest 20.0.4 20.0.7 20.0.10
35-40s 90-100s 20-25s

Case 2

test 20.0.7 20.0.10
creates many Timeout errors 1995ms 325ms
throwing 1000 Errors 5042ms 355ms
throwing 1000 Errors, not touching Error.stack 4ms 5ms

Case 3

prior to 20.0.10 20.0.10
45s 10s

All these performance improvements were obtained by using source-map-support's caching. A test repo that we were examining showed very little difference between using pure jest (with javascript) compared to the typescript version using ts-jest

@GeeWee
Copy link

GeeWee commented Aug 30, 2017

Worth mentioning is, we used sourcemap-support in both those versions, but one version was without source-map-supports caching.

@cpojer
Copy link
Member

cpojer commented Aug 31, 2017

Cool! Would you mind rebasing and making CI green before we can merge this?

@cpojer
Copy link
Member

cpojer commented Sep 30, 2017

Ping! It would be great to get this into Jest now, but it requires a rebase :)

Under the new transformer rules, if the transformer provides us a
sourcemap, we'll take it, regardless of the coverage settings. This
means source maps are cached alongside the originals, and `Runtime`
will populate its `_sourceMapRegistry`
@felipeochoa
Copy link
Contributor Author

I've rebased but now there are a ton of tests failing unrelated to these changes. I think it's the same problem as in #4430 (comment). On my machine the set of failing tests is the same on this PR as on an master.

It was already included as a transitive dependency:
babel-core -> babel-register -> source-map-support
package.json Outdated
@@ -73,6 +73,7 @@
"rollup-plugin-node-globals": "^1.1.0",
"rollup-plugin-node-resolve": "^3.0.0",
"slash": "^1.0.0",
"source-map-support": "^0.5.0",
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here? Shouldn't it be a dependency of jest-jasmine2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I just moved it there

@SimenB
Copy link
Member

SimenB commented Oct 8, 2017

I ran the test suite locally, and on my machine the failing test matches the failing test on circle, so I just updated it and pushed. Hopefully stuff is green 🙂

ProTip:tm: if you have a failing test suite which passes on Jest's CI is to do yarn clean-all && yarn to make sure you have a clean slate locally

@SimenB
Copy link
Member

SimenB commented Oct 8, 2017

There are now less failures on CI, it just seems to be a hash that fails. I'm not sure how that hash is calculated? If it's machine specific it should not be included in the snapshot.

Seeing as the same test fails on circle and travis, but with different hashes there, hash should probably be deleted from the map. @cpojer agreed?

@cpojer
Copy link
Member

cpojer commented Oct 8, 2017

Yap that makes sense @SimenB.

@codecov-io
Copy link

Codecov Report

Merging #3458 into master will decrease coverage by 0.53%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3458      +/-   ##
==========================================
- Coverage   56.17%   55.64%   -0.54%     
==========================================
  Files         194      186       -8     
  Lines        6542     6351     -191     
  Branches        3        3              
==========================================
- Hits         3675     3534     -141     
+ Misses       2866     2817      -49     
+ Partials        1        0       -1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/index.js 0% <0%> (ø) ⬆️
packages/jest-runtime/src/script_transformer.js 89.78% <100%> (-0.08%) ⬇️
...est-worker/src/__performance_tests__/workers/pi.js
...r/src/__performance_tests__/workers/jest_worker.js
packages/jest-worker/src/worker.js
...ages/jest-worker/src/__performance_tests__/test.js
...r/src/__performance_tests__/workers/worker_farm.js
packages/jest-worker/src/child.js
packages/jest-worker/src/types.js
packages/jest-worker/src/index.js
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab6d017...4269625. Read the comment docs.

@felipeochoa
Copy link
Contributor Author

@cpojer @SimenB I removed the hash from the snapshots and everything is green now!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants