feat: adds --all functionality #158

j03m · 2019-10-29T19:21:48Z

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 · 2019-10-29T19:21:55Z

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 · 2019-10-29T19:25:49Z

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 · 2019-10-29T19:42:46Z

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 · 2019-10-30T11:53:20Z

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.

lib/parse-args.js

lib/report.js

test/fixtures/all/vanilla/loaded.js

bcoe · 2019-11-06T06:12:24Z

@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 · 2019-11-06T21:58:16Z

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

j03m · 2019-11-08T11:58:38Z

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 · 2019-11-08T20:37:52Z

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

j03m · 2019-11-09T22:24:58Z

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 · 2019-11-09T23:18:21Z

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 · 2019-11-10T00:30:58Z

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 · 2019-11-10T02:02:09Z

@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:

let's try to isolate some of the --all functionality into a module, perhaps create-fake-report.js, in lib/?
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).
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 · 2019-11-12T13:30:17Z

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 · 2019-11-17T20:08:58Z

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 · 2019-11-18T17:33:16Z

@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 · 2019-11-19T13:15:47Z

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!

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 v8 record and reported as 0 lines/branches/functions covered. Note: This uses the empty v8 approach instead of the empty report approach Fix html report --all should be boolean Update snapshot fix async function WIP - changing --all a bit to create a fake v8 coverage entry and additional args changes WIP - read source maps for faked entries WIP WIP Moved approach to empty v8 blocks

bcoe · 2019-11-19T23:20:09Z

@j03m thank you, I appreciate the hard work.

bcoe

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

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

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

bcoe · 2019-11-23T01:33:00Z

lib/report.js

@@ -127,6 +129,29 @@ class Report {
    return sources
  }

