[Proposal] Watch plugins API #5399

rogeliog · 2018-01-26T06:22:14Z

This PR adds watch plugins to Jest... Feedback is more than welcome!

Pending Items

Update Changelog
Add docs for watch plugins.
I added shouldRunTestSuite(...) to JestHooks but I still need to implement it.
Fix skipped tests
Make sure things work well in non interactive envirnoments
Make sure things work when save a file and you have an active plugin.

NOTE: once shouldRunTestSuite(...) it would be interesting if we can reimplement the "failed tests mode" as a plugin that hooks into onTestComplete(...) for keeping track of the results and shouldRunTestSuite(...) to prevent running test suites that didn't fail

Open questions

Given that plugins are instantiable objects, I'm a bit worried about how we are going to document the interface for creating a plugin. Should we change them to a function?
Right now it shows the plugins in a different order than master [Need to fix before merging]. How should we define a way of sorting the plugins?
Seems like at some point we will need to expand plugins to work in "non-watch" mode... any thoughts?

Plugin API

Feedback on the API is more than welcome.

WatchPlugin {
  showPrompt: (globalConfig: GlobalConfig, updateConfigAndRun: Function) => Promise<*>,
  getUsageRow: (globalConfig: GlobalConfig, hasFailedSnapshots: boolean) => ?{
    key: number,
    prompt: string,
    hide?: boolean,
  },
  onData: (value: string) => void,

  registerHooks: (hooks: JestHookSubscriber) => void,
}

First part: Watch mode usage interactions.

showPrompt(...): This gives control to the plugin to manage test execution. It provides a updateConfigAndRun function and returns a promise that can be resolved when the control needs to be given back to Jest.
onData(...): A helper function for capturing keystrokes when the plugin is active.
getUsageRow(...): Returns the usage row information, and allows for dynamically updating the text, hiding, etc. Example: { key: S, prompt: 'filter by @tags' }

Second part: `registerHooks`

This is how WatchPlugins will hook into different parts of the Jest.

How this could be expanded to support different plugin needs?

Right now the only hook that I created is testRunComplete and shouldRunTestSuite. If needed we could have hooks like onTestSuiteStart, onTestSuiteComplete, etc. This will allow us to have well defined, extension points.
Hooks with a return value can also be supported, for example shouldRunTestSuite(...): Promise<boolean>.

Click here to see original proposal

[WIP] Plugin Proposal

cc: @cpojer, @orta , @thymikee

It is still in a super rough spot, but I think this architecture might allow us to have a simple yet powerful API.

Context and current state of proposal

This PR is still dirty and WIP.

It makes each and every row in the current usage a watch plugin. For example UpdateSnapshotsInteractivePlugin, TestPathWatchPlugin or even 'trivial' stuff like QuitPlugin.
It removes all the knowledge of what each key is responsible of doing from watch.js.

Pending items

Given that I was trying to make sure that every of the existing functionality could be "migrated" to a plugin. I temporarily removed the ability to add external plugins, I need to add that back
Keep making the rest of the keys plugins.
Add flow types.
Remove unused code and a do lot of cleanup.
Fix failing tests.
Refine the API
Add docs

Which files should I focus on to better understand the proposal?

Any file in the jest-cli/src/plugins directory, specially plugins/test_path_pattern.js and plugins/update_snapthots_interactive.js
The switch statement jest-cli/src/watch.js

Plugin API

WatchPlugin {
  showPrompt: (globalConfig: GlobalConfig, updateConfigAndRun: Function) => Promise<*>,
  getUsageRow: (globalConfig: GlobalConfig, hasFailedSnapshots: boolean) => ?{
    key: number,
    prompt: string,
    hide?: boolean,
  },
  onData: (value: string) => void,

  registerHooks: (hooks: JestHooks) => void,
}

Currently, there are two parts of the plugin system: which maybe could benefit from better names.

First part: Watch mode usage interactions.

