feat: Add error handling for nonexistent file case with --file option #5086

khoaHyh · 2024-01-24T06:54:48Z

🔎 Overview

Added validation to the file(s) passed in to the --file option by resolving the argument to an absolute path and check for its existence. We now log a warning and exit if the file does not exist.
- the errors for --require bubble up to the middleware in lib/cli/run.js. See here. This specific error occurs in lib/cli/run-helpers.js in the singleRun method when we load files asynchronously. Since we were lacking error handling there, the errors appeared the way they previously did before these changes.
Added a test to assert our changes

- added error handling when using the --file flag to do it the way --require does - added a test to assert that we throw the same type of error

linux-foundation-easycla · 2024-01-24T06:54:52Z

The committers listed above are authorized under a signed CLA.

✅ login: khoaHyh / name: Khoa Huynh (b6d6dc2, 909834d, cf69df1, 0e1e84a, 9b3d5d2, a6bdef4, 4aad184, f8aedff, 6d4170e, 8983b84, bd47d79, ea092be, dce26f4, 9685b82, 4c9f318, 21e566d, bad240d, 645e0b0, b905d21, 1a9f155)

- require.resolve() by Node.js follows a Node.js module resolution algo which includes checking if the resolved path actually exists on the file system.

coveralls · 2024-02-07T19:16:58Z

coverage: 94.257% (-0.1%) from 94.358%
when pulling 1a9f155 on khoaHyh:issue/4047
into 472a8be on mochajs:master.

JoshuaKGoldberg

Cool! 😎

Requesting changes on adding a bit more testing. But also let's talk about the direction a bit - I feel nervous adding in process.exit/throws deep within code. Thanks for sending!

test/integration/options/file.spec.js

JoshuaKGoldberg · 2024-03-04T18:52:52Z

lib/cli/collect-files.js

+        `${symbols.warning} Warning: Cannot find test file "${file}" at the path "${fileAbsolutePath}"`
+      );
+      console.warn(`${warningMessage}\n`);
+      process.exit(1);


[Refactor] I don't love having a process.exit deep within code. n/no-process-exit has good reasoning around it.

It's also not really a "warning" yellow if the whole thing exits. At that point it's an error (red).

I think a more future-facing, approach would be either:

(less work, but still partially flawed) have this function throw a special error that any higher-up handler(s) know how to log nicely

(more work, but more understandable long-term) have this function return an object explaining its results, and have higher-up handler(s) work nicely with it

I haven't looked deeply enough to know whether either or both is feasible. Before we go into a whole deep dive, what do you think about this direction?

Good to learn about the no-process-exit, thanks! I think the latter where we return an object and having the higher-up handler(s) deal with it sounds like the approach here. The error wasn't handled in run-helpers so maybe we can add something so that bubbles up to run.js like the other methods?

Yeah that approach sounds like a good one to try!

khoaHyh · 2024-03-24T23:30:12Z

lib/cli/collect-files.js

