Skip to content

test_runner: allow special characters in snapshot keys#57017

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 3 commits into from
Feb 19, 2025
Merged

test_runner: allow special characters in snapshot keys #57017

nodejs-github-bot merged 3 commits into from
Feb 19, 2025

Conversation

@Ceres6
Copy link
Contributor

@Ceres6Ceres6 commented Feb 12, 2025

Use JSONStringify to serialise snapshot keys to allow special characters such as \r to be used in test names

Fixes: #56836

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 12, 2025

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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 12, 2025
@Ceres6
Copy link
ContributorAuthor

Ceres6 commented Feb 12, 2025

cc @cjihrig@pmarchini@melusc

@codecov
Copy link

codecovbot commented Feb 12, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.07%. Comparing base (1671921) to head (02bbd1d).
Report is 100 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57017 +/- ## ========================================== - Coverage 89.15% 89.07% -0.09%  ========================================== Files 665 665 Lines 192798 193312 +514 Branches 37130 37263 +133 ========================================== + Hits 171886 172187 +301 - Misses 13673 13829 +156 - Partials 7239 7296 +57
Files with missing linesCoverage Δ
lib/internal/test_runner/snapshot.js98.08% <100.00%> (+0.04%)⬆️

... and 72 files with indirect coverage changes

pmarchini
pmarchini reviewed Feb 12, 2025
View reviewed changes
cjihrig
cjihrig reviewed Feb 13, 2025
View reviewed changes
lib/internal/test_runner/snapshot.js Outdated

setSnapshot(id,value){
this.snapshots[templateEscape(id)]=value;
this.snapshots[keyEscape(id)]=value;
Copy link
Contributor

@cjihrigcjihrigFeb 13, 2025

Choose a reason for hiding this comment

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

Since this appears to duplicate much of templateEscape(), couldn't we do this instead: keyEscape(templateEscape(id))

Copy link
ContributorAuthor

@Ceres6Ceres6Feb 14, 2025

Choose a reason for hiding this comment

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

I tried it but then it starts failing as we over-escape things, the problem is with the \\ to \\\\ replace

test/parallel/test-runner-snapshot-tests.js
Comment on lines +161 to +162
file.setSnapshot('\r 1','test');
t.assert.strictEqual(file.getSnapshot('\\r 1'),'test');
Copy link
Contributor

@cjihrigcjihrigFeb 13, 2025

Choose a reason for hiding this comment

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

I think the bug happens while reading the snapshots back. I think for this case, it would be better to test a full round trip (write the file and then read it back) using the public APIs. It would probably be good to check a couple more cases besides just \r since in #56836 (comment), it was reported that this happens for a range of inputs (not saying we should test every single input though).

Copy link
ContributorAuthor

@Ceres6Ceres6Feb 14, 2025

Choose a reason for hiding this comment

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

The test was failing before the check, but I will add an e2e test

Copy link
ContributorAuthor

@Ceres6Ceres6Feb 14, 2025

Choose a reason for hiding this comment

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

AFAICS the range in that comment is the block of high surrogate and low surrogate pairs which are reserved for utf-16 and shouldn't appear, but I'm not an expert on this, so not sure

@targos
Copy link
Member

targos commented Feb 14, 2025

There's a typo in the commit message: especial -> special

@aduh95
Copy link
Contributor

aduh95 commented Feb 15, 2025

Also, the subsystem should be test_runner:, not test:

@Ceres6Ceres6force-pushed the feat/test-snapshot-special-characters branch from b91c14b to 06b3b94CompareFebruary 17, 2025 18:00
pmarchini
pmarchini reviewed Feb 17, 2025
View reviewed changes
@Ceres6Ceres6force-pushed the feat/test-snapshot-special-characters branch from db614e3 to 02bbd1dCompareFebruary 18, 2025 09:18
pmarchini
pmarchini approved these changes Feb 18, 2025
View reviewed changes
Copy link
Member

@pmarchinipmarchini left a comment

Choose a reason for hiding this comment

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

LGTM

@pmarchinipmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@aduh95aduh95 changed the title test: allow especial characters in snapshot keystest_runner: allow special characters in snapshot keysFeb 18, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 18, 2025

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

@pmarchinipmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2025
@atlowChemiatlowChemi added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 19, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 19, 2025
@nodejs-github-botnodejs-github-bot merged commit baa60ce into nodejs:mainFeb 19, 2025
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 19, 2025

Landed in baa60ce

acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
@Ceres6@acidiney
test_runner: allow special characters in snapshot keys
5a76af1
Fixes: nodejs#56836 PR-URL: nodejs#57017 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
targos pushed a commit that referenced this pull request Feb 24, 2025
@Ceres6@targos
test_runner: allow special characters in snapshot keys
a424c15
Fixes: #56836 PR-URL: #57017 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Feb 25, 2025
Merged
targos pushed a commit that referenced this pull request Feb 25, 2025
@Ceres6@targos
test_runner: allow special characters in snapshot keys
2929fc6
Fixes: #56836 PR-URL: #57017 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
aduh95 pushed a commit that referenced this pull request Apr 2, 2025
@Ceres6@aduh95
test_runner: allow special characters in snapshot keys
5726fcc
Fixes: #56836 PR-URL: #57017 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
aduh95 pushed a commit that referenced this pull request Apr 3, 2025
@Ceres6@aduh95
test_runner: allow special characters in snapshot keys
57a1c19
Fixes: #56836 PR-URL: #57017 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
@aduh95aduh95 mentioned this pull request Apr 11, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@Ceres6@RafaelGSS
test_runner: allow special characters in snapshot keys
5d740c8
Fixes: #56836 PR-URL: #57017 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@Ceres6@RafaelGSS
test_runner: allow special characters in snapshot keys
3ed3ba4
Fixes: #56836 PR-URL: #57017 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes

@pmarchinipmarchinipmarchini approved these changes

@atlowChemiatlowChemiatlowChemi approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test runner cannot find snapshot with \r in title

7 participants

@Ceres6@nodejs-github-bot@targos@aduh95@cjihrig@pmarchini@atlowChemi