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

feat: initial support for source-maps (only supports one to one mapping) #19

Merged
merged 8 commits into from
Apr 29, 2019

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Apr 24, 2019

What is this?

This PR adds support for source-maps, using an algorithm similar to that implemented in istanbul-lib-sourcemaps (which @loganfsmyth helped with some of the edge-cases around).

Why this is this important

This will allow V8's built in coverage to be more easily used with projects that use TypeScript, Babel, etc.

Extra eyeballs very much appreciated

  • V8 coverage is in byte offsets, source-maps expect line/column offsets.
  • blocks in V8 don't necessarily have a 1:1 correspondence to the elements that mappings are generated for in source-maps.

☝️ these two facts combine to make this a fairly gnarly algorithm:

  1. we're trying to remap back and forth between line/column positions and byte offsets.
  2. we're trying to pick the best possible matching element in the original source (with exact mappings often missing).

tldr; I've probably screwed some things up, and would love an extra set of eyes

CC: @fitzgen, @hashseed, @schuay, @isaacs.

BREAKING CHANGE: v8-to-istanbul is now async, due to source-map dependency.

@bcoe bcoe requested review from coreyfarrell and shinnn April 24, 2019 02:10
@istanbuljs istanbuljs deleted a comment from coveralls Apr 24, 2019
@bcoe
Copy link
Member Author

bcoe commented Apr 24, 2019

see: #20, we need a more methodical way to build out tests.

// the backflips we perform to remap absolute to relative positions.
const rawSourceMap = convertSourceMap.fromSource(rawSource) || convertSourceMap.fromMapFileSource(rawSource, dirname(this.path))
if (rawSourceMap) {
this.path = resolve(dirname(this.path), rawSourceMap.sourcemap.sources[0])
Copy link

Choose a reason for hiding this comment

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

This only accounts for the first source file when there can be many. For example I'm working with a file that has 259 sources in it.

Copy link
Member Author

@bcoe bcoe Apr 25, 2019

Choose a reason for hiding this comment

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

@Friss it also doesn't account for relative vs., absolute paths.

I think it might be worth pulling in this module from Istanbuljs:

https://github.com/istanbuljs/istanbuljs/blob/master/packages/istanbul-lib-source-maps/lib/pathutils.js

And this logic:

https://github.com/istanbuljs/istanbuljs/blob/master/packages/istanbul-lib-source-maps/lib/map-store.js#L131


This is the use-case of using a tool that combines JavaScript files?

Copy link

Choose a reason for hiding this comment

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

Yeah we build our apps with webpack and has the babel toolchain etc.

@Friss
Copy link

Friss commented Apr 25, 2019

(Posting on the main thread so it doesn't get lost)

The main idea I'm trying to solve is have puppeteer visit all the routes in our apps and record code coverage and then report that out to istanbul.

My first attempt (before this PR was opened) was to pull together the internals of istanbul to do it and I got pretty close. The files were all mapped out but the coverage was off. https://gist.github.com/Friss/8121c0462544d7515c498b512dbe9303#file-franken-code-coverage-js

Once I saw this PR I started looking into using this to do the same thing https://gist.github.com/Friss/8121c0462544d7515c498b512dbe9303#file-custom-code-coverage-js which is where I hit the roadblock of this only mapping back to 1 file. (I had solved the relative file problem)

@bcoe
Copy link
Member Author

bcoe commented Apr 25, 2019

@Friss in what ways was the coverage off? I've ran into some issue myself, which seem to have been somewhat addressed with changes to the underlying source-map library -- wondering if we're hitting the same thing.

My issue was that the entire header of TypeScript files was marked as uncovered, even though it was covered.

@Friss
Copy link

Friss commented Apr 25, 2019

@bcoe For me the branches were accounted for but still marked as 0% used. And then as far as statements and functions it was 0/0

Screen Shot 2019-04-25 at 7 30 02 PM
Screen Shot 2019-04-25 at 7 34 02 PM

This is the file's coverage before I tried to map it back.
Screen Shot 2019-04-25 at 7 35 35 PM

@bcoe bcoe changed the title feat: support for source-maps feat: initial support for source-maps (only supports one to one mapping) Apr 27, 2019
@bcoe
Copy link
Member Author

bcoe commented Apr 27, 2019

@Friss having thought a bit about what it would take to make v8-to-istanbul support a 1:many mapping of source-files, I feel it's beyond the scope of this first pass at source-map support. The library was originally designed with the idea that a single source-file would be output in the Istanbul content generated, see:

  toIstanbul () {
    const istanbulInner = Object.assign(
      { path: this.path },
      this._statementsToIstanbul(),
      this._branchesToIstanbul(),
      this._functionsToIstanbul()
    )
    const istanbulOuter = {}
    istanbulOuter[this.path] = istanbulInner
    return istanbulOuter
  }

This PR is a step in the right direction, because we've added the a CovSource class, I'm picturing that with further refactoring work this will allow us to output an Istanbul report that contains multiple sources rather than one.

I'm going to work on getting this work over the finish line supporting the 1:1 use-case, let's open an issue to extend on this work to support multiple sources.

@bcoe bcoe merged commit 580ebb7 into master Apr 29, 2019
@bcoe bcoe deleted the source-maps branch April 29, 2019 06:49
bcoe added a commit that referenced this pull request Apr 29, 2019
BREAKING CHANGE: v8-to-istanbul is now async, making it possible to use the latest source-map library
@bcoe bcoe mentioned this pull request Apr 29, 2019
3 tasks
@Friss
Copy link

Friss commented Apr 29, 2019

@bcoe Sounds good 👍 its definitely a step in the right direction.

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