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

test: migrate tests to use node:test module for better test structure for report worker by mertcanaltin · Pull Request #56038 · nodejs/node · GitHub

/ node Public

test: migrate tests to use node:test module for better test structure for report worker - #56038

Closed
mertcanaltin wants to merge 3 commits into
nodejs:mainfrom
mertcanaltin:test/refactor-report-worker
ConversationCommits (3)ChecksFiles changed
Closed

test: migrate tests to use node:test module for better test structure for report worker#56038
mertcanaltin wants to merge 3 commits into
nodejs:mainfrom
mertcanaltin:test/refactor-report-worker

Conversation

mertcanaltin commented Nov 27, 2024

Copy link
Copy Markdown
Member

Refactored report-worker tests to use the node:test module for better structure and consistency.

this is part of a long pull request: #56024

nodejs-github-bot added needs-ci PRs that need a full CI run. report Issues and PRs related to process.report. test Issues and PRs related to the tests. labels Nov 27, 2024

codecov Bot commented Nov 27, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.99%. Comparing base (9029aec) to head (d634665).
Report is 680 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #56038      +/-   ##
==========================================
+ Coverage   87.95%   87.99%   +0.04%     
==========================================
  Files         656      656              
  Lines      188372   189103     +731     
  Branches    35979    36003      +24     
==========================================
+ Hits       165687   166409     +722     
- Misses      15851    15858       +7     
- Partials     6834     6836       +2

see 59 files with indirect coverage changes

pmarchini reviewed Dec 4, 2024
View reviewed changes
Comment thread test/report/test-report-worker.js Outdated
Comment on lines +11 to +12
test('Worker threads report basic information', async (t) => {
await t.test('should include basic information about Worker threads', async () => {

pmarchini Dec 4, 2024

Copy link
Copy Markdown
Member

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

nit: Could we also use describe/it to avoid using await t.test?

Not entirely sure about this: could we set concurrency: true, or is sequentiality needed?

mertcanaltin Dec 6, 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

I followed what you said, thank you, the test is more readable

mertcanaltin Dec 6, 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

I hope I understood correctly, when I used --test-concurrency I saw that the tests were successfully synchronized

mertcanaltin mentioned this pull request Dec 24, 2024
Closed
mertcanaltin added the blocked PRs that are blocked by other issues or PRs. label Dec 24, 2024

mertcanaltin commented Dec 24, 2024

Copy link
Copy Markdown
Member Author

#54796, #56027, #55716
I added a blocker tag because it is linked

lpinca requested changes Dec 28, 2024
View reviewed changes

lpinca left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

I'm blocking this because there is an approval now and as discussed in other places this is not an improvement in my opinion. I will remove the block once TSC officially decides that a mass refactor to node:test is good for the project.

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

lpinca lpinca requested changes
jasnell jasnell approved these changes
pmarchini pmarchini approved these changes

Assignees

No one assigned

Labels

blocked PRs that are blocked by other issues or PRs. needs-ci PRs that need a full CI run. report Issues and PRs related to process.report. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL