feat: support monocart report #521

cenfun · 2024-02-20T06:55:39Z

@bcoe This PR is to add Monocart report support for c8.

In order to adapt to c8, Monocart has made many adjustments, such as supporting Node 14 and allowing empty coverage for --all option, and so on.
Finally, it can pass the test today.
Please let me know if there are any issues, and I will provide more information as necessary.
Thanks

Checklist

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

bcoe · 2024-02-20T19:32:27Z

@cenfun thanks for the contribution, this looks really good.

Question, how do you represent the empty coverage in Istanbul format? This seems like something I could fix in v8-to-istanbul as well?

Currently this is what an uncovered file looks like when running --all:

{
  "/Users/bencoe/google/oss/c8-515/foo.js": {
    "path": "/Users/bencoe/google/oss/c8-515/foo.js",
    "all": true,
    "statementMap": {
      "0": {
        "start": {
          "line": 1,
          "column": 0
        },
        "end": {
          "line": 1,
          "column": 0
        }
      }
    },
    "s": {
      "0": 1
    },
    "branchMap": {},
    "b": {},
    "fnMap": {},
    "f": {}
  }

cenfun · 2024-02-21T01:34:19Z

Question, how do you represent the empty coverage in Istanbul format?

When running --all, in fact, it's adding empty coverage for untested files.

1, Adding an empty entry file for all the files. see here

{
  empty: true,
  type: 'js',
  url,
  source
}

2, Removing the empty file if it has coverage data after test.
3, Adding the empty coverage for the empty files (untested), see here

// if empty = true
item.functions = [{
    functionName: '',
    ranges: [{
        startOffset: 0,
        endOffset: item.source.length,
        count: 0
    }]
}];

So the whole file is uncovered, and everything is uncovered too.
I believe the step 3 is the same as c8. https://github.com/bcoe/c8/blob/main/lib/report.js#L272-L281

Another difference is, Monocart will parse the empty lines and comment lines in the file and exclude them from being uncovered.

For issue #515, if it is caused by the --all option, you may try adding step1 and step2 to correct it, as merging a mock's empty coverage data with real coverage data could potentially cause issues. That's why we have step1 and step2.

Try to remove some of the emptyReports that have been tested before the mergeProcessCovs.
https://github.com/bcoe/c8/blob/main/lib/report.js#L191-L196

bcoe · 2024-02-21T18:22:11Z

I believe the step 3 is the same as c8.

I think your approach is a bit better, because you don't end up with 0/1 branch coverage and 0/1 function coverage, rather you just end up with entries in the report that are more clearly "empty".

One issue however, is that I believe running with --100 would pass if every file that's loaded had 100% coverage, but there are some files that are not tested at all.

Along with adopting your approach, I think c8 should be updated to throw an error on --100 and similar if there are uncovered files.

jakebailey · 2024-02-22T05:01:04Z

This is a bit of a drive-by, but I tried this PR out in hopes it would fix #502; I get loads of these messages:

[MCR] failed to parse source file: src/services/formatting/formatting.ts Unexpected token (82:7)
[MCR] failed to parse source file: src/services/formatting/formattingContext.ts The keyword 'enum' is reserved (14:13)
[MCR] failed to parse source file: src/services/formatting/formattingScanner.ts Unexpected token (29:7)
[MCR] failed to parse source file: src/services/formatting/rule.ts Unexpected token (10:7)
[MCR] failed to parse source file: src/services/formatting/rules.ts Unexpected token (37:7)
[MCR] failed to parse source file: src/services/formatting/rulesMap.ts Unexpected token (19:40)
[MCR] failed to parse source file: src/services/formatting/smartIndenter.ts Unexpected token (62:7)
[MCR] failed to parse source file: src/services/getEditsForFileRename.ts Unexpected token (55:11)
[MCR] failed to parse source file: src/services/goToDefinition.ts Unexpected token (115:47)

Which seems problematic (given how much code out there is actually TypeScript).

But, the coverage looks more reasonable! With the experimental flag:

  checker.ts                                              |   95.21 |    95.41 |    98.4 |   95.29 | ...49050,49100,49114,49163,49199,49220-49223,49298,49323-49326,49342,49374,49540,49545,49706,49940,50204,50375,50551,50602,50926,50947-50948,50960,51069,51079,51116,51146,51153,51160,51195,51202

Compare to c8 as-is (definitely wrong):

  checker.ts                                              |   99.99 |    95.38 |   98.68 |   99.99 | 51117-51118

And in our --no-bundle mode (which runs tests without esbuild-ing it, so is very slow at runtime):

  checker.ts                                              |   97.56 |    95.88 |   98.68 |   97.56 | ...49221-49222,49224-49225,49299-49300,49324-49325,49327-49328,49343-49344,49375-49376,49546,49707-49708,49941,50004-50005,50205-50206,50603-50604,50927-50929,50948-50952,50961-50962,51117-51118

Not totally sure what the 2% difference is, but I'm happy that maybe there's a shred of hope to get good coverage info without nyc.

jakebailey · 2024-02-22T05:03:33Z

Well, the output coverage is a little... weird:

cenfun · 2024-02-22T09:35:56Z

@bcoe Yes, agree. Checking the coverage of untested files is truly a challenge, especially for checking functions and branches. For example --branches=80 it requires to know:

1, the total of branches.
2, the percent of branches (uncovered branches / total)

And the total of branches might be 0 if a file doesn't have any branches. At this point, the percentage doesn't make sense, we should skip the checking when total = 0, or it can be simply assumed that the percent is 100%.

The current challenge is that we don't know the total count of these metrics for the untested files.

For v8-to-istanbul, it doesn't parse the files, hence it doesn't know the total.
For Monocart, it can parse JavaScript into AST, so it can get the total. However, the untested files could be in any format, such as .ts, .vue, .svelte and so on, so it cannot be successfully parsed if it's not in the standard ECMA format. (this is why @jakebailey got the parser error message, but I have removed this message in version 2.5.4)

Anyway, I made a small fixing, see commit

if (file && file.empty) {
    process.exitCode = 1
    console.error(
      'ERROR: Empty coverage (untested file) does not meet threshold for ' +
        path.relative('./', file.sourcePath).replace(/\\/g, '/')
    )
    return
  }

When the checked file is an empty coverage, it always prompts that the threshold is not passed. Is this okay?

@jakebailey Thanks for the feedback. Can you provide a small repo which reproduces your issue? Perhaps I can help fix it.
And, I noticed that in your screenshot you are using the Istanbul's html report, which requires converting v8 format to Istanbul, so there might be some other issues such as problems with sourcemaps. Could you try the native v8 report?

c8 --experimental-monocart --reporter=v8 --reporter=console-details node foo.js

jakebailey · 2024-02-23T03:24:38Z

Can you provide a small repo which reproduces your issue? Perhaps I can help fix it.

I'm afraid I don't have a small repo; I'm running this on TypeScript itself: https://github.com/microsoft/TypeScript

Using c8 npm test -- --no-lint.

I noticed that in your screenshot you are using the Istanbul's html report, which requires converting v8 format to Istanbul, so there might be some other issues such as problems with sourcemaps. Could you try the native v8 report?

I tried with those flags, and get a similar result in the console:

checker.ts │  95.24 % │    95.22 % │  95.42 % │   98.41 % │  93.39 % │ 1511-1519,1530,1535,1542-1550,1563,1586,1595,16...

The HTML output looks a lot better in this PR than with monocart-coverage-reports alone (which did not correctly handle source maps):

Though it's not super clear to me in general how to read this info compared to strictly "hits per line".

But, I'm personally quite happy with this PR.

Scrolling through, there are some oddities, like:

Running c8 --experimental-monocart --reporter=v8 --reporter=console-details npm test -- --no-lint will get you a result, if you're looking for a stress test.

cenfun · 2024-02-23T13:37:41Z

@jakebailey thank you for providing the test case. I was able to fix few more issues in version 2.5.5

fixed UI performance issue. There is a file checker.ts is so big (2.8M), it could cause the rendering to freeze.
fixed the sourcemap issue. The sourcemap file run.js.map does not include the sourcesContent. For c8, the cource-map-cache does not include the source code but lineLengths (fake source).
some UI minor changes.
For the green x number button, it is actually the execution counts on its range, mouse hover on the green button will show the range. Just add a count switch button to display or hide the execution counts if it's too much and affects readability.
For hits per line, I thought that the native V8 coverage provides coverage for bytes, not lines, perhaps it's still not quite accustomed.

jakebailey · 2024-02-24T00:38:08Z

Thanks! I didn't test with mcr directly, just this PR again, and it all seems to be working from what I can tell. Looking forward to having something that works.

ericmorand · 2024-05-08T08:46:49Z

@cenfun I published a minimal repository to test c8 with bundles - namely rollup generated bundles: https://github.com/ericmorand/c8-with-bundle

This PR seems to fix C8 issues but I'll perform some more testings with some edge cases.

bcoe · 2024-06-09T23:49:46Z

@ericmorand how has your testing been going. This is a pretty big addition to the library, which is one reason I've been hesitant to land. But, if it's solving user problems, I think we should figure out how to expose Monocart.

@cenfun my other hesitancy with landing has been that lib/monocart-report.js seems to essentially introduce an alternate path through the codebase that repeats logic similar to report.js (my hope was that we could instead figure out some minor changes to the existing contract in report.js that would perhaps work with both Monocart or v8-to-istanbul). My concern is maintenance burden, since both files do similar things like instantiating test-exclude, rewriting file paths

cenfun · 2024-06-10T06:50:14Z

@bcoe good suggestion, thanks. I've already updated it, please review it.

bcoe · 2024-06-10T15:27:35Z

README.md

@@ -119,6 +120,12 @@ The `--100` flag can be set for the `check-coverage` as well:
 c8 check-coverage --100
 ```

+## Using Monocart coverage reports (experimental)
+[Monocart](https://github.com/cenfun/monocart-coverage-reports) will bring additional support for native V8 coverage reports, for example:


I think we should explain what monocart is a bit better. It's an alternate tool for converting raw V8 coverage reports to other coverage formats, as an alternate to v8-to-istanbul and istanbul reports.

I have added some explanations, please take a look.

bcoe · 2024-06-10T15:28:42Z

lib/report.js

@@ -89,6 +95,128 @@ class Report {
    }
  }

+  async runMonocart () {


👍 I prefer now that we're sharing some of the same behaviour in report.js.

README.md

Co-authored-by: Benjamin E. Coe <[email protected]>

…onocart-report

lib/report.js

bcoe · 2024-06-11T14:25:12Z

@cenfun running the test suite once deletes the index.js file in monocart reports:

bash-3.2$ cat node_modules/monocart-coverage-reports/lib/index.
index.d.ts    index.js.bak  index.mjs

I think we should drop this logic from tests:

        const mcrPath = require.resolve('monocart-coverage-reports')
        rename(mcrPath, mcrPath + '.bak', (err) => {
          if (err) { throw err }
        })

It seems fragile and I think you could just mock a method like this:

  async getMonocart () {
    let MCR
    try {
      MCR = await this.importMonocart();
    } catch (e) {
      console.info(e);
      console.error('--experimental-monocart requires the plugin monocart-coverage-reports. Run: "npm i monocart-coverage-reports --save-dev"')
      process.exit(1)
    }
    return MCR
  }

  async importMonocart () {
    await import('monocart-coverage-reports')
  }

so, mock importMonocart to throw.

cenfun · 2024-06-11T15:20:21Z

@bcoe it‘s done, please review.

bcoe · 2024-06-11T16:16:17Z

@cenfun this looks good to me, but I'm having test failures locally:

2 failing

  1) c8
       --extension
         includes coverage when extensions specified with --all:

      AssertionError: expected value to match snapshot c8 --extension includes coverage when extensions specified with --all 1
      + expected - actual

       hey
       ---------------------------------------|---------|----------|---------|---------|------------------------
       File                                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s      
       ---------------------------------------|---------|----------|---------|---------|------------------------
      -All files                              |     3.2 |    11.11 |    5.76 |     3.2 |                        
      +All files                              |    3.21 |    12.24 |    6.38 |    3.21 |                        
        c8                                    |       0 |        0 |       0 |       0 |                        
         index.js                             |       0 |        0 |       0 |       0 | 1                      
        c8/bin                                |       0 |        0 |       0 |       0 |                        
         c8.js                                |       0 |        0 |       0 |       0 | 1-43                   
        c8/coverage                           |       0 |        0 |       0 |       0 |                        
         block-navigation.js                  |       0 |        0 |       0 |       0 | 1-87                   
      -  coverage-data.js                     |       0 |        0 |       0 |       0 | 1                      
         prettify.js                          |       0 |        0 |       0 |       0 | 1-2                    
         sorter.js                            |       0 |        0 |       0 |       0 | 1-196                  
      - c8/coverage/assets                    |       0 |        0 |       0 |       0 |                        
      -  monocart-code-viewer.js              |       0 |        0 |       0 |       0 | 1                      
      -  monocart-coverage-app.js             |       0 |        0 |       0 |       0 | 1                      
      -  monocart-formatter.js                |       0 |        0 |       0 |       0 | 1                      
      -  turbogrid.js                         |       0 |        0 |       0 |       0 | 1                      
        c8/lib                                |       0 |        0 |       0 |       0 |                        
         parse-args.js                        |       0 |        0 |       0 |       0 | 1-229                  
         report.js                            |       0 |        0 |       0 |       0 | 1-542                  
         source-map-from-file.js              |       0 |        0 |       0 |       0 | 1-100                  
      
      at Context.<anonymous> (test/integration.js:697:40)
      at process.processImmediate (node:internal/timers:476:21)

  2) c8 mergeAsync
       --extension
         includes coverage when extensions specified with --all:

      AssertionError: expected value to match snapshot c8 mergeAsync --extension includes coverage when extensions specified with --all 1
      + expected - actual

       hey
       ---------------------------------------|---------|----------|---------|---------|------------------------
       File                                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s      
       ---------------------------------------|---------|----------|---------|---------|------------------------
      -All files                              |     3.2 |    11.11 |    5.76 |     3.2 |                        
      +All files                              |    3.21 |    12.24 |    6.38 |    3.21 |                        
        c8                                    |       0 |        0 |       0 |       0 |                        
         index.js                             |       0 |        0 |       0 |       0 | 1                      
        c8/bin                                |       0 |        0 |       0 |       0 |                        
         c8.js                                |       0 |        0 |       0 |       0 | 1-43                   
        c8/coverage                           |       0 |        0 |       0 |       0 |                        
         block-navigation.js                  |       0 |        0 |       0 |       0 | 1-87                   
      -  coverage-data.js                     |       0 |        0 |       0 |       0 | 1                      
         prettify.js                          |       0 |        0 |       0 |       0 | 1-2                    
         sorter.js                            |       0 |        0 |       0 |       0 | 1-196                  
      - c8/coverage/assets                    |       0 |        0 |       0 |       0 |                        
      -  monocart-code-viewer.js              |       0 |        0 |       0 |       0 | 1                      
      -  monocart-coverage-app.js             |       0 |        0 |       0 |       0 | 1                      
      -  monocart-formatter.js                |       0 |        0 |       0 |       0 | 1                      
      -  turbogrid.js                         |       0 |        0 |       0 |       0 | 1                      
        c8/lib                                |       0 |        0 |       0 |       0 |                        
         parse-args.js                        |       0 |        0 |       0 |       0 | 1-229                  
         report.js                            |       0 |        0 |       0 |       0 | 1-542                  
         source-map-from-file.js              |       0 |        0 |       0 |       0 | 1-100                  
      
      at Context.<anonymous> (test/integration.js:697:40)
      at process.processImmediate (node:internal/timers:476:21)
    ```

cenfun · 2024-06-11T16:26:58Z

@bcoe It seems that there are some legacy files on your local, possibly generated from a previous running but not test.
The ci is passed.
can you try removing the coverage dir and test again?

bcoe

LGTM! thank you for all the hard work getting this ready to land.

cenfun force-pushed the monocart-report branch 4 times, most recently from e58292a to fad76db Compare March 1, 2024 10:19

cenfun force-pushed the monocart-report branch from fad76db to 114be25 Compare March 5, 2024 01:51

cenfun force-pushed the monocart-report branch from a3dbad3 to ea1b279 Compare March 15, 2024 11:44

feat: support monocart report

2308d4a

cenfun force-pushed the monocart-report branch from ea1b279 to 2308d4a Compare March 15, 2024 11:47

Update version 2.7.3

caae21f

jakebailey mentioned this pull request May 8, 2024

Coverage information too high for large esbuild produced bundle #502

Open

2.7.10

1a7d020

cenfun force-pushed the monocart-report branch from de8d71a to 1a7d020 Compare May 8, 2024 12:06

Merge branch 'main' into monocart-report

59a6161

better integration into the report.js

4e856a2

bcoe reviewed Jun 10, 2024

View reviewed changes

doc: explain for monocart

28be91c

bcoe reviewed Jun 11, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

cenfun and others added 10 commits June 11, 2024 12:14

Update README.md

27966b3

Co-authored-by: Benjamin E. Coe <[email protected]>

move MCR to peerDependencies

5aa4422

Update lib/parse-args.js

9566904

Co-authored-by: Benjamin E. Coe <[email protected]>

Merge branch 'monocart-report' of https://github.com/cenfun/c8 into m…

2218b53

…onocart-report

resolve conflicts

237d262

Merge remote-tracking branch 'origin/main' into monocart-report

4830f62

resolve conflicts

1d2ff88

move to try/catch

85303b4

test case for monocart installation

714cdd6

optimize arguments

c88abbf

bcoe reviewed Jun 11, 2024

View reviewed changes

lib/report.js Outdated Show resolved Hide resolved

bcoe and others added 2 commits June 11, 2024 10:07

Update lib/report.js

6fd00e5

fixed unit test

4b0947a

updated the test case for import-mcr

67bbd57

bcoe approved these changes Jun 11, 2024

View reviewed changes

bcoe merged commit 2e5e297 into bcoe:main Jun 11, 2024
4 checks passed

github-actions bot mentioned this pull request Jun 11, 2024

chore(main): release 10.1.0 #533

Merged

jlenon7 mentioned this pull request Jul 3, 2024

[Snyk] Upgrade c8 from 10.0.0 to 10.1.1 AthennaIO/Test#133

Merged

cenfun deleted the monocart-report branch July 23, 2024 00:13

This was referenced Sep 7, 2024

[Snyk] Upgrade: , , c8, rimraf, rollup, typescript, vitest leonardoadame/Affiliate-tech#734

Open

[Snyk] Upgrade: , , c8, rimraf, rollup, typescript, vitest leonardoadame/Affiliate-tech#771

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support monocart report #521

feat: support monocart report #521

cenfun commented Feb 20, 2024 •

edited

Loading

bcoe commented Feb 20, 2024

cenfun commented Feb 21, 2024 •

edited

Loading

bcoe commented Feb 21, 2024

jakebailey commented Feb 22, 2024

jakebailey commented Feb 22, 2024

cenfun commented Feb 22, 2024 •

edited

Loading

jakebailey commented Feb 23, 2024

cenfun commented Feb 23, 2024 •

edited

Loading

jakebailey commented Feb 24, 2024

ericmorand commented May 8, 2024

bcoe commented Jun 9, 2024

cenfun commented Jun 10, 2024

bcoe Jun 10, 2024

cenfun Jun 10, 2024

bcoe Jun 10, 2024

bcoe commented Jun 11, 2024

cenfun commented Jun 11, 2024

bcoe commented Jun 11, 2024

cenfun commented Jun 11, 2024 •

edited

Loading

bcoe left a comment

feat: support monocart report #521

feat: support monocart report #521

Conversation

cenfun commented Feb 20, 2024 • edited Loading

Checklist

bcoe commented Feb 20, 2024

cenfun commented Feb 21, 2024 • edited Loading

bcoe commented Feb 21, 2024

jakebailey commented Feb 22, 2024

jakebailey commented Feb 22, 2024

cenfun commented Feb 22, 2024 • edited Loading

jakebailey commented Feb 23, 2024

cenfun commented Feb 23, 2024 • edited Loading

jakebailey commented Feb 24, 2024

ericmorand commented May 8, 2024

bcoe commented Jun 9, 2024

cenfun commented Jun 10, 2024

bcoe Jun 10, 2024

Choose a reason for hiding this comment

cenfun Jun 10, 2024

Choose a reason for hiding this comment

bcoe Jun 10, 2024

Choose a reason for hiding this comment

bcoe commented Jun 11, 2024

cenfun commented Jun 11, 2024

bcoe commented Jun 11, 2024

cenfun commented Jun 11, 2024 • edited Loading

bcoe left a comment

Choose a reason for hiding this comment

cenfun commented Feb 20, 2024 •

edited

Loading

cenfun commented Feb 21, 2024 •

edited

Loading

cenfun commented Feb 22, 2024 •

edited

Loading

cenfun commented Feb 23, 2024 •

edited

Loading

cenfun commented Jun 11, 2024 •

edited

Loading