test_runner: avoid coverage report partial file names

[pmarchini]

Fixes: #51299

I noticed that the issue has been stalled for months, even though a PR with the solution was practically accepted. I removed some duplication and fixed a small bug (which caused empty lines to be printed, breaking the table), but otherwise, I used the initially accepted proposal.

P.S.: I've included the user who initially started the work as a co-author.

P.P.S.: I'm wondering if it might make sense to move the report generation out of the utils file. Given the amount of logic involved, it could be beneficial to place it in a separate file to increase cohesion.

P.P.P.S.: I noticed that no tests were added in the current PR because it should already be covered, but I wonder if it might be worth creating specific unit tests for getCoverageReport.

EDIT: I changed the implementation proposing a tree view instead of a multiline, for this reason I removed the co-author.

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[codecov]

Codecov Report

Attention: Patch coverage is 3.38983% with 114 lines in your changes missing coverage. Please review.

Project coverage is 88.02%. Comparing base (6db320a) to head (d933e3f).
Report is 526 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/utils.js	3.38%	114 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54379      +/-   ##
==========================================
- Coverage   88.07%   88.02%   -0.06%     
==========================================
  Files         651      651              
  Lines      183538   183619      +81     
  Branches    35861    35853       -8     
==========================================
- Hits       161652   161628      -24     
- Misses      15145    15233      +88     
- Partials     6741     6758      +17     

Files with missing lines	Coverage Δ
lib/internal/test_runner/utils.js	55.74% <3.38%> (-8.11%)	⬇️

... and 33 files with indirect coverage changes

[cjihrig]

Should there be any test changes for this PR?

[pmarchini]

I would say yes, but I noticed that a previous PR was almost accepted, and I didn't investigate further, my bad!

