From 06bbe2dc51b429b74066d66d16f740ae5cf38d6d Mon Sep 17 00:00:00 2001 From: Sean Poulter Date: Thu, 18 Jan 2018 04:57:07 -0500 Subject: [PATCH] Add an option to spawn command in a shell (#5340) * Add tests for Process.js * Add option to spawn command inside a shell * Add tests for constructor, start, and closeProcess * Add option to spawn command in a shell * Add #5340 to change log * Fix build errors --- CHANGELOG.md | 2 + packages/jest-editor-support/src/Process.js | 15 +- packages/jest-editor-support/src/Runner.js | 20 +- .../src/__tests__/process.test.js | 129 +++++++++ .../src/__tests__/runner.test.js | 274 ++++++++++++++++-- packages/jest-editor-support/src/types.js | 6 + 6 files changed, 403 insertions(+), 43 deletions(-) create mode 100644 packages/jest-editor-support/src/__tests__/process.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index fbeddcd66f0b..8088ce9eb0e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## master +* `[jest-editor-support]` Add option to spawn command in shell ([#5340](https://github.com/facebook/jest/pull/5340)) + ## jest 22.1.2 ### Fixes diff --git a/packages/jest-editor-support/src/Process.js b/packages/jest-editor-support/src/Process.js index b3b0986726d2..b0af41d1d0a3 100644 --- a/packages/jest-editor-support/src/Process.js +++ b/packages/jest-editor-support/src/Process.js @@ -8,8 +8,8 @@ */ import {ChildProcess, spawn} from 'child_process'; - import ProjectWorkspace from './project_workspace'; +import type {SpawnOptions} from './types'; /** * Spawns and returns a Jest process with specific args @@ -20,6 +20,7 @@ import ProjectWorkspace from './project_workspace'; export const createProcess = ( workspace: ProjectWorkspace, args: Array, + options?: SpawnOptions = {}, ): ChildProcess => { // A command could look like `npm run test`, which we cannot use as a command // as they can only be the first command, so take out the command, and add @@ -31,10 +32,9 @@ export const createProcess = ( const runtimeArgs = [].concat(initialArgs, args); // If a path to configuration file was defined, push it to runtimeArgs - const configPath = workspace.pathToConfig; - if (configPath !== '') { + if (workspace.pathToConfig) { runtimeArgs.push('--config'); - runtimeArgs.push(configPath); + runtimeArgs.push(workspace.pathToConfig); } // To use our own commands in create-react, we need to tell the command that @@ -42,5 +42,10 @@ export const createProcess = ( const env = process.env; env['CI'] = 'true'; - return spawn(command, runtimeArgs, {cwd: workspace.rootPath, env}); + const spawnOptions = { + cwd: workspace.rootPath, + env, + shell: options.shell, + }; + return spawn(command, runtimeArgs, spawnOptions); }; diff --git a/packages/jest-editor-support/src/Runner.js b/packages/jest-editor-support/src/Runner.js index eb6bc54efff3..7f3e005a8a5e 100644 --- a/packages/jest-editor-support/src/Runner.js +++ b/packages/jest-editor-support/src/Runner.js @@ -7,7 +7,7 @@ * @flow */ -import type {Options, MessageType} from './types'; +import type {Options, MessageType, SpawnOptions} from './types'; import {messageTypes} from './types'; import {ChildProcess, spawn} from 'child_process'; @@ -27,6 +27,7 @@ export default class Runner extends EventEmitter { _createProcess: ( workspace: ProjectWorkspace, args: Array, + options?: SpawnOptions, ) => ChildProcess; watchMode: boolean; options: Options; @@ -64,7 +65,11 @@ export default class Runner extends EventEmitter { args.push(this.options.testFileNamePattern); } - this.debugprocess = this._createProcess(this.workspace, args); + const options = { + shell: this.options.shell, + }; + + this.debugprocess = this._createProcess(this.workspace, args, options); this.debugprocess.stdout.on('data', (data: Buffer) => { // Make jest save to a file, otherwise we get chunked data // and it can be hard to put it back together. @@ -118,10 +123,13 @@ export default class Runner extends EventEmitter { runJestWithUpdateForSnapshots(completion: any, args: string[]) { const defaultArgs = ['--updateSnapshot']; - const updateProcess = this._createProcess(this.workspace, [ - ...defaultArgs, - ...(args ? args : []), - ]); + + const options = {shell: this.options.shell}; + const updateProcess = this._createProcess( + this.workspace, + [...defaultArgs, ...(args ? args : [])], + options, + ); updateProcess.on('close', () => { completion(); }); diff --git a/packages/jest-editor-support/src/__tests__/process.test.js b/packages/jest-editor-support/src/__tests__/process.test.js new file mode 100644 index 000000000000..117cbf2d0b0e --- /dev/null +++ b/packages/jest-editor-support/src/__tests__/process.test.js @@ -0,0 +1,129 @@ +/** + * Copyright (c) 2014-present, Facebook, Inc. All rights reserved. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +'use strict'; + +jest.mock('child_process'); + +import {createProcess} from '../Process'; +import {spawn} from 'child_process'; + +describe('createProcess', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('spawns the process', () => { + const workspace: any = {pathToJest: ''}; + const args = []; + createProcess(workspace, args); + + expect(spawn).toBeCalled(); + }); + + it('spawns the command from workspace.pathToJest', () => { + const workspace: any = {pathToJest: 'jest'}; + const args = []; + createProcess(workspace, args); + + expect(spawn.mock.calls[0][0]).toBe('jest'); + expect(spawn.mock.calls[0][1]).toEqual([]); + }); + + it('spawns the first arg from workspace.pathToJest split on " "', () => { + const workspace: any = {pathToJest: 'npm test --'}; + const args = []; + createProcess(workspace, args); + + expect(spawn.mock.calls[0][0]).toBe('npm'); + expect(spawn.mock.calls[0][1]).toEqual(['test', '--']); + }); + + it('fails to spawn the first quoted arg from workspace.pathToJest', () => { + const workspace: any = { + pathToJest: + '"../build scripts/test" --coverageDirectory="../code coverage"', + }; + const args = []; + createProcess(workspace, args); + + expect(spawn.mock.calls[0][0]).not.toBe('"../build scripts/test"'); + expect(spawn.mock.calls[0][1]).not.toEqual([ + '--coverageDirectory="../code coverage"', + ]); + }); + + it('appends args', () => { + const workspace: any = {pathToJest: 'npm test --'}; + const args = ['--option', 'value', '--another']; + createProcess(workspace, args); + + expect(spawn.mock.calls[0][1]).toEqual(['test', '--', ...args]); + }); + + it('sets the --config arg to workspace.pathToConfig', () => { + const workspace: any = { + pathToConfig: 'non-standard.jest.js', + pathToJest: 'npm test --', + }; + const args = ['--option', 'value']; + createProcess(workspace, args); + + expect(spawn.mock.calls[0][1]).toEqual([ + 'test', + '--', + '--option', + 'value', + '--config', + 'non-standard.jest.js', + ]); + }); + + it('defines the "CI" environment variable', () => { + const expected = { + ...process.env, + CI: 'true', + }; + + const workspace: any = {pathToJest: ''}; + const args = []; + createProcess(workspace, args); + + expect(spawn.mock.calls[0][2].env).toEqual(expected); + }); + + it('sets the current working directory of the child process', () => { + const workspace: any = { + pathToJest: '', + rootPath: 'root directory', + }; + const args = []; + createProcess(workspace, args); + + expect(spawn.mock.calls[0][2].cwd).toBe(workspace.rootPath); + }); + + it('should not set the "shell" property when "options" are not provided', () => { + const workspace: any = {pathToJest: ''}; + const args = []; + createProcess(workspace, args); + + expect(spawn.mock.calls[0][2].shell).not.toBeDefined(); + }); + + it('should set the "shell" property when "options" are provided', () => { + const expected = {}; + const workspace: any = {pathToJest: ''}; + const args = []; + const options: any = {shell: expected}; + createProcess(workspace, args, options); + + expect(spawn.mock.calls[0][2].shell).toBe(expected); + }); +}); diff --git a/packages/jest-editor-support/src/__tests__/runner.test.js b/packages/jest-editor-support/src/__tests__/runner.test.js index c8b9c0de7908..8fd651296338 100644 --- a/packages/jest-editor-support/src/__tests__/runner.test.js +++ b/packages/jest-editor-support/src/__tests__/runner.test.js @@ -9,15 +9,14 @@ 'use strict'; -const {EventEmitter} = require('events'); -const path = require('path'); -const {readFileSync} = require('fs'); -const fixtures = path.resolve(__dirname, '../../../../fixtures'); -import ProjectWorkspace from '../project_workspace'; -import {messageTypes} from '../types'; - -// Replace `readFile` with `readFileSync` so we don't get multiple threads -jest.doMock('fs', () => { +jest.mock('../Process'); +jest.mock('child_process', () => ({spawn: jest.fn()})); +jest.mock('os', () => ({tmpdir: jest.fn()})); +jest.mock('fs', () => { + // $FlowFixMe requireActual + const readFileSync = require.requireActual('fs').readFileSync; + + // Replace `readFile` with `readFileSync` so we don't get multiple threads return { readFile: (path, type, closure) => { const data = readFileSync(path); @@ -27,23 +26,243 @@ jest.doMock('fs', () => { }; }); -// Let's us use a per-test "jest process" -let mockDebugProcess = {}; -const mockCreateProcess = jest.fn(() => mockDebugProcess); -jest.doMock('../Process.js', () => { - return { - createProcess: mockCreateProcess, - }; -}); +const path = require('path'); +const fixtures = path.resolve(__dirname, '../../../../fixtures'); +import ProjectWorkspace from '../project_workspace'; +import {messageTypes} from '../types'; -const Runner = require('../Runner').default; +import {default as Runner} from '../Runner'; +import {createProcess} from '../Process'; +import {tmpdir} from 'os'; +import {spawn} from 'child_process'; +import {readFileSync} from 'fs'; +import EventEmitter from 'events'; +import type {ChildProcess} from 'child_process'; + +describe('Runner', () => { + describe('constructor', () => { + it('does not set watchMode', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + + expect(sut.watchMode).not.toBeDefined(); + }); + + it('sets the output filepath', () => { + tmpdir.mockReturnValueOnce('tmpdir'); + + const workspace: any = {}; + const sut = new Runner(workspace); + + expect(sut.outputPath).toBe('tmpdir/jest_runner.json'); + }); + + it('sets the default options', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + + expect(sut.options).toEqual({}); + }); + + it('sets the options', () => { + const workspace: any = {}; + const options = {}; + const sut = new Runner(workspace, options); + + expect(sut.options).toBe(options); + }); + }); + + describe('start', () => { + beforeEach(() => { + jest.resetAllMocks(); + + (createProcess: any).mockImplementationOnce( + (workspace, args, options) => { + const process: any = new EventEmitter(); + process.stdout = new EventEmitter(); + process.stderr = new EventEmitter(); + return process; + }, + ); + }); + + it('will not start when started', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + + sut.start(); + sut.start(); + + expect(createProcess).toHaveBeenCalledTimes(1); + }); + + it('sets watchMode', () => { + const expected = true; + + const workspace: any = {}; + const sut = new Runner(workspace); + sut.start(expected); + + expect(sut.watchMode).toBe(expected); + }); + + it('calls createProcess', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + sut.start(false); + + expect((createProcess: any).mock.calls[0][0]).toBe(workspace); + }); + + it('calls createProcess with the --json arg', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + sut.start(false); + + expect((createProcess: any).mock.calls[0][1]).toContain('--json'); + }); + + it('calls createProcess with the --useStderr arg', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + sut.start(false); + + expect((createProcess: any).mock.calls[0][1]).toContain('--useStderr'); + }); + + it('calls createProcess with the --jsonOutputFile arg for Jest 17 and below', () => { + const workspace: any = {localJestMajorVersion: 17}; + const sut = new Runner(workspace); + sut.start(false); + + const args = (createProcess: any).mock.calls[0][1]; + const index = args.indexOf('--jsonOutputFile'); + expect(index).not.toBe(-1); + expect(args[index + 1]).toBe(sut.outputPath); + }); + + it('calls createProcess with the --outputFile arg for Jest 18 and above', () => { + const workspace: any = {localJestMajorVersion: 18}; + const sut = new Runner(workspace); + sut.start(false); + + const args = (createProcess: any).mock.calls[0][1]; + const index = args.indexOf('--outputFile'); + expect(index).not.toBe(-1); + expect(args[index + 1]).toBe(sut.outputPath); + }); + + it('calls createProcess with the --watch arg when provided', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + sut.start(true); + + expect((createProcess: any).mock.calls[0][1]).toContain('--watch'); + }); + + it('calls createProcess with the --testNamePattern arg when provided', () => { + const expected = 'testNamePattern'; + + const workspace: any = {}; + const options = {testNamePattern: expected}; + const sut = new Runner(workspace, options); + sut.start(false); + + const args = (createProcess: any).mock.calls[0][1]; + const index = args.indexOf('--testNamePattern'); + expect(index).not.toBe(-1); + expect(args[index + 1]).toBe(expected); + }); + + it('calls createProcess with a test path pattern when provided', () => { + const expected = 'testPathPattern'; + const workspace: any = {}; + const options = {testFileNamePattern: expected}; + const sut = new Runner(workspace, options); + sut.start(false); + + expect((createProcess: any).mock.calls[0][1]).toContain(expected); + }); + + it('calls createProcess with the shell option when provided', () => { + const workspace: any = {}; + const options = {shell: true}; + const sut = new Runner(workspace, options); + sut.start(false); + + expect((createProcess: any).mock.calls[0][2]).toEqual({shell: true}); + }); + }); + + describe('closeProcess', () => { + let platformPV; + + beforeEach(() => { + jest.resetAllMocks(); + platformPV = process.platform; + + // Remove the "process.platform" property descriptor so it can be writable. + delete process.platform; + }); + + afterEach(() => { + process.platform = platformPV; + }); + + it('does nothing if the runner has not started', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + sut.closeProcess(); + + expect(spawn).not.toBeCalled(); + }); + + it('spawns taskkill to close the process on Windows', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + process.platform = 'win32'; + sut.debugprocess = ({pid: 123}: any); + sut.closeProcess(); + + expect(spawn).toBeCalledWith('taskkill', ['/pid', '123', '/T', '/F']); + }); + + it('calls kill() to close the process on POSIX', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + process.platform = 'posix'; + const kill = jest.fn(); + sut.debugprocess = ({kill}: any); + sut.closeProcess(); + + expect(kill).toBeCalledWith(); + }); + + it('clears the debugprocess property', () => { + const workspace: any = {}; + const sut = new Runner(workspace); + sut.debugprocess = ({kill: () => {}}: any); + sut.closeProcess(); + + expect(sut.debugprocess).not.toBeDefined(); + }); + }); +}); describe('events', () => { - let runner; - let fakeProcess; + let runner: Runner; + let fakeProcess: ChildProcess; beforeEach(() => { - mockCreateProcess.mockClear(); + jest.resetAllMocks(); + + fakeProcess = (new EventEmitter(): any); + fakeProcess.stdout = new EventEmitter(); + fakeProcess.stderr = new EventEmitter(); + + (createProcess: any).mockImplementation(() => fakeProcess); + const workspace = new ProjectWorkspace( '.', 'node_modules/.bin/jest', @@ -51,11 +270,6 @@ describe('events', () => { 18, ); runner = new Runner(workspace); - fakeProcess = (new EventEmitter(): any); - fakeProcess.stdout = new EventEmitter(); - fakeProcess.stderr = new EventEmitter(); - mockDebugProcess = fakeProcess; - mockDebugProcess.kill = jest.fn(); // Sets it up and registers for notifications runner.start(); @@ -91,15 +305,11 @@ describe('events', () => { expect(close).toBeCalled(); }); - it('should only start one jest process at a time', () => { - runner.start(); - expect(mockCreateProcess).toHaveBeenCalledTimes(1); - }); - it('should start jest process after killing the old process', () => { runner.closeProcess(); runner.start(); - expect(mockCreateProcess).toHaveBeenCalledTimes(2); + + expect(createProcess).toHaveBeenCalledTimes(2); }); describe('stdout.on("data")', () => { diff --git a/packages/jest-editor-support/src/types.js b/packages/jest-editor-support/src/types.js index 2eabcfa984fd..22debbe9296d 100644 --- a/packages/jest-editor-support/src/types.js +++ b/packages/jest-editor-support/src/types.js @@ -12,6 +12,10 @@ export type Location = { line: number, }; +export type SpawnOptions = { + shell?: boolean, +}; + import type {ChildProcess} from 'child_process'; import type ProjectWorkspace from './project_workspace'; @@ -19,9 +23,11 @@ export type Options = { createProcess?: ( workspace: ProjectWorkspace, args: Array, + options?: SpawnOptions, ) => ChildProcess, testNamePattern?: string, testFileNamePattern?: string, + shell?: boolean, }; /**