From 319584eca4d4495d602efd1eec1318e63a8655de Mon Sep 17 00:00:00 2001 From: Zack McCartney Date: Tue, 4 May 2021 15:10:08 -0400 Subject: [PATCH 1/4] Prevent bin process from crashing on sigint when sent during npm init dialog --- lib/commands/new.js | 13 +++++++++++ test/index.js | 53 +++++++++++++++++++++++++++++++++++++++++++++ test/run-util.js | 10 +++++++-- 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/lib/commands/new.js b/lib/commands/new.js index a49c173..bf8a1e8 100644 --- a/lib/commands/new.js +++ b/lib/commands/new.js @@ -59,7 +59,16 @@ module.exports = async ({ cwd, dir, ctx }) => { Helpers.writeFile(Path.join(dir, 'README.md'), '') ]); + + const ignore = () => { + + /* $lab:coverage:off$ */ + ctx.options.out.write('\n'); + /* $lab:coverage:on$ */ + }; + try { + process.on('SIGINT', ignore); await internals.npmInit(dir, ctx); } catch (ignoreErr) { @@ -69,6 +78,9 @@ module.exports = async ({ cwd, dir, ctx }) => { ) + '\n' ); } + finally { + process.removeListener('SIGINT', ignore); + } let finalPkg = await Helpers.readFile(Path.join(dir, 'package.json')); @@ -156,6 +168,7 @@ internals.npmInit = (cwd, ctx) => { subproc.once('close', (code) => { + // code is null on Ctrl-C (SIGINT) if (code !== 0) { return reject(new Error(`Failed with code: ${code}`)); } diff --git a/test/index.js b/test/index.js index f5ad79e..e35f016 100644 --- a/test/index.js +++ b/test/index.js @@ -825,6 +825,59 @@ describe('hpal', () => { expect(`${logError}`).to.contain('your current branch \'master\' does not have any commits'); }); + it('creates a new pal project when bailing on `npm init` by ^C press (SIGINT).', { timeout: 5000 }, async (flags) => { + + flags.onCleanup = async () => await rimraf('new/sigint-on-npm-init'); + + const result = await RunUtil.bin(['new', 'closet/new/sigint-on-npm-init'], null, true); + + expect(result.output).to.contain('New pal project created in closet/new/sigint-on-npm-init'); + expect(result.errorOutput).to.contain('Bailed on `npm init`, but continuing to setup your project with an incomplete package.json file.'); + + const results = await Promise.all([ + read('new/sigint-on-npm-init/package.json'), + read('new/sigint-on-npm-init/README.md'), + exists('new/sigint-on-npm-init/lib/index.js'), + exists('new/sigint-on-npm-init/test/index.js'), + exec('git remote', 'new/sigint-on-npm-init'), + exec('git tag', 'new/sigint-on-npm-init'), + exec('git ls-files -m', 'new/sigint-on-npm-init'), + returnError(exec('git log', 'new/sigint-on-npm-init')) + ]); + + const pkgAsString = results[0]; + const pkg = JSON.parse(pkgAsString); + const readme = results[1]; + const readmeH1 = readme.trim().substring(2); + const lib = results[2]; + const test = results[3]; + const remotes = results[4][0].split('\n'); + const tags = results[5][0].split('\n'); + const modifiedFiles = results[6][0].trim(); + const logError = results[7]; + + expect(pkg.name).to.not.exist(); + expect(pkg.version).to.not.exist(); + expect(pkg.dependencies).to.exist(); + expect(pkg.devDependencies).to.exist(); + expect(Object.keys(pkg)).to.equal(bailedPkgKeysOrder); + expect(Object.keys(pkg.dependencies)).to.equal(Object.keys(pkg.dependencies).sort()); + expect(Object.keys(pkg.devDependencies)).to.equal(Object.keys(pkg.devDependencies).sort()); + expect(pkgAsString.endsWith('\n')).to.equal(true); + expect(readmeH1).to.equal('sigint-on-npm-init'); + expect(lib).to.exist(); + expect(test).to.exist(); + expect(remotes).to.contain('pal'); + expect(tags).to.contain('swagger'); + expect(tags).to.contain('custom-swagger'); + expect(tags).to.contain('deployment'); + expect(tags).to.contain('objection'); + expect(tags).to.contain('templated-site'); + expect(tags).to.contain('fancy-templated-site'); + expect(modifiedFiles).to.equal(''); + expect(`${logError}`).to.contain('your current branch \'master\' does not have any commits'); + }); + it('fails in a friendly way when trying to create a project in a non-empty directory.', async () => { const result = await RunUtil.cli(['new', 'project-already-exists'], 'new'); diff --git a/test/run-util.js b/test/run-util.js index 832f8f0..9d53338 100644 --- a/test/run-util.js +++ b/test/run-util.js @@ -6,12 +6,12 @@ const Stream = require('stream'); const DisplayError = require('../lib/display-error'); const Hpal = require('..'); -exports.bin = (argv, cwd) => { +exports.bin = (argv, cwd, bailOnNpmInit) => { return new Promise((resolve, reject) => { const path = Path.join(__dirname, '..', 'bin', 'hpal'); - const cli = ChildProcess.spawn('node', [].concat(path, argv), { cwd: cwd || __dirname }); + const cli = ChildProcess.spawn('node', [].concat(path, argv), { cwd: cwd || __dirname, ...(bailOnNpmInit && { detached: true }) }); let output = ''; let errorOutput = ''; @@ -21,6 +21,12 @@ exports.bin = (argv, cwd) => { output += data; combinedOutput += data; + + if (bailOnNpmInit && ~data.toString().indexOf('Press ^C at any time to quit.')) { + // negative process id kills all processes led by the CLI process (process group id = cli.pid) + // this group includes the npm init child process spawned by the new command + process.kill(-cli.pid, 'SIGINT'); + } }); cli.stderr.on('data', (data) => { From b1a7adce80bbe5cee4f97b9d4a9a8f3c810a5926 Mon Sep 17 00:00:00 2001 From: Zack McCartney Date: Tue, 4 May 2021 15:52:33 -0400 Subject: [PATCH 2/4] Refactor child process mgmt to hopefully work on Windows --- lib/commands/new.js | 30 ++++++++++++++++++------------ test/run-util.js | 6 ++---- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/lib/commands/new.js b/lib/commands/new.js index bf8a1e8..18d83d3 100644 --- a/lib/commands/new.js +++ b/lib/commands/new.js @@ -59,17 +59,12 @@ module.exports = async ({ cwd, dir, ctx }) => { Helpers.writeFile(Path.join(dir, 'README.md'), '') ]); - - const ignore = () => { - - /* $lab:coverage:off$ */ - ctx.options.out.write('\n'); - /* $lab:coverage:on$ */ - }; - + let reject; try { - process.on('SIGINT', ignore); - await internals.npmInit(dir, ctx); + const dialog = internals.npmInit(dir, ctx); + reject = dialog.reject; + process.on('SIGINT', reject); + await dialog.promise; } catch (ignoreErr) { ctx.options.err.write( @@ -79,7 +74,7 @@ module.exports = async ({ cwd, dir, ctx }) => { ); } finally { - process.removeListener('SIGINT', ignore); + process.removeListener('SIGINT', reject); } let finalPkg = await Helpers.readFile(Path.join(dir, 'package.json')); @@ -144,7 +139,8 @@ internals.ensureGitAndNpm = async ({ colors }) => { internals.npmInit = (cwd, ctx) => { - return new Promise((resolve, reject) => { + const npmInitDialog = {}; + npmInitDialog.promise = new Promise((resolve, reject) => { // There is no way to cover this on a single platform /* $lab:coverage:off$ */ @@ -153,6 +149,14 @@ internals.npmInit = (cwd, ctx) => { const subproc = ChildProcess.spawn(npmCmd, ['init'], { cwd }); + /* $lab:coverage:off$ */ + npmInitDialog.reject = () => { + + subproc.kill('SIGINT'); + ctx.options.out.write('\n'); + }; + /* $lab:coverage:on$ */ + subproc.stdout.pipe(ctx.options.out, { end: false }); ctx.options.in.pipe(subproc.stdin); subproc.stderr.pipe(ctx.options.err, { end: false }); @@ -176,4 +180,6 @@ internals.npmInit = (cwd, ctx) => { return resolve(); }); }); + + return npmInitDialog; }; diff --git a/test/run-util.js b/test/run-util.js index 9d53338..788aeb7 100644 --- a/test/run-util.js +++ b/test/run-util.js @@ -11,7 +11,7 @@ exports.bin = (argv, cwd, bailOnNpmInit) => { return new Promise((resolve, reject) => { const path = Path.join(__dirname, '..', 'bin', 'hpal'); - const cli = ChildProcess.spawn('node', [].concat(path, argv), { cwd: cwd || __dirname, ...(bailOnNpmInit && { detached: true }) }); + const cli = ChildProcess.spawn('node', [].concat(path, argv), { cwd: cwd || __dirname }); let output = ''; let errorOutput = ''; @@ -23,9 +23,7 @@ exports.bin = (argv, cwd, bailOnNpmInit) => { combinedOutput += data; if (bailOnNpmInit && ~data.toString().indexOf('Press ^C at any time to quit.')) { - // negative process id kills all processes led by the CLI process (process group id = cli.pid) - // this group includes the npm init child process spawned by the new command - process.kill(-cli.pid, 'SIGINT'); + cli.kill('SIGINT'); } }); From 3cfbd526b5bfa096b49373ef38b20bbeef1e06ba Mon Sep 17 00:00:00 2001 From: Zack McCartney Date: Tue, 4 May 2021 16:47:22 -0400 Subject: [PATCH 3/4] Try windows platform check for npm bail test --- test/run-util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/run-util.js b/test/run-util.js index 788aeb7..f2c45a0 100644 --- a/test/run-util.js +++ b/test/run-util.js @@ -23,7 +23,7 @@ exports.bin = (argv, cwd, bailOnNpmInit) => { combinedOutput += data; if (bailOnNpmInit && ~data.toString().indexOf('Press ^C at any time to quit.')) { - cli.kill('SIGINT'); + cli.kill(process.platform === 'win32' ? undefined : 'SIGINT'); } }); From f0126c5101fd66dfdebba97d63d99db78d4590cb Mon Sep 17 00:00:00 2001 From: Zack McCartney Date: Tue, 4 May 2021 17:10:43 -0400 Subject: [PATCH 4/4] Revert failed process mgmt refactor, skip SIGINT test on Windows --- lib/commands/new.js | 29 +++++++++++------------------ test/index.js | 2 +- test/run-util.js | 6 ++++-- 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/lib/commands/new.js b/lib/commands/new.js index 18d83d3..6b141f8 100644 --- a/lib/commands/new.js +++ b/lib/commands/new.js @@ -59,12 +59,16 @@ module.exports = async ({ cwd, dir, ctx }) => { Helpers.writeFile(Path.join(dir, 'README.md'), '') ]); - let reject; + const ignore = () => { + + /* $lab:coverage:off$ */ + ctx.options.out.write('\n'); + /* $lab:coverage:on$ */ + }; + try { - const dialog = internals.npmInit(dir, ctx); - reject = dialog.reject; - process.on('SIGINT', reject); - await dialog.promise; + process.on('SIGINT', ignore); + await internals.npmInit(dir, ctx); } catch (ignoreErr) { ctx.options.err.write( @@ -74,7 +78,7 @@ module.exports = async ({ cwd, dir, ctx }) => { ); } finally { - process.removeListener('SIGINT', reject); + process.removeListener('SIGINT', ignore); } let finalPkg = await Helpers.readFile(Path.join(dir, 'package.json')); @@ -139,8 +143,7 @@ internals.ensureGitAndNpm = async ({ colors }) => { internals.npmInit = (cwd, ctx) => { - const npmInitDialog = {}; - npmInitDialog.promise = new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { // There is no way to cover this on a single platform /* $lab:coverage:off$ */ @@ -149,14 +152,6 @@ internals.npmInit = (cwd, ctx) => { const subproc = ChildProcess.spawn(npmCmd, ['init'], { cwd }); - /* $lab:coverage:off$ */ - npmInitDialog.reject = () => { - - subproc.kill('SIGINT'); - ctx.options.out.write('\n'); - }; - /* $lab:coverage:on$ */ - subproc.stdout.pipe(ctx.options.out, { end: false }); ctx.options.in.pipe(subproc.stdin); subproc.stderr.pipe(ctx.options.err, { end: false }); @@ -180,6 +175,4 @@ internals.npmInit = (cwd, ctx) => { return resolve(); }); }); - - return npmInitDialog; }; diff --git a/test/index.js b/test/index.js index e35f016..e61795b 100644 --- a/test/index.js +++ b/test/index.js @@ -825,7 +825,7 @@ describe('hpal', () => { expect(`${logError}`).to.contain('your current branch \'master\' does not have any commits'); }); - it('creates a new pal project when bailing on `npm init` by ^C press (SIGINT).', { timeout: 5000 }, async (flags) => { + it('creates a new pal project when bailing on `npm init` by ^C press (SIGINT).', { timeout: 5000, skip: process.platform === 'win32' }, async (flags) => { flags.onCleanup = async () => await rimraf('new/sigint-on-npm-init'); diff --git a/test/run-util.js b/test/run-util.js index f2c45a0..9d53338 100644 --- a/test/run-util.js +++ b/test/run-util.js @@ -11,7 +11,7 @@ exports.bin = (argv, cwd, bailOnNpmInit) => { return new Promise((resolve, reject) => { const path = Path.join(__dirname, '..', 'bin', 'hpal'); - const cli = ChildProcess.spawn('node', [].concat(path, argv), { cwd: cwd || __dirname }); + const cli = ChildProcess.spawn('node', [].concat(path, argv), { cwd: cwd || __dirname, ...(bailOnNpmInit && { detached: true }) }); let output = ''; let errorOutput = ''; @@ -23,7 +23,9 @@ exports.bin = (argv, cwd, bailOnNpmInit) => { combinedOutput += data; if (bailOnNpmInit && ~data.toString().indexOf('Press ^C at any time to quit.')) { - cli.kill(process.platform === 'win32' ? undefined : 'SIGINT'); + // negative process id kills all processes led by the CLI process (process group id = cli.pid) + // this group includes the npm init child process spawned by the new command + process.kill(-cli.pid, 'SIGINT'); } });