Skip to content

test: unify assertSnapshot stacktrace transform#61665

Open
legendecas wants to merge 1 commit intonodejs:mainfrom
legendecas:test-assert-snapshot-stack
Open

test: unify assertSnapshot stacktrace transform#61665
legendecas wants to merge 1 commit intonodejs:mainfrom
legendecas:test-assert-snapshot-stack

Conversation

@legendecas
Copy link
Member

The snapshotted stack frames should hide node internal stack frames so
that general node core development does not need updating the snapshot.

Internal stack frames are replaced with at <node-internal-frames>.

For userland stack frames, they are highly fixture related and any
fixture change should reflect in a change of the snapshot. Additionally,
the line and column number are highly relevant to the correctness of the
snapshot, these should not be redacted. A change in node core that
affects userland stack frames should be alarming and be reflected in the
snapshots.

Features like test runner and source map support both should snapshot
userland stack frames to ensure that userland code locations are
computed correctly.

@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 Feb 4, 2026
@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.76%. Comparing base (b864049) to head (9903cf5).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61665      +/-   ##
==========================================
+ Coverage   89.74%   89.76%   +0.01%     
==========================================
  Files         675      675              
  Lines      204601   204601              
  Branches    39325    39321       -4     
==========================================
+ Hits       183616   183653      +37     
+ Misses      13273    13227      -46     
- Partials     7712     7721       +9     

see 28 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.

@legendecas legendecas force-pushed the test-assert-snapshot-stack branch from d0d45b2 to 9f7ada0 Compare February 4, 2026 14:05
);

// eslint-disable-next-line no-control-regex
child.stdout.on('data', (d) => process.stdout.write(d.toString().replace(/[^\x00-\x7F]/g, '').replace(/\u001b\[\d+m/g, '')));
Copy link
Member Author

Choose a reason for hiding this comment

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

This was basically skipping the test of unusual chars in CWD on the CI.

@legendecas legendecas force-pushed the test-assert-snapshot-stack branch from 5c4203b to d72d40b Compare February 4, 2026 15:13
@legendecas
Copy link
Member Author

@nodejs/test_runner would you mind taking a look at this? Thank you!

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

Amazing work. I wonder if we should expose some of this on node:util

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 9, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 9, 2026
@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas force-pushed the test-assert-snapshot-stack branch 3 times, most recently from 4b94f49 to 9903cf5 Compare February 9, 2026 15:55
@legendecas
Copy link
Member Author

Rebased to address conflicts.

@legendecas legendecas force-pushed the test-assert-snapshot-stack branch from 9903cf5 to 8393c87 Compare February 9, 2026 20:52
@nodejs-github-bot
Copy link
Collaborator

The snapshotted stack frames should hide node internal stack frames so
that general node core development does not need updating the snapshot.

For userland stack frames, they are highly fixture related and any
fixture change should reflect in a change of the snapshot. Additionally,
the line and column number are highly relevant to the correctness of the
snapshot, these should not be redacted. A change in node core that
affects userland stack frames should be alarming and be reflected in the
snapshots.

Features like test runner and source map support both should snapshot
userland stack frames to ensure that userland code locations are
computed correctly.
@legendecas legendecas force-pushed the test-assert-snapshot-stack branch from 8393c87 to c2dc969 Compare February 9, 2026 21:48
@nodejs-github-bot
Copy link
Collaborator

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 Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants