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

[wip] test: Migrate tests to use node:test module for better test structure #56024

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mertcanaltin
Copy link
Member

hello I am converting old tests to node:test and I continue to work on it now fyi

@nodejs-github-bot 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
@anonrig
Copy link
Member

anonrig commented Nov 27, 2024

Please wait for #56027 before continuing on this work.

@anonrig
Copy link
Member

anonrig commented Nov 27, 2024

Also, a single pull-request with 1000 lines changes is extremely hard to review. I recommend splitting this, and justifying the reasoning behind this change.

@mertcanaltin
Copy link
Member Author

thank you very much for the suggestion, I will divide this pr as you said, I can even link it into a topic or tasks @anonrig

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.96%. Comparing base (9029aec) to head (c0d09d1).
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56024   +/-   ##
=======================================
  Coverage   87.95%   87.96%           
=======================================
  Files         656      656           
  Lines      188372   188372           
  Branches    35979    35975    -4     
=======================================
+ Hits       165687   165694    +7     
+ Misses      15851    15839   -12     
- Partials     6834     6839    +5     

see 26 files with indirect coverage changes

@anonrig
Copy link
Member

anonrig commented Nov 27, 2024

thank you very much for the suggestion, I will divide this pr as you said, I can even link it into a topic or tasks @anonrig

@mertcanaltin I've asked you to wait on progressing such changes due to the pending PR and discussion around refactoring node test usage. We are not in alignment to make these kind of changes, yet.

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Nov 27, 2024

thank you very much for the suggestion, I will divide this pr as you said, I can even link it into a topic or tasks @anonrig

@mertcanaltin I've asked you to wait on progressing such changes due to the pending PR and discussion around refactoring node test usage. We are not in alignment to make these kind of changes, yet.

I understand, I'm very sorry. Should I consider closing these pull requests? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

3 participants