Check watch plugins for key conflicts (#6697)

## Summary Watch plugins now are checked for key conflicts… - Some built-in keys remain overridable (specificically `t` and `p`). - Any key registered by a third-party watch plugin cannot be overridden by one listed later in the plugins list config. Fixes #6693. Refs #6473. ## Test plan Additional tests are provided that check every conflict / overwritable scenario discussed. ## Request for feedback / “spec” evolution The “spec” is an ongoing discussion in #6693 — in particular, the overwritability of some built-in keys, such as `a`, `f` and `o`, may be subject to discussion. This PR tracks the decisions in there and may evolve a bit still. Ping @SimenB @thymikee @rogeliog for review and discussion.
jestjs · Aug 9, 2018 · f8f966b · f8f966b
1 parent 08cb885
commit f8f966b
Show file tree

Hide file tree

Showing 5 changed files with 250 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 - `[jest-each]` introduces `%#` option to add index of the test to its title ([#6414](https://github.com/facebook/jest/pull/6414))
 - `[pretty-format]` Support serializing `DocumentFragment` ([#6705](https://github.com/facebook/jest/pull/6705))
 - `[jest-validate]` Add `recursive` and `recursiveBlacklist` options for deep config checks ([#6802](https://github.com/facebook/jest/pull/6802))
+- `[jest-cli]` Check watch plugins for key conflicts ([#6697](https://github.com/facebook/jest/pull/6697))
 
 ### Fixes
 

diff --git a/docs/WatchPlugins.md b/docs/WatchPlugins.md
@@ -202,3 +202,30 @@ class MyWatchPlugin {
   constructor({config}) {}
 }
 ```
+
+## Choosing a good key
+
+Jest allows third-party plugins to override some of its built-in feature keys, but not all. Specifically, the following keys are **not overwritable** :
+
+- `c` (clears filter patterns)
+- `i` (updates non-matching snapshots interactively)
+- `q` (quits)
+- `u` (updates all non-matching snapshots)
+- `w` (displays watch mode usage / available actions)
+
+The following keys for built-in functionality **can be overwritten** :
+
+- `p` (test filename pattern)
+- `t` (test name pattern)
+
+Any key not used by built-in functionality can be claimed, as you would expect. Try to avoid using keys that are difficult to obtain on various keyboards (e.g. `é`, `€`), or not visible by default (e.g. many Mac keyboards do not have visual hints for characters such as `|`, `\`, `[`, etc.)
+
+### When a conflict happens
+
+Should your plugin attempt to overwrite a reserved key, Jest will error out with a descriptive message, something like:
+
+> Watch plugin YourFaultyPlugin attempted to register key <q>, that is reserved internally for quitting watch mode. Please change the configuration key for this plugin.
+
+Third-party plugins are also forbidden to overwrite a key reserved already by another third-party plugin present earlier in the configured plugins list (`watchPlugins` array setting). When this happens, you’ll also get an error message that tries to help you fix that:
+
+> Watch plugins YourFaultyPlugin and TheirFaultyPlugin both attempted to register key <x>. Please change the key configuration for one of the conflicting plugins to avoid overlap.
diff --git a/packages/jest-cli/src/__tests__/__snapshots__/watch.test.js.snap b/packages/jest-cli/src/__tests__/__snapshots__/watch.test.js.snap
@@ -36,7 +36,7 @@ Watch Usage
 ]
 `;
 
-exports[`Watch mode flows allows WatchPlugins to override internal plugins 1`] = `
+exports[`Watch mode flows allows WatchPlugins to override eligible internal plugins 1`] = `
 Array [
   "
 Watch Usage
@@ -60,8 +60,8 @@ Watch Usage
  › Press p to filter by a filename regex pattern.
  › Press t to filter by a test name regex pattern.
  › Press q to quit watch mode.
+ › Press r to do something else.
  › Press s to do nothing.
- › Press u to do something else.
  › Press Enter to trigger a test run.
 ",
   ],

diff --git a/packages/jest-cli/src/__tests__/watch.test.js b/packages/jest-cli/src/__tests__/watch.test.js
@@ -78,7 +78,7 @@ jest.doMock(
     class WatchPlugin2 {
       getUsageInfo() {
         return {
-          key: 'u',
+          key: 'r',
           prompt: 'do something else',
         };
       }
@@ -323,7 +323,7 @@ describe('Watch mode flows', () => {
     expect(apply).toHaveBeenCalled();
   });
 
-  it('allows WatchPlugins to override internal plugins', async () => {
+  it('allows WatchPlugins to override eligible internal plugins', async () => {
     const run = jest.fn(() => Promise.resolve());
     const pluginPath = `${__dirname}/__fixtures__/plugin_path_override`;
     jest.doMock(
@@ -364,6 +364,138 @@ describe('Watch mode flows', () => {
     expect(run).toHaveBeenCalled();
   });
 
+  describe('when dealing with potential watch plugin key conflicts', () => {
+    it.each`
+      key    | plugin
+      ${'q'} | ${'Quit'}
+      ${'u'} | ${'UpdateSnapshots'}
+      ${'i'} | ${'UpdateSnapshotsInteractive'}
+    `(
+      'forbids WatchPlugins overriding reserved internal plugins',
+      ({key, plugin}) => {
+        const run = jest.fn(() => Promise.resolve());
+        const pluginPath = `${__dirname}/__fixtures__/plugin_bad_override_${key}`;
+        jest.doMock(
+          pluginPath,
+          () =>
+            class OffendingWatchPlugin {
+              constructor() {
+                this.run = run;
+              }
+              getUsageInfo() {
+                return {
+                  key,
+                  prompt: `custom "${key.toUpperCase()}" plugin`,
+                };
+              }
+            },
+          {virtual: true},
+        );
+
+        expect(() => {
+          watch(
+            Object.assign({}, globalConfig, {
+              rootDir: __dirname,
+              watchPlugins: [{config: {}, path: pluginPath}],
+            }),
+            contexts,
+            pipe,
+            hasteMapInstances,
+            stdin,
+          );
+        }).toThrowError(
+          new RegExp(
+            `Watch plugin OffendingWatchPlugin attempted to register key <${key}>,\\s+that is reserved internally for .+\\.\\s+Please change the configuration key for this plugin\\.`,
+            'm',
+          ),
+        );
+      },
+    );
+
+    // The jury's still out on 'a', 'c', 'f', 'o', 'w' and '?'…
+    // See https://github.com/facebook/jest/issues/6693
+    it.each`
+      key    | plugin
+      ${'t'} | ${'TestNamePattern'}
+      ${'p'} | ${'TestPathPattern'}
+    `(
+      'allows WatchPlugins to override non-reserved internal plugins',
+      ({key, plugin}) => {
+        const run = jest.fn(() => Promise.resolve());
+        const pluginPath = `${__dirname}/__fixtures__/plugin_valid_override_${key}`;
+        jest.doMock(
+          pluginPath,
+          () =>
+            class ValidWatchPlugin {
+              constructor() {
+                this.run = run;
+              }
+              getUsageInfo() {
+                return {
+                  key,
+                  prompt: `custom "${key.toUpperCase()}" plugin`,
+                };
+              }
+            },
+          {virtual: true},
+        );
+
+        watch(
+          Object.assign({}, globalConfig, {
+            rootDir: __dirname,
+            watchPlugins: [{config: {}, path: pluginPath}],
+          }),
+          contexts,
+          pipe,
+          hasteMapInstances,
+          stdin,
+        );
+      },
+    );
+
+    it('forbids third-party WatchPlugins overriding each other', () => {
+      const pluginPaths = ['Foo', 'Bar'].map(ident => {
+        const run = jest.fn(() => Promise.resolve());
+        const pluginPath = `${__dirname}/__fixtures__/plugin_bad_override_${ident.toLowerCase()}`;
+        jest.doMock(
+          pluginPath,
+          () => {
+            class OffendingThirdPartyWatchPlugin {
+              constructor() {
+                this.run = run;
+              }
+              getUsageInfo() {
+                return {
+                  key: '!',
+                  prompt: `custom "!" plugin ${ident}`,
+                };
+              }
+            }
+            OffendingThirdPartyWatchPlugin.displayName = `Offending${ident}ThirdPartyWatchPlugin`;
+            return OffendingThirdPartyWatchPlugin;
+          },
+          {virtual: true},
+        );
+        return pluginPath;
+      });
+
+      expect(() => {
+        watch(
+          Object.assign({}, globalConfig, {
+            rootDir: __dirname,
+            watchPlugins: pluginPaths.map(path => ({config: {}, path})),
+          }),
+          contexts,
+          pipe,
+          hasteMapInstances,
+          stdin,
+        );
+      }).toThrowError(
+        /Watch plugins OffendingFooThirdPartyWatchPlugin and OffendingBarThirdPartyWatchPlugin both attempted to register key <!>\.\s+Please change the key configuration for one of the conflicting plugins to avoid overlap\./m,
+      );
+    });
+  });
+
   it('allows WatchPlugins to be configured', async () => {
     const pluginPath = `${__dirname}/__fixtures__/plugin_path_with_config`;
     jest.doMock(

diff --git a/packages/jest-cli/src/watch.js b/packages/jest-cli/src/watch.js
@@ -37,6 +37,7 @@ import {
   getSortedUsageRows,
   filterInteractivePlugins,
 } from './lib/watch_plugins_helpers';
+import {ValidationError} from 'jest-validate';
 import activeFilters from './lib/active_filters_message';
 
 let hasExitListener = false;
@@ -49,6 +50,18 @@ const INTERNAL_PLUGINS = [
   QuitPlugin,
 ];
 
+const RESERVED_KEY_PLUGINS = new Map([
+  [
+    UpdateSnapshotsPlugin,
+    {forbiddenOverwriteMessage: 'updating snapshots', key: 'u'},
+  ],
+  [
+    UpdateSnapshotsInteractivePlugin,
+    {forbiddenOverwriteMessage: 'updating snapshots interactively', key: 'i'},
+  ],
+  [QuitPlugin, {forbiddenOverwriteMessage: 'quitting watch mode'}],
+]);
+
 export default function watch(
   initialGlobalConfig: GlobalConfig,
   contexts: Array<Context>,
@@ -122,6 +135,21 @@ export default function watch(
   });
 
   if (globalConfig.watchPlugins != null) {
+    const watchPluginKeys = new Map();
+    for (const plugin of watchPlugins) {
+      const reservedInfo = RESERVED_KEY_PLUGINS.get(plugin.constructor) || {};
+      const key = reservedInfo.key || getPluginKey(plugin, globalConfig);
+      if (!key) {
+        continue;
+      }
+      const {forbiddenOverwriteMessage} = reservedInfo;
+      watchPluginKeys.set(key, {
+        forbiddenOverwriteMessage,
+        overwritable: forbiddenOverwriteMessage == null,
+        plugin,
+      });
+    }
+
     for (const pluginWithConfig of globalConfig.watchPlugins) {
       // $FlowFixMe dynamic require
       const ThirdPartyPlugin = require(pluginWithConfig.path);
@@ -130,6 +158,8 @@ export default function watch(
         stdin,
         stdout: outputStream,
       });
+      checkForConflicts(watchPluginKeys, plugin, globalConfig);
+
       const hookSubscriber = hooks.getSubscriber();
       if (plugin.apply) {
         plugin.apply(hookSubscriber);
@@ -286,11 +316,7 @@ export default function watch(
     const matchingWatchPlugin = filterInteractivePlugins(
       watchPlugins,
       globalConfig,
-    ).find(plugin => {
-      const usageData =
-        (plugin.getUsageInfo && plugin.getUsageInfo(globalConfig)) || {};
-      return usageData.key === key;
-    });
+    ).find(plugin => getPluginKey(plugin, globalConfig) === key);
 
     if (matchingWatchPlugin != null) {
       // "activate" the plugin, which has jest ignore keystrokes so the plugin
@@ -379,6 +405,61 @@ export default function watch(
   return Promise.resolve();
 }
 
+const checkForConflicts = (watchPluginKeys, plugin, globalConfig) => {
+  const key = getPluginKey(plugin, globalConfig);
+  if (!key) {
+    return;
+  }
+
+  const conflictor = watchPluginKeys.get(key);
+  if (!conflictor || conflictor.overwritable) {
+    watchPluginKeys.set(key, {
+      overwritable: false,
+      plugin,
+    });
+    return;
+  }
+
+  let error;
+  if (conflictor.forbiddenOverwriteMessage) {
+    error = `
+  Watch plugin ${chalk.bold.red(
+    getPluginIdentifier(plugin),
+  )} attempted to register key ${chalk.bold.red(`<${key}>`)},
+  that is reserved internally for ${chalk.bold.red(
+    conflictor.forbiddenOverwriteMessage,
+  )}.
+  Please change the configuration key for this plugin.`.trim();
+  } else {
+    const plugins = [conflictor.plugin, plugin]
+      .map(p => chalk.bold.red(getPluginIdentifier(p)))
+      .join(' and ');
+    error = `
+  Watch plugins ${plugins} both attempted to register key ${chalk.bold.red(
+      `<${key}>`,
+    )}.
+  Please change the key configuration for one of the conflicting plugins to avoid overlap.`.trim();
+  }
+
+  throw new ValidationError('Watch plugin configuration error', error);
+};
+
+const getPluginIdentifier = plugin =>
+  // This breaks as `displayName` is not defined as a static, but since
+  // WatchPlugin is an interface, and it is my understanding interface
+  // static fields are not definable anymore, no idea how to circumvent
+  // this :-(
+  // $FlowFixMe: leave `displayName` be.
+  plugin.constructor.displayName || plugin.constructor.name;
+
+const getPluginKey = (plugin, globalConfig) => {
+  if (typeof plugin.getUsageInfo === 'function') {
+    return (plugin.getUsageInfo(globalConfig) || {}).key;
+  }
+
+  return null;
+};
+
 const usage = (
   globalConfig,
   watchPlugins: Array<WatchPlugin>,