GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — test_runner: fix escaping in snapshot tests by JulianKniephoff · Pull Request #53833 · nodejs/node

Conversation

JulianKniephoff commented Jul 13, 2024

Copy link

Copy Markdown

Contributor

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 serialize escapes 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nodejs-github-bot commented Jul 13, 2024

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/test_runner

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Jul 13, 2024

avivkeller reviewed

Jul 13, 2024

View reviewed changes

Comment thread

lib/internal/test_runner/snapshot.js Outdated

avivkeller Jul 13, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

I believe this should be replaced with a primordial implementation, see https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig Jul 13, 2024

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JulianKniephoff Jul 13, 2024

Copy link

Copy Markdown

Contributor Author

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.

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]);
}

🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig Jul 13, 2024

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JulianKniephoff Jul 13, 2024

Copy link

Copy Markdown

Contributor Author

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.

Oh sorry, I overlooked that section. 😅 Alright, thanks, I will make the change, then! 😀

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig reviewed

Jul 13, 2024

View reviewed changes

cjihrig left a comment

Copy link

Copy Markdown

Contributor

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.

Thanks for this. I think it looks good. Just a couple minor comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/test_runner/snapshot.js Outdated

cjihrig Jul 13, 2024

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/fixtures/test-runner/snapshots/unit.js Outdated

cjihrig Jul 13, 2024

Copy link

Copy Markdown

Contributor

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.

Can you add some more escape characters that are also used by templateEscape().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JulianKniephoff reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

test/fixtures/test-runner/snapshots/unit.js Outdated

cjihrig Jul 13, 2024

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JulianKniephoff Jul 13, 2024

Copy link

Copy Markdown

Contributor Author

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.

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. 🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig Jul 13, 2024

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JulianKniephoff reacted with thumbs up emoji

All reactions

👍 1 reaction

JulianKniephoff force-pushed the fix-escapes-in-snapshots branch from f22be60 to e708766 Compare

July 13, 2024 16:41

cjihrig approved these changes

Jul 13, 2024

View reviewed changes

anonrig approved these changes

Jul 13, 2024

View reviewed changes


          test_runner: fix escaping in snapshot tests

404bbc1

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

July 13, 2024 17:12

JulianKniephoff commented Jul 14, 2024

Copy link

Copy Markdown

Contributor Author

I fixed the missing semicolon (sorry about that 🙈), but I have no idea what to do about the failing test-linux job. 🤔

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Jul 14, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Jul 14, 2024

nodejs-github-bot commented Jul 14, 2024

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60303/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jul 14, 2024 •

edited by cjihrig

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60308/ 🟡

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow approved these changes

Jul 14, 2024

View reviewed changes

cjihrig added author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed needs-ci PRs that need a full CI run. labels

Jul 14, 2024

jasnell approved these changes

Jul 15, 2024

View reviewed changes

MoLow added the commit-queue PRs queued for automated landing through the Commit Queue. label

Jul 15, 2024

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Jul 15, 2024

nodejs-github-bot merged commit 4b4a931 into nodejs:main

Jul 15, 2024

nodejs-github-bot commented Jul 15, 2024

Copy link

Copy Markdown

Collaborator

Landed in 4b4a931

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig mentioned this pull request

Jul 15, 2024

test_runner: refactor snapshots to support multiple files in the same process #53853

Closed

aduh95 pushed a commit that referenced this pull request

Jul 16, 2024


          test_runner: fix escaping in snapshot tests

c590828

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]>

aduh95 mentioned this pull request

Jul 16, 2024

V22.5.0 proposal #53826

Merged

ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request

Jul 18, 2024


          test_runner: fix escaping in snapshot tests

d1f038f

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 label

Sep 21, 2024

Reviewers

avivkeller

avivkeller left review comments

jasnell

jasnell approved these changes

anonrig

anonrig approved these changes

cjihrig

cjihrig approved these changes

MoLow

MoLow approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

test_runner

Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

JulianKniephoff commented Jul 13, 2024

Uh oh!

nodejs-github-bot commented Jul 13, 2024

Uh oh!

avivkeller Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulianKniephoff commented Jul 14, 2024

Uh oh!

nodejs-github-bot commented Jul 14, 2024

Uh oh!

nodejs-github-bot commented Jul 14, 2024 • edited by cjihrig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 15, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

avivkeller Jul 13, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Jul 14, 2024 •

edited by cjihrig

Loading

Uh oh!

There was an error while loading. Please reload this page.