Skip to content

Commit

Permalink
Watch plugins now are checked for key conflicts…
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
tdd committed Jul 22, 2018
1 parent a10bc90 commit 47368a1
Show file tree
Hide file tree
Showing 4 changed files with 241 additions and 9 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
",
],
Expand Down
158 changes: 156 additions & 2 deletions packages/jest-cli/src/__tests__/watch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`;
Expand Down Expand Up @@ -43,6 +44,7 @@ jest.mock(
);

jest.doMock('chalk', () => new chalk.constructor({enabled: false}));
jest.doMock('exit', () => exitMock);
jest.doMock(
'../runJest',
() =>
Expand Down Expand Up @@ -78,7 +80,7 @@ jest.doMock(
class WatchPlugin2 {
getUsageInfo() {
return {
key: 'u',
key: 'r',
prompt: 'do something else',
};
}
Expand All @@ -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;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
84 changes: 79 additions & 5 deletions packages/jest-cli/src/watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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>,
Expand Down

0 comments on commit 47368a1

Please sign in to comment.