From 47368a1a07ec5552c9e143482f9c03ebc0e581ba Mon Sep 17 00:00:00 2001 From: Christophe Porteneuve Date: Sun, 15 Jul 2018 11:15:09 +0200 Subject: [PATCH] =?UTF-8?q?Watch=20plugins=20now=20are=20checked=20for=20k?= =?UTF-8?q?ey=20conflicts=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- CHANGELOG.md | 4 + .../__snapshots__/watch.test.js.snap | 4 +- packages/jest-cli/src/__tests__/watch.test.js | 158 +++++++++++++++++- packages/jest-cli/src/watch.js | 84 +++++++++- 4 files changed, 241 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5687739574bb..73095a61a807 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,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 - `[babel-jest]` Make `getCacheKey()` take into account `createTransformer` options ([#6699](https://github.com/facebook/jest/pull/6699)) 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..e0c9960bf401 100644 --- a/packages/jest-cli/src/__tests__/watch.test.js +++ b/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( diff --git a/packages/jest-cli/src/watch.js b/packages/jest-cli/src/watch.js index c7501a839c3c..00c02d227ba5 100644 --- a/packages/jest-cli/src/watch.js +++ b/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, @@ -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,