Increase test coverage, including incrementNodeInspectorPort (#1428)

* Add tests for incrementNodeInspectorPort

* Fill out incrementNodeInspectorPort

* Remove unused code from optionMissingArgument

* Add tests for exit and custom throw

* Error if call storeOptionsAsProperties with options already stored

* Add test for bad params to parse

* Error if parse "from" is unsupported

* Can set alias after adding external command

* Add test for invalid position to addHelpText

* Clarify throw rather than display error

* Add special case which displays help

* Add test for incorrect return type on deprecated callback

* Add test for implicit electron args

* Add test for help requested on unknown subcommand

* Add test for help suggestion including command path

index.js

@@ -3,7 +3,7 @@
  */
 
 const EventEmitter = require('events').EventEmitter;
-const spawn = require('child_process').spawn;
+const childProcess = require('child_process');
 const path = require('path');
 const fs = require('fs');
 
@@ -1335,15 +1335,15 @@ class Command extends EventEmitter {
         // add executable arguments to spawn
         args = incrementNodeInspectorPort(process.execArgv).concat(args);
 
-        proc = spawn(process.argv[0], args, { stdio: 'inherit' });
+        proc = childProcess.spawn(process.argv[0], args, { stdio: 'inherit' });
       } else {
-        proc = spawn(bin, args, { stdio: 'inherit' });
+        proc = childProcess.spawn(bin, args, { stdio: 'inherit' });
       }
     } else {
       args.unshift(bin);
       // add executable arguments to spawn
       args = incrementNodeInspectorPort(process.execArgv).concat(args);
-      proc = spawn(process.execPath, args, { stdio: 'inherit' });
+      proc = childProcess.spawn(process.execPath, args, { stdio: 'inherit' });
     }
 
     const signals = ['SIGUSR1', 'SIGUSR2', 'SIGTERM', 'SIGINT', 'SIGHUP'];
@@ -1672,13 +1672,8 @@ class Command extends EventEmitter {
    * @api private
    */
 
-  optionMissingArgument(option, flag) {
-    let message;
-    if (flag) {
-      message = `error: option '${option.flags}' argument missing, got '${flag}'`;
-    } else {
-      message = `error: option '${option.flags}' argument missing`;
-    }
+  optionMissingArgument(option) {
+    const message = `error: option '${option.flags}' argument missing`;
     this._displayError(1, 'commander.optionMissingArgument', message);
   };
 

tests/command.addHelpText.test.js

@@ -77,6 +77,13 @@ describe('program calls to addHelpText', () => {
     expect(writeSpy).toHaveBeenNthCalledWith(4, 'after\n');
     expect(writeSpy).toHaveBeenNthCalledWith(5, 'afterAll\n');
   });
+
+  test('when "silly" position then throw', () => {
+    const program = new commander.Command();
+    expect(() => {
+      program.addHelpText('silly', 'text');
+    }).toThrow();
+  });
 });
 
 describe('program and subcommand calls to addHelpText', () => {

tests/command.alias.test.js

@@ -87,3 +87,12 @@ test('when set aliases then can get aliases', () => {
   program.aliases(aliases);
   expect(program.aliases()).toEqual(aliases);
 });
+
+test('when set alias on executable then can get alias', () => {
+  const program = new commander.Command();
+  const alias = 'abcde';
+  program
+    .command('external', 'external command')
+    .alias(alias);
+  expect(program.commands[0].alias()).toEqual(alias);
+});

tests/command.exitOverride.test.js

@@ -278,3 +278,26 @@ describe('.exitOverride and error details', () => {
     expectCommanderError(caughtErr, 1, 'commander.invalidOptionArgument', "error: option '--colour <shade>' argument 'green' is invalid. NO");
   });
 });
+
+test('when no override and error then exit(1)', () => {
+  const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => { });
+  const program = new commander.Command();
+  program.configureOutput({ outputError: () => {} });
+  program.parse(['--unknownOption'], { from: 'user' });
+  expect(exitSpy).toHaveBeenCalledWith(1);
+  exitSpy.mockRestore();
+});
+
+test('when custom processing throws custom error then throw custom error', () => {
+  function justSayNo(value) {
+    throw new Error('custom');
+  }
+  const program = new commander.Command();
+  program
+    .exitOverride()
+    .option('-s, --shade <value>', 'specify shade', justSayNo);
+
+  expect(() => {
+    program.parse(['--shade', 'green'], { from: 'user' });
+  }).toThrow('custom');
+});

