GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: allow special characters in snapshot keys by Ceres6 · Pull Request #57017 · nodejs/node

Conversation

@Ceres6

@Ceres6 Ceres6 commented Feb 12, 2025

Copy link
Copy Markdown
Contributor

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

Fixes: #56836

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Feb 12, 2025

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

Ceres6 commented Feb 12, 2025

Copy link
Copy Markdown
Contributor Author

cc @cjihrig @pmarchini @melusc
All reactions

Sorry, something went wrong.

Uh oh!

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

codecov Bot commented Feb 12, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pmarchini
pmarchini requested a review from cjihrig February 12, 2025 21:54
pmarchini
pmarchini reviewed Feb 12, 2025
View reviewed changes
Comment thread lib/internal/test_runner/snapshot.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig reviewed Feb 13, 2025
View reviewed changes
Comment thread lib/internal/test_runner/snapshot.js Outdated

@cjihrig cjihrig Feb 13, 2025

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.

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

Sorry, something went wrong.

Uh oh!

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

@Ceres6 Ceres6 Feb 14, 2025

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-runner-snapshot-tests.js Outdated
Comment on lines 161 to 162

@cjihrig cjihrig Feb 13, 2025

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pmarchini reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Ceres6 Ceres6 Feb 14, 2025

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 was failing before the check, but I will add an e2e test

Sorry, something went wrong.

Uh oh!

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

@Ceres6 Ceres6 Feb 14, 2025

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@targos

targos commented Feb 14, 2025

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

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

aduh95 commented Feb 15, 2025

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Ceres6
test_runner: allow special characters in snapshot keys
2945e44 
Fixes: nodejs#56836
@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
Comment thread test/fixtures/test-runner/snapshots/special-character.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-runner-snapshot-tests.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Ceres6 added 2 commits February 18, 2025 10:16
@Ceres6
fixup! test_runner: allow special characters in snapshot keys
3ec6510
@Ceres6
fixup! test_runner: allow special characters in snapshot keys
02bbd1d
@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

@pmarchini pmarchini left a comment

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pmarchini pmarchini 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 Feb 18, 2025
@github-actions 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 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
cjihrig
cjihrig approved these changes Feb 18, 2025
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Feb 18, 2025

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pmarchini pmarchini added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 18, 2025
atlowChemi
atlowChemi approved these changes Feb 19, 2025
View reviewed changes
@atlowChemi atlowChemi added the commit-queue PRs queued for automated landing through the Commit Queue. label Feb 19, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Feb 19, 2025
@nodejs-github-bot
nodejs-github-bot merged commit baa60ce into nodejs:main Feb 19, 2025
@nodejs-github-bot

nodejs-github-bot commented Feb 19, 2025

Copy link
Copy Markdown
Collaborator

Landed in baa60ce
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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-actions github-actions Bot mentioned this pull request Feb 25, 2025
2025-02-26, Version 23.9.0 (Current) #57207
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]>
@aduh95 aduh95 mentioned this pull request Apr 11, 2025
2025-04-23, Version 22.15.0 'Jod' (LTS), @UlisesGascon prepared by @RafaelGSS #57840
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 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

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

Uh oh!

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

7 participants

@Ceres6 @nodejs-github-bot @targos @aduh95 @cjihrig @pmarchini @atlowChemi
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.