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

CLI Mode where snapshots must be approved before they are saved #1798

Closed
suchipi opened this issue Sep 25, 2016 · 10 comments · Fixed by #3831
Closed

CLI Mode where snapshots must be approved before they are saved #1798

suchipi opened this issue Sep 25, 2016 · 10 comments · Fixed by #3831

Comments

@suchipi
Copy link
Contributor

suchipi commented Sep 25, 2016

Do you want to request a feature or report a bug?

Feature Request

What is the current behavior?

I sometimes like to write my tests before writing my implementation (TDD/BDD/etc). Occasionally while doing this, I need to assert some large output that I feel is best handled using a snapshot. However, I don't want to write the snapshot to disk until the code has been implemented, because I'll have to keep updating it. So I write this a lot:

expect(actual).toEqual("SNAPSHOT")

I use this as a placeholder that I expect to fail, and then once I'm ready to write the snapshot, I change it to the toMatchSnapshot matcher, then go review the snapshot (but if it was wrong, then I need to either keep updating it, or delete the snapshot).

It'd be nice if the watcher had a mode where it didn't write snapshots until I approved them, and if I didn't approve them by the time tests had to run again, it would move on without writing anything. As a bonus, I think requiring approval would help encourage me to verify my snapshot content rather than writing whatever the current behavior is and calling it good, at risk that it's not (which is very easy to do with snapshots right now).

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal repository on GitHub that we can npm install and npm test.

N/A

What is the expected behavior?

N/A

Run Jest again with --debug and provide the full configuration it prints. Please mention your node and npm version and operating system.

N/A

@withinboredom
Copy link

Maybe something like git add -up

@anilanar
Copy link
Contributor

anilanar commented Oct 2, 2016

Maybe it makes sense to require -u for snapshot creation.

When I first use toMatchSnapshot, I need to find the snapshot file and review it manually. Why not stop creating snapshot files unless -u flag is passed?

This would be a breaking change though.

@thymikee
Copy link
Collaborator

I think we could connect this with interactively updating the snapshots so it's more convenient.

@just-boris
Copy link
Contributor

just-boris commented Feb 7, 2017

Come up with the same idea. How about extra flag --updateSnapshotInteractive, where a user will be asked before updating every snapshot? Also, --updateSnapshot will effectively become something like --updateAllSnapshots.

How about actually start working on it? I can contribute, if I will get a starting point for it.

@cpojer
Copy link
Member

cpojer commented Feb 7, 2017

I think here is where I'd start:

  • Make this work only for watch mode (this is supposed to be interactive).
  • Record all the failures (Jest is already doing this).
  • When the tests are run, it will say "Snapshots failed, press u to update all or i to update them interactively"
  • When "i" is pressed, clear the screen and run each test one by one. When it fails, ask to press "a" to accept or "s" to skip.

I'd start looking at watch.js and getting the list of test results and then trying to apply that as "pattern" + testNamePattern (See #1860, which we may need for this as well).

@genintho
Copy link
Contributor

To fix this issue, I came up with this.

http://g.recordit.co/0vJJvBDofA.gif

http://g.recordit.co/0vJJvBDofA.gif

I tried to show each individual failed snapshot, but did not succeed yet.

I would love some feedback on the UX before submitting a PR.

@cpojer
Copy link
Member

cpojer commented Jun 11, 2017

That's awesome. Please submit a PR.

@genintho
Copy link
Contributor

I submitted a PR, but need some help on how to write tests for the "watch.js".

@AndrewSouthpaw
Copy link
Contributor

I was just thinking about this feature and wanting to implement it, thanks for making it happen @genintho!

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants