Skip to content

test_runner: add t.assert.fileSnapshot()#56459

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
cjihrig:file-snapshots
Jan 9, 2025
Merged

test_runner: add t.assert.fileSnapshot()#56459
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
cjihrig:file-snapshots

Conversation

@cjihrig

@cjihrig cjihrig commented Jan 3, 2025

Copy link
Copy Markdown
Contributor

This commit adds a t.assert.fileSnapshot() API to the test runner. This is similar to how snapshot tests work in core, as well as userland options such as toMatchFileSnapshot().

@nodejs-github-bot

nodejs-github-bot commented Jan 3, 2025

Copy link
Copy Markdown
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 Jan 3, 2025
@codecov

codecov Bot commented Jan 3, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 94.68085% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.17%. Comparing base (dc5d0f9) to head (e80cc64).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/snapshot.js 94.25% 5 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #56459   +/-   ##
=======================================
  Coverage   89.16%   89.17%           
=======================================
  Files         662      662           
  Lines      191668   191731   +63     
  Branches    36884    36894   +10     
=======================================
+ Hits       170908   170967   +59     
+ Misses      13621    13620    -1     
- Partials     7139     7144    +5
Files with missing lines Coverage Δ
lib/internal/test_runner/test.js 96.93% <100.00%> (+<0.01%) ⬆️
lib/internal/test_runner/snapshot.js 98.04% <94.25%> (-1.96%) ⬇️

... and 25 files with indirect coverage changes

@cjihrig cjihrig added semver-minor PRs that contain new features and should be released in the next minor version. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 3, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2025
@nodejs-github-bot

nodejs-github-bot commented Jan 3, 2025

Copy link
Copy Markdown
Collaborator

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

mcollina
mcollina approved these changes Jan 4, 2025
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

I think adding a base path that's going to be applied to all non-absolute paths would be cool. Something like require('node:test').setFileSnapshotDir('/my/path').

@nodejs-github-bot

nodejs-github-bot commented Jan 4, 2025 •
edited by cjihrig
Loading

Copy link
Copy Markdown
Collaborator

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

@cjihrig

cjihrig commented Jan 4, 2025

Copy link
Copy Markdown
Contributor Author

That would be easy to implement. My only concern would be that we already have snapshot. setResolveSnapshotPath() for customizing the location of regular snapshot files. I don't think that API would work well for this use case. That API is intended to map a test file to a path that will contain multiple snapshots. For this use case, we need something very different. I guess a separate API for that wouldn't be the end of the world though.

@cjihrig
test_runner: add t.assert.fileSnapshot()
e80cc64 
This commit adds a t.assert.fileSnapshot() API to the test runner.
This is similar to how snapshot tests work in core, as well as
userland options such as toMatchFileSnapshot().
@cjihrig

cjihrig commented Jan 9, 2025

Copy link
Copy Markdown
Contributor Author

Rebased due to conflicts. Approval/re-approval requested.

@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2025
@nodejs-github-bot

nodejs-github-bot commented Jan 9, 2025

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jan 9, 2025

Copy link
Copy Markdown
Collaborator

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

@pmarchini pmarchini added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 9, 2025
@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 9, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 9, 2025
@nodejs-github-bot nodejs-github-bot merged commit 19c8cc1 into nodejs:main Jan 9, 2025
@nodejs-github-bot

nodejs-github-bot commented Jan 9, 2025

Copy link
Copy Markdown
Collaborator

Landed in 19c8cc1

@cjihrig cjihrig deleted the file-snapshots branch January 9, 2025 21:40
@github-actions github-actions Bot mentioned this pull request Jan 28, 2025
Merged
@github-actions github-actions Bot mentioned this pull request Feb 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@MoLow MoLow MoLow 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. semver-minor PRs that contain new features and should be released in the next minor version. 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.

7 participants

@cjihrig @nodejs-github-bot @mcollina @jasnell @MoLow @atlowChemi @pmarchini