I believe the reason for no broken tests is that we are missing this case in the tests.
(The coverage also shows the same thing; I've just seen the CI results.)

[cjihrig]

We should definitely add tests.

[pmarchini]

We should definitely add tests.

I'm on it. Thanks for your input, @cjihrig! 😄

While trying to replicate the issue, I've noticed that the proposed changes only partially solve it, so I've implemented a potential solution.
My only concern at the moment is that, if I'm not mistaken, we don't have any tests that cover the output under different "width" conditions.
I was thinking about adding some tests for getCoverageReport, but I'm worried it might become too "white-box".

Do you have any suggestions?

Thanks in advance 🚀

[cjihrig]

I was thinking about adding some tests for getCoverageReport.

I think that's a good idea.

[pmarchini]

I was thinking about adding some tests for getCoverageReport.

I think that's a good idea.

Hey @cjihrig, I've prepared 2 different ways of testing this behavior: one more black-box and one more white-box.
I would greatly appreciate your feedback on this if possible 😄

getCoverageReport test example: #54506
output snapshot tests: #54494

The first one is just an example, and many more test cases should be added.
The same goes for the second one.

Thank you very much for your incredible support 🚀

[cjihrig]

one more black-box and one more white-box

For testing the formatting of the coverage report, I personally prefer the snapshot based approach.

Thank you very much for your incredible support

Thanks for helping to improve the test runner!

[avivkeller]

Co-author: Medhansh404 [email protected]

The syntax for Co-Authors is:

Co-Authored-By: Medhansh404 <[email protected]>

[cjihrig]

@pmarchini is this still supposed to be in draft mode?

[pmarchini]

Hey @cjihrig, yes, I'm reworking it and will come up with a proposal in 1-2 days

[pmarchini]

Hey @cjihrig,

While working on this, I compared our output with that of other coverage tools. I've looked at many of them, and for the "base" format, I believe a "file tree" would definitely be more readable and accessible, even with "narrower" widths.

Honestly, I think that simply defining a better "minimum width" for each column, giving more priority to the file name, could be a solution, but only a partial one. If the final user of the command (without specific reporters) is the developer, then I believe the focus should be on providing human-readable output.

The actual behavior(when we have many uncovered lines), in a terminal under 100 columns, is the following one:

TAP version 13
# Subtest: Coverage Print Fixed Width 100
ok 1 - Coverage Print Fixed Width 100
  ---
  duration_ms: *
  ...
1..1
# tests 1
# suites 0
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms *
# start of coverage report
# --------------------------------------------------------------------------------------------------
# file     | line % | branch % | funcs % | uncovered lines
# --------------------------------------------------------------------------------------------------
# …ap/a.js |  55.77 |   100.00 |    0.00 | 5-7 9-11 13-15 17-19 29-30 40-42 45-47 50-52
# …ap/b.js |  45.45 |   100.00 |    0.00 | 5-7 9-11
# …ines.js |  50.99 |    42.86 |    1.92 | 5-7 9-11 13-15 17-19 29-30 40-42 45-47 50-52 55-57 59-6…
# …nes.mjs | 100.00 |   100.00 |  100.00 | 
# --------------------------------------------------------------------------------------------------
# all fil… |  52.80 |    60.00 |    1.61 |
# --------------------------------------------------------------------------------------------------
# end of coverage report

I was thinking about something like this:

TAP version 13
# Subtest: Coverage Print Fixed Width 100
ok 1 - Coverage Print Fixed Width 100
  ---
  duration_ms: *
  ...
1..1
# tests 1
# suites 0
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms *
# start of coverage report
# --------------------------------------------------------------------------------------------------
# file                        | line % | branch % | funcs % | uncovered lines
# --------------------------------------------------------------------------------------------------
# test
#  fixtures
#   test-runner
#    coverage-snap
#     a.js                    |  55.77 |   100.00 |    0.00 | 5-7 9-11 13-15 17-19 29-30 40-4…
#     b.js                    |  45.45 |   100.00 |    0.00 | 5-7 9-11
#    output
#     coverage-width-100.mjs  | 100.00 |   100.00 |  100.00 | 
# --------------------------------------------------------------------------------------------------
# all files                   |  60.81 |   100.00 |    0.00 |
# --------------------------------------------------------------------------------------------------
# end of coverage report

What do you think about this?

(this is only a quickly implemented POC, but I would like to ask for feedback before implementing unrequested features)

[avivkeller]

believe the focus should be on providing human-readable output

IIUC spec is supposed to be human readable, and tap is supposed to be based on https://node-tap.org/tap-format/

[pmarchini]

@redyetidev, you're right. I think I expressed myself poorly.

What I meant was to avoid, where possible, truncations or ellipses that make the content of the output difficult to understand, without changing the output format.

I believe the most important information is the file itself in conjunction with the coverage percentage. At the moment, the main focus of the output seems to be the uncovered lines

[cjihrig]

What do you think about this?

Seems fine to me. I don't have many opinions on how any of the reporters actually display data as long as it's reasonable and performant.

[RuggeroVisintin]

@pmarchini as discussed offline, if the principle is to keep this human readable, I would do my best to preserve the filename readability, even in the worst-case scenario of a long hierarchy or filename, by chunking and sending the filename on a new line if needed.

The same mechanism could also be applied to new lines if needed/wanted.

An example of how the output might look like

TAP version 13
# Subtest: Coverage Print Fixed Width 100
ok 1 - Coverage Print Fixed Width 100
  ---
  duration_ms: *
  ...
1..1
# tests 1
# suites 0
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms *
# start of coverage report
# --------------------------------------------------------------------------------------------------
# file                        | line % | branch % | funcs % | uncovered lines
# --------------------------------------------------------------------------------------------------
# test
#  fixtures
#   test-runner
#    coverage-snap
#     a.js                    |  55.77 |   100.00 |    0.00 | 5-7 9-11 13-15 17-19 29-30 40-4…
#     b.js                    |  45.45 |   100.00 |    0.00 | 5-7 9-11
#    output
#     long-file-name-that-goes|        |          |         |
#     ↵ -on-a-new-line.js     | 100.00 |   100.00 |  100.00 |
#     coverage-width-100.mjs  | 100.00 |   100.00 |  100.00 | 
# --------------------------------------------------------------------------------------------------
# all files                   |  60.81 |   100.00 |    0.00 |
# --------------------------------------------------------------------------------------------------
# end of coverage report

The formatting isn't necessarily the best but you get the idea 😁. WDYAT?

[avivkeller]

This modifies an existing behavior, is it semver-major?

[pmarchini]

This modifies an existing behavior, is it semver-major?

@redyetidev, if this proposed solution is accepted, then yes, it will be a breaking change.

At the moment, I think this PR should be used for discussion.

By the way, I believe that any change in this part of the codebase is likely to be breaking in many edge cases.

[pmarchini]

I noticed that the CI was failing during the rebase, so I rebased the branch locally.
hope this fixes the issue

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/62456/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/62490/

[pmarchini]

Hey guys, is this PR ready to be landed or is something blocking it?
Thanks 😁

[avivkeller]

Hey guys, is this PR ready to be landed or is something blocking it?

It's been marked author ready - Meaning there is nothing blocking it. Sooner or later, someone will add commit-queue, allowing this to land 🚀

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54379
✔  Done loading data for nodejs/node/pull/54379
----------------------------------- PR info ------------------------------------
Title      test_runner: avoid coverage report partial file names (#54379)
Author     Pietro Marchini <[email protected]> (@pmarchini)
Branch     pmarchini:issue/51299 -> nodejs:main
Labels     experimental, author ready, coverage, commit-queue-squash, test_runner
Commits    13
 - test_runner: avoid coverage report partial file names
 - test_runner: code coverage files tree view
 - test_runner: remove unnecessary padding
 - test_runner: use primordials
 - test_runner: improve readability
 - test_runner: support tty
 - test_runner: revert truncateEnd
 - test_runner: lint fix
 - test_runner: update spaces calculation
 - test: add coverage edge case test
 - test: force color
 - test_runner: support windows and skip color tests when needed
 - test_runner: print tree correctly - Windows
Committers 1
 - Pietro Marchini <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54379
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54379
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - test_runner: avoid coverage report partial file names
   ⚠  - test_runner: code coverage files tree view
   ⚠  - test_runner: remove unnecessary padding
   ⚠  - test_runner: use primordials
   ⚠  - test_runner: improve readability
   ⚠  - test_runner: support tty
   ⚠  - test_runner: revert truncateEnd
   ⚠  - test_runner: lint fix
   ⚠  - test_runner: update spaces calculation
   ⚠  - test: add coverage edge case test
   ⚠  - test: force color
   ⚠  - test_runner: support windows and skip color tests when needed
   ⚠  - test_runner: print tree correctly - Windows
   ℹ  This PR was created on Wed, 14 Aug 2024 16:32:46 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54379#pullrequestreview-2282060963
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54379#pullrequestreview-2283580312
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/54379#pullrequestreview-2289793953
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-09-17T00:43:47Z: https://ci.nodejs.org/job/node-test-pull-request/62490/
- Querying data for job/node-test-pull-request/62490/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10925186243

[aduh95]

Landed in 50136a1