Skip to content

test: make snapshot path matching CWD-agnostic#61846

Open
drtootsie wants to merge 2 commits intonodejs:mainfrom
drtootsie:fix-cwd-agnostic-snapshot-tests
Open

test: make snapshot path matching CWD-agnostic#61846
drtootsie wants to merge 2 commits 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.65%. Comparing base (5babc8d) to head (381c9a7).
⚠️ Report is 49 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61846      +/-   ##
==========================================
- Coverage   89.78%   89.65%   -0.13%     
==========================================
  Files         674      676       +2     
  Lines      204957   206240    +1283     
  Branches    39391    39506     +115     
==========================================
+ Hits       184011   184902     +891     
- Misses      13214    13466     +252     
- Partials     7732     7872     +140     

see 150 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