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

Output created snapshot when using --ci option #3693

Merged
merged 1 commit into from
Aug 24, 2017
Merged

Output created snapshot when using --ci option #3693

merged 1 commit into from
Aug 24, 2017

Conversation

lstkz
Copy link
Contributor

@lstkz lstkz commented May 30, 2017

Summary

I need to review created snapshots. By default, it just creates a new snapshot without any information and to do the review I must find this snapshot manually in test.snap.
When I use the --ci flag, I can review the snapshot, and when it looks ok, I can just press u.

Test plan
Added "Received value" + snapshot content.
image

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@codecov-io
Copy link

Codecov Report

Merging #3693 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3693      +/-   ##
==========================================
- Coverage   62.72%   62.65%   -0.07%     
==========================================
  Files         183      183              
  Lines        6701     6708       +7     
  Branches        6        6              
==========================================
  Hits         4203     4203              
- Misses       2495     2502       +7     
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-snapshot/src/index.js 50% <ø> (ø) ⬆️
...s/jest-editor-support/src/parsers/BabylonParser.js 98.64% <0%> (ø) ⬆️
packages/jest-environment-node/src/index.js 68.18% <0%> (ø) ⬆️
packages/jest-util/src/FakeTimers.js 91% <0%> (ø) ⬆️
packages/jest-cli/src/runJest.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c91bd5...5fa8dd4. Read the comment docs.

@excitement-engineer
Copy link
Contributor

Hey @lsentkiewicz! I like your idea, this is something that I am also running into: Jest creates snapshots automatically which forces me to go to the corresponding .snap file in order to verify if the snapshot is correct. You need to be very diligent not to make mistakes if a single test run generates multiple snapshots over multiple test files.

Showing the actual snapshot in the terminal is a great improvement in my opinion! I am wondering if it would actually improve the quality of snapshot reviews if Jest didn't write snapshots to a .snap file by default (when --ci is not set). This way the user will first see the snapshot in the terminal and then decide using the update flag whether it should be written to a file or not. Perhaps this will stimulate more thorough reviews of the snapshots. What do you think?

@lstkz
Copy link
Contributor Author

lstkz commented May 30, 2017

Yes, that would be very good. Also, if there are many snapshots, it would be good to confirm it one by one. Currently, pressing u updates everything.
Is it possible to create an interactive mode? When Jest finds a new snapshot, it asks the user to update it or not. It can be combined with the watch mode.

@excitement-engineer
Copy link
Contributor

Yes this would definitely be a very useful thing to have. It would be similar to how you can press p in watch mode to filter by a filename regex pattern and the choose the specific file(s) to run using a typeahead. The watch mode could be used to selectively remove obsolete snapshots, update snapshots and possibly write snapshots to file using the same typeahead behaviour. Perhaps this would be useful for people to have:)

@excitement-engineer
Copy link
Contributor

Note however that this would entail expanding the TestResult type to contain the name of the snapshot so that we can actually display these names in the watch mode. This is explained in more depth in PR #3660.

@lstkz
Copy link
Contributor Author

lstkz commented Aug 17, 2017

Hi @excitement-engineer

Any update here? I use --ci option all the time, so I can see the snapshot result before creating it.
It would be great if we can merge it.

@cpojer cpojer merged commit 93aaf54 into jestjs:master Aug 24, 2017
@cpojer
Copy link
Member

cpojer commented Aug 24, 2017

I'm fine with this, and if this workflow works for you, then we should support it. We will have a snapshot acceptance mode at some point, but we aren't there yet :)

@github-actions
Copy link

This pull request 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 this pull request may close these issues.

5 participants