From aeec37a0f1ebee474bbf61ee8b253804ddd0613a Mon Sep 17 00:00:00 2001
From: Christopher Hiller <boneskull@boneskull.com>
Date: Tue, 25 Aug 2020 11:17:29 -0700
Subject: [PATCH] prefer all mocha flags over node flags; closes #4417

- no more `require` special-case
- future-proofs `mocha` against the introduction of conflicting flags in `node`
---
 lib/cli/node-flags.js                 | 13 ++++-----
 lib/cli/run-option-metadata.js        | 27 ++++++++++++++++--
 test/node-unit/cli/node-flags.spec.js | 40 +++++++++++++--------------
 3 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/lib/cli/node-flags.js b/lib/cli/node-flags.js
index b7cc9a3864..1203ad2ea8 100644
--- a/lib/cli/node-flags.js
+++ b/lib/cli/node-flags.js
@@ -7,6 +7,7 @@
  */
 
 const nodeFlags = process.allowedNodeEnvironmentFlags;
+const {isMochaFlag} = require('./run-option-metadata');
 const unparse = require('yargs-unparser');
 
 /**
@@ -43,16 +44,14 @@ exports.isNodeFlag = (flag, bareword = true) => {
     flag = flag.replace(/^--?/, '');
   }
   return (
-    // treat --require/-r as Mocha flag even though it's also a node flag
-    !(flag === 'require' || flag === 'r') &&
     // check actual node flags from `process.allowedNodeEnvironmentFlags`,
     // then historical support for various V8 and non-`NODE_OPTIONS` flags
     // and also any V8 flags with `--v8-` prefix
-    ((nodeFlags && nodeFlags.has(flag)) ||
-      debugFlags.has(flag) ||
-      /(?:preserve-symlinks(?:-main)?|harmony(?:[_-]|$)|(?:trace[_-].+$)|gc(?:[_-]global)?$|es[_-]staging$|use[_-]strict$|v8[_-](?!options).+?$)/.test(
-        flag
-      ))
+    (!isMochaFlag(flag) && nodeFlags && nodeFlags.has(flag)) ||
+    debugFlags.has(flag) ||
+    /(?:preserve-symlinks(?:-main)?|harmony(?:[_-]|$)|(?:trace[_-].+$)|gc(?:[_-]global)?$|es[_-]staging$|use[_-]strict$|v8[_-](?!options).+?$)/.test(
+      flag
+    )
   );
 };
 
diff --git a/lib/cli/run-option-metadata.js b/lib/cli/run-option-metadata.js
index da3b7d995d..33cd15ae08 100644
--- a/lib/cli/run-option-metadata.js
+++ b/lib/cli/run-option-metadata.js
@@ -12,7 +12,7 @@
  * @type {{string:string[]}}
  * @private
  */
-exports.types = {
+const TYPES = (exports.types = {
   array: [
     'extension',
     'file',
@@ -58,7 +58,7 @@ exports.types = {
     'slow',
     'timeout'
   ]
-};
+});
 
 /**
  * Option aliases keyed by canonical option name.
@@ -88,3 +88,26 @@ exports.aliases = {
   ui: ['u'],
   watch: ['w']
 };
+
+const ALL_MOCHA_FLAGS = Object.keys(TYPES).reduce((acc, key) => {
+  // gets all flags from each of the fields in `types`, adds those,
+  // then adds aliases of each flag (if any)
+  TYPES[key].forEach(flag => {
+    acc.add(flag);
+    const aliases = exports.aliases[flag] || [];
+    aliases.forEach(alias => {
+      acc.add(alias);
+    });
+  });
+  return acc;
+}, new Set());
+
+/**
+ * Returns `true` if the provided `flag` is known to Mocha.
+ * @param {string} flag - Flag to check
+ * @returns {boolean} If `true`, this is a Mocha flag
+ * @private
+ */
+exports.isMochaFlag = flag => {
+  return ALL_MOCHA_FLAGS.has(flag.replace(/^--?/, ''));
+};
diff --git a/test/node-unit/cli/node-flags.spec.js b/test/node-unit/cli/node-flags.spec.js
index f7167fa527..838d659fab 100644
--- a/test/node-unit/cli/node-flags.spec.js
+++ b/test/node-unit/cli/node-flags.spec.js
@@ -1,22 +1,34 @@
 'use strict';
 
-const nodeEnvFlags = process.allowedNodeEnvironmentFlags;
+const nodeEnvFlags = [...process.allowedNodeEnvironmentFlags];
 const {
   isNodeFlag,
   impliesNoTimeouts,
   unparseNodeFlags
 } = require('../../../lib/cli/node-flags');
 
+const {isMochaFlag} = require('../../../lib/cli/run-option-metadata');
+
 describe('node-flags', function() {
   describe('isNodeFlag()', function() {
     describe('for all allowed node environment flags', function() {
-      // NOTE: this is not stubbing nodeEnvFlags in any way, so relies on
-      // the userland polyfill to be correct.
-      nodeEnvFlags.forEach(envFlag => {
-        it(`${envFlag} should return true`, function() {
-          expect(isNodeFlag(envFlag), 'to be true');
+      nodeEnvFlags
+        .filter(flag => !isMochaFlag(flag))
+        .forEach(envFlag => {
+          it(`${envFlag} should return true`, function() {
+            expect(isNodeFlag(envFlag), 'to be true');
+          });
+        });
+    });
+
+    describe('for all allowed node env flags which conflict with mocha flags', function() {
+      nodeEnvFlags
+        .filter(flag => isMochaFlag(flag))
+        .forEach(envFlag => {
+          it(`${envFlag} should return false`, function() {
+            expect(isNodeFlag(envFlag), 'to be false');
+          });
         });
-      });
     });
 
     describe('when expecting leading dashes', function() {
@@ -24,11 +36,6 @@ describe('node-flags', function() {
         expect(isNodeFlag('throw-deprecation', false), 'to be false');
         expect(isNodeFlag('--throw-deprecation', false), 'to be true');
       });
-
-      it('should return false for --require/-r', function() {
-        expect(isNodeFlag('--require', false), 'to be false');
-        expect(isNodeFlag('-r', false), 'to be false');
-      });
     });
 
     describe('special cases', function() {
@@ -132,14 +139,5 @@ describe('node-flags', function() {
         ['--v8-numeric-one=1', '--v8-boolean-one', '--v8-numeric-two=2']
       );
     });
-
-    it('should special-case "--require"', function() {
-      // note the only way for this to happen IN REAL LIFE is if you use "--require esm";
-      // mocha eats all --require args otherwise.
-      expect(unparseNodeFlags({require: 'mcrib'}), 'to equal', [
-        '--require',
-        'mcrib'
-      ]);
-    });
   });
 });