feat: adds --all functionality

[j03m]

Implements an --all flag for a src directory to consider
for coverage. If supplied, c8 will glob the directory respecting the
--include and --exclude parameters for src files. All source files
will be included in the final report. If a file is not found in the
v8 coverage output, it will be initialized with an empty record and
reported as 0 lines/branches/functions covered.

Checklist

• npm test, tests passing
• npm run test:snap (to update the snapshot)
• tests and/or benchmarks are included
• documentation is changed or added

[j03m]

Status: This seems to be satisfying my usecase for all. I'm issuing this as a draft however, because there is still an open issue around the way html reports are generated. If a file is unloaded/empty the HTML report generator seems to fail when trying to generate src level page for the file with the exception below. I'm working out why now.

Cannot read property 'locations' of undefined
TypeError: Cannot read property 'locations' of undefined
    at D:\dev\c8\node_modules\istanbul-reports\lib\html\annotator.js:132:50
    at Array.forEach ()
    at annotateBranches (D:\dev\c8\node_modules\istanbul-reports\lib\html\annotator.js:128:30)
    at Object.annotateSourceCode (D:\dev\c8\node_modules\istanbul-reports\lib\html\annotator.js:238:9)
    at HtmlReport.onDetail (D:\dev\c8\node_modules\istanbul-reports\lib\html\index.js:265:27)
    at Visitor. [as onDetail] (D:\dev\c8\node_modules\istanbul-lib-report\lib\tree.js:34:30)
    at ReportNode.Node.visit (D:\dev\c8\node_modules\istanbul-lib-report\lib\tree.js:114:17)
    at D:\dev\c8\node_modules\istanbul-lib-report\lib\tree.js:118:15
    at Array.forEach ()
    at ReportNode.Node.visit (D:\dev\c8\node_modules\istanbul-lib-report\lib\tree.js:117:24)

[j03m]

Sample txt report (from snapshot):

--------------|----------|----------|----------|----------|-------------------|
File          |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
--------------|----------|----------|----------|----------|-------------------|
All files     |    64.29 |    66.67 |       50 |    64.29 |                   |
 vanilla      |    78.26 |       75 |      100 |    78.26 |                   |
  loaded.js   |    73.68 |    71.43 |      100 |    73.68 |      4,5,16,17,18 |
  main.js     |      100 |      100 |      100 |      100 |                   |
 vanilla/dir  |        0 |        0 |        0 |        0 |                   |
  unloaded.js |        0 |        0 |        0 |        0 |         1,2,3,4,5 |
--------------|----------|----------|----------|----------|-------------------|

[coveralls]

Pull Request Test Coverage Report for Build 499

• 80 of 81 (98.77%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.3%) to 94.083%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/source-map-from-file.js	24	25	96.0%

Totals
Change from base Build 491:	0.3%
Covered Lines:	556
Relevant Lines:	579

---

💛 - Coveralls

[j03m]

Thought: Should --all also be allowed to be empty and then default to process.cwd to be more like nyc? Seems so. Will add.

[bcoe]

This is looking like really solid, thanks for putting this work in 😄, and sorry for the slow review.

One thought I have, we might be able to avoid zeroing out the Istanbul format report, and be able to use existing functionality, if we tried to populate a dummy V8 format coverage report, rather than Istanbul see.

[bcoe]

@j03m I did a little playing, I think you could potentially get away with something like this, and piggyback off the existing merge logic:

  /**
   * Returns the merged V8 process coverage.
   *
   * The result is computed from the individual process coverages generated
   * by Node. It represents the sum of their counts.
   *
   * @return {ProcessCov} Merged V8 process coverage.
   * @private
   */
   _getMergedProcessCov () {
    const { mergeProcessCovs } = require('@bcoe/v8-coverage')
    const v8ProcessCovs = []
    for (const v8ProcessCov of this._loadReports()) {
      if (this._isCoverageObject(v8ProcessCov)) {
        if (v8ProcessCov['source-map-cache']) {
          Object.assign(this.sourceMapCache, v8ProcessCov['source-map-cache'])
        }
        v8ProcessCovs.push(this._normalizeProcessCov(v8ProcessCov))
      }
    }
    
    if (this.all) {
      const emptyReports = []
      v8ProcessCovs.unshift({result: emptyReports})
      this.exclude.globSync(process.cwd()).forEach((f) => {
        const ext = extname(f)
        if (ext === '.js' || ext === '.ts' || ext === '.mjs') {
          const stat = statSync(f)
          emptyReports.push({
            scriptId: v4(),
            url: resolve(f),
            functions: [{
              functionName: '',
              ranges: [{
                startOffset: 0,
                endOffset: stat.size,
                count: 0
              }],
              isBlockCoverage: true
            }]
          })
        }
      })
    }
            
    return mergeProcessCovs(v8ProcessCovs)
  }

