Skip to content
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

test_runner: allow special characters in snapshot keys #57017

Merged
merged 3 commits into from
Feb 19, 2025
Merged

test_runner: allow special characters in snapshot keys #57017

merged 3 commits into from
Feb 19, 2025
+53 −1

Conversation

Ceres6
Copy link
Contributor

@Ceres6 Ceres6 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

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 Feb 12, 2025
@Ceres6
Copy link
Contributor Author

Ceres6 commented Feb 12, 2025

cc @cjihrig @pmarchini @melusc

@codecov Codecov
Copy link

codecov bot 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 lines Coverage Δ
lib/internal/test_runner/snapshot.js 98.08% <100.00%> (+0.04%) ⬆️

... and 72 files with indirect coverage changes

@pmarchini pmarchini requested a review from cjihrig February 12, 2025 21:54
cjihrig
cjihrig reviewed Feb 13, 2025
View reviewed changes
lib/internal/test_runner/snapshot.js Outdated
@@ -77,7 +77,7 @@ class SnapshotFile {
}

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

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
Contributor Author

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

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
Contributor Author

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
Contributor Author

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:

@Ceres6 Ceres6 force-pushed the feat/test-snapshot-special-characters branch from b91c14b to 06b3b94 Compare February 17, 2025 18:00
@Ceres6 Ceres6 force-pushed the feat/test-snapshot-special-characters branch from db614e3 to 02bbd1d Compare February 18, 2025 09:18
pmarchini
pmarchini approved these changes Feb 18, 2025
View reviewed changes
Copy link
Member

@pmarchini pmarchini left a comment

Choose a reason for hiding this comment

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

LGTM

@pmarchini pmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2025
@github-actions github-actions bot 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
@aduh95 aduh95 changed the title test: allow especial characters in snapshot keys test_runner: allow special characters in snapshot keys Feb 18, 2025
@nodejs-github-bot
Copy link
Collaborator

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

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

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 <pietro.marchini94@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
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 <pietro.marchini94@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@github-actions github-actions bot 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 <pietro.marchini94@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@pmarchini pmarchini pmarchini approved these changes

@atlowChemi atlowChemi atlowChemi approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. 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.

Test runner cannot find snapshot with \r in title
7 participants
@Ceres6 @nodejs-github-bot @targos @aduh95 @cjihrig @pmarchini @atlowChemi