Skip to content

Comments

test: make snapshot path matching CWD-agnostic#61846

Open
drtootsie wants to merge 1 commit intonodejs:mainfrom
drtootsie:fix-cwd-agnostic-snapshot-tests
Open

test: make snapshot path matching CWD-agnostic#61846
drtootsie wants to merge 1 commit intonodejs:mainfrom
drtootsie:fix-cwd-agnostic-snapshot-tests

Conversation

@drtootsie
Copy link

Fixes: #61303

Summary

The snapshot path transformation was incorrectly matching partial paths like /node within /node_modules or nodejs.org, causing false replacements.

Changes

Added negative lookahead (?![\\w/]) to the regex patterns in test/common/assertSnapshot.js to ensure only complete path segments are matched.

This prevents partial matches in:

  • Directory names (e.g., /node_modules)
  • URLs (e.g., nodejs.org)
  • Other paths containing the CWD as a substring

Testing

Requires building Node.js and running the test suite:

./configure && make -j4
make test-only

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 15, 2026
if (range.count !== 0 ||
range.ignoredLines === range.lines.length) {
branchesCovered++;
// Skip branches that are entirely on ignored lines
Copy link
Member

Choose a reason for hiding this comment

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

related?

Copy link
Contributor

Choose a reason for hiding this comment

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

No that's from #61845

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 16, 2026
@codecov
Copy link

codecov bot commented Feb 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.73%. Comparing base (9cc7fcc) to head (f95913e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #61846   +/-   ##
=======================================
  Coverage   89.72%   89.73%           
=======================================
  Files         675      675           
  Lines      204797   204799    +2     
  Branches    39344    39350    +6     
=======================================
+ Hits       183752   183774   +22     
+ Misses      13324    13297   -27     
- Partials     7721     7728    +7     
Files with missing lines Coverage Δ
lib/internal/test_runner/coverage.js 73.00% <100.00%> (+0.07%) ⬆️

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

@aduh95 aduh95 removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 16, 2026
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Can you separate out the unrelated changes please.

Fixes: nodejs#61303

The snapshot path transformation was incorrectly matching partial
paths like '/node' within '/node_modules' or 'nodejs.org', causing
false replacements.

Added negative lookahead (?![\w/]) to the regex patterns to ensure
only complete path segments are matched, preventing partial matches
in directory names and URLs.
@drtootsie drtootsie force-pushed the fix-cwd-agnostic-snapshot-tests branch from f95913e to 975d553 Compare February 22, 2026 05:48
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. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some tests are not fully CWD-agnostic

7 participants