-
Notifications
You must be signed in to change notification settings - Fork 91
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: support monocart report #521
Conversation
@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 Currently this is what an uncovered file looks like when running {
"/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": {}
} |
When running
{
empty: true,
type: 'js',
url,
source
}
// 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. 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 Try to remove some of the |
I think your approach is a bit better, because you don't end up with One issue however, is that I believe running with Along with adopting your approach, I think c8 should be updated to throw an error on |
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:
Which seems problematic (given how much code out there is actually TypeScript). But, the coverage looks more reasonable! With the experimental flag:
Compare to c8 as-is (definitely wrong):
And in our
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. |
@bcoe Yes, agree. Checking the coverage of untested files is truly a challenge, especially for checking
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.
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. c8 --experimental-monocart --reporter=v8 --reporter=console-details node foo.js |
I'm afraid I don't have a small repo; I'm running this on TypeScript itself: https://github.com/microsoft/TypeScript Using
I tried with those flags, and get a similar result in the console:
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 |
@jakebailey thank you for providing the test case. I was able to fix few more issues in version 2.5.5
|
Thanks! I didn't test with |
e58292a
to
fad76db
Compare
@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. |
@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 |
@bcoe good suggestion, thanks. I've already updated it, please review it. |
README.md
Outdated
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some explanations, please take a look.
@@ -89,6 +95,128 @@ class Report { | |||
} | |||
} | |||
|
|||
async runMonocart () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I prefer now that we're sharing some of the same behaviour in report.js
.
@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 passingnpm run test:snap
(to update the snapshot)