Skip to content

test_runner: allow special characters in snapshot keys#57017

Merged
nodejs-github-bot merged 3 commits intonodejs:mainfrom
Ceres6:feat/test-snapshot-special-characters
Feb 19, 2025
Merged

test_runner: allow special characters in snapshot keys#57017
nodejs-github-bot merged 3 commits intonodejs:mainfrom
Ceres6:feat/test-snapshot-special-characters

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

nodejs-github-bot commented Feb 12, 2025

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

@cjihrig cjihrig Feb 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
Contributor Author

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

@cjihrig cjihrig Feb 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
Contributor Author

@Ceres6 Ceres6 Feb 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
Contributor Author

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

@Ceres6 Ceres6 force-pushed the feat/test-snapshot-special-characters branch from b91c14b to 06b3b94 Compare February 17, 2025 18:00
pmarchini
pmarchini reviewed Feb 17, 2025
View reviewed changes
@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

nodejs-github-bot commented Feb 18, 2025

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

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 <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>
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 <pietro.marchini94@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
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 <pietro.marchini94@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@aduh95 aduh95 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 <pietro.marchini94@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
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 <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