Skip to content

src: improve error handling in cares_wrap - #57937

Closed
jasnell wants to merge 2 commits into
nodejs:mainfrom
jasnell:jasnell/cares_wrap_errorhandling
Closed

src: improve error handling in cares_wrap#57937
jasnell wants to merge 2 commits into
nodejs:mainfrom
jasnell:jasnell/cares_wrap_errorhandling

Conversation

@jasnell

@jasnelljasnell commented Apr 19, 2025

Copy link
Copy Markdown
Member

Improve error handling in cares_wrap by replacing ToLocalChecked() and Check() calls.
Reduce some code duplication using macros

@jasnell
jasnell requested review from aduh95 and anonrigApril 19, 2025 22:16
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. labels Apr 19, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 19, 2025

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Apr 19, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 33.94309% with 325 lines in your changes missing coverage. Please review.

Project coverage is 90.19%. Comparing base (25842c5) to head (d1771d9).
Report is 41 commits behind head on main.

Files with missing linesPatch %Lines
src/cares_wrap.cc34.08%246 Missing and 77 partials ⚠️
src/cares_wrap.h0.00%1 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #57937 +/- ##
==========================================
- Coverage 92.26% 90.19% -2.07% 
==========================================
Files 325 630 +305 Lines 126673 186174 +59501 Branches 20783 36562 +15779 ==========================================
+ Hits 116869 167923 +51054 - Misses 9576 11066 +1490 - Partials 228 7185 +6957 
Files with missing linesCoverage Δ
src/cares_wrap.h78.97% <0.00%> (ø)
src/cares_wrap.cc54.38% <34.08%> (ø)

... and 410 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jasnell added a commit that referenced this pull request Apr 27, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
jasnell added a commit that referenced this pull request Apr 27, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@jasnell

Copy link
Copy Markdown
MemberAuthor

Landed in e716081...49bb0ae

@jasnelljasnell closed this Apr 27, 2025
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 19, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 19, 2025
PR-URL: #57937
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@ghostghost mentioned this pull request Jun 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.caresIssues and PRs related to the c-ares dependency or the cares_wrap binding.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@jasnell@nodejs-github-bot@anonrig