nodejs
test_runner: avoid coverage report partial file names
Hey, this PR should address issue #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.
Co-author: Medhansh404 [email protected]
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 :warning: |
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%) |
:arrow_down: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.)
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 π
I was thinking about adding some tests for getCoverageReport.
I think that's a good idea.
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 π
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!
Co-author: Medhansh404 [email protected]
The syntax for Co-Authors is:
Co-Authored-By: Medhansh404 <[email protected]>
Hey @cjihrig, yes, I'm reworking it and will come up with a proposal in 1-2 days
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)
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/
@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
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.
@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?
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.
@cjihrig, @RedYetiDev, I think that this PR is ready to be reviewed.
I tried to refactor the code a little bit to improve readability and added some tests for color and truncation cases.
Regarding the semver-major, does it also apply also to experimental features?
Regarding the semver-major, does it also apply also to experimental features?
I have not reviewed the code, but semver does not apply to experimental features (although we should not break things if we can avoid it). We also have this in the docs:
The exact output of these reporters is subject to change between versions of Node.js, and should not be relied on programmatically. If programmatic access to the test runner's output is required, use the events emitted by the TestsStream.
@mcollina, I just fixed a broken test. Could I please ask you to trigger the CI again? π