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

test_runner: omit excluded coverage directories on the C++ side by avivkeller · Pull Request #65371 · nodejs/node · GitHub

/ node Public

test_runner: omit excluded coverage directories on the C++ side - #65371

Open
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:omit-on-cpp
ConversationCommits (1)ChecksFiles changed
Open

test_runner: omit excluded coverage directories on the C++ side#65371
avivkeller wants to merge 1 commit into
nodejs:mainfrom
avivkeller:omit-on-cpp

Conversation

avivkeller commented Aug 18, 2026

Copy link
Copy Markdown
Member

By omitting the values we know will not be needed for test coverage from being serialized/deserialized, we can save time during coverage reporting.

We know that:

  • node:* is not included in coverage
  • /node_modules/ is not included coverage if coverageIncludeGlobs is not set

So, we omit on those cases.

nodejs-github-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/inspector
  • @nodejs/test_runner

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 18, 2026

avivkeller commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

We don't have benchmarks for this yet, but a local fixture took 0.93s (after) vs 1.16s (before). Not sure if the speedup is worth the "hackiness" of JSON manipulations in this PR.

(I'll fix the errors after a round of feedback)

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.99237% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (45d6d31) to head (db1d6bf).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector_profiler.cc 74.38% 13 Missing and 18 partials ⚠️
lib/internal/test_runner/coverage.js 30.00% 7 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #65371      +/-   ##
==========================================
- Coverage   90.13%   90.12%   -0.01%     
==========================================
  Files         752      752              
  Lines      251872   251981     +109     
  Branches    47359    47388      +29     
==========================================
+ Hits       227033   227108      +75     
- Misses      16167    16187      +20     
- Partials     8672     8686      +14
Files with missing lines Coverage Δ
src/inspector_profiler.h 85.71% <ø> (ø)
lib/internal/test_runner/coverage.js 59.22% <30.00%> (-0.35%) ⬇️
src/inspector_profiler.cc 82.29% <74.38%> (+0.34%) ⬆️

... and 27 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.

avivkeller requested a review from atlowChemi August 19, 2026 20:35
avivkeller mentioned this pull request Aug 20, 2026
Open
atlowChemi reviewed Aug 20, 2026
View reviewed changes

atlowChemi left a comment

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

On the JS side - LGTM.
I can't really say much about the src/ though 🙂

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

atlowChemi atlowChemi left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL