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

Documentation for Interactive Snapshot Update #5291

Merged
merged 18 commits into from
Feb 10, 2018
Merged

Documentation for Interactive Snapshot Update #5291

merged 18 commits into from
Feb 10, 2018

Conversation

genintho
Copy link
Contributor

Update the documentation and the changelog for #3831 (review)

Fixes #5288

@genintho
Copy link
Contributor Author

I could not find in the docs a section where the watch mode is describe. Should it be created?

@codecov-io
Copy link

codecov-io commented Jan 12, 2018

Codecov Report

Merging #5291 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5291   +/-   ##
=======================================
  Coverage   61.32%   61.32%           
=======================================
  Files         205      205           
  Lines        6909     6909           
  Branches        3        3           
=======================================
  Hits         4237     4237           
  Misses       2671     2671           
  Partials        1        1

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 f040725...bedff40. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Jan 12, 2018

I could not find in the docs a section where the watch mode is describe. Should it be created?

I think a separate section for the watch mode would be awesome. Especially now that it's pluggable and people can create their own

@rickhanlonii
Copy link
Member

I think the changelog is unnecessary now @genintho since it has already landed in the current version

If creating the whole watch mode doc is too much to get this off the ground, you could consider adding interactive snapshot testing to the snapshots doc after the Updating Snapshots section

Something like this (which we can move to the Watch Mode page once it exists):

Interactive Updates

Snapshots can also be updated one at a time in the Jest watch mode:

[screenshot highlighting watch menu option "i"]

Once you enter the Interactive Snapshot Updates mode, Jest will walk you though the failed snapshots one at a time and give you an opportunity to review the failed output.

From here you can choose to update that snapshot or skip to the next:

[gif of updating and skipping]

@genintho
Copy link
Contributor Author

I think the changelog is unnecessary now @genintho since it has already landed in the current version

I disagree with this. Not everybody update their libraries constantly and having the ability to read all the things that change in an efficient format is really valuable.

@rickhanlonii
Copy link
Member

Sorry, I was wrong. I thought I saw this in the Changelog in the last update I made

We agree that it should be there, my mistake 😄

@genintho
Copy link
Contributor Author

Updating Snapshots section has been updated with the proposed change

screen shot 2018-01-18 at 23 44 02

I decided to use 2 screenshots instead of a gif but I have made
if we think it is better

@thymikee
Copy link
Collaborator

How about 1 screenshot and 1 gif? 😃 This is looking really nice!

@@ -121,6 +121,20 @@ You can try out this functionality by cloning the
[snapshot example](https://github.com/facebook/jest/tree/master/examples/snapshot),
modifying the `Link` component, and running Jest.

#### Interactive Updates

Snapshots can also be updated one at a time in the Jest watch mode:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not entirely true, as interaction with snapshots happens test file by test file, not test by test. Can you fix the description to be more accurate (including the paragraph below)?
We'd like to avoid issues about this feature not working as expected (but we'll get there eventually)

Copy link
Member

Choose a reason for hiding this comment

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

@thymikee good catch, this is my bad as it's in my copy above

Related to this, the skip option still says "Press s to skip the current snapshot"

@genintho
Copy link
Contributor Author

genintho commented Feb 3, 2018

Updated.
Please update or let me know what I should put for the description if it is not working out. I am not a native English speaker, so the grammar might be incorrect 😄

CHANGELOG.md Outdated
@@ -34,6 +34,8 @@
not generate additional (invalid) paths by prepending each ancestor of `cwd`
to the absolute path. Additionally, this fixes functionality in Windows OS.
([#5398](https://github.com/facebook/jest/pull/5398))
* `[doc]` Add information about Interactive Snapshot update
Copy link
Member

Choose a reason for hiding this comment

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

nit - this is [docs] everywhere else

![](/jest/img/content/interactiveSnapshot.png)

Once you enter the Interactive Snapshot Updates mode, Jest will walk you though
the failed snapshots of one test file at a time and give you an opportunity to
Copy link
Member

Choose a reason for hiding this comment

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

I think there's an extra "of" here - should be:

Jest will walk you through the failed snapshots one test file at a time...

Also though is a typo, should be through

@genintho
Copy link
Contributor Author

genintho commented Feb 3, 2018

updated again. Finger cross!

@genintho
Copy link
Contributor Author

genintho commented Feb 7, 2018

small bump 😄

@SimenB
Copy link
Member

SimenB commented Feb 7, 2018

@rickhanlonii mind making sure this lands?

@genintho could you try a rebase on master? If we're lucky it'll fix CI

@genintho
Copy link
Contributor Author

Rebased twice, and the tests are still broken with the same circle CI internal error.

Copy link
Member

@rickhanlonii rickhanlonii 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 made a few minor wording updates

@rickhanlonii rickhanlonii merged commit af19110 into jestjs:master Feb 10, 2018
@genintho genintho deleted the 3831-doc-interactive-mode branch June 2, 2018 03:42
@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 12, 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.

Docs + Changelog for #3831
6 participants