Skip to content

Support statement coverage #54530

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

Open
avivkeller opened this issue Aug 24, 2024 · 6 comments
Open

Support statement coverage #54530

avivkeller opened this issue Aug 24, 2024 · 6 comments
Labels
coverage Issues and PRs related to native coverage support. feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@avivkeller
Copy link
Member

avivkeller commented Aug 24, 2024

I think the Node.js coverage reporter should include support for reporting statement coverage.

My idea is to parse the source code into an Abstract Syntax Tree (AST) using acorn-walk, and then use the Statement callback to extract statements. They could then be converted to CoverageStatements, (similarly to how the getLines function works with lines to CoverageLines).

These statements could then be mapped to ranges in the same way that lines are handled in mapRangeToLines.

I saw a (extremely complicated) way of doing this in https://github.com/cenfun/monocart-coverage-reports/, so it appears to be possible.

@avivkeller avivkeller added feature request Issues that request new features to be added to Node.js. coverage Issues and PRs related to native coverage support. labels Aug 24, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Aug 24, 2024
@avivkeller avivkeller moved this from Awaiting Triage to Triaged in Node.js feature requests Aug 24, 2024
@avivkeller avivkeller added the test_runner Issues and PRs related to the test runner subsystem. label Aug 24, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Aug 24, 2024

I see at least two issues with doing this:

  • This will hurt performance of coverage, particularly on large projects.
  • We would be parsing the code with a different parser than the one V8 uses. This should almost always be fine, but could have some issues particularly when new syntax is introduced.

Of course there is some code complexity to doing this too. While I agree that statement coverage would be nice to have, I haven't seen any users of the test runner asking for it either. My understanding (which @MoLow seemed to confirm) is that C8 does not report this metric from V8 coverage. If they aren't doing it, I don't see a great reason for us to do it either.

@cenfun
Copy link

cenfun commented Aug 27, 2024

The latest version of C8 can do this with --experimental-monocart

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

As an alternative, we can also use the custom test reporter

node --test-reporter=node-monocart-coverage --test tests

see node-monocart-coverage

Indeed, it is extremely complicated and hurt performance.
There is a benchmark for reference, TypeScript is currently using c8 --experimental-monocart to generate coverage reports.

  • The TypeScript source code have almost 17M
  • It takes almost 19 seconds to generate the coverage reports using the default GitHub Action environment (test job)

image

Check TypeScript coverage job in CI
See microsoft/TypeScript#58850

@AriPerkkio
Copy link

If this was implemented directly in V8, whole JS/Node ecosystem could benefit from this. Adding AST based coverage analysis is not ideal for performance.

Copy link
Contributor

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Feb 27, 2025
@ljharb
Copy link
Member

ljharb commented Feb 27, 2025

This is pretty important since basically every other coverage tool in the ecosystem has this 4th metric. Perhaps it would make more sense to see if someone can contribute it to v8 directly? That way all of the valid downsides listed in #54530 (comment) wouldn't apply.

@ljharb ljharb removed the stale label Feb 27, 2025
@cenfun
Copy link

cenfun commented Feb 28, 2025

The compromise is that the Node test runner can export all the native V8 coverage data, as well as the source code and sourcemap, in custom reports test:coverage event. This should be relatively easy and allow third-party tools to extend.

const { Transform } = require('node:stream');

const customReporter = new Transform({
  writableObjectMode: true,
  transform(event, encoding, callback) {
    switch (event.type) {
      // ...
      case 'test:coverage': {

        const { rawV8CoverageData, sourcesAndSourcemaps } = event.data;

        break;
      }
    }
  },
});

module.exports = customReporter;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage Issues and PRs related to native coverage support. feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
Development

No branches or pull requests

5 participants