-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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: migrate tests to use node:test module for better test structure for report worker #56038
base: main
Are you sure you want to change the base?
test: migrate tests to use node:test module for better test structure for report worker #56038
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
test/report/test-report-worker.js
Outdated
test('Worker threads report basic information', async (t) => { | ||
await t.test('should include basic information about Worker threads', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed what you said, thank you, the test is more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope I understood correctly, when I used --test-concurrency I saw that the tests were successfully synchronized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Refactored report-worker tests to use the node:test module for better structure and consistency.
this is part of a long pull request: #56024