@@ -30,7 +32,7 @@ module.exports = ({
  sort,
  spec
 } = {}) => {
-  const unmatched = [];
+  const unmatchedSpecFiles = [];


Renamed variable for semantic clarity

khoaHyh

@JoshuaKGoldberg tagging for re-review just in case this disappeared into the backlog abyss 😅

JoshuaKGoldberg · 2024-05-30T19:01:15Z

Oh! It did, sorry and thank you!

JoshuaKGoldberg

This looks great to me, thanks! I have no gripes about the code changes themselves and appreciate the clarity of the new unmatchedSpecFiles name. 😄

Will wait for another maintainer to review. In the interim, just leaving a few small nitpicks, nothing I'm passionate about.

JoshuaKGoldberg · 2024-05-30T20:30:04Z

lib/cli/collect-files.js

@@ -92,4 +118,10 @@ module.exports = ({
 * @property {string[]} file - List of additional files to include
 * @property {boolean} recursive - Find files recursively
 * @property {boolean} sort - Sort test files
+ * @typedef {Object} UnmatchedFile - Diagnostic object containing unmatched files


[Docs] I believe it's standard to have a separate JSDoc tag for each object declaration?

Suggested change

* @typedef {Object} UnmatchedFile - Diagnostic object containing unmatched files

*/

/**

* @typedef {Object} UnmatchedFile - Diagnostic object containing unmatched files

JoshuaKGoldberg · 2024-05-30T20:32:04Z

lib/cli/run-helpers.js

+  const fileCollectionObj = collectFiles(fileCollectParams);
+
+  if (fileCollectionObj.unmatchedFiles.length > 0) {
+    fileCollectionObj.unmatchedFiles.forEach(({pattern, absolutePath}) => {
+      console.error(
+        ansi.yellow(
+          `Warning: Cannot find any files matching pattern "${pattern}" at the absolute path "${absolutePath}"`
+        )
+      );
+    });
+    console.log(
+      'No test file(s) found with the given pattern, exiting with code 1'
+    );
+    return mocha.run(exitMocha(1));
+  }


[Refactor] This is about the same as the previous block. Up for extracting out a function?

Having two areas of code that are roughly the same isn't the worst thing in the world, but given how they're pretty much exactly identical - I think it'd make the code more readable & easy to understand. Plus if we have to change it later, it'll be easy to forget one.

JoshuaKGoldberg · 2024-05-30T20:33:36Z

test/integration/options/file.spec.js

+  it('should provide warning for nonexistent cjs file extensions', function (done) {
+    const nonexistentCjsArg = 'nonexistent.test.js';


[Naming] This confused me:

nonexistent cjs file extensions
.test.js

Is the intent that it's generally testing CommonJS files (.js)? I think the name could be a bit more clear. Maybe ...

Suggested change

it('should provide warning for nonexistent cjs file extensions', function (done) {

const nonexistentCjsArg = 'nonexistent.test.js';

it('should provide warning for nonexistent js file extensions', function (done) {

const nonexistentCjsArg = 'nonexistent.test.js';

JoshuaKGoldberg · 2024-05-30T20:33:56Z

test/integration/options/file.spec.js

+    });
+  });
+
+  it('should log a warning if a nonexistent file is specified', function (done) {


[Naming]

Suggested change

it('should log a warning if a nonexistent file is specified', function (done) {

it('should log a warning if a nonexistent file with an unknown extension is specified', function (done) {

feat: handle nonexistent files passed to --file

bd47d79

- added error handling when using the --file flag to do it the way --require does - added a test to assert that we throw the same type of error

khoaHyh marked this pull request as ready for review January 24, 2024 06:57

khoaHyh changed the title ~~ISSUE #4047: Add error handling for nonexistent file case with --file option~~ feat: Add error handling for nonexistent file case with --file option Jan 24, 2024

khoaHyh added 3 commits January 24, 2024 13:02

refactor: remove path.resolve()

4aad184

- require.resolve() by Node.js follows a Node.js module resolution algo which includes checking if the resolved path actually exists on the file system.

add comment to new code in collect-files.js

8983b84

fix: add back absolute path resolving

b6d6dc2

khoaHyh mentioned this pull request Feb 7, 2024

🚀 Feature: Better error for --file option with non-existent file #4047

Open

khoaHyh added 3 commits February 6, 2024 22:59

refactor: log warning and remove call stack

9b3d5d2

revert changes to bin/mocha.js

645e0b0

improve test case

0e1e84a

khoaHyh added 2 commits February 10, 2024 23:19

Merge branch 'master' into issue/4047

9685b82

Merge branch 'master' into issue/4047

f8aedff

JoshuaKGoldberg requested changes Mar 4, 2024

View reviewed changes

JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 4, 2024

khoaHyh added 2 commits March 24, 2024 19:25

Merge branch 'master' into issue/4047

a6bdef4

throw error and have handler work with it

909834d

khoaHyh commented Mar 24, 2024

View reviewed changes

khoaHyh requested a review from JoshuaKGoldberg March 24, 2024 23:30

khoaHyh added 9 commits March 24, 2024 20:00

change collectFiles to return object

cf69df1

clean up

b905d21

exit mocha immediately on missing file

ea092be

new log message

6d4170e

add tests

bad240d

Merge branch 'master' into issue/4047

21e566d

Merge branch 'master' into issue/4047

dce26f4

Merge branch 'master' into issue/4047

4c9f318

Merge remote-tracking branch 'upstream/master' into issue/4047

1a9f155

khoaHyh commented May 29, 2024

View reviewed changes

JoshuaKGoldberg approved these changes May 30, 2024

View reviewed changes

JoshuaKGoldberg removed the status: waiting for author waiting on response from OP - more information needed label May 30, 2024

JoshuaKGoldberg requested a review from a team May 30, 2024 20:37

JoshuaKGoldberg added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add error handling for nonexistent file case with --file option #5086

feat: Add error handling for nonexistent file case with --file option #5086

khoaHyh commented Jan 24, 2024 •

edited by JoshuaKGoldberg

linux-foundation-easycla bot commented Jan 24, 2024 •

edited

coveralls commented Feb 7, 2024 •

edited

JoshuaKGoldberg left a comment

JoshuaKGoldberg Mar 4, 2024 •

edited

khoaHyh Mar 10, 2024

JoshuaKGoldberg Mar 10, 2024

khoaHyh Mar 24, 2024

khoaHyh left a comment

JoshuaKGoldberg commented May 30, 2024 •

edited

JoshuaKGoldberg left a comment

JoshuaKGoldberg May 30, 2024

JoshuaKGoldberg May 30, 2024

JoshuaKGoldberg May 30, 2024

JoshuaKGoldberg May 30, 2024

		it('should provide warning for nonexistent cjs file extensions', function (done) {
		const nonexistentCjsArg = 'nonexistent.test.js';

	it('should log a warning if a nonexistent file is specified', function (done) {
	it('should log a warning if a nonexistent file with an unknown extension is specified', function (done) {

feat: Add error handling for nonexistent file case with --file option #5086

Are you sure you want to change the base?

feat: Add error handling for nonexistent file case with --file option #5086

Conversation

khoaHyh commented Jan 24, 2024 • edited by JoshuaKGoldberg

🔎 Overview

linux-foundation-easycla bot commented Jan 24, 2024 • edited

coveralls commented Feb 7, 2024 • edited

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 4, 2024 • edited

Choose a reason for hiding this comment

khoaHyh Mar 10, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 10, 2024

Choose a reason for hiding this comment

khoaHyh Mar 24, 2024

Choose a reason for hiding this comment

khoaHyh left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented May 30, 2024 • edited

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg May 30, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg May 30, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg May 30, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg May 30, 2024

Choose a reason for hiding this comment

khoaHyh commented Jan 24, 2024 •

edited by JoshuaKGoldberg

linux-foundation-easycla bot commented Jan 24, 2024 •

edited

coveralls commented Feb 7, 2024 •

edited

JoshuaKGoldberg Mar 4, 2024 •

edited

JoshuaKGoldberg commented May 30, 2024 •

edited