showPrompt(...): This gives control to the plugin to manage test execution. It provides a updateConfigAndRun function and returns a promise that can be resolved when the control needs to be given back to Jest.
onData(...): A helper function for capturing keystrokes when the plugin is active.
getUsageRow(...): Returns the usage row information, and allows for dynamically updating the text, hiding, etc. Example: { key: S, prompt: 'filter by @tags' }

Second part: `registerHooks`

NOTE: This is my first time using webpack/tapable, so let me know if I'm thinking it at the completely wrong level.

This is how WatchPlugins will hook into different parts of the Jest. It is powered by webpack/tapable which is what Webpack uses for their plugin system. This is inspired by how Webpack plugins work.

How this could be expanded to support different plugin needs?

Right now the only hook that I created is testRunComplete.f needed we could have hooks like testSuiteStart, testSuiteComplete, testPass, testFail by leveraging webpack/tapable. This will allow us to have well defined, extension points.
Hooks with a return value can also be supported, for example shouldRunTest(...): boolean or shouldRunTestSuite(...): boolean.

Improvements & Questions

It might be a good idea to wrap the tapable API so that the user does not have to worry about tap, tapPromise, etc. Also to prevent from calling hooks.
There might be some abstractions that need to be created to better support plugins.
Why an instance? Because it makes it easy for plugins to keep state.
I understand that there are still a lot of unknown unknowns.

rogeliog · 2018-01-26T06:57:08Z

packages/jest-cli/src/watch.js


 let hasExitListener = false;

+const internalPlugins = [


This will get cleaned up

rogeliog · 2018-01-26T06:57:24Z

packages/jest-cli/src/watch.js

+  };
+
+  const watchPlugins: Array<WatchPlugin> = internalPlugins
+    .map(pluginPath => require(pluginPath).default)


All of this will get cleaned up

rogeliog · 2018-01-26T06:57:36Z

packages/jest-cli/src/watch.js

+      .map(p => p.getUsageRow(globalConfig, hasSnapshotFailure))
+      .map(p => (p ? p.key : undefined))
+      .filter(p => p !== undefined)
+      .sort((a, b) => a - b)


cpojer · 2018-01-26T09:28:32Z

Woah, this is extremely exciting, I didn't know your changes were going to be so extensive. This really makes Jest's watch mode a general purpose system that we should extract into jest-watch (in follow-up PRs). I would love to use this for Metro's dev server as well (cc @rafeca, @arcanis).

I think the fact that you are able to support all existing features of watch mode with plugins now is a good sign that this API design solves our needs well. I would recommend going with that, polishing it up, and shipping it before we consider further changes. This PR unlocks a new set of capabilities that we haven't thought of yet so I wanna be cautious that we don't waste too much time bikeshedding on the API. We can always change it later.

The only concern I have is around tapable. I do not think it provides value and think we can replicate it's functionality within Jest. Do you think that would be ok?

rogeliog · 2018-01-26T21:33:43Z

@cpojer hehe awesome, I'll start by polishing it.

I like the power that tapable provides, but I agree that it might be too early to justify it.

SimenB · 2018-02-05T15:39:35Z

This has conflicts, and failing appveyor CI (I think that one just needs a restart, though)

SimenB

Mostly just questions, this is looking really awesome!

SimenB · 2018-02-05T15:40:51Z

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