Perhaps we could then pull the brunt of this into a helper called, createCoverageObject, which takes size, and path.

Curious to know whether this approach works with the tests you've pulled together.

[j03m]

Okay cool. Thank you for reviewing. I will take a look at this approach and circle back.

[j03m]

Functions seem to report as 100% using the above method, ala:

--------------|----------|----------|----------|----------|-------------------|
File          |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
--------------|----------|----------|----------|----------|-------------------|
All files     |    64.29 |    66.67 |      100 |    64.29 |                   |
 vanilla      |    78.26 |       75 |      100 |    78.26 |                   |
  loaded.js   |    73.68 |    71.43 |      100 |    73.68 |      4,5,16,17,18 |
  main.js     |      100 |      100 |      100 |      100 |                   |
 vanilla/dir  |        0 |        0 |      100 |        0 |                   |
  unloaded.js |        0 |        0 |      100 |        0 |         1,2,3,4,5 |
--------------|----------|----------|----------|----------|-------------------|

I still think its a better approach though, I'm investigating how to handle this.

[bcoe]

thanks @j03m for digging into this 👍 if we get this feature right, it's a huge improvement 😄

[j03m]

Just picked this up again. Another issue, seems to be that when dealing with typescript sourcemaps the converter seems to initialize statements to covered as well. This is the result of my unit test using ts-node:

**Edit: I'm not sure if its the converter. May just be how the reporter interprets it.

--------------------|----------|----------|----------|----------|-------------------|
File                |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
--------------------|----------|----------|----------|----------|-------------------|
All files           |    82.14 |    55.56 |      100 |    82.14 |                   |
 ts-source-maps     |    78.26 |     62.5 |      100 |    78.26 |                   |
  loaded.ts         |    73.68 |    71.43 |      100 |    73.68 |      4,5,16,17,18 |
  main.ts           |      100 |        0 |      100 |      100 |                 4 |
 ts-source-maps/dir |      100 |        0 |      100 |      100 |                   |
  unloaded.ts       |      100 |        0 |      100 |      100 |                 5 |
--------------------|----------|----------|----------|----------|-------------------|

Looking at a fix here as well. May require a change to the converter? Not sure yet.

[j03m]

Okay for the functions being reported as 100%. This was pretty easy to track down. The issue comes down to this logic in istanbul-coverage: https://github.com/istanbuljs/istanbuljs/blob/master/packages/istanbul-lib-coverage/lib/percent.js#L9 where if I have 0 total of 0 covered, it sets the percentage to 100%.

In our converter, when it evaluates our "fake" function here: https://github.com/istanbuljs/v8-to-istanbul/blob/master/lib/v8-to-istanbul.js#L99 we don't create a function because the name is "" which evals to false, we skip that if block.

I don't have enough context to know if either of these should be changed. However, changing our fake coverage blocks to have a default function name of "c8-fake-function" does the trick. The converter believes we have 1 function which is uncovered so the percentage becomes 0.

Looking into the typescript issue next.

[j03m]

I think the issue with typescript is that the fake entries don't have a sourcemap cache entry (because they were never loaded). That seems solvable by locating the sourcemap for the targeted file and also faking the sourcemap cache. I'm not 100% sure yet.

However, I wanted to reconsider, is this actually a better approach? It seems like we're required to do a lot more fakery vs simply creating the empty istanbul report entry. The fake bits are at a much higher level and have to survive being manipulated by many more paths of code. It seems like it may be more brittle now that I'm exploring what is involved.

Not sure, I will definitely defer to your expertise here and am happy to continue down this path, but I know originally I think you were concerned about how the empty istanbul records would merge. I'm not sure I fully understood that concern. Could you elaborate there?

In the interim I'll see if I can figure out this sourcemap issue.

[bcoe]

@j03m if you're finding it significantly more work to use the V8 output, vs., the Istanbul report, I'm open to your original approach, this would bring us back to the original review, mainly:

