whitespace trimming approach to fix #81

[glromeo]

This PR aims at improving the precision of the v8 range mapping trimming the whitespaces.

e.g. #81 is caused by:

v8-to-istanbul/lib/v8-to-istanbul.js

Lines 179 to 181 in 53c1cd8

	if (startCol <= line.startCol && endCol >= line.endCol && !line.ignore) {
	line.count = range.count
	}

because the v8 startOffset is 50 while the line startCol is 48.

I introduced minCol and maxCol which take into account the gaps caused by whitespaces and now the matching
seems more accurate.

With the trimming in place c8 tests are all green except from the following 7 cases:

    source-maps
      ✔ does not attempt to load source map URLs that aren't (195ms)
      TypeScript
        ✔ remaps branches (219ms)
        1) remaps classes
      UglifyJS
        ✔ remaps branches (221ms)
        2) remaps classes
      nyc
        ✔ remaps branches (217ms)
        3) remaps classes
      rollup
        ✔ remaps branches (216ms)
        4) remaps classes
      ts-node
        5) reads source-map from cache, and applies to coverage
    --all
      ✔ reports coverage for unloaded js files as 0 for line, branch and function (311ms)
      6) reports coverage for unloaded transpiled ts files as 0 for line, branch and function
      7) reports coverage for unloaded ts files as 0 for line, branch and function when using ts-node
      ✔ should allow for --all to be used in conjunction with --check-coverage (298ms)
      ✔ should allow for --all to be used with the check-coverage command (2 invocations) (544ms)

These differences look promising (the new range cover the method signatures which in the past were missing):

  1) AssertionError: expected value to match snapshot c8 source-maps TypeScript remaps classes 1
      + expected - actual
       -----------------------|---------|----------|---------|---------|-------------------
       File                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       -----------------------|---------|----------|---------|---------|-------------------
      -All files              |   75.75 |    85.71 |      60 |   75.75 |                   
      - classes.typescript.ts |   75.75 |    85.71 |      60 |   75.75 | 12-13,20-22,26-28 
      +All files              |   81.81 |    85.71 |      60 |   81.81 |                   
      + classes.typescript.ts |   81.81 |    85.71 |      60 |   81.81 | 12-13,21-22,27-28 
       -----------------------|---------|----------|---------|---------|-------------------

  2) AssertionError: expected value to match snapshot c8 source-maps UglifyJS remaps classes 1
      + expected - actual
       ------------|---------|----------|---------|---------|-------------------
       File        | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ------------|---------|----------|---------|---------|-------------------
      -All files   |   77.77 |       80 |      60 |   77.77 |                   
      - classes.js |   77.77 |       80 |      60 |   77.77 | 6-7,14-15,20-21   
      +All files   |   85.18 |       80 |      60 |   85.18 |                   
      + classes.js |   85.18 |       80 |      60 |   85.18 | 6-7,15,21         
       ------------|---------|----------|---------|---------|-------------------

  3) AssertionError: expected value to match snapshot c8 source-maps nyc remaps classes 1
      + expected - actual
       ------------|---------|----------|---------|---------|-------------------
       File        | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ------------|---------|----------|---------|---------|-------------------
      -All files   |   70.37 |    83.33 |      60 |   70.37 |                   
      - classes.js |   70.37 |    83.33 |      60 |   70.37 | 7-8,14-16,20-22   
      +All files   |   77.77 |    83.33 |      60 |   77.77 |                   
      + classes.js |   77.77 |    83.33 |      60 |   77.77 | 7-8,15-16,21-22   
       ------------|---------|----------|---------|---------|-------------------
  4) AssertionError: expected value to match snapshot c8 source-maps rollup remaps classes 1
      + expected - actual
       ------------|---------|----------|---------|---------|-------------------
       File        | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ------------|---------|----------|---------|---------|-------------------
      -All files   |   71.42 |    83.33 |      60 |   71.42 |                   
      +All files   |   78.57 |    83.33 |      60 |   78.57 |                   
        class-1.js |     100 |      100 |     100 |     100 |                   
      - class-2.js |   65.21 |    83.33 |      60 |   65.21 | 7-8,14-16,20-22   
      + class-2.js |   73.91 |    83.33 |      60 |   73.91 | 7-8,15-16,21-22   
       ------------|---------|----------|---------|---------|-------------------
  5) AssertionError: expected value to match snapshot c8 source-maps ts-node reads source-map from cache, and applies to coverage 1
      + expected - actual
       ------------------|---------|----------|---------|---------|-------------------
       File              | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       ------------------|---------|----------|---------|---------|-------------------
      -All files         |   85.29 |    83.33 |      80 |   85.29 |                   
      - ts-node-basic.ts |   85.29 |    83.33 |      80 |   85.29 | 12-13,27-29       
      +All files         |   88.23 |    83.33 |      80 |   88.23 |                   
      + ts-node-basic.ts |   88.23 |    83.33 |      80 |   88.23 | 12-13,28-29       
       ------------------|---------|----------|---------|---------|-------------------

