-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
test: unify assertSnapshot stacktrace transform #61665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61665 +/- ##
==========================================
+ Coverage 89.74% 89.75% +0.01%
==========================================
Files 675 675
Lines 204601 204642 +41
Branches 39325 39323 -2
==========================================
+ Hits 183616 183681 +65
+ Misses 13273 13234 -39
- Partials 7712 7727 +15 🚀 New features to boost your workflow:
|
d0d45b2 to
9f7ada0
Compare
| ); | ||
|
|
||
| // 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, ''))); |
There was a problem hiding this comment.
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.
5c4203b to
d72d40b
Compare
|
@nodejs/test_runner would you mind taking a look at this? Thank you! |
MoLow
left a comment
There was a problem hiding this 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
4b94f49 to
9903cf5
Compare
|
Rebased to address conflicts. |
9903cf5 to
8393c87
Compare
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.
8393c87 to
c2dc969
Compare
|
@MoLow @pmarchini CI is now green, would you mind taking another look? Thank you! |
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.