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.

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

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

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 3, 2025

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

codecov bot commented Jan 3, 2025

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
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

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
Copy link
Collaborator

nodejs-github-bot commented Jan 4, 2025

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 4, 2025

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.

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

cjihrig commented Jan 9, 2025

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@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
73 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 19c8cc1

@cjihrig cjihrig deleted the file-snapshots branch January 9, 2025 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants