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

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

Merged

Conversation

pmarchini
Copy link
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
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 pmarchini marked this pull request as ready for review August 22, 2024 10:04
@pmarchini pmarchini force-pushed the test/different-widths-runner-coverage branch from d8bb713 to 1cf8c89 Compare August 22, 2024 11:08
@pmarchini pmarchini marked this pull request as draft August 22, 2024 11:55
Copy link

codecov bot commented Aug 22, 2024

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 pmarchini force-pushed the test/different-widths-runner-coverage branch from 1cf8c89 to e6a18f8 Compare August 22, 2024 13:50
@pmarchini pmarchini marked this pull request as ready for review August 22, 2024 13:58
@pmarchini pmarchini marked this pull request as draft August 22, 2024 13:58
@pmarchini pmarchini marked this pull request as ready for review August 22, 2024 15:50
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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
Copy link
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 pmarchini force-pushed the test/different-widths-runner-coverage branch from 3e546b4 to 3f5526e Compare August 23, 2024 11:25
@pmarchini
Copy link
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.

# --------------------------------------------------------------------------------------------------
# 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
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@OnlyAviv OnlyAviv added the coverage Issues and PRs related to native coverage support. label Aug 23, 2024
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2024
@github-actions 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
Copy link
Collaborator

@pmarchini
Copy link
Member Author

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

@nodejs-github-bot
Copy link
Collaborator

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, 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 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 nodejs-github-bot merged commit bec3425 into nodejs:main Aug 24, 2024
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in bec3425

RafaelGSS pushed a commit that referenced this pull request Aug 25, 2024
@RafaelGSS RafaelGSS mentioned this pull request Aug 25, 2024
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants