Skip to content
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

More detail from test_runner coverage report #49303

Closed
philnash opened this issue Aug 24, 2023 · 1 comment · Fixed by #49320
Closed

More detail from test_runner coverage report #49303

philnash opened this issue Aug 24, 2023 · 1 comment · Fixed by #49320
Labels
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

@philnash
Copy link
Contributor

What is the problem this feature will solve?

First, I understand that test coverage is still experimental. I'm hoping to move it in the direction of less experimental! I've been working on reporters for both test executions and then looking into test coverage reporters.

I've been trying to implement both the SonarQube generic test coverage report and then also an lcov-text report. With the current output of the test:coverage event for a reporter, there isn't enough detail to produce either of these reports.

What is the feature you are proposing to solve the problem?

I propose that the test:coverage event produces more data. Namely:

  • Coverage count for each line of code
  • Detail on functions, their name, first line number, and coverage count
  • Detail on branches, their first line number, and coverage count

As part of this, I would also propose that the array of uncoveredLineNumbers is removed, as it is serviced by coverage counts per line.

I would like to see each file object in the summary look like the following, with keys for functions, branches and lines:

{
  "path": "/path/to/index.js",
  "totalLineCount": 9,
  "totalBranchCount": 3,
  "totalFunctionCount": 1,
  "coveredLineCount": 7,
  "coveredBranchCount": 2,
  "coveredFunctionCount": 1,
  "coveredLinePercent": 77.77777777777779,
  "coveredBranchPercent": 66.66666666666666,
  "coveredFunctionPercent": 100,
  "functions": [
    {
      "name": "conditionalAdd",
      "count": 1,
      "line": 3
    }
  ],
  "branches": [
    {
      "line": 1,
      "count": 1
    },
    {
      "line": 3,
      "count": 1
    },
    {
      "line": 6,
      "count": 0
    }
  ],
  "lines": [
    {
      "line": 1,
      "count": 1
    },
    {
      "line": 2,
      "count": 1
    },
    {
      "line": 3,
      "count": 1
    },
    {
      "line": 4,
      "count": 1
    },
    {
      "line": 5,
      "count": 1
    },
    {
      "line": 6,
      "count": 1
    },
    {
      "line": 7,
      "count": 0
    },
    {
      "line": 8,
      "count": 0
    },
    {
      "line": 9,
      "count": 1
    }
  ]
}

I have a basic implementation of this, and would be happy to tidy it up for a pull request as well as update the existing reporters to handle the new format. I also wondered whether an lcov-text coverage report would be worth providing out of the box or whether that is better in a userland module?

What alternatives have you considered?

I considered keeping the uncoveredLineNumbers array, but that puts more work on report implementors to merge an array of numbers and an array of objects if they want to report in order.

There is certainly a consideration that this will be a large object. I considered whether sending the data as tuple style arrays (e.g. for a function [name, count, line], for a line [line,count] would save memory. It likely would, but I don't know how important that is, so I erred towards developer experience and using objects for the data.

@philnash philnash added the feature request Issues that request new features to be added to Node.js. label Aug 24, 2023
@atlowChemi atlowChemi added the test_runner Issues and PRs related to the test runner subsystem. label Aug 24, 2023
@cjihrig
Copy link
Contributor

cjihrig commented Aug 24, 2023

I have a basic implementation of this, and would be happy to tidy it up for a pull request as well as update the existing reporters to handle the new format.

As long as it works as well or better than what is currently there, I say go for it.

philnash added a commit to philnash/node that referenced this issue Aug 25, 2023
This is a breaking change for the format of test:coverage events. But the test coverage is still experimental, so I don't believe it requires a semver-major bump.
Fixes nodejs#49303
philnash added a commit to philnash/node that referenced this issue Aug 25, 2023
This is a breaking change for the format of test:coverage events. But
the test coverage is still experimental, so I don't believe it requires
a semver-major bump.

Fixes nodejs#49303
nodejs-github-bot pushed a commit that referenced this issue Aug 30, 2023
This is a breaking change for the format of test:coverage events. But
the test coverage is still experimental, so I don't believe it requires
a semver-major bump.

Fixes #49303

PR-URL: #49320
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
UlisesGascon pushed a commit that referenced this issue Sep 10, 2023
This is a breaking change for the format of test:coverage events. But
the test coverage is still experimental, so I don't believe it requires
a semver-major bump.

Fixes #49303

PR-URL: #49320
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
This is a breaking change for the format of test:coverage events. But
the test coverage is still experimental, so I don't believe it requires
a semver-major bump.

Fixes nodejs#49303

PR-URL: nodejs#49320
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
None yet
Development

Successfully merging a pull request may close this issue.

3 participants