Upgrade to commander 7 (#486)

This allows getting rid of all workarounds we had for commander.
rtfeldman · Jan 15, 2021 · fbb6f2a · fbb6f2a
1 parent 4b17314
commit fbb6f2a
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 123 deletions.
diff --git a/lib/Report.js b/lib/Report.js
@@ -4,16 +4,7 @@
 // https://github.com/prettier/prettier/issues/2597
 const Report /*: 'console' | 'json' | 'junit' */ = 'console';
 
-function parse(string /*: string */) /*: typeof Report */ {
-  switch (string) {
-    case 'console':
-    case 'json':
-    case 'junit':
-      return string;
-    default:
-      throw new Error(`unknown reporter: ${string}`);
-  }
-}
+const all = ['json', 'junit', 'console'];
 
 function isMachineReadable(report /*: typeof Report */) /*: boolean */ {
   switch (report) {
@@ -27,6 +18,6 @@ function isMachineReadable(report /*: typeof Report */) /*: boolean */ {
 
 module.exports = {
   Report,
-  parse,
+  all,
   isMachineReadable,
 };
diff --git a/lib/elm-test.js b/lib/elm-test.js
@@ -1,6 +1,6 @@
 // @flow
 
-const { program } = require('commander');
+const { InvalidOptionArgumentError, Option, program } = require('commander');
 const fs = require('fs');
 const os = require('os');
 const path = require('path');
@@ -17,42 +17,17 @@ const RunTests = require('./RunTests');
 
 void Report;
 
-// TODO(https://github.com/rtfeldman/node-test-runner/pull/465): replace this
-// function with commander's custom error messages once
-// https://github.com/tj/commander.js/pull/1392 lands and is released.
-const parsePositiveInteger = (flag /*: string */) => (
-  string /*: string */
-) /*: number */ => {
+const parsePositiveInteger = (string /*: string */) /*: number */ => {
   const number = Number(string);
   if (!/^\d+$/.test(string)) {
-    console.error(
-      `error: option '${flag}' expected one or more digits, but got: ${string}`
-    );
-    throw process.exit(1);
+    throw new InvalidOptionArgumentError('Expected one or more digits.');
   } else if (!Number.isFinite(number)) {
-    console.error(
-      `error: option '${flag}' expected a finite number, but got: ${number}`
-    );
-    throw process.exit(1);
+    throw new InvalidOptionArgumentError('Expected a finite number.');
   } else {
     return number;
   }
 };
 
-// TODO(https://github.com/rtfeldman/node-test-runner/pull/465): replace this
-// function with commander's validating sets of strings once
-// https://github.com/tj/commander.js/issues/518 is released (probably v7).
-const parseReport = (flag /*: string */) => (
-  string /*: string */
-) /*: typeof Report.Report */ => {
-  try {
-    return Report.parse(string);
-  } catch (error) {
-    console.error(`error: option '${flag}' ${error.message}`);
-    throw process.exit(1);
-  }
-};
-
 function findClosestElmJson(dir /*: string */) /*: string | void */ {
   const entry = ElmJson.getPath(dir);
   return fs.existsSync(entry)
@@ -99,33 +74,6 @@ function getPathToElmBinary(compiler /*: string | void */) /*: string */ {
   }
 }
 
-// Unfortunately commander is very permissive about extra arguments. Therefore,
-// we manually check for excessive arguments.
-// See: https://github.com/tj/commander.js/issues/1268
-function handleTooManyArgs(action) {
-  return (...args) => {
-    if (args.length < 2) {
-      action(...args);
-    } else {
-      // The arguments to Commander actions are:
-      // expectedCliArg1, expectedCliArg2, expectedCliArgN, Cmd, restCliArgs
-      const rest = args[args.length - 1];
-      if (rest.length > 0) {
-        const expected = args.length - 2;
-        const s = expected === 1 ? '' : 's';
-        console.error(
-          `Expected ${expected} argument${s}, but got ${
-            expected + rest.length
-          }.`
-        );
-        process.exit(1);
-      } else {
-        action(...args);
-      }
-    }
-  };
-}
-
 const examples = `
 elm-test
   Run tests in the tests/ folder
@@ -138,7 +86,7 @@ function main() {
   process.title = 'elm-test';
 
   program
-    .storeOptionsAsProperties(false)
+    .allowExcessArguments(false)
     .name('elm-test')
     .usage('[options] [globs...]')
     .description(examples)
@@ -150,23 +98,24 @@ function main() {
       'Use a custom path to an Elm executable (default: elm)',
       undefined
     )
-    .option(
-      '--seed <int>',
-      'Run with a previous fuzzer seed',
-      parsePositiveInteger('--seed <int>'),
-      Math.floor(Math.random() * 407199254740991) + 1000
+    .addOption(
+      new Option('--seed <int>', 'Run with a previous fuzzer seed')
+        .default(Math.floor(Math.random() * 407199254740991) + 1000, 'random')
+        .argParser(parsePositiveInteger)
     )
     .option(
       '--fuzz <int>',
       'Run with each fuzz test performing this many iterations',
-      parsePositiveInteger('--fuzz <int>'),
+      parsePositiveInteger,
       100
     )
-    .option(
-      '--report <json|junit|console>',
-      'Print results to stdout in the given format',
-      parseReport('--report <json|junit|console>'),
-      'console'
+    .addOption(
+      new Option(
+        '--report <format>',
+        'Print results to stdout in the given format'
+      )
+        .default('console')
+        .choices(Report.all)
     )
     .option('--watch', 'Run tests on file changes', false)
     .version(packageInfo.version, '--version', 'Print version and exit')
@@ -176,53 +125,49 @@ function main() {
   program
     .command('init')
     .description('Create example tests')
-    .action(
-      handleTooManyArgs(() => {
-        const options = program.opts();
-        const pathToElmBinary = getPathToElmBinary(options.compiler);
-        const project = getProject('init');
-        try {
-          Install.install(project, pathToElmBinary, 'elm-explorations/test');
-          fs.mkdirSync(project.testsDir, { recursive: true });
-          fs.copyFileSync(
-            path.join(__dirname, '..', 'templates', 'tests', 'Example.elm'),
-            path.join(project.testsDir, 'Example.elm')
-          );
-        } catch (error) {
-          console.error(error.message);
-          throw process.exit(1);
-        }
-        console.log(
-          '\nCheck out the documentation for getting started at https://package.elm-lang.org/packages/elm-explorations/test/latest'
+    .action(() => {
+      const options = program.opts();
+      const pathToElmBinary = getPathToElmBinary(options.compiler);
+      const project = getProject('init');
+      try {
+        Install.install(project, pathToElmBinary, 'elm-explorations/test');
+        fs.mkdirSync(project.testsDir, { recursive: true });
+        fs.copyFileSync(
+          path.join(__dirname, '..', 'templates', 'tests', 'Example.elm'),
+          path.join(project.testsDir, 'Example.elm')
         );
-        process.exit(0);
-      })
-    );
+      } catch (error) {
+        console.error(error.message);
+        throw process.exit(1);
+      }
+      console.log(
+        '\nCheck out the documentation for getting started at https://package.elm-lang.org/packages/elm-explorations/test/latest'
+      );
+      process.exit(0);
+    });
 
   program
     .command('install <package>')
     .description(
       'Like `elm install package`, except it installs to "test-dependencies" in your elm.json'
     )
-    .action(
-      handleTooManyArgs((packageName) => {
-        const options = program.opts();
-        const pathToElmBinary = getPathToElmBinary(options.compiler);
-        const project = getProject('install');
-        try {
-          const result = Install.install(project, pathToElmBinary, packageName);
-          // This mirrors the behavior of `elm install` passing a package that is
-          // already installed. Say it's already installed, then exit 0.
-          if (result === 'AlreadyInstalled') {
-            console.log('It is already installed!');
-          }
-          process.exit(0);
-        } catch (error) {
-          console.error(error.message);
-          process.exit(1);
+    .action((packageName) => {
+      const options = program.opts();
+      const pathToElmBinary = getPathToElmBinary(options.compiler);
+      const project = getProject('install');
+      try {
+        const result = Install.install(project, pathToElmBinary, packageName);
+        // This mirrors the behavior of `elm install` passing a package that is
+        // already installed. Say it's already installed, then exit 0.
+        if (result === 'AlreadyInstalled') {
+          console.log('It is already installed!');
         }
-      })
-    );
+        process.exit(0);
+      } catch (error) {
+        console.error(error.message);
+        process.exit(1);
+      }
+    });
 
   program
     .command('make [globs...]')
@@ -234,7 +179,10 @@ function main() {
       const make = async () => {
         await Generate.generateElmJson(project, () => null);
         await Compile.compileSources(
-          FindTests.resolveGlobs(testFileGlobs, project.rootDir),
+          FindTests.resolveGlobs(
+            testFileGlobs.length === 0 ? [project.testsDir] : testFileGlobs,
+            project.rootDir
+          ),
           project.generatedCodeDir,
           pathToElmBinary,
           options.report

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -46,7 +46,7 @@
   "dependencies": {
     "chalk": "^4.1.0",
     "chokidar": "^3.4.2",
-    "commander": "^6.2.0",
+    "commander": "^7.0.0",
     "cross-spawn": "^7.0.3",
     "elm-tooling": "^1.0.0",
     "glob": "^7.1.6",

diff --git a/tests/ci.js b/tests/ci.js
@@ -115,6 +115,11 @@ describe('Testing elm-test on an example application', () => {
     const runResult = execElmTest([args], cwd);
     assertTestFailure(runResult);
   }).timeout(60000);
+
+  it('Should successfully run `elm-test make`', () => {
+    const runResult = execElmTest(['make'], cwd);
+    assertTestSuccess(runResult);
+  }).timeout(60000);
 });
 
 describe('Testing elm-test on an example package', () => {
@@ -131,6 +136,11 @@ describe('Testing elm-test on an example package', () => {
     const runResult = execElmTest([args], cwd);
     assertTestFailure(runResult);
   }).timeout(60000);
+
+  it('Should successfully run `elm-test make`', () => {
+    const runResult = execElmTest(['make'], cwd);
+    assertTestSuccess(runResult);
+  }).timeout(60000);
 });
 
 describe('Testing elm-test on example-application-src', () => {