test_runner: fix escaping in snapshot tests #53833
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
lib/internal/test_runner/snapshot.js
Outdated
| @@ -147,6 +147,9 @@ class SnapshotManager { | |||
| } | |||
|
|
|||
| this.snapshots = context.exports; | |||
| for (const [key, value] of Object.entries(this.snapshots)) { | |||
There was a problem hiding this comment.
I believe this should be replaced with a primordial implementation, see https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md ?
There was a problem hiding this comment.
This will look something like this:
// Add to the imports at the top of the file.
const {
ObjectEntries,
} = primordials;
const entries = ObjectEntries(context.exports);
for (let i = 0; i < entries.length; i++) {
const { 0: key, 1: value } = entries[i];
this.snapshots[key] = templateEscape(value);
}(Let's also update to operator on context.exports before assigning to this.snapshots.
There was a problem hiding this comment.
Out of curiosity: Can you explain why this shouldn't use array destructuring and for-of?
Also, what about something like this, which looks a bit more natural to me, and also avoids these, and even the need for the primordial:
for (const key in context.exports) {
this.snapshots[key] = templateEscape(context.exports[key]);
}🤔
There was a problem hiding this comment.
I believe for-in loops are allowed, so that should be fine.
Can you explain why this shouldn't use array destructuring and for-of?
The idea is that user code can change the way those things work and potentially break the test runner. There is an explanation of both of those in https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md#implicit-use-of-user-mutable-methods.
There was a problem hiding this comment.
Oh sorry, I overlooked that section. 😅 Alright, thanks, I will make the change, then! 😀
lib/internal/test_runner/snapshot.js
Outdated
| @@ -147,6 +147,9 @@ class SnapshotManager { | |||
| } | |||
|
|
|||
| this.snapshots = context.exports; | |||
| for (const [key, value] of Object.entries(this.snapshots)) { | |||
There was a problem hiding this comment.
This will look something like this:
// Add to the imports at the top of the file.
const {
ObjectEntries,
} = primordials;
const entries = ObjectEntries(context.exports);
for (let i = 0; i < entries.length; i++) {
const { 0: key, 1: value } = entries[i];
this.snapshots[key] = templateEscape(value);
}(Let's also update to operator on context.exports before assigning to this.snapshots.
| @@ -22,3 +22,7 @@ test('`${foo}`', async (t) => { | |||
| const options = { serializers: [() => { return '***'; }]}; | |||
| t.assert.snapshot('snapshotted string', options); | |||
| }); | |||
|
|
|||
| test('escapes', async (t) => { | |||
There was a problem hiding this comment.
Can you add some more escape characters that are also used by templateEscape().
| @@ -22,3 +22,7 @@ test('`${foo}`', async (t) => { | |||
| const options = { serializers: [() => { return '***'; }]}; | |||
| t.assert.snapshot('snapshotted string', options); | |||
| }); | |||
|
|
|||
| test('escapes', async (t) => { | |||
There was a problem hiding this comment.
It would be good to add a test where the test name includes the problematic escape characters since that is serialized as the snapshot ID.
There was a problem hiding this comment.
The test before mine does that, doesn't it? I could combine these, of course, but that one sets a custom serializer, so I didn't want to tamper with it. 🤔
There was a problem hiding this comment.
Yes, please don't change the existing tests. It would be good to have coverage without custom serializers, and they are pretty cheap to add.
JulianKniephoff
force-pushed
the
fix-escapes-in-snapshots
branch
from
f22be60
to
e708766
Compare
Snapshotted values are escaped after serialization. This happens when serializing a value for comparison when snapshots already exist, and also when updating them. That is, snapshots are escaped in the internal storage, but when written to disk, one "level" of escaping is removed. That escaping is never added back when reading the snapshots back. This makes even the simplest test trying to serialize a string with an escape code in it fail, like the one I added here.
JulianKniephoff
force-pushed
the
fix-escapes-in-snapshots
branch
from
e708766
to
404bbc1
Compare
|
I fixed the missing semicolon (sorry about that 🙈), but I have no idea what to do about the failing |
github-actions
bot
removed
the
request-ci
cjihrig
added
author ready
MoLow
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 4b4a931 |
Snapshotted values are escaped after serialization. This happens when serializing a value for comparison when snapshots already exist, and also when updating them. That is, snapshots are escaped in the internal storage, but when written to disk, one "level" of escaping is removed. That escaping is never added back when reading the snapshots back. This makes even the simplest test trying to serialize a string with an escape code in it fail, like the one I added here. PR-URL: #53833 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
Snapshotted values are escaped after serialization. This happens when serializing a value for comparison when snapshots already exist, and also when updating them. That is, snapshots are escaped in the internal storage, but when written to disk, one "level" of escaping is removed. That escaping is never added back when reading the snapshots back. This makes even the simplest test trying to serialize a string with an escape code in it fail, like the one I added here. PR-URL: #53833 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
Snapshotted values are escaped after serialization. This happens when serializing a value for comparison when snapshots already exist, and also when updating them. That is, snapshots are escaped in the internal storage, but when written to disk, one "level" of escaping is removed. That escaping is never added back when reading the snapshots back. This makes even the simplest test trying to serialize a string with an escape code in it fail, like the one I added here. PR-URL: nodejs#53833 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos
added
the
dont-land-on-v20.x
Snapshotted values are escaped after serialization. This happens when serializing a value for comparison when snapshots already exist, and also when updating them. That is, snapshots are escaped in the internal storage, but when written to disk, one "level" of escaping is removed. That escaping is never added back when reading the snapshots back. This makes even the simplest test trying to serialize a string with an escape code in it fail, like the one I added here.
Honestly I'm not quite sure about the approach here: An alternative would be to not escape snapshots internally, and only do so when writing them to disk/interpolating them into the appropriate JS-code. I haven't fully thought this through, though, and there is also a test explicitly ensuring that
serializeescapes values, so there might be a good reason for it.Another thing I'm not sure about is whether the test I added is sufficient, because it feels kind of indirect. Let me know if you want to see that changed.