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 jestjs#6693.
Refs jestjs#6473.

Author: tdd

CHANGELOG.md

@@ -20,6 +20,10 @@
 
 - `[jest-changed-files]` limit git and hg commands to specified roots ([#6732](https://github.com/facebook/jest/pull/6732))
 
+### Features
+
+- `[jest-cli]` Watch plugin key registrations are now checked for conflicts. Some built-in keys, such as `p` and `t` for test limitation, are overridable, but most built-in keys are reserved (e.g. `q`, `a` or `o`), and any key registered by a third-party plugin cannot be overriden by another third-party plugin. Explicit error messages are logged to the console and Jest exits with code 64 (`EX_USAGE`).
+
 ### Fixes
 
 - `[jest-circus]` Fix retryTimes so errors are reset before re-running ([#6762](https://github.com/facebook/jest/pull/6762))

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.
 ",
   ],

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

@@ -12,6 +12,7 @@ import chalk from 'chalk';
 import TestWatcher from '../TestWatcher';
 import {JestHook, KEYS} from 'jest-watcher';
 
+const exitMock = jest.fn();
 const runJestMock = jest.fn();
 const watchPluginPath = `${__dirname}/__fixtures__/watch_plugin`;
 const watchPlugin2Path = `${__dirname}/__fixtures__/watch_plugin2`;
@@ -43,6 +44,7 @@ jest.mock(
 );
 
 jest.doMock('chalk', () => new chalk.constructor({enabled: false}));
+jest.doMock('exit', () => exitMock);
 jest.doMock(
   '../runJest',
   () =>
@@ -78,7 +80,7 @@ jest.doMock(
     class WatchPlugin2 {
       getUsageInfo() {
         return {
-          key: 'u',
+          key: 'r',
           prompt: 'do something else',
         };
       }
@@ -96,6 +98,7 @@ const watch = require('../watch').default;
 const nextTick = () => new Promise(res => process.nextTick(res));
 
 afterEach(runJestMock.mockReset);
+afterEach(exitMock.mockReset);
 
 describe('Watch mode flows', () => {
   let pipe;
@@ -323,7 +326,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 +367,157 @@ describe('Watch mode flows', () => {
     expect(run).toHaveBeenCalled();
   });
 
+  describe('when dealing with potential watch plugin key conflicts', () => {
+    beforeEach(() => {
+      jest.spyOn(console, 'error');
+      console.error.mockImplementation(() => {});
+    });
+
+    afterEach(() => {
+      console.error.mockRestore();
+    });
+
+    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},
+        );
+
+        watch(
+          Object.assign({}, globalConfig, {
+            rootDir: __dirname,
+            watchPlugins: [{config: {}, path: pluginPath}],
+          }),
+          contexts,
+          pipe,
+          hasteMapInstances,
+          stdin,
+        );
+
+        expect(console.error).toHaveBeenLastCalledWith(
+          expect.stringMatching(
+            new RegExp(
+              `Jest configuration error: watch plugin OffendingWatchPlugin attempted to register key <${key}>, that is reserved internally for .+\\.\\s+Please change the configuration key for this plugin\\.`,
+              'm',
+            ),
+          ),
+        );
+
+        expect(exitMock).toHaveBeenCalled();
+      },
+    );
+
+    // 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,
+        );
+
+        expect(exitMock).not.toHaveBeenCalled();
+      },
+    );
+
+    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;
+      });
+
+      watch(
+        Object.assign({}, globalConfig, {
+          rootDir: __dirname,
+          watchPlugins: pluginPaths.map(path => ({config: {}, path})),
+        }),
+        contexts,
+        pipe,
+        hasteMapInstances,
+        stdin,
+      );
+
+      expect(console.error).toHaveBeenLastCalledWith(
+        expect.stringMatching(
+          /Jest configuration error: 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,
+        ),
+      );
+
+      expect(exitMock).toHaveBeenCalled();
+    });
+  });
+
   it('allows WatchPlugins to be configured', async () => {
     const pluginPath = `${__dirname}/__fixtures__/plugin_path_with_config`;
     jest.doMock(

packages/jest-cli/src/watch.js

@@ -49,6 +49,18 @@ const INTERNAL_PLUGINS = [
   QuitPlugin,
 ];
 
+const RESERVED_KEY_PLUGINS = new Map([
+  [
+    UpdateSnapshotsPlugin,
+    {forbiddenOverwriteMessage: 'updating snapshots ballpark', 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 +134,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 +157,8 @@ export default function watch(
         stdin,
         stdout: outputStream,
       });
+      checkForConflicts(watchPluginKeys, plugin, globalConfig);
+
       const hookSubscriber = hooks.getSubscriber();
       if (plugin.apply) {
         plugin.apply(hookSubscriber);
@@ -286,11 +315,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 +404,55 @@ 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 = `Jest configuration error: watch plugin ${getPluginIdentifier(
+      plugin,
+    )} attempted to register key <${key}>, that is reserved internally for ${
+      conflictor.forbiddenOverwriteMessage
+    }.   Please change the configuration key for this plugin.`;
+  } else {
+    const plugins = [conflictor.plugin, plugin]
+      .map(getPluginIdentifier)
+      .join(' and ');
+    error = `Jest configuration error: watch plugins ${plugins} both attempted to register key <${key}>. Please change the key configuration for one of the conflicting plugins to avoid overlap.`;
+  }
+  console.error('\n\n' + chalk.red(error));
+  exit(64); // EX_USAGE
+};
+
+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>,