tests/command.help.test.js

@@ -88,6 +88,13 @@ test('when call outputHelp(cb) then display cb output', () => {
   writeSpy.mockClear();
 });
 
+test('when call deprecated outputHelp(cb) with wrong callback return type then throw', () => {
+  const program = new commander.Command();
+  expect(() => {
+    program.outputHelp((helpInformation) => 3);
+  }).toThrow();
+});
+
 test('when command sets deprecated noHelp then not displayed in helpInformation', () => {
   const program = new commander.Command();
   program

tests/command.helpCommand.test.js

@@ -21,6 +21,22 @@ describe('help command listed in helpInformation', () => {
     expect(helpInformation).toMatch(/help \[command\]/);
   });
 
+  test('when program has subcommands and specify only unknown option then display help', () => {
+    const program = new commander.Command();
+    program
+      .configureHelp({ formatHelp: () => '' })
+      .exitOverride()
+      .allowUnknownOption()
+      .command('foo');
+    let caughtErr;
+    try {
+      program.parse(['--unknown'], { from: 'user' });
+    } catch (err) {
+      caughtErr = err;
+    }
+    expect(caughtErr.code).toEqual('commander.help');
+  });
+
   test('when program has subcommands and suppress help command then no help command', () => {
     const program = new commander.Command();
     program.addHelpCommand(false);
@@ -67,6 +83,16 @@ describe('help command processed on correct command', () => {
     }).toThrow('program');
   });
 
+  test('when "program help unknown" then program', () => {
+    const program = new commander.Command();
+    program.exitOverride();
+    program.command('sub1');
+    program.exitOverride(() => { throw new Error('program'); });
+    expect(() => {
+      program.parse('node test.js help unknown'.split(' '));
+    }).toThrow('program');
+  });
+
   test('when "program help sub1" then sub1', () => {
     const program = new commander.Command();
     program.exitOverride();

tests/command.parse.test.js

@@ -4,7 +4,7 @@ const commander = require('../');
 // https://github.com/electron/electron/issues/4690#issuecomment-217435222
 // https://www.electronjs.org/docs/api/process#processdefaultapp-readonly
 
-describe('.parse() user args', () => {
+describe('.parse() args from', () => {
   test('when no args then use process.argv and app/script/args', () => {
     const program = new commander.Command();
     const hold = process.argv;
@@ -14,7 +14,16 @@ describe('.parse() user args', () => {
     expect(program.args).toEqual(['user']);
   });
 
-  // implicit also supports detecting electron but more implementation knowledge required than useful to test
+  test('when no args and electron properties and not default app then use process.argv and app/args', () => {
+    const program = new commander.Command();
+    const holdArgv = process.argv;
+    process.versions.electron = '1.2.3';
+    process.argv = 'node user'.split(' ');
+    program.parse();
+    delete process.versions.electron;
+    process.argv = holdArgv;
+    expect(program.args).toEqual(['user']);
+  });
 
   test('when args then app/script/args', () => {
     const program = new commander.Command();
@@ -51,6 +60,13 @@ describe('.parse() user args', () => {
     program.parse('user'.split(' '), { from: 'user' });
     expect(program.args).toEqual(['user']);
   });
+
+  test('when args from "silly" then throw', () => {
+    const program = new commander.Command();
+    expect(() => {
+      program.parse(['node', 'script.js'], { from: 'silly' });
+    }).toThrow();
+  });
 });
 
 describe('return type', () => {
@@ -72,3 +88,11 @@ describe('return type', () => {
     expect(result).toBe(program);
   });
 });
+
+// Easy mistake to make when writing unit tests
+test('when parse strings instead of array then throw', () => {
+  const program = new commander.Command();
+  expect(() => {
+    program.parse('node', 'test');
+  }).toThrow();
+});

tests/command.unknownCommand.test.js

@@ -62,4 +62,20 @@ describe('unknownOption', () => {
     }
     expect(caughtErr.code).toBe('commander.unknownCommand');
   });
+
+  test('when unknown subcommand then help suggestion includes command path', () => {
+    const program = new commander.Command();
+    program
+      .exitOverride()
+      .command('sub')
+      .command('subsub');
+    let caughtErr;
+    try {
+      program.parse('node test.js sub unknown'.split(' '));
+    } catch (err) {
+      caughtErr = err;
+    }
+    expect(caughtErr.code).toBe('commander.unknownCommand');
+    expect(writeErrorSpy.mock.calls[0][0]).toMatch('test sub');
+  });
 });

tests/commander.configureCommand.test.js

@@ -76,3 +76,11 @@ test('when storeOptionsAsProperties(false) then opts+command passed to action',
   program.parse(['node', 'test', 'value']);
   expect(callback).toHaveBeenCalledWith('value', program.opts(), program);
 });
+
+test('when storeOptionsAsProperties() after adding option then throw', () => {
+  const program = new commander.Command();
+  program.option('--port <number>', 'port number', '80');
+  expect(() => {
+    program.storeOptionsAsProperties(false);
+  }).toThrow();
+});

tests/incrementNodeInspectorPort.test.js

@@ -0,0 +1,135 @@
+const childProcess = require('child_process');
+const path = require('path');
+const commander = require('../');
+
+describe('incrementNodeInspectorPort', () => {
+  let spawnSpy;
+  let signalSpy;
+  const oldExecArgv = process.execArgv;
+
+  beforeAll(() => {
+    spawnSpy = jest.spyOn(childProcess, 'spawn').mockImplementation(() => {
+      return {
+        on: () => {}
+      };
+    });
+    signalSpy = jest.spyOn(process, 'on').mockImplementation(() => {});
+  });
+
+  afterEach(() => {
+    spawnSpy.mockClear();
+  });
+
+  afterAll(() => {
+    spawnSpy.mockRestore();
+    signalSpy.mockRestore();
+    process.execArgv = oldExecArgv;
+  });
+
+  function makeProgram() {
+    const program = new commander.Command();
+    const fileWhichExists = path.join(__dirname, './fixtures/pm-cache.js');
+    program.command('cache', 'stand-alone command', { executableFile: fileWhichExists });
+    return program;
+  }
+
+  function extractMockExecArgs(mock) {
+    return mock.mock.calls[0][1].slice(0, -1);
+  }
+
+  test('when --inspect then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect=127.0.0.1:9230']);
+  });
+
+  test('when --inspect=100 then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect=100'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect=127.0.0.1:101']);
+  });
+
+  test('when --inspect=1.2.3.4:100 then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect=1.2.3.4:100'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect=1.2.3.4:101']);
+  });
+
+  test('when --inspect=1.2.3.4 then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect=1.2.3.4'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect=1.2.3.4:9230']);
+  });
+
+  test('when --inspect-brk then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect-brk'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect-brk=127.0.0.1:9230']);
+  });
+
+  test('when --inspect-brk=100 then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect-brk=100'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect-brk=127.0.0.1:101']);
+  });
+
+  test('when --inspect-brk=1.2.3.4 then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect-brk=1.2.3.4'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect-brk=1.2.3.4:9230']);
+  });
+
+  test('when --inspect-brk=1.2.3.4:100 then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect-brk=1.2.3.4:100'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect-brk=1.2.3.4:101']);
+  });
+
+  test('when --inspect-port=100 then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect-port=100'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect-port=127.0.0.1:101']);
+  });
+
+  test('when --inspect-port=1.2.3.4:100 then bump port', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect-port=1.2.3.4:100'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect-port=1.2.3.4:101']);
+  });
+
+  test('when --inspect-unexpected then unchanged', () => {
+    const program = makeProgram();
+    process.execArgv = ['--inspect-unexpected'];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--inspect-unexpected']);
+  });
+
+  test('when --frozen-intrinsics  then unchanged', () => {
+    const program = makeProgram();
+    process.execArgv = ['--frozen-intrinsics '];
+    program.parse(['node', 'test', 'cache']);
+    const execArgs = extractMockExecArgs(spawnSpy);
+    expect(execArgs).toEqual(['--frozen-intrinsics ']);
+  });
+});