Instead I am still investigating these two cases (the min/max ranges seem to be remapped correctly by source maps while somethins is off with this.rawSource which breaks trimRange):

  6) AssertionError: expected value to match snapshot c8 --all reports coverage for unloaded transpiled ts files as 0 for line, branch and function 1
      + expected - actual
       -----------------|---------|----------|---------|---------|-------------------
       File             | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       -----------------|---------|----------|---------|---------|-------------------
      -All files        |   67.85 |    57.14 |      50 |   67.85 |                   
      - ts-compiled     |    82.6 |    66.66 |     100 |    82.6 |                   
      -  loaded.ts      |   78.94 |    66.66 |     100 |   78.94 | 4-5,17-18         
      +All files        |   64.28 |    57.14 |      50 |   64.28 |                   
      + ts-compiled     |   78.26 |    66.66 |     100 |   78.26 |                   
      +  loaded.ts      |   73.68 |    66.66 |     100 |   73.68 | 4-5,16-18         
         main.ts        |     100 |      100 |     100 |     100 |                   
        ts-compiled/dir |       0 |        0 |       0 |       0 |                   
         unloaded.ts    |       0 |        0 |       0 |       0 | 1-5               
       -----------------|---------|----------|---------|---------|-------------------

  7) AssertionError: expected value to match snapshot c8 --all reports coverage for unloaded ts files as 0 for line, branch and function when using ts-node 1
      + expected - actual
       --------------|---------|----------|---------|---------|-------------------
       File          | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
       --------------|---------|----------|---------|---------|-------------------
      -All files     |   67.85 |    57.14 |      50 |   67.85 |                   
      - ts-only      |    82.6 |    66.66 |     100 |    82.6 |                   
      -  loaded.ts   |   78.94 |    66.66 |     100 |   78.94 | 4-5,17-18         
      +All files     |   64.28 |    57.14 |      50 |   64.28 |                   
      + ts-only      |   78.26 |    66.66 |     100 |   78.26 |                   
      +  loaded.ts   |   73.68 |    66.66 |     100 |   73.68 | 4-5,16-18         
         main.ts     |     100 |      100 |     100 |     100 |                   
        ts-only/dir  |       0 |        0 |       0 |       0 |                   
         unloaded.ts |       0 |        0 |       0 |       0 | 1-5               
       --------------|---------|----------|---------|---------|-------------------

I preferred to open the PR early to start receiving feedback. I will try to keep the noise to the minimum though!

I am keen to optimise the code but that once we are sure that the changes in the coverage are all good.

[glromeo]

Here are some updated comparisons of the c8 html reports generated for node-fetch.
On the left side the results with the new code, on the right side using v8 v8.1.1



In the last picture in particular it appears that the new reports are more accurate

[glromeo]

@bcoe this version fixes #172 (comment)

[bcoe]

@glromeo sorry for the slow turnaround, I don't have much time for OSS outside of work currently (so it goes).

the node-fetch output looks identical, unless I'm missing something?

Happen to have an example of the incorrect behavior in an HTML report, would love to see the fix in action visually.

[glromeo]

@bcoe no worries I understand...if anything thank you for carving out the time to look at my PR

Indeed the coverage summaries are the same which is good but if you look carefully at the iteration count
you will notice the differences... the left hand side (new) has more accurate counts which reassures me that
the change is an improvement.

[bcoe]

the left hand side (new) has more accurate counts which reassures me that
the change is an improvement.

gotcha.

[bcoe]

@glromeo apologies for the delay on this, unfortunately it's hard to do much open source work with my day job.

Would still love to land this work, as it seems like this and your other PR unblock some issues on c8.

[glromeo]

@bcoe I am getting my head around the discrepancies and errors I now see since latest changes... I hope to have a fix soon

[bcoe]

@glromeo it looks like this work has stalled a bit, closing for now, but please feel free to pick up again.