Implement pluggable watch mode #4841

wbinnssmith · 2017-11-05T04:37:34Z

Resolves #4824

This is my first nontrivial contribution to Jest so please be gentle :)

This makes watch mode pluggable, allowing for arbitrary watch plugin
modules to be registered in the global config and used to generate
prompt messages and key listeners when watch mode is activated.

When a particular plugin is activated, Jest no longer handles keypresses
until the plugin calls the end callback (see #4824 for design details)

This currently does not implement any existing watch functionality as a
plugin and is rather adding the infrastructure to do so. That will
follow soon :)

@cpojer @aaronabramov I'm not sure if the globalConfig is the best place
for this (to be honest I can't make sense of the the sheer number of
config types). I also haven't gotten around to running a manual test for
this quite yet. More interested in design feedback at this point :)

Test Plan: jest

codecov-io · 2017-11-05T05:23:17Z

Codecov Report

Merging #4841 into master will decrease coverage by 0.36%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #4841      +/-   ##
==========================================
- Coverage   59.64%   59.27%   -0.37%     
==========================================
  Files         194      201       +7     
  Lines        6502     6691     +189     
  Branches        3        4       +1     
==========================================
+ Hits         3878     3966      +88     
- Misses       2624     2725     +101

Impacted Files	Coverage Δ
packages/jest-config/src/valid_config.js	`100% <ø> (ø)`	⬆️
packages/jest-config/src/index.js	`23.8% <ø> (ø)`	⬆️
packages/jest-cli/src/lib/watch_plugin_registry.js	`73.68% <73.68%> (ø)`
packages/jest-cli/src/watch.js	`75.93% <87.5%> (+2.38%)`	⬆️
packages/jest-cli/src/pluralize.js	`0% <0%> (-100%)`	⬇️
packages/jest-cli/src/get_no_test_found_message.js	`0% <0%> (-66.67%)`	⬇️
packages/jest-cli/src/get_no_test_found.js	`0% <0%> (-57.15%)`	⬇️
packages/jest-cli/src/run_jest.js	`0% <0%> (-51.86%)`	⬇️
packages/jest-snapshot/src/mock_serializer.js	`50% <0%> (-50%)`	⬇️
packages/jest-cli/src/reporter_dispatcher.js	`25% <0%> (-50%)`	⬇️
... and 31 more

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 67b4534...f7620f8. Read the comment docs.

SimenB · 2017-11-06T06:34:33Z

packages/jest-cli/src/__tests__/fixtures/watch_plugin.js

