From 24db067cbab07e1bdee491d41f1b47d23e309ff4 Mon Sep 17 00:00:00 2001 From: Sean Poulter Date: Wed, 17 Jan 2018 20:30:37 -0500 Subject: [PATCH 1/6] Add tests for Process.js --- packages/jest-editor-support/src/Process.js | 5 +- .../src/__tests__/process.test.js | 119 ++++++++++++++++++ 2 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 packages/jest-editor-support/src/__tests__/process.test.js diff --git a/packages/jest-editor-support/src/Process.js b/packages/jest-editor-support/src/Process.js index b3b0986726d2..0812300ada74 100644 --- a/packages/jest-editor-support/src/Process.js +++ b/packages/jest-editor-support/src/Process.js @@ -31,10 +31,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 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..7f32e4ad7421 --- /dev/null +++ b/packages/jest-editor-support/src/__tests__/process.test.js @@ -0,0 +1,119 @@ +/** + * 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(); + }); +}); From 90b4c1ed9e4afb4413598023acd2bee8940ffb62 Mon Sep 17 00:00:00 2001 From: Sean Poulter Date: Wed, 17 Jan 2018 20:31:55 -0500 Subject: [PATCH 2/6] Add option to spawn command inside a shell --- packages/jest-editor-support/src/Process.js | 10 ++++++++-- .../jest-editor-support/src/__tests__/process.test.js | 10 ++++++++++ packages/jest-editor-support/src/types.js | 4 ++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/jest-editor-support/src/Process.js b/packages/jest-editor-support/src/Process.js index 0812300ada74..296ccfba896d 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 @@ -41,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, + ...options, + }; + return spawn(command, runtimeArgs, spawnOptions); }; diff --git a/packages/jest-editor-support/src/__tests__/process.test.js b/packages/jest-editor-support/src/__tests__/process.test.js index 7f32e4ad7421..117cbf2d0b0e 100644 --- a/packages/jest-editor-support/src/__tests__/process.test.js +++ b/packages/jest-editor-support/src/__tests__/process.test.js @@ -116,4 +116,14 @@ describe('createProcess', () => { 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/types.js b/packages/jest-editor-support/src/types.js index 2eabcfa984fd..030a81e7daed 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'; From 43428f8b42aae3234b85955bac95912cd48dd487 Mon Sep 17 00:00:00 2001 From: Sean Poulter Date: Wed, 17 Jan 2018 20:35:44 -0500 Subject: [PATCH 3/6] Add tests for constructor, start, and closeProcess --- .../src/__tests__/runner.test.js | 259 ++++++++++++++++-- 1 file changed, 229 insertions(+), 30 deletions(-) diff --git a/packages/jest-editor-support/src/__tests__/runner.test.js b/packages/jest-editor-support/src/__tests__/runner.test.js index c8b9c0de7908..12c03893261e 100644 --- a/packages/jest-editor-support/src/__tests__/runner.test.js +++ b/packages/jest-editor-support/src/__tests__/runner.test.js @@ -9,15 +9,12 @@ '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'; - +jest.mock('../Process'); +jest.mock('child_process', () => ({spawn: jest.fn()})); +jest.mock('os', () => ({tmpdir: jest.fn()})); +jest.mock('fs', () => { + const readFileSync = require.requireActual('fs').readFileSync; // Replace `readFile` with `readFileSync` so we don't get multiple threads -jest.doMock('fs', () => { return { readFile: (path, type, closure) => { const data = readFileSync(path); @@ -27,23 +24,234 @@ 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'; + +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); + }); + }); + + 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(); }); -const Runner = require('../Runner').default; + 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 +259,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 +294,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")', () => { From b134dcf4f87fc2f97744fe32aefb36e8a8a297c4 Mon Sep 17 00:00:00 2001 From: Sean Poulter Date: Wed, 17 Jan 2018 21:32:47 -0500 Subject: [PATCH 4/6] Add option to spawn command in a shell --- packages/jest-editor-support/src/Runner.js | 20 +++++++++++++------ .../src/__tests__/runner.test.js | 13 ++++++++++-- packages/jest-editor-support/src/types.js | 2 ++ 3 files changed, 27 insertions(+), 8 deletions(-) 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__/runner.test.js b/packages/jest-editor-support/src/__tests__/runner.test.js index 12c03893261e..576c93b4e0a3 100644 --- a/packages/jest-editor-support/src/__tests__/runner.test.js +++ b/packages/jest-editor-support/src/__tests__/runner.test.js @@ -14,7 +14,7 @@ jest.mock('child_process', () => ({spawn: jest.fn()})); jest.mock('os', () => ({tmpdir: jest.fn()})); jest.mock('fs', () => { const readFileSync = require.requireActual('fs').readFileSync; -// Replace `readFile` with `readFileSync` so we don't get multiple threads + // Replace `readFile` with `readFileSync` so we don't get multiple threads return { readFile: (path, type, closure) => { const data = readFileSync(path); @@ -182,6 +182,15 @@ describe('Runner', () => { 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', () => { @@ -226,7 +235,7 @@ describe('Runner', () => { sut.closeProcess(); expect(kill).toBeCalledWith(); -}); + }); it('clears the debugprocess property', () => { const workspace: any = {}; diff --git a/packages/jest-editor-support/src/types.js b/packages/jest-editor-support/src/types.js index 030a81e7daed..22debbe9296d 100644 --- a/packages/jest-editor-support/src/types.js +++ b/packages/jest-editor-support/src/types.js @@ -23,9 +23,11 @@ export type Options = { createProcess?: ( workspace: ProjectWorkspace, args: Array, + options?: SpawnOptions, ) => ChildProcess, testNamePattern?: string, testFileNamePattern?: string, + shell?: boolean, }; /** From affe7bca21cac16430fef93c1492aac26bf1c9b7 Mon Sep 17 00:00:00 2001 From: Sean Poulter Date: Wed, 17 Jan 2018 21:41:28 -0500 Subject: [PATCH 5/6] Add #5340 to change log --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) 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 From f557bce4eb5fa788814acd48da74a2190f5fcdf0 Mon Sep 17 00:00:00 2001 From: Sean Poulter Date: Wed, 17 Jan 2018 23:13:04 -0500 Subject: [PATCH 6/6] Fix build errors --- packages/jest-editor-support/src/Process.js | 2 +- packages/jest-editor-support/src/__tests__/runner.test.js | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/jest-editor-support/src/Process.js b/packages/jest-editor-support/src/Process.js index 296ccfba896d..b0af41d1d0a3 100644 --- a/packages/jest-editor-support/src/Process.js +++ b/packages/jest-editor-support/src/Process.js @@ -45,7 +45,7 @@ export const createProcess = ( const spawnOptions = { cwd: workspace.rootPath, env, - ...options, + shell: options.shell, }; return spawn(command, runtimeArgs, spawnOptions); }; diff --git a/packages/jest-editor-support/src/__tests__/runner.test.js b/packages/jest-editor-support/src/__tests__/runner.test.js index 576c93b4e0a3..8fd651296338 100644 --- a/packages/jest-editor-support/src/__tests__/runner.test.js +++ b/packages/jest-editor-support/src/__tests__/runner.test.js @@ -13,7 +13,9 @@ 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) => {