1. let's try to isolate some of the --all functionality into a module, perhaps create-fake-report.js, in lib/?
2. let's make sure we test a bunch of edge-cases tsnode, vs., pre-transpiled code (we should make sure we're appropriately picking up source-maps, and running exclude at the appropriate time, e.g., we don't want to exclude loading a .js file that remaps to a .ts file).
3. let's use the same approach as nyc, where --all is just a boolean flag, and uses the current process.cwd().

I trust your opinion on which approach for creating an empty report is less ridiculous 😄

[j03m]

I don't know if its more work specifically. I looked at this a bit more last night and I think my largest concern at this point might be performance.

I think (I'm not 100% sure as I haven't gotten it working yet) that the current approach will require the a good deal more IO so that soucemaps will work correctly. Currently we statSync to get the number of lines. But in addition I think we need to:

• Read the body of compiled file and parse out the source map url
• Read the body of the sourcemap to be included in the source-map-cache

I think that could add up for large projects? That said, I've written some of this and it looks like it will work and arguably we can write an optimization to only do this IO when we know we have an unloaded file which will help.

Anyhow, I feel like I should at least get it working as a POC so we can get a full look. We can always regress back to the previous approach.

[j03m]

I think if we can make the change I referenced here: istanbuljs/v8-to-istanbul#75 that supply the v8 coverage will work fine.

[bcoe]

@j03m 👋 I put a bunch of hours in on the weekend working on --all and tested against some of Google's libraries 👍 I think I have a hybrid approach using some of my suggestions, and a bunch of your work, just haven't gotten it over the finish line yet.

[j03m]

Okay cool - pulling in your PR branch for v8-to-istanbul solved the last issues I was grappling with. I pushed the current state of my work which seems to be functionally equivalent to the last PR in terms of results, but using the empty v8 blocks.

I didn't spend more time polishing it, but I figured I would share the state in case you want to compare notes and if this is of any use to your PR.

I'm going to pause here and work on this POC in our environment until you are done mostly to see if anything else might be missing from our use case. But, if there is anything I can do help pls let me know!

[bcoe]

@j03m thank you, I appreciate the hard work.

[bcoe]

This is incredible work thank you @j03m, my last couple nits:

1. we should probably pull that source-map helper into a file, and make sure it has a license header if it's borrowed code.
2. we should update the README.md with mention of --all.

🎉 thanks for working with me to reach an awesome implementation.

[bcoe]

I'm perfectly comfortable with us floating our own version of this method, since we're currently only using one bit of the library (I'd rather not add an additional library).

Did we get this code directly from convert-source-map, because I think we should pull this into a lib/ folder, carrying over the license header from that project.

[j03m]

I think this is okay. The code isn't from covert-source-map, but after I wrote it I noticed we had convert-source-map in the dependency tree of v8-to-istanbul and were using it. So I thought we would be better served by using a function from there, but I don't feel strongly about it. I'll remove the TODO.

[j03m]

Oh woa, apologies for the long absence, I was under the impression you were going to use a different PR. I came to check if you had pushed that. I can make the changes asked for here?

[j03m]

Looks like tests lint are failing though :/ looking.

**edit: Its the lint, will fix.

[j03m]

Few changes in bound.

[j03m]

I think this is okay. The code isn't from covert-source-map, but after I wrote it I noticed we had convert-source-map in the dependency tree of v8-to-istanbul and were using it. So I thought we would be better served by using a function from there, but I don't feel strongly about it. I'll remove the TODO.

[j03m]

Scrutinizing this a bit now - this should be a Set

[j03m]

your tests,

[j03m]

Couple of minor things on my end.

[j03m]

you

[j03m]

c8,

,obeying....flags,

[j03m]

Lost the {} here. Not sure if that is an issue.

[j03m]

Hmmm, hold on merge. Something is amiss here. Coveralls is reporting this line as uncovered: https://coveralls.io/builds/27279321/source?filename=lib/report.js#L169. That shouldn't be the case.

[j03m]

Okay solved. If you look at the commit for 65a5453 I was including *.ts for the compiled tests. This works for ts-node but not for something like tsc where you need to run against your dist and map back to src. I fixed that test and fixed the snapshot.

[j03m]

This should be g2g now!

[bcoe]

@j03m awesome work 👌

[j03m]

Thanks for all the advice/help!