@@ -0,0 +1,5 @@
+module.exports = {
+  enter() {}, // override in test
+  key: 0x73, // S key


lower-case s?

SimenB · 2017-11-06T06:35:50Z

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

+  });
+
+  it('prevents Jest from handling keys when active and returns control when end is called', () => {
+    // load the plugin before `watch` does and patch it with a spy


could use jest.doMock for this, but doesn't hurt to do it manually

SimenB · 2017-11-06T06:37:42Z

packages/jest-cli/src/lib/watch_plugin_registry.js

+    }
+
+    const plugin: WatchPlugin = ((maybePlugin: any): WatchPlugin);
+    // TODO: Reject registering when another plugin has claimed the key?


Not sure here. Could we separate between stuff built-in to jest (so you can override e.g p) with stuff from userland (to avoid hard-to=debug conflicts)?

Maybe printing a warning is enough?

SimenB · 2017-11-06T06:44:58Z

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

@@ -89,6 +89,86 @@ describe('Watch mode flows', () => {
    expect(pipe.write.mock.calls.reverse()[0]).toMatchSnapshot();
  });

+  it('resolves relative to the package root', async () => {
+    expect(async () => {


need to await this expect (if not, just remove async from the top)?

I think the story around asynctoThrow is pretty confusing, I usually have to tweak the code to be certain the test actually tests anyting.

What about await expect(watch(...)).resolves.toBeDefined()? Or just await watch(...), without the expect (as the test will fail if there are rejected promises)?

SimenB · 2017-11-06T06:45:46Z

Left a couple inline comments. Overall I'm really excited about this :D Overall direction LGTM, but I haven't really touched the watch-code before.

SimenB · 2017-11-06T19:48:59Z

(Lint errors on ci)

rogeliog · 2017-11-06T19:48:13Z

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

 const watch = require('../watch').default;
 afterEach(runJestMock.mockReset);

-describe('Watch mode flows', () => {
+describe.only('Watch mode flows', () => {


Nit: .only should be removed

rogeliog · 2017-11-06T19:51:57Z

test_utils.js

@@ -51,6 +51,7 @@ const DEFAULT_GLOBAL_CONFIG: GlobalConfig = {
  verbose: false,
  watch: false,
  watchAll: false,
+  watchPlugins: [],


With Jest Runners we have the problem that they are hard to configure... This is mainly because there is no way to pass options to runners, I wonder if we would have a similar problem with plugins

Proposed solution for customizing runners: #4278
Example of how existing runners need to do configuration right now: https://github.com/jest-community/jest-runner-eslint#options

I really like the idea of using an array for customization

without options: watchPlugins: ['my-plugin']

with options: watchPlugins: [['my-plugin', { someOptions: true, other: 'foo' }]]

^ I like that, yes.

rogeliog · 2017-11-06T19:55:42Z

packages/jest-cli/src/lib/watch_plugin_registry.js

+
+    // Since we're loading the module from a dynamic path, assert its shape
+    // before assuming it's a valid watch plugin.
+    if (typeof maybePlugin !== 'object') {


I think we can probably use https://github.com/facebook/jest/tree/master/packages/jest-get-type for all of these

rogeliog · 2017-11-06T19:59:50Z

packages/jest-cli/src/watch.js

  const onKeypress = (key: string) => {
    if (key === KEYS.CONTROL_C || key === KEYS.CONTROL_D) {
      process.exit(0);
      return;
    }

+    if (activePlugin != null) {


Not sure that @cpojer and @aaronabramov think but I feel that it would be interesting to internally implement p and t as plugins so that they are the codebase just treat them as yet another plugin... It might also help to both use them as plugin references and also to always have something to validate and test that api...

Nvm, I just read #4824 (comment)

rogeliog · 2017-11-06T20:00:44Z

packages/jest-cli/src/types.js

+export type WatchPlugin = {
+  key: number,
+  prompt: string,
+  enter: (globalConfig: GlobalConfig, end: () => mixed) => mixed,


~~Does end only gets called once? If so, what about changing the api to be promise based?~~

Nvm, I just read #4824 (comment)

rogeliog

This is great! I'm excited about it!

cpojer · 2017-11-07T09:43:16Z

Thanks for all this great work. I'd like to understand how much work it is to get typeaheads as a plugin to work with this. @wbinnssmith are you planning on resurrecting the old code in a plugin or was @rogeliog going to do this? There is a jest-community org on GitHub where this could live.

My ideal end-state is this one:

Typeahead overwrites the inbuilt command.
When installing the typeahead plugin, we'll ideally auto-load it in Jest (based on a jest-plugin-* prefix possibly.

rogeliog · 2017-11-07T19:52:16Z

I agree with @cpojer it would also be great to eventually provide a way where any plugin can easily leverage the typeahead infra

@cpojer

Resolves jestjs#4824 This is my first nontrivial contribution to Jest so please be gentle :) This makes watch mode pluggable, allowing for arbitrary watch plugin modules to be registered in the global config and used to generate prompt messages and key listeners when watch mode is activated. When a particular plugin is activated, Jest no longer handles keypresses until the plugin calls the `end` callback (see jestjs#4824 for design details) This currently does not implement any existing watch functionality as a plugin and is rather adding the infrastructure to do so. That will follow soon :) @cpojer @aaronabramov I'm not sure if the globalConfig is the best place for this (to be honest I can't make sense of the the sheer number of config types). I also haven't gotten around to running a manual test for this quite yet. More interested in design feedback at this point :) Test Plan: `jest`

wbinnssmith · 2017-11-09T19:44:51Z

Now that you mention options, I think it would be best to have a first-class option that lets the user customize the trigger key for a given plugin. A plugin could still configure a preferred key in the case the user doesn't override it.

I'm thinking that with the addition of options, plugins should instead export a function that is given the set of options and looks something like this:

For example:

module.exports = (opts) => ({
  preferredKey: 's'.codePointAt(0),
  action: 'filter by awesomeness',
  enter: (globalConfig, startRun, end) => {
     ...
  },
});

Looking at existing watch usage, I think this shape is beginning to make sense to me:

type WatchPlugin = (options: {triggerKey: number}) => WatchDescriptor;

type StartRun = (globalConfig: GlobalConfig) => Promise<mixed>;

type WatchDescriptor = {
  preferredKey: number, // code point of trigger key
  action: string, // prompt is "press key to " + action
  enter: (
    globalConfig: GlobalConfig, 
    startRun: StartRun, 
    end: () => mixed,
  ),
}

If the user passes options.triggerKey we'll store that instead of the plugin's preferredKey. This way the user can always override the plugin's key and prevent collisions that would otherwise make two plugins incompatible.

Let me know what you think or if more examples would clarify :D And of course I'm open to alternatives.

SimenB · 2017-11-09T19:46:48Z

I like that idea!

cpojer · 2017-11-10T10:09:42Z

This is really great work. I agree with @rogeliog we should convert t and p into plugins so that you can base the new typeahead on that same code possibly. Thanks sooooo much for doing this.

pedrottimark · 2017-11-10T17:41:09Z

@wbinnssmith Thank you! For your info, being gentle of course :)

After yarn clean-all && yarn the following snapshot passes for

yarn jest --testNamePattern=watch

but fails for either

yarn jest -i
yarn jest watch

  ● Watch mode flows › shows prompts for WatchPlugins in alphabetical order

    expect(value).toMatchSnapshot()
    
    Received value does not match stored snapshot 1.
    
    - Snapshot
    + Received
    
    @@ -1,7 +1,13 @@
      Array [
        Array [
    +     "",
    +   ],
    +   Array [
    +     "Determining test suites to run...",
    +   ],
    +   Array [
          "
      Watch Usage
       › Press a to run all tests.
       › Press p to filter by a filename regex pattern.
       › Press t to filter by a test name regex pattern.
      
      at Object.<anonymous> (packages/jest-cli/src/__tests__/watch.test.js:134:35)

lgandecki · 2017-11-20T06:39:32Z

Thanks for the work @wbinnssmith ! This would be great to have, as, if I understand correctly, this way we can use webdriverio REPL ( http://webdriver.io/api/utility/debug.html ) inside jest tests. (otherwise jest catches whatever I try to type inside the debugger)
Right now the e2e webdriverio/chimp tests are the only place where I still use mocha, personally :)

github-actions · 2021-05-13T02:30:57Z

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 Nov 5, 2017

wbinnssmith force-pushed the wbinnssmith/pluggable-watch-mode branch 2 times, most recently from 5fd1272 to 446560a Compare November 5, 2017 04:59

SimenB requested review from cpojer, rogeliog and boujeepossum November 6, 2017 06:30

SimenB reviewed Nov 6, 2017

View reviewed changes

rogeliog suggested changes Nov 6, 2017

View reviewed changes

wbinnssmith force-pushed the wbinnssmith/pluggable-watch-mode branch 3 times, most recently from f3cc96b to 5fa8647 Compare November 7, 2017 02:51

wbinnssmith force-pushed the wbinnssmith/pluggable-watch-mode branch from 5fa8647 to 4f757f7 Compare November 9, 2017 19:13

wbinnssmith force-pushed the wbinnssmith/pluggable-watch-mode branch from 4f757f7 to f7620f8 Compare November 9, 2017 19:23

cpojer merged commit 9c52d27 into jestjs:master Nov 10, 2017

pedrottimark mentioned this pull request Nov 10, 2017

Fix prettier lint errors in watch_plugin_registry.js #4871

Merged

SimenB mentioned this pull request Nov 14, 2017

add only-failures mode #4886

Merged

rogeliog mentioned this pull request Nov 21, 2017

Couple of small fixes for watchPlugins #4925

Merged

SimenB mentioned this pull request Dec 19, 2017

Bring back typeahead as a watch-mode plugin #4931

Closed

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

Implement pluggable watch mode #4841

Implement pluggable watch mode #4841

wbinnssmith commented Nov 5, 2017

codecov-io commented Nov 5, 2017 •

edited

Loading

SimenB Nov 6, 2017

SimenB Nov 6, 2017

SimenB Nov 6, 2017

SimenB Nov 6, 2017

SimenB commented Nov 6, 2017

SimenB commented Nov 6, 2017

rogeliog Nov 6, 2017

rogeliog Nov 6, 2017

cpojer Nov 7, 2017

rogeliog Nov 6, 2017

rogeliog Nov 6, 2017 •

edited

Loading

rogeliog Nov 6, 2017 •

edited

Loading

rogeliog left a comment

cpojer commented Nov 7, 2017

rogeliog commented Nov 7, 2017

wbinnssmith commented Nov 9, 2017 •

edited

Loading

SimenB commented Nov 9, 2017

cpojer commented Nov 10, 2017

pedrottimark commented Nov 10, 2017

lgandecki commented Nov 20, 2017

github-actions bot commented May 13, 2021

Implement pluggable watch mode #4841

Implement pluggable watch mode #4841

Conversation

wbinnssmith commented Nov 5, 2017

codecov-io commented Nov 5, 2017 • edited Loading

Codecov Report

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

SimenB commented Nov 6, 2017

SimenB commented Nov 6, 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

rogeliog Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

rogeliog Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

rogeliog left a comment

Choose a reason for hiding this comment

cpojer commented Nov 7, 2017

rogeliog commented Nov 7, 2017

wbinnssmith commented Nov 9, 2017 • edited Loading

SimenB commented Nov 9, 2017

cpojer commented Nov 10, 2017

pedrottimark commented Nov 10, 2017

lgandecki commented Nov 20, 2017

github-actions bot commented May 13, 2021

codecov-io commented Nov 5, 2017 •

edited

Loading

rogeliog Nov 6, 2017 •

edited

Loading

rogeliog Nov 6, 2017 •

edited

Loading

wbinnssmith commented Nov 9, 2017 •

edited

Loading