fix: handle case of invalid package.json with no explicit config (#5198)

* fix: report syntax errors in package.json (fixes: #5141) * fix(tests): incorrect test (should use existing result) Since `readFileSync` is only stubbed `onFirstCall` we get a different answer the second time around which is probably Not What You Want. But also we *already checked that config = false*. So you could just remove this test, it does nothing useful. * fix: separate read and parse errors * fix: clarify invalid JSON Co-authored-by: Josh Goldberg ✨ <[email protected]> * fix(test): expect only a SyntaxError nothing else --------- Co-authored-by: Josh Goldberg ✨ <[email protected]>
mochajs · Oct 29, 2024 · f72bc17 · f72bc17
1 parent 68803b6
commit f72bc17
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 10 deletions.
diff --git a/lib/cli/options.js b/lib/cli/options.js
@@ -181,22 +181,36 @@ const loadPkgRc = (args = {}) => {
   result = {};
   const filepath = args.package || findUp.sync(mocharc.package);
   if (filepath) {
+    let configData;
     try {
-      const pkg = JSON.parse(fs.readFileSync(filepath, 'utf8'));
+      configData = fs.readFileSync(filepath, 'utf8');
+    } catch (err) {
+      // If `args.package` was explicitly specified, throw an error
+      if (filepath == args.package) {
+        throw createUnparsableFileError(
+          `Unable to read ${filepath}: ${err}`,
+          filepath
+        );
+      } else {
+        debug('failed to read default package.json at %s; ignoring',
+              filepath);
+        return result;
+      }
+    }
+    try {
+      const pkg = JSON.parse(configData);
       if (pkg.mocha) {
         debug('`mocha` prop of package.json parsed: %O', pkg.mocha);
         result = pkg.mocha;
       } else {
         debug('no config found in %s', filepath);
       }
     } catch (err) {
-      if (args.package) {
-        throw createUnparsableFileError(
-          `Unable to read/parse ${filepath}: ${err}`,
-          filepath
-        );
-      }
-      debug('failed to read default package.json at %s; ignoring', filepath);
+      // If JSON failed to parse, throw an error.
+      throw createUnparsableFileError(
+        `Unable to parse ${filepath}: ${err}`,
+        filepath
+      );
     }
   }
   return result;

diff --git a/test/node-unit/cli/options.spec.js b/test/node-unit/cli/options.spec.js
@@ -149,7 +149,7 @@ describe('options', function () {
                 loadOptions('--package /something/wherever --require butts');
               },
               'to throw',
-              'Unable to read/parse /something/wherever: bad file message'
+              'Unable to read /something/wherever: bad file message'
             );
           });
         });
@@ -199,6 +199,36 @@ describe('options', function () {
           });
         });
 
+        describe('when path to package.json unspecified and package.json exists but is invalid', function () {
+          beforeEach(function () {
+            const filepath = '/some/package.json';
+            readFileSync = sinon.stub();
+            // package.json
+            readFileSync
+              .onFirstCall()
+              .returns('{definitely-invalid');
+            findConfig = sinon.stub().returns('/some/.mocharc.json');
+            loadConfig = sinon.stub().returns({});
+            findupSync = sinon.stub().returns(filepath);
+            loadOptions = proxyLoadOptions({
+              readFileSync,
+              findConfig,
+              loadConfig,
+              findupSync
+            });
+          });
+
+          it('should throw', function () {
+            expect(
+              () => {
+                loadOptions();
+              },
+              'to throw',
+                /SyntaxError/,
+            );
+          });
+        });
+
         describe('when called with package = false (`--no-package`)', function () {
           let result;
           beforeEach(function () {
@@ -287,7 +317,7 @@ describe('options', function () {
           });
 
           it('should set config = false', function () {
-            expect(loadOptions(), 'to have property', 'config', false);
+            expect(result, 'to have property', 'config', false);
           });
         });