-  it('prevents Jest from handling keys when active and returns control when end is called', () => {
+  // TODO: Fix or remove before merging
+  xit('prevents Jest from handling keys when active and returns control when end is called', () => {


seems like something we'd want, isn't it?

Correct, this is one of the pending items that I have at the top.

SimenB · 2018-02-05T15:42:39Z

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

-    ['t', 'e', 's', 't']
+    await nextTick();
+
+    [('t', 'e', 's', 't')]


why the parens? this makes it just return the last one, and we get an array of ['t'] (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comma_Operator)

🤦‍♂️ 🤦‍♂️ 🤦‍♂️ I have no idea how that happened

SimenB · 2018-02-05T15:44:31Z

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

+        ? chalk.dim('test name ') + chalk.yellow('/' + testNamePattern + '/')
+        : null,
+    ]
+      .filter(f => !!f)


.filter(Boolean)? Or just remove the double bang, not really needed (as you check against null)

Sorry, I just moved that from watch.js, changing it now

SimenB · 2018-02-05T15:45:46Z

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

+      .filter(f => !!f)
+      .join(', ');
+
+    const messages = ['\n' + chalk.bold('Active Filters: ') + filters];


Why in an array? Did you mean to concat with filters instead of joining on line 27?

Yeah this doesn't seem to be needed, same as above just moved to from watch.js

SimenB · 2018-02-05T15:49:02Z

packages/jest-cli/src/plugins/quit.js

+  showPrompt(): Promise<void> {
+    this._stdout.write('\n');
+    process.exit(0);
+    return Promise.resolve();


can just mark the function as async and drop the return, although the transpiled code is pretty ugly. Up to to you 🙂

(would be cool if babel just wrapped the return value in Promise.resolve if there were no awaits in an async function)

SimenB · 2018-02-05T15:58:35Z

packages/jest-cli/src/watch.js

        });
-        startRun(globalConfig);
        break;
      case KEYS.F:


should this be a plugin as well?

Correct, this one should be a plugin... I can do it in this PR of as follow up PRs

I also want to wait for it because it will require some refactor

SimenB · 2018-02-05T15:59:09Z

packages/jest-cli/src/watch.js

+    .map(p => p.getUsageRow(globalConfig, hasSnapshotFailure))
+    .filter(usage => usage && !usage.hide)
+    // $FlowFixMe a and b will not be null or undefined
+    .sort((a, b) => a.key - b.key);


I like q being at the bottom, no biggie, though 🙂

SimenB · 2018-02-05T15:59:46Z

packages/jest-cli/src/watch.js

+
+  const thirdPartyPlugins = watchPlugins
+    .slice(INTERNAL_PLUGINS.length)
+    .map(p => p.getUsageRow(globalConfig, hasSnapshotFailure))


hasSnapshotFailure seems really specific, IDK if there's a better place to stick it, though

testResults array would maybe make more sense, possible with some properties like hasSnapshotFailure: boolean on it for ease of use

Agree!

This is something that I was thinking over the weekend, I'm glad that you felt the same way!

My thoughts are that getUsageRow() should only take in the globalFilter.

Stuff like hasSnapshotFailure (which as you mentioned only a small chunk of plugins will use) can be accessed from thetestRunComplete hook.

hooks.testRunComplete(results => { this._hasSnapshotFailure = !!results.snapshot.failure; });

Unrelated comment

Which makes me think that we could take this approach even further and not even pass globalConfig as a param of getUsageRow, but instead if the plugin needs access to the most up to date globalConfig it can get it from a globalConfigChanged hook

hooks.globalConfigChanged(globalConfig => { this._globalConfig = globalConfig; });

What are your thoughts on this?

SimenB · 2018-02-05T16:03:13Z

packages/jest-cli/src/watch.js

-        );
-        break;
      case KEYS.QUESTION_MARK:
        break;


not for this PR, but should this break be here? Seems to make sense for w and ? to behave the same here.

SimenB · 2018-02-05T16:03:52Z

packages/jest-cli/src/watch.js

+      plugin =>
+        chalk.dim(' \u203A Press') +
+        ' ' +
+        // $FlowFixMe plugin will be present


can't we fix the type definition so the ignore isn't needed?

rogeliog · 2018-02-06T06:01:44Z

I'm still worried about how hard it is going to be to communicate the current API for watch plugins.

Should it be an instantiable object that extendsWatchPlugin?
Should it be an instantiable object that implements all the methods?
We can also take an approach similar to Webpack, and do something like "a watch plugin is an instantiable object with an apply method" and then we inject everything through jestHooks, even getUsageRow and showPrompt

cc: @cpojer @SimenB

codecov-io · 2018-02-06T06:07:58Z

Codecov Report

Merging #5399 into master will increase coverage by <.01%.
The diff coverage is 73.39%.

@@            Coverage Diff             @@
##           master    #5399      +/-   ##
==========================================
+ Coverage   62.23%   62.24%   +<.01%     
==========================================
  Files         205      212       +7     
  Lines        6932     6955      +23     
  Branches        4        3       -1     
==========================================
+ Hits         4314     4329      +15     
- Misses       2617     2625       +8     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-cli/src/run_jest.js	`50% <0%> (-2.55%)`	⬇️
packages/jest-cli/src/plugins/test_path_pattern.js	`100% <100%> (ø)`
...ackages/jest-cli/src/lib/active_filters_message.js	`100% <100%> (ø)`
packages/jest-cli/src/plugins/test_name_pattern.js	`100% <100%> (ø)`
packages/jest-cli/src/plugins/update_snapshots.js	`100% <100%> (ø)`
packages/jest-cli/src/plugins/quit.js	`33.33% <33.33%> (ø)`
...st-cli/src/plugins/update_snapshots_interactive.js	`37.5% <37.5%> (ø)`
packages/jest-cli/src/watch_plugin.js	`50% <50%> (ø)`
packages/jest-cli/src/jest_hooks.js	`55.55% <55.55%> (ø)`
packages/jest-cli/src/watch.js	`73.52% <83.33%> (+2.9%)`	⬆️
... and 11 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 bda02e6...3dc8c06. Read the comment docs.

cpojer · 2018-02-06T12:53:46Z

Awesome, I hit the merge button but I do think there are some follow-ups, like you mentioned yourself.

getUsageRow: could we rename this and change behavior? I don't think row is a great name. Could we also change it instead of returning a hide field to return null or an object, where the presence of an object means it is activated and null means it is disabled? I propose naming it something like getUsageInfo or getUsageData but there may be better names.

What's your concrete next step? I think the current API looks good as a start and we can bring back the typeahead plugin with this. Are you planning on making this happen? :)

rickhanlonii · 2018-02-06T14:26:29Z

Could consider:

WatchPlugin {
  apply: (hooks: JestHookSubscriber) => void,
  run: (globalConfig: GlobalConfig, updateConfigAndRun: Function) => Promise<*>,
  onKey: (value: string) => void,
  getPrompt: (globalConfig: GlobalConfig, hasFailedSnapshots: boolean) => ?{
    key: number,
    prompt: string,
  },
}

apply - Apply hooks into different parts of Jest.
run - Gives control to the plugin to manage test execution.
onKey - Handle keystrokes when the plugin is running.
getPrompt - Returns the key prompt information.

I took run and apply from the webpack plugin names, and onKey from typical on handler naming and getPrompt as a literal description of its responsibility. I admit that Key and prompt may be too specific to the current use case so those could be Data and Usage respectively

rogeliog · 2018-02-06T18:30:31Z

@cpojer I just created #5478 to keep track of that.

@rickhanlonii I like that idea! What do you think @cpojer?

cpojer · 2018-02-06T20:26:33Z

I'm on board with that. Do you wanna change it?

SimenB · 2018-02-11T12:58:14Z

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

+
+    const messages = ['\n' + chalk.bold('Active Filters: ') + filters];
+
+    return messages.filter(message => !!message).join(delimiter);


this is still weird - messages will always be an array of size 1

github-actions · 2021-05-12T21:06:41Z

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.

rogeliog added 8 commits January 17, 2018 08:50

Start adding tapable

396cd18

Use test_path_pattern as a plugin

c04edc7

Use test_name_pattern as a plugin

1f74aa1

Use quit as a plugin

03b53b1

Fix test interruption

1aeafb8

Use update snapshot as a plugin

b40f604

Use update snapshot interactive as a plugin

6e97a6e

Change API to use a class instance

ef14ade

facebook-github-bot added the CLA Signed ✔️ label Jan 26, 2018

rogeliog requested review from cpojer, orta and thymikee January 26, 2018 06:22

rogeliog commented Jan 26, 2018

View reviewed changes

rogeliog added 2 commits January 26, 2018 14:01

Merge branch 'master' into simpler-plugins

8b999ed

A bit of clean up and make tests pass

2604965

seanpoulter mentioned this pull request Jan 27, 2018

Let stdin be passed through to jest-cli #5017

Closed

SimenB mentioned this pull request Jan 30, 2018

Bring back typeahead as a watch-mode plugin #4931

Closed

rogeliog added 8 commits February 1, 2018 16:43

Change plugin implementation to not use tapable

ffc85f1

Better sorting implementation

053d9e4

Add back third party plugin functionality

2036be9

Fix flow

40c4265

Merge branch 'master' into simpler-plugins

44c82ba

Fix ESLint

7767b71

Reset file to state of master

2b084f8

Update failing snapshot

58f1717

rogeliog changed the title ~~[WIP][Proposals] Watch plugins API~~ [Proposals] Watch plugins API Feb 2, 2018

rogeliog requested a review from SimenB February 2, 2018 17:24

SimenB approved these changes Feb 5, 2018

View reviewed changes

rogeliog added 8 commits February 5, 2018 19:29

Merge branch 'master' into simpler-plugins

13f08e5

Remove hasSnapshotFailure and hasSnapshotFailureInteractive

d4ecb92

Async await for showPrompt and clear active plugin on file change

8f6513e

Fix snapshot failure

401e44b

Reenable tests

9ac7d94

Implement shouldRunTestSuite

25e0c4b

Add changelog

4a68617

Clean up watch.js a bit

3dc8c06

rogeliog changed the title ~~[Proposals] Watch plugins API~~ [Proposal] Watch plugins API Feb 6, 2018

cpojer merged commit 4561959 into jestjs:master Feb 6, 2018

rogeliog deleted the simpler-plugins branch February 6, 2018 18:20

rogeliog mentioned this pull request Feb 6, 2018

Todo items for watch plugins #5478

Closed

7 tasks

flegall mentioned this pull request Feb 7, 2018

Integrate with watch mode of test frameworks jsGiven/jsGiven#137

Open

SimenB reviewed Feb 11, 2018

View reviewed changes

renovate bot mentioned this pull request Feb 14, 2018

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

Open

rogeliog mentioned this pull request Feb 28, 2018

Fix update snapshot when mode: watchAll #5696

Merged

JonathanTR mentioned this pull request Apr 25, 2018

Cannot determine how Jest is skipping tests #4763

Closed

azz mentioned this pull request Jun 2, 2018

Add Command Hooks to Custom Runners #5212

Closed

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


		const messages = ['\n' + chalk.bold('Active Filters: ') + filters];

		return messages.filter(message => !!message).join(delimiter);

Uh oh!

[Proposal] Watch plugins API #5399

[Proposal] Watch plugins API #5399

Uh oh!

Conversation

rogeliog commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pending Items

Open questions

Plugin API

First part: Watch mode usage interactions.

Second part: registerHooks

How this could be expanded to support different plugin needs?

[WIP] Plugin Proposal

Context and current state of proposal

Which files should I focus on to better understand the proposal?

Plugin API

First part: Watch mode usage interactions.

Second part: registerHooks

How this could be expanded to support different plugin needs?

Improvements & Questions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpojer commented Jan 26, 2018

Uh oh!

rogeliog commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimenB commented Feb 5, 2018

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogeliog Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogeliog Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogeliog Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogeliog Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Unrelated comment

Uh oh!

Choose a reason for hiding this comment

rogeliog commented Jan 26, 2018 •

edited

Loading

Second part: `registerHooks`

Second part: `registerHooks`

rogeliog commented Jan 26, 2018 •

edited

Loading

rogeliog Feb 6, 2018 •

edited

Loading

rogeliog Feb 6, 2018 •

edited

Loading

rogeliog Feb 6, 2018 •

edited

Loading

rogeliog Feb 5, 2018 •

edited

Loading