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: improve debugger custom test #55767

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

Conversation

mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Nov 7, 2024

I tried to improve this test using node:test

@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 Nov 7, 2024
@mertcanaltin mertcanaltin changed the title Mert/improve debugger custom test test: improve debugger custom test Nov 7, 2024
@mertcanaltin mertcanaltin force-pushed the mert/improve-debugger-custom-test branch 2 times, most recently from f939522 to 1d905c4 Compare November 7, 2024 16:26
@mertcanaltin mertcanaltin force-pushed the mert/improve-debugger-custom-test branch from 1d905c4 to fbcd2ad Compare November 7, 2024 16:30
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (7788999) to head (fbcd2ad).
Report is 295 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55767      +/-   ##
==========================================
- Coverage   88.41%   88.40%   -0.01%     
==========================================
  Files         654      654              
  Lines      187752   187752              
  Branches    36125    36127       +2     
==========================================
- Hits       165993   165983      -10     
- Misses      15000    15007       +7     
- Partials     6759     6762       +3     

see 34 files with indirect coverage changes

@mertcanaltin mertcanaltin added the review wanted PRs that need reviews. label Nov 9, 2024
@mertcanaltin mertcanaltin added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels Dec 23, 2024
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

I only see a refactor to use node:test for no good reason. There is only a simple test, no subtests. This is not an improvement in my opinion.

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Dec 24, 2024

I only see a refactor to use node:test for no good reason. There is only a simple test, no subtests. This is not an improvement in my opinion.

I agree with your comment that the change does not provide a significant contribution to the test file however, the motivation behind this change was to establish a standardized structure in the codebase and transition to the modern node:test module provided by Node.js

@lpinca
Copy link
Member

lpinca commented Dec 24, 2024

I know and there is no consensus yet (#54796, #56027, #55716).

@mertcanaltin
Copy link
Member Author

I know and there is no consensus yet (#54796, #56027, #55716).

thanks for sharing, can I mark this with blocker tag then ?

by the way, I tried to migrate some tests by grouping them

#56039
#56038
#56037
#56036
#56035
#56034
#56033
#56032
#56031
#56030
#56029

@lpinca
Copy link
Member

lpinca commented Dec 24, 2024

There is already a request for changes so it does not make much sense to add the "blocked" label. Feel free to add it to the other PRs.

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. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants