Interactive Snapshot Update mode #3831

genintho · 2017-06-15T04:56:00Z

This pull request is introducing a new option in jest watch mode allowing people to update the failed snapshot of one test suite at a time.

It seems possible to update only the snapshot of 1 test, but I want to make sure my approach is correct.

I am having trouble to finish the tests for this change, especially the modification of watch.js. I tried to inspired myself from the packages/jest-cli/src/__tests__/watch-filename-pattern-mode-test.js file, but I would appreciate some advice. The file watch-snapshot-interactive-test.js is left in a "work in progress state".

thymikee · 2017-06-19T21:05:39Z

This is great ❤️. Can't wait to lay my eyes on the implementation and post some feedback.

genintho · 2017-06-23T22:49:53Z

Can't wait for the feedback.

cpojer · 2017-06-29T09:19:03Z

packages/jest-cli/src/SnapshotInteractiveMode.js

@@ -0,0 +1,138 @@
+/**


This will need Facebook's copyright header. Can you copy it from other files?

thymikee · 2017-06-29T11:20:35Z

packages/jest-cli/src/watch.js

@@ -48,6 +50,11 @@ const watch = (
  const prompt = new Prompt();
  const testPathPatternPrompt = new TestPathPatternPrompt(pipe, prompt);
  const testNamePatternPrompt = new TestNamePatternPrompt(pipe, prompt);
+
+  const snapshotInteracticeMode = new SnapshotInteractiveMode(pipe);


typo: snapshotInteracticeMode -> snapshotInteractiveMode

thymikee · 2017-06-29T11:25:37Z

packages/jest-cli/src/watch.js

+          snapshotInteracticeMode.run(
+            failedSnapshotTestPaths,
+            (path: string, jestRunnerOptions: Object) => {
+              updateRunnerPatternMatching('watch', '', path, jestRunnerOptions);


jestRunnerOptions -> runnerOptions

thymikee · 2017-06-29T11:29:53Z

packages/jest-cli/src/SnapshotInteractiveMode.js

+    this._pipe.write(ansiEscapes.cursorSavePosition);
+    this._pipe.write(ansiEscapes.cursorTo(0, 0));
+
+    const title = rightPad(' -> Interactive Snapshot Update Activated <-');


Why rightPad? You can offset this from left by 4 spaces. Also I'd rename this to just:
Interactive Snapshot Update Mode

Also, this text appears in "random" position.

Can we put it on top? Or even resign, as we have information about Interactive Snapshot Progress below.

thymikee · 2017-06-29T11:31:06Z

packages/jest-cli/src/SnapshotInteractiveMode.js

+      '\n' + chalk.bold('Watch Usage'),
+      chalk.dim(' \u203A Press ') +
+        'u' +
+        chalk.dim(' to update failing snapshots.'),


I'd rather like something more descriptive:
Press u to update failing snapshots for this test.

thymikee · 2017-06-29T11:31:51Z

packages/jest-cli/src/SnapshotInteractiveMode.js

+            chalk.dim(' to skip the current snapshot..')
+        : '',
+      chalk.dim(' \u203A Press ') +
+        'q' +


Change to Esc, like we do in Pattern Mode

thymikee · 2017-06-29T11:34:32Z

packages/jest-cli/src/SnapshotInteractiveMode.js

+    const messages = [
+      '\n' + chalk.bold('Interactive Snapshot Progress'),
+      ' \u203A ' + stats,
+      '\n' + chalk.bold('Watch Usage'),


Change to Interactive Snapshot Mode Usage

thymikee · 2017-06-29T11:35:00Z

packages/jest-cli/src/SnapshotInteractiveMode.js

+      this._testFilePaths.length > 1
+        ? chalk.dim(' \u203A Press ') +
+            's' +
+            chalk.dim(' to skip the current snapshot..')


Remove the extra dot

thymikee · 2017-06-29T11:35:54Z

packages/jest-cli/src/SnapshotInteractiveMode.js

+        : '',
+      chalk.dim(' \u203A Press ') +
+        'q' +
+        chalk.dim(' to quit interactive snapshot mode.'),


to quit Interactive Snapshot Update Mode

thymikee · 2017-06-29T11:36:18Z

packages/jest-cli/src/SnapshotInteractiveMode.js

+      case KEYS.ESCAPE:
+        this.abort();
+        break;
+


Can we remove these newlines?

thymikee · 2017-06-29T11:36:29Z

packages/jest-cli/src/SnapshotInteractiveMode.js

+        this._run({});
+        break;
+      default:
+        console.log('got key event', key);


Remove this.

thymikee

Hey, this looks like a good start.
I've left some comments on how we can improve this.
I still need to think about if this approach is correct, but we'll probably need another watch refactor, as it's becoming a mess.

thymikee · 2017-06-29T11:50:55Z

packages/jest-cli/src/watch.js

@@ -126,10 +133,20 @@ const watch = (
      results => {
        isRunning = false;
        hasSnapshotFailure = !!results.snapshot.failure;
+        failedSnapshotTestPaths = getFailedSnapshotTests(results);


This function filters the test paths based on failed snapshot flag, that's why this tool now works on per file basis.
We could get each test using similar logic as coded in testNamePatternPrompt. You should be able to reuse it and feed this to snapshotInteractiveMode.

Conflicts: packages/jest-cli/src/watch.js

…acted

genintho · 2017-08-08T04:21:44Z

@thymikee
I am getting this error while testing my branch

TypeError: Watcher is not a constructor
    at createWatcher (/Users/xt-pro/Dev/jest/packages/jest-haste-map/build/index.js:597:23)
    at Array.map (native)
    at HasteMap._watch (/Users/xt-pro/Dev/jest/packages/jest-haste-map/build/index.js:744:25)
    at _buildPromise._buildFileMap.then.then.hasteMap (/Users/xt-pro/Dev/jest/packages/jest-haste-map/build/index.js:276:21)
    at <anonymous>

Do you know by any chance what would create it? Thanks

SimenB · 2017-10-30T09:28:30Z

I'd love to have it merged. CI is green, so it might be enough to just rebase it. I can have a look this evening

genintho · 2017-10-30T16:22:50Z

The error I commented about above is still happening for me. Not sure what can cause it ATM.

SimenB · 2017-10-30T16:40:14Z

Can you rebase and push?

cpojer · 2017-12-15T11:04:26Z

Hey everyone! What is the status of this? A lot of people would love to have this feature but the PR is stale and needs a bunch of rebases.

phra · 2017-12-19T11:53:16Z

this would be awesome to have! i can maybe rebase the branch and try to fix the conflicts.. is someone already working on this? maybe @genintho ?

Conflicts: packages/jest-cli/src/constants.js packages/jest-cli/src/lib/terminal_utils.js packages/jest-cli/src/watch.js packages/jest-util/src/index.js

genintho · 2017-12-19T20:41:16Z

Rebase done,
I can not make the following test pass,

 FAIL  integration_tests/__tests__/timeouts.test.js (9.343s)
  ● exceeds the timeout
  ● does not exceed the timeout

SimenB · 2017-12-20T08:27:54Z

@genintho CI is green, thanks for rebasing!

@thymikee could you review again?

Only thing I see is missing update to the changelog, code itself LGTM. I'm not familiar with the watch part of the codebase, though (I probably should sit down and try to learn it)

genintho · 2017-12-21T11:01:45Z

@SimenB If you could test the code changes too, that would be appreciated.

genintho · 2017-12-23T16:47:44Z

I guess this needs to be re-implemented to be a plugin for the watch mode?

thymikee · 2017-12-23T16:54:25Z

Not necessarily I'd say. Btw, I'm keeping testing this feature on my little to-do list, it's just postponed a bit because of holiday.

SimenB · 2017-12-23T17:17:57Z

If it can be implemented as a plugin, that would be preferable. But it's not a necessity for merge, I'd say 🙂

thymikee

Left some comments, only couple of them need to be addressed, but generally it looks good and works well 👍

thymikee · 2017-12-20T11:19:09Z

.gitignore

@@ -27,3 +27,6 @@ lerna-debug.log
 npm-debug.log
 npm-debug.log*
 yarn-error.log*
+
+# JetBrains IDE
+.idea/*


This should probably be a part of your global gitignore, but I'm not against leaving this here

thymikee · 2018-01-06T11:36:45Z

packages/jest-cli/src/snapshot_interactive_mode.js

+
+  _drawUIOverlay() {
+    this._pipe.write(ansiEscapes.scrollDown);
+    this._pipe.write(ansiEscapes.scrollDown);


Any reason why do we have to scroll down? Removing these 2 lines leaves the functionality unchanged on iTerm and macOS Terminal.

thymikee · 2018-01-06T11:37:56Z

packages/jest-cli/src/snapshot_interactive_mode.js

+        chalk.dim(' to trigger a test run.'),
+    ];
+
+    this._pipe.write(messages.filter(message => !!message).join('\n') + '\n');


nit: you can do messages.filter(Boolean), not an issue though

thymikee · 2018-01-06T11:45:12Z

packages/jest-cli/src/snapshot_interactive_mode.js

+      return;
+    }
+
+    this._testFilePaths.shift();


Just a reminder, than shifting is slower than poping from an array, but the difference would be unnoticeable here (unless you have 10k failing tests you want to go one by one)

thymikee · 2018-01-06T11:45:39Z

packages/jest-cli/src/watch.js

@@ -60,6 +61,8 @@ export default function watch(
  const prompt = new Prompt();
  const testPathPatternPrompt = new TestPathPatternPrompt(outputStream, prompt);
  const testNamePatternPrompt = new TestNamePatternPrompt(outputStream, prompt);
+  const snapshotInteracticeMode = new SnapshotInteractiveMode(outputStream);


typo: snapshotInteractiveMode

thymikee · 2018-01-06T11:50:59Z

packages/jest-util/src/__tests__/get_failed_snapshot.test.js

@@ -0,0 +1,60 @@
+import getFailedSnapshotTests from '../get_failed_snapshot_tests';


Missing a license not with @flow pragma

thymikee · 2018-01-06T11:51:52Z

packages/jest-cli/src/__tests__/snapshot_interactive_mode.test.js

@@ -0,0 +1,135 @@
+const chalk = require('chalk');


Missing a license not with @flow pragma

cpojer · 2018-01-06T13:48:08Z

@genintho thanks for sticking with us for such a long time, we are sorry for not being able to get to this earlier. Do you mind making a final iteration based on @thymikee's comments so we can merge this and ship this for all Jest users? Thanks!

thymikee

@mjesun update the snapshots and you should be good to merge this :)

cpojer · 2018-01-11T16:46:08Z

Awesome! So excited to see this. Let's merge it, publish 22.1.0 and make some tweets about this!

genintho · 2018-01-11T17:25:32Z

@mjesun thank you for taking the time to fix the issues. I was planning do it this week end.

mjesun · 2018-01-11T17:51:16Z

You're welcome! ☺️Thanks for taking the time to implement the feature, it's a really cool one!

SimenB

This is missing documentation (and an entry in the changelog).

Code LGTM 🙂

EDIT: It doesn't seem like we have much docs about the watch mode, that's kinda sad

cpojer · 2018-01-11T23:56:04Z

@SimenB I merged this because the PR was in progess for 6 months and I don't want to get it stale again. I filed a task to track docs+changelog updates.

@genintho @mjesun would either of you be able to work on this in a new PR? It's this task: #5288

* add changelog record for #3831 * nicer description * new section in snapshot doc about interactive mode * optimized screen shot * document itself in changelog * replace png by gif * fix description * typos * Move changelog update to master * Updates to changelog language * Minor language updates * rm "the" * s/file/suite

github-actions · 2021-05-12T13:15:04Z

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.

facebook-github-bot added the CLA Signed ✔️ label Jun 15, 2017

genintho added 4 commits June 24, 2017 14:14

Prototype Interactive Snapshot updates

947805e

SnapshotInteractive logic moved into a dedicate file

290aaab

Unit tests for SnapshotInteractiveMode

cbced77

Remove unfinish test

916bcf2

cpojer requested a review from thymikee June 27, 2017 08:31

cpojer reviewed Jun 29, 2017

View reviewed changes

thymikee reviewed Jun 29, 2017

View reviewed changes

thymikee requested changes Jun 29, 2017

View reviewed changes

thymikee reviewed Jun 29, 2017

View reviewed changes

thymikee mentioned this pull request Jul 12, 2017

feature - update a specific part of a snapshot #4015

Closed

genintho added 6 commits August 5, 2017 19:31

Merge remote-tracking branch 'upstream/master' into snap-interactive-up

81d73e1

Conflicts: packages/jest-cli/src/watch.js

Ignore JetBrains IDE meta data folder

736e666

SnapshotInteractive callback use a bool to mark snapshot update

32127da

Fix the Snapshot Interactive tests by using bool

8068f36

Remove unused function, but logic to update snapshot need to be abstr…

0c7343d

…acted

New Snapshot file

8b1b701

Fix code style of snapshot interactive test

45e4e1f

genintho added 2 commits December 19, 2017 19:25

Merge remote-tracking branch 'upstream/master' into snap-interactive-up

eec3d43

Conflicts: packages/jest-cli/src/constants.js packages/jest-cli/src/lib/terminal_utils.js packages/jest-cli/src/watch.js packages/jest-util/src/index.js

Fixes after rebase

f8d4c38

thymikee requested changes Jan 6, 2018

View reviewed changes

thymikee approved these changes Jan 11, 2018

View reviewed changes

Patch with @thymikee review

c545898

SimenB requested changes Jan 11, 2018

View reviewed changes

cpojer merged commit 2dd69b9 into jestjs:master Jan 11, 2018

cpojer mentioned this pull request Jan 11, 2018

Docs + Changelog for #3831 #5288

Closed

genintho mentioned this pull request Jan 12, 2018

Documentation for Interactive Snapshot Update #5291

Merged

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

genintho deleted the snap-interactive-up branch June 2, 2018 03:43

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

		@@ -0,0 +1,60 @@
		import getFailedSnapshotTests from '../get_failed_snapshot_tests';

Interactive Snapshot Update mode #3831

Interactive Snapshot Update mode #3831

Conversation

genintho commented Jun 15, 2017

thymikee commented Jun 19, 2017

genintho commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

thymikee Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genintho commented Aug 8, 2017

SimenB commented Oct 30, 2017

genintho commented Oct 30, 2017 • edited Loading

SimenB commented Oct 30, 2017

cpojer commented Dec 15, 2017

phra commented Dec 19, 2017

genintho commented Dec 19, 2017

SimenB commented Dec 20, 2017 • edited Loading

genintho commented Dec 21, 2017

genintho commented Dec 23, 2017

thymikee commented Dec 23, 2017

SimenB commented Dec 23, 2017

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Jan 6, 2018

thymikee left a comment

Choose a reason for hiding this comment

cpojer commented Jan 11, 2018

genintho commented Jan 11, 2018

mjesun commented Jan 11, 2018

SimenB left a comment • edited Loading

Choose a reason for hiding this comment

cpojer commented Jan 11, 2018

github-actions bot commented May 12, 2021

thymikee Jun 29, 2017 •

edited

Loading

thymikee Jun 29, 2017 •

edited

Loading

genintho commented Oct 30, 2017 •

edited

Loading

SimenB commented Dec 20, 2017 •

edited

Loading

SimenB left a comment •

edited

Loading