+  /**
+   * //TODO: use https://www.npmjs.com/package/convert-source-map


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.

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.

package.json

j03m · 2019-11-25T15:50:50Z

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 · 2019-11-25T15:51:02Z

Looks like ~~tests~~ lint are failing though :/ looking.

**edit: Its the lint, will fix.

j03m

Few changes in bound.

j03m · 2019-11-27T12:04:57Z

lib/report.js

@@ -127,6 +129,29 @@ class Report {
    return sources
  }

+  /**
+   * //TODO: use https://www.npmjs.com/package/convert-source-map


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 · 2019-11-27T13:08:46Z

lib/report.js

@@ -139,14 +142,50 @@ class Report {
  _getMergedProcessCov () {
    const { mergeProcessCovs } = require('@bcoe/v8-coverage')
    const v8ProcessCovs = []
+    const fileIndex = new Map() // Map<string, bool>


Scrutinizing this a bit now - this should be a Set

j03m · 2019-11-27T13:09:15Z

README.md

+## Checking for "full" source coverage using `--all`
+
+By default v8 will only give us coverage for files that were loaded by the engine. If there are source files in your 
+project that are flexed in production but not in your tests your coverage numbers will not reflect this. For example,


your tests,

j03m

Couple of minor things on my end.

j03m · 2019-11-27T13:09:47Z

README.md

+
+By default v8 will only give us coverage for files that were loaded by the engine. If there are source files in your 
+project that are flexed in production but not in your tests your coverage numbers will not reflect this. For example,
+if you your project's `main.js` loads `a.js` and `b.js` but your unit tests only load `a.js` your total coverage 


j03m · 2019-11-27T13:10:04Z

README.md

+if you your project's `main.js` loads `a.js` and `b.js` but your unit tests only load `a.js` your total coverage 
+could show as `100%` for `a.js` when in fact both `main.js` and `b.js` are uncovered.  
+
+By supplying `--all` to c8 all files in `cwd` obeying the `--include` and `--exclude` flags will be loaded into the


c8,

,obeying....flags,

j03m · 2019-11-27T13:11:38Z

lib/report.js

@@ -56,8 +60,8 @@ class Report {
    // use-case.
    if (this._allCoverageFiles) return this._allCoverageFiles

+    const map = libCoverage.createCoverageMap()


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

j03m · 2019-11-27T13:16:53Z

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 · 2019-11-28T14:48:19Z

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 · 2019-11-28T14:48:29Z

This should be g2g now!

bcoe · 2019-11-28T17:31:56Z

@j03m awesome work 👌

j03m · 2019-11-29T12:47:46Z

Thanks for all the advice/help!

j03m mentioned this pull request Oct 29, 2019

Feature: c8 --all #76

Closed

bcoe requested changes Nov 5, 2019

View reviewed changes

bcoe mentioned this pull request Nov 17, 2019

fix!: consistently resolve paths to absolute form istanbuljs/v8-to-istanbul#72

Merged

j03m mentioned this pull request Nov 17, 2019

Bad mappings when applying coverage istanbuljs/v8-to-istanbul#75

Closed

j03m force-pushed the implement--all branch 2 times, most recently from 2062766 to ff17bed Compare November 19, 2019 13:12

j03m force-pushed the implement--all branch from ff17bed to 4ffb5c7 Compare November 19, 2019 14:59

Merge branch 'master' into implement--all

99a6601

bcoe approved these changes Nov 23, 2019

View reviewed changes

bcoe reviewed Nov 23, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

Merge branch 'master' into implement--all

38199f1

j03m commented Nov 27, 2019

View reviewed changes

j03m added 2 commits November 27, 2019 07:18

Feedback post v8 mock changes

674be8f

Update README.me

9b5763e

j03m commented Nov 27, 2019

View reviewed changes

j03m added 2 commits November 28, 2019 09:36

Fix broken test

65a5453

README fixes. Map -> Set

2156f19

bcoe changed the title ~~Implement --all functionality~~ feat: adds --all functionality Nov 28, 2019

bcoe merged commit 2eb631e into bcoe:master Nov 28, 2019

robpalme mentioned this pull request May 18, 2020

Suggestion: --exclude-after-remap #224

Closed

github-actions bot mentioned this pull request Apr 20, 2022

chore(main): release 8.0.0 #394

Closed

mcknasty pushed a commit to mcknasty/c8 that referenced this pull request Feb 2, 2024

feat: adds --all functionality (bcoe#158)

dd01e0d

feat: adds --all functionality #158

feat: adds --all functionality #158

Conversation

j03m commented Oct 29, 2019 • edited Loading

Checklist

j03m commented Oct 29, 2019

j03m commented Oct 29, 2019

coveralls commented Oct 29, 2019 • edited Loading

Pull Request Test Coverage Report for Build 499

💛 - Coveralls

j03m commented Oct 30, 2019

bcoe left a comment

Choose a reason for hiding this comment

bcoe commented Nov 6, 2019

j03m commented Nov 6, 2019

j03m commented Nov 8, 2019

bcoe commented Nov 8, 2019

j03m commented Nov 9, 2019 • edited Loading

j03m commented Nov 9, 2019 • edited Loading

j03m commented Nov 10, 2019

bcoe commented Nov 10, 2019

j03m commented Nov 12, 2019

j03m commented Nov 17, 2019

bcoe commented Nov 18, 2019

j03m commented Nov 19, 2019 • edited Loading

bcoe commented Nov 19, 2019

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j03m commented Nov 25, 2019

j03m commented Nov 25, 2019 • edited Loading

j03m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j03m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j03m commented Nov 27, 2019

j03m commented Nov 28, 2019

j03m commented Nov 28, 2019

bcoe commented Nov 28, 2019

j03m commented Nov 29, 2019

j03m commented Oct 29, 2019 •

edited

Loading

coveralls commented Oct 29, 2019 •

edited

Loading

j03m commented Nov 9, 2019 •

edited

Loading

j03m commented Nov 9, 2019 •

edited

Loading

j03m commented Nov 19, 2019 •

edited

Loading

j03m commented Nov 25, 2019 •

edited

Loading