Skip to content

Conversation

khoaHyh
Copy link
Contributor

@khoaHyh khoaHyh commented Jan 24, 2024

🔎 Overview

Fixes #4047

  • 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

Screenshot from 2024-02-07 00-34-30

- 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
Copy link

linux-foundation-easycla bot commented Jan 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@khoaHyh khoaHyh marked this pull request as ready for review January 24, 2024 06:57
@khoaHyh 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
- 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
Copy link

coveralls commented Feb 7, 2024

Coverage Status

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

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP or other posters - more information needed label Mar 4, 2024
@khoaHyh khoaHyh requested a review from JoshuaKGoldberg March 24, 2024 23:30
Copy link
Contributor Author

@khoaHyh khoaHyh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@JoshuaKGoldberg JoshuaKGoldberg merged commit dbe229d into mochajs:main Jun 25, 2024
@voxpelli
Copy link
Member

Thanks for stepping in and merging @JoshuaKGoldberg 🙏 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Better error for --file option with non-existent file
4 participants