From f8f966b3001c0c788b0aae7dbf67e5cb2b19b151 Mon Sep 17 00:00:00 2001 From: Christophe Porteneuve Date: Thu, 9 Aug 2018 12:12:11 +0200 Subject: [PATCH] Check watch plugins for key conflicts (#6697) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. --- CHANGELOG.md | 1 + docs/WatchPlugins.md | 27 ++++ .../__snapshots__/watch.test.js.snap | 4 +- packages/jest-cli/src/__tests__/watch.test.js | 136 +++++++++++++++++- packages/jest-cli/src/watch.js | 91 +++++++++++- 5 files changed, 250 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7dfa0a36eee..a28b710420af 100644 --- 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 index 3ed58effc606..695b35bcc476 100644 --- 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 , 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 . 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 index 88ce3867a970..85d1828379c5 100644 --- 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 index fb601380a558..f37c0172df7c 100644 --- 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 index c7501a839c3c..90e9f1212f83 100644 --- 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, @@ -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,