FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add tests for runner coverage with different stdout column widths by pmarchini · Pull Request #54494 · nodejs/node · GitHub

/ node Public

test: add tests for runner coverage with different stdout column widths - #54494

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pmarchini:test/different-widths-runner-coverage
Aug 24, 2024
ConversationCommits (2)ChecksFiles changed
Merged

test: add tests for runner coverage with different stdout column widths#54494
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pmarchini:test/different-widths-runner-coverage

Conversation

pmarchini commented Aug 22, 2024

Copy link
Copy Markdown
Member

While working on #51299, I saw that we don't have snapshot tests related to the test runner's coverage output under different process.stdout.columns.
I think that we should add these tests before working on #51299 in order to have snapshots of the actual behavior before changing it

nodejs-github-bot commented Aug 22, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 22, 2024
pmarchini marked this pull request as ready for review August 22, 2024 10:04
pmarchini force-pushed the test/different-widths-runner-coverage branch from d8bb713 to 1cf8c89 Compare August 22, 2024 11:08
pmarchini marked this pull request as draft August 22, 2024 11:55

codecov Bot commented Aug 22, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.34%. Comparing base (cc26951) to head (3f5526e).
Report is 332 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54494      +/-   ##
==========================================
- Coverage   87.34%   87.34%   -0.01%     
==========================================
  Files         649      649              
  Lines      182544   182576      +32     
  Branches    35030    35041      +11     
==========================================
+ Hits       159445   159471      +26     
- Misses      16372    16380       +8     
+ Partials     6727     6725       -2

see 33 files with indirect coverage changes

pmarchini force-pushed the test/different-widths-runner-coverage branch from 1cf8c89 to e6a18f8 Compare August 22, 2024 13:50
pmarchini marked this pull request as ready for review August 22, 2024 13:58
pmarchini marked this pull request as draft August 22, 2024 13:58
pmarchini marked this pull request as ready for review August 22, 2024 15:50
pmarchini mentioned this pull request Aug 22, 2024
Merged
cjihrig approved these changes Aug 22, 2024
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some of the files are missing newlines at the bottom (GitHub shows a red circle with a line through it). LGTM other than that though. Thanks!

Some of the fixtures also don't follow the style guidelines because we don't lint the fixtures. Let's keep them though since it gives us a wider range of inputs to test coverage on.

By the way, I realize you were testing the formatting of the coverage report here. But, by any chance did you verify the data in the coverage report manually? I'm just asking because I'd like to try getting code coverage stable in the future, so any bugs we can get rid of are one step closer to that.

pmarchini commented Aug 22, 2024

Copy link
Copy Markdown
Member Author

@cjihrig, No, I didn't, but while fixing the styling issues and newlines, I'm also going to take a look at the quality of the coverage itself 😁

pmarchini force-pushed the test/different-widths-runner-coverage branch from 3e546b4 to 3f5526e Compare August 23, 2024 11:25

pmarchini commented Aug 23, 2024

Copy link
Copy Markdown
Member Author

Hey @cjihrig, I linted the fixture files as much as possible without breaking the tests.
I couldn't import common because the coverage changes depending on the OS, etc.
I've noticed that something similar has been done for other coverage-related fixtures as well.

In the meantime, I added more uncovered lines to create a repro for #51299.

pmarchini commented Aug 23, 2024
View reviewed changes
Comment thread test/fixtures/test-runner/output/coverage-width-100-uncovered-lines.snapshot
# --------------------------------------------------------------------------------------------------
# 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

pmarchini Aug 23, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@cjihrig this is the same problem reported in the other ticket

avivkeller added the coverage Issues and PRs related to native coverage support. label Aug 23, 2024
cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2024

nodejs-github-bot commented Aug 23, 2024

Copy link
Copy Markdown
Collaborator

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

pmarchini commented Aug 23, 2024

Copy link
Copy Markdown
Member Author

CI (node-test-linux-ubuntu2204-64) is failing due to a flaky test reported in issue 52550.

nodejs-github-bot commented Aug 23, 2024

Copy link
Copy Markdown
Collaborator

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

cjihrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Aug 24, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 24, 2024
nodejs-github-bot merged commit bec3425 into nodejs:main Aug 24, 2024

nodejs-github-bot commented Aug 24, 2024

Copy link
Copy Markdown
Collaborator

Landed in bec3425

RafaelGSS pushed a commit that referenced this pull request Aug 25, 2024
test: add tests for runner coverage with different stdout column widths
a0f44a8 
PR-URL: #54494
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS mentioned this pull request Aug 25, 2024
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
test: add tests for runner coverage with different stdout column widths
8a4f8a9 
PR-URL: #54494
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. coverage Issues and PRs related to native coverage support. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL