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

Implement pluggable watch mode #4841

Merged
merged 1 commit into from Nov 10, 2017

Conversation

Projects
None yet
8 participants
@wbinnssmith
Member

wbinnssmith commented Nov 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 5, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

codecov-io commented Nov 5, 2017

Codecov Report

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

Impacted file tree graph

@@            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 SimenB requested review from cpojer, rogeliog and aaronabramov Nov 6, 2017

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 6, 2017

Collaborator

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.

Collaborator

SimenB commented Nov 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 6, 2017

Collaborator

(Lint errors on ci)

Collaborator

SimenB commented Nov 6, 2017

(Lint errors on ci)

Show outdated Hide outdated packages/jest-cli/src/__tests__/watch.test.js Outdated
@@ -51,6 +51,7 @@ const DEFAULT_GLOBAL_CONFIG: GlobalConfig = {
verbose: false,
watch: false,
watchAll: false,
watchPlugins: [],

This comment has been minimized.

@rogeliog

rogeliog Nov 6, 2017

Collaborator

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' }]]
@rogeliog

rogeliog Nov 6, 2017

Collaborator

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' }]]

This comment has been minimized.

@cpojer

cpojer Nov 7, 2017

Contributor

^ I like that, yes.

@cpojer

cpojer Nov 7, 2017

Contributor

^ I like that, yes.

Show outdated Hide outdated packages/jest-cli/src/lib/watch_plugin_registry.js Outdated
const onKeypress = (key: string) => {
if (key === KEYS.CONTROL_C || key === KEYS.CONTROL_D) {
process.exit(0);
return;
}
if (activePlugin != null) {

This comment has been minimized.

@rogeliog

rogeliog Nov 6, 2017

Collaborator

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

rogeliog Nov 6, 2017

Collaborator

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)

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

This comment has been minimized.

@rogeliog

rogeliog Nov 6, 2017

Collaborator

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

Nvm, I just read #4824 (comment)

@rogeliog

rogeliog Nov 6, 2017

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Nov 7, 2017

Contributor

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.
Contributor

cpojer commented Nov 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rogeliog

rogeliog Nov 7, 2017

Collaborator

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

Collaborator

rogeliog commented Nov 7, 2017

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

Implement pluggable watch mode
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`
@wbinnssmith

This comment has been minimized.

Show comment
Hide comment
@wbinnssmith

wbinnssmith Nov 9, 2017

Member

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.

Member

wbinnssmith commented Nov 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 9, 2017

Collaborator

I like that idea!

Collaborator

SimenB commented Nov 9, 2017

I like that idea!

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

5 checks passed

ci/circleci: test-and-deploy-website Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Nov 10, 2017

Contributor

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.

Contributor

cpojer commented Nov 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pedrottimark

pedrottimark Nov 10, 2017

Collaborator

@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)
Collaborator

pedrottimark commented Nov 10, 2017

@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)

@SimenB SimenB referenced this pull request Nov 14, 2017

Merged

add only-failures mode #4886

@lgandecki

This comment has been minimized.

Show comment
Hide comment
@lgandecki

lgandecki Nov 20, 2017

Contributor

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 :)

Contributor

lgandecki commented Nov 20, 2017

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment