Attempt to softly interrupt watch (#1599) #1709

thymikee · 2016-09-15T19:33:31Z

This is an attempt to solving #1599.

The PR features a new option: i for "softly" interrupting watch (forces tasks to fail without exiting the process, so you're able to run the tests again, e.g. with adjusted pattern or so).

The watchInterrupt flag is passed to runTest() through argv variable (not sure if that's the best idea though but hey, tried and worked as a proof of concept)

This is WIP, would definitely appreciate some help, as this is my first PR here.

codecov-io · 2016-09-15T19:47:02Z

Current coverage is 87.42% (diff: 100%)

Merging #1709 into master will not change coverage

@@             master      #1709   diff @@
==========================================
  Files            38         38          
  Lines          1217       1217          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1064       1064          
  Misses          153        153          
  Partials          0          0

Powered by Codecov. Last update fd6b124...a593adf

thymikee · 2016-09-15T22:19:16Z

This is how the output looks after hitting interruption key.

cpojer

This is an amazing implementation. I could have never thought of using argv for this, super original :D

Since we'd like to make sure this is solid and isn't going to trouble future generations, I recommend a slightly different approach:

Let's create a "WatchState" class in jest.js that has isInterrupted and setState. By default it's state is something like {interrupted: false}. We pass on the watch state into runTests and on every test result we ask watchState.isInterrupted() and then cancel just like you did in your initial diff.

I also think we might be able to simply kill workerfarm after each test result like if (watchState.isInterrupted()) { workerFarm.end(); } – that way you might not need to make the change in runTests.js.

Please make sure the new implementation works both when using jest -i --watch and jest --watch – the first one runs all the tests in the same process.

(Note: we are going to refactor the watch command into a separate module, for now just put everything into jest.js that needs to be there).

cpojer · 2016-09-17T05:32:49Z

packages/jest-cli/src/TestRunner.js

-      : this._createParallelTestRun(testPaths, onTestResult, onRunFailure);
+      ? this._createInBandTestRun(testPaths, argv, onTestResult, onRunFailure)
+      :
+      this._createParallelTestRun(testPaths, argv, onTestResult, onRunFailure);


nit: this style isn't great. Let's rename onTestResult to onResult and onRunFailure to onFailure. That should be sufficiently descriptive and allow you to bring this back into 80 chars.

cpojer · 2016-09-17T05:33:56Z

packages/jest-cli/src/jest.js

@@ -282,6 +287,7 @@ const runCLI = (
                  ? chalk.dim(' \u203A Press ') + 'u' + chalk.dim(' to update failing snapshots.')
                  : null,
                chalk.dim(' \u203A Press ') + 'p' + chalk.dim(' to filter by a filename regex pattern.'),
+                chalk.dim(' \u203A Press ') + 'i' + chalk.dim(' to interrupt watch mode.'),


can we call this c for cancel and say Press c to cancel current test run? I think that is clearer.

Ideally in the future we'll print this only when tests are actually running, but for now this is fine :)

Sure, i works better for something like interactive I now think, but I had this interruption in mind. Updated to c and cancel as requested

cpojer · 2016-09-17T05:38:32Z

packages/jest-cli/src/jest.js

@@ -347,6 +353,10 @@ const runCLI = (
              pipe.write('\n');
              writeCurrentPattern();
              break;
+            case KEYS.I:
+              argv.watchInterrupt = true;
+              console.log('\n' + chalk.bold('Watch Interrupted...'));


it should be pipe.write(chalk.bold('Test run interrupted')). Let's get rid of the ... here too.

thymikee · 2016-09-17T14:49:59Z

Yeah, passing the message through argv was kind of a hack, because this is a new environment for me and I wasn't sure how to pass the variable other way.

WatchState feels way better. With the last commit I've replaced argv with this class. The whole functionality works much faster. For parallel run the interruption is instant. For band run it needs to wait for the initiated singular test to end, which introduces a delay.

I guess I should write a test for WatchState class, shouldn't I?

BTW. This is the first time for me using flowtype and already loved it.

thymikee · 2016-09-19T19:06:57Z

Had a while so I've tweaked the messaging a bit. It's also visible while running tests.

While tests are running:

After c is pressed:

cpojer

This is great. I had a couple smaller comments and I'll probably need to experiment with the promises here a little bit myself, but it is almost ready to ship :)

cpojer · 2016-09-20T02:04:48Z

packages/jest-cli/src/TestRunner.js

@@ -30,6 +30,7 @@ const promisify = require('./lib/promisify');
 const runTest = require('./runTest');
 const snapshot = require('jest-snapshot');
 const workerFarm = require('worker-farm');
+const WatchState = require('./jest').WatchState;


hm, this circular dependency is not great. Can we put WatchState into a separate file next to TestRunner.js?

I was just about to ask about it earlier :D Fix'd with latest commit.

cpojer · 2016-09-20T02:05:26Z

packages/jest-cli/src/reporters/DefaultReporter.js

@@ -69,8 +69,11 @@ class DefaultReporter extends BaseReporter {
      results.numRuntimeErrorTestSuites;
    if (process.stdout.isTTY && remaining > 0) {
      process.stderr.write(RUNNING_TEST_COLOR(
-        `Running ${pluralize('test suite', remaining)}...`,
+        `Running ${pluralize('test suite', remaining)}... `,


can we remove this again and add a space in front of the message on #75?

cpojer · 2016-09-20T02:06:48Z

packages/jest-cli/src/reporters/DefaultReporter.js

      ));
+      if (config.watch) {
+        process.stderr.write(RUNNING_TEST_COLOR('Press c to cancel '));


And we can get rid of the space at the end here. Let's do:

process.stderr.write(' ' + RUNNING_TEST_COLOR('Press ') + 'c' + RUNNING_TEST_COLOR('c to cancel'));

cpojer · 2016-09-20T02:08:11Z

packages/jest-cli/src/jest.js

 };

+type WatchStateState = {


This kind of tells me we should find a new name for WatchState :D

TestWatcher maybe?

thymikee · 2016-09-22T05:32:36Z

Forgot to mention I've updated the TestWatcher with subscription API, so the interruption feels instant.

cpojer · 2016-09-24T09:06:20Z

@thymikee would you mind rebasing your diff? We just merged the concurrent reporter so we made some changes in the same modules :(

thymikee · 2016-09-24T09:46:52Z

@cpojer sure, give me a sec ;)

thymikee · 2016-09-24T12:08:58Z

@cpojer Rebased. I'd appreciate further testing of the feature. Some snapshots are not passing, but I'd rather like you to check if it's intentional.

cpojer · 2016-09-28T08:58:25Z

@thymikee I polished the implementation a little bit and I think it works pretty well now both in the concurrent and in-band mode. Would you mind giving it a try and letting me know what you think? I removed the "Press c to cancel a test run"-line so we aren't making the watcher messy, especially since cancel can only be used during a run. However, with the new concurrent reporter, printing a message during the run on how to abort also seems kind of odd. Do you have any suggestions on where to hint at that pressing "c" will cancel the run?

Also cc @gaearon because he requested this feature. Try it out!

thymikee · 2016-09-28T10:23:30Z

@cpojer definitely appreciate the simpler EventEmitter-based solution and cleanup! (Guess I just wanted to play with subscription mechanism)

Letting user know about the possibility to cancel tests is indeed hard to fit into concurrent reporter. What about adjusting the message in Watch Usage section, to stick a little from the rest? E.g.

To cancel already running tests press c.

gaearon · 2016-09-28T11:06:14Z

Do we need a separate command? Can we make existing commands just keep working instead?

Basically all I wanted is to be able to p, type another pattern, press Enter, and have Jest acknowledge that during the test run. Same with o and a.

cpojer · 2016-09-28T12:13:15Z

@gaearon you are saying that any existing command should cancel if a test run is active?

gaearon · 2016-09-28T15:18:01Z

I think that on confirmation (p confirms on <Enter>, a, o, and <Enter> itself confirm immediately) the existing command should cancel, clear the screen, and the new command should run.

thymikee · 2016-09-28T15:23:54Z

Wouldn't the console be too crowded if the user hits p, types a pattern and have all the tests running in the meantime, jumping around? I think better UX (for p command) would be to clear the screen immediately and then re-run tests with new pattern on <Enter>.

cpojer · 2016-09-29T05:13:21Z

This is what it looks like now:

Took @gaearon's suggestion, even hitting "enter" during the run will abort. Let's see how people like it.

* Attempt to softly interrupt watch * Polish implementation.

github-actions · 2021-05-13T22:07:18Z

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.

ghost added the CLA Signed ✔️ label Sep 15, 2016

cpojer requested changes Sep 17, 2016

View reviewed changes

thymikee force-pushed the watch-interrupt branch from fe5e5cd to d4bb347 Compare September 17, 2016 14:34

ghost added the CLA Signed ✔️ label Sep 17, 2016

thymikee force-pushed the watch-interrupt branch from d4bb347 to 06d643a Compare September 19, 2016 18:59

ghost added the CLA Signed ✔️ label Sep 19, 2016

cpojer requested changes Sep 20, 2016

View reviewed changes

thymikee force-pushed the watch-interrupt branch from 06d643a to 228e568 Compare September 20, 2016 05:39

facebook-github-bot added the CLA Signed ✔️ label Sep 20, 2016

thymikee force-pushed the watch-interrupt branch from 228e568 to e9f9de7 Compare September 20, 2016 17:37

ghost added the CLA Signed ✔️ label Sep 20, 2016

cpojer mentioned this pull request Sep 22, 2016

Watch Mode, process exits if a test fails and bail is set to true #1771

Closed

ghost added the CLA Signed ✔️ label Sep 24, 2016

thymikee force-pushed the watch-interrupt branch from ffd44b9 to 4296f3f Compare September 24, 2016 12:05

ghost added the CLA Signed ✔️ label Sep 24, 2016

thymikee force-pushed the watch-interrupt branch from 4296f3f to 96ba286 Compare September 27, 2016 21:34

ghost added the CLA Signed ✔️ label Sep 27, 2016

cpojer force-pushed the watch-interrupt branch from 96ba286 to 8d1cdbb Compare September 28, 2016 08:38

ghost added the CLA Signed ✔️ label Sep 28, 2016

cpojer force-pushed the watch-interrupt branch 2 times, most recently from 7de829a to 26820e7 Compare September 28, 2016 08:39

cpojer force-pushed the watch-interrupt branch 4 times, most recently from 06b14a0 to d7d5823 Compare September 28, 2016 08:42

ghost added the CLA Signed ✔️ label Sep 28, 2016

Attempt to softly interrupt watch

d4e094f

cpojer force-pushed the watch-interrupt branch from d7d5823 to a593adf Compare September 28, 2016 08:59

cpojer approved these changes Sep 28, 2016

View reviewed changes

ghost added the CLA Signed ✔️ label Sep 28, 2016

Polish implementation.

a5456bb

cpojer force-pushed the watch-interrupt branch from a593adf to a5456bb Compare September 29, 2016 05:12

facebook-github-bot added the CLA Signed ✔️ label Sep 29, 2016

cpojer merged commit 7a36a03 into jestjs:master Sep 29, 2016

mthmulders pushed a commit to mthmulders/jest that referenced this pull request Oct 10, 2016

Attempt to softly interrupt watch (jestjs#1599) (jestjs#1709)

e694ffe

* Attempt to softly interrupt watch * Polish implementation.

thymikee deleted the watch-interrupt branch February 19, 2017 16:29

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

Attempt to softly interrupt watch (jestjs#1599) (jestjs#1709)

585bd4e

* Attempt to softly interrupt watch * Polish implementation.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to softly interrupt watch (#1599) #1709

Attempt to softly interrupt watch (#1599) #1709

thymikee commented Sep 15, 2016

codecov-io commented Sep 15, 2016 •

edited

thymikee commented Sep 15, 2016

cpojer left a comment

cpojer Sep 17, 2016

cpojer Sep 17, 2016

thymikee Sep 17, 2016

cpojer Sep 17, 2016 •

edited

thymikee commented Sep 17, 2016

thymikee commented Sep 19, 2016

cpojer left a comment

cpojer Sep 20, 2016

thymikee Sep 20, 2016

cpojer Sep 20, 2016

cpojer Sep 20, 2016

cpojer Sep 20, 2016

thymikee Sep 20, 2016

cpojer Sep 20, 2016

thymikee commented Sep 22, 2016

cpojer commented Sep 24, 2016

thymikee commented Sep 24, 2016

thymikee commented Sep 24, 2016

cpojer commented Sep 28, 2016

thymikee commented Sep 28, 2016 •

edited

gaearon commented Sep 28, 2016

cpojer commented Sep 28, 2016

gaearon commented Sep 28, 2016 •

edited

thymikee commented Sep 28, 2016

cpojer commented Sep 29, 2016

github-actions bot commented May 13, 2021

Attempt to softly interrupt watch (#1599) #1709

Attempt to softly interrupt watch (#1599) #1709

Conversation

thymikee commented Sep 15, 2016

codecov-io commented Sep 15, 2016 • edited

Current coverage is 87.42% (diff: 100%)

thymikee commented Sep 15, 2016

cpojer 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

cpojer Sep 17, 2016 • edited

Choose a reason for hiding this comment

thymikee commented Sep 17, 2016

thymikee commented Sep 19, 2016

cpojer 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

thymikee commented Sep 22, 2016

cpojer commented Sep 24, 2016

thymikee commented Sep 24, 2016

thymikee commented Sep 24, 2016

cpojer commented Sep 28, 2016

thymikee commented Sep 28, 2016 • edited

gaearon commented Sep 28, 2016

cpojer commented Sep 28, 2016

gaearon commented Sep 28, 2016 • edited

thymikee commented Sep 28, 2016

cpojer commented Sep 29, 2016

github-actions bot commented May 13, 2021

codecov-io commented Sep 15, 2016 •

edited

cpojer Sep 17, 2016 •

edited

thymikee commented Sep 28, 2016 •

edited

gaearon commented Sep 28, 2016 •

edited