fix: error handling for unexpected numeric arguments passed to cli

[Dinika]

PR Checklist

• Addresses an existing open issue: fixes TypeError: arg.split is not a function when passed a numerical argument #5028
• That issue was marked as status: accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

This PR adds error handling for TypeError: arg.split is not a function which is thrown when an unexpected numerical argument is passed to mocha cli. Hopefully the thrown error message is clearer to understand (I'm open to suggestions to further improve it).

This custom error is thrown in following cases:

• A numeric argument was passed to cli (such as mocha 123)
• A numeric argument was passed as a value to a mocha option that does not accept numerical values (such as mocha --delay 123)

The error is not thrown in if a numerical value is passed to mocha flag which can handle numerical values (such as mocha --retries 2) or if numerical value is passed to node options.

PS - Apologies for posting this PR before receiving a reply to this comment I posted in the issue.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Dinika / name: Dinika (4e8ffce, 09574d7, a8ea4dd, 74c13fd, 8f727b3, 163dd91, 26dd073, 912bdd1, 1deca6b, 662a04e, 6c33877, 19bd2a4, f894ed1)

[JoshuaKGoldberg]

Thanks for getting this started! 🙌 The unit tests in particular are lovely.

Requesting changes on a different strategy for detecting invalid args.

[Dinika]

@JoshuaKGoldberg Thanks so much for the review. I've revised the PR as per the suggestions.

[JoshuaKGoldberg]

This looks great, thank you! The approach seems very reasonable and straightforward. Just requesting changes on including a bit more information in the thrown error.

The [Style] nit is definitely not blocking if you want to assertively say that it's numerical, not numeric 😄

Thanks!

[JoshuaKGoldberg]

Btw, whenever you want a re-review, the re-request review button works.

[Dinika]

I do not have permissions to check why the "netlify/mocha/deploy-preview" job is failing. Lemme know if there's something I need/can to do to fix it. Thanks!

[JoshuaKGoldberg]

Wow this really ballooned in scope! On the one hand, I feel bad that what looked like a really small-scoped good first issue is turning into a lot more work. On the other hand, the implementation is looking great and is touching on some edge cases Mocha doesn't yet handle well. So this is looking like a really great new feature addition! 🚀

[JoshuaKGoldberg]

[Question] Why isNumeric(arg) &&? Wouldn't we want to check this for other string flags too?

Unless I'm missing something, could this be trimmed down to?:

Suggested change

	if (isNumeric(arg) && (!maybeFlag || !isMochaFlag(maybeFlag))) {
	if (!maybeFlag || !isMochaFlag(maybeFlag)) {

[Dinika]

Hmm, I think one reason why I am only handling numeric args is because all other types of arguments don't really make mocha crash (summary in the comment below). They are converted to positional string arguments, mocha tries to find files matching those strings and if it doesn't find those files, it prints a warning.

Example: mocha --global var1,var2 var3 true false my-test.spec.js prints this warning and carries on testing my-test.spec.js:

Warning: Cannot find any files matching pattern "var3"
Warning: Cannot find any files matching pattern "true"
Warning: Cannot find any files matching pattern "false"

Do you think I should instead throw an error in these cases?

[JoshuaKGoldberg]

[Feature] This check seems legitimate and good. I like it. But, it feels odd to me that it's only implemented for expected numeric types. There are booleans and strings too. I think it'd be weird as a user to only get this report for an incorrect number, but not incorrect booleans or strings.

What do you think about generalizing it to all argument types?

I.e. instead of:

if numeric and expected_type !== 'number':
  throw

Doing roughly:

if actual_type !== expected_type:
  throw

[Dinika]

Based on this and the previous comment, I've summarized the current behaviour of mocha for different flags and values data-type.

Columns show flags and rows show values passed to flags.

Red dot 🔴 is placed on cells where mocha currently crashes without a good error message. So far, this PR has addressed only these cases.
Orange dot 🟠 is placed where the behaviour could be improved perhaps. Mocha does not crash in these situations. Lemme know if I should improve the behaviour in this PR.

Value / Flag	No flags	number ⚑	boolean ⚑	array ⚑	string ⚑
number value	🔴 Eg: mocha 123 - Mocha crashes with error "arg.split()"	Eg: mocha --retries 123 - 123 is assigned to retries	🔴 Eg: mocha --delay 123: delay is assigned value "true". 123 is treated as positional arg. Mocha crashes with error "arg.split()". Mocha does not accept filenames with only numeric chars	Eg: mocha --file 123: assigns value "123" to the flag. Appropriate error/warning is thrown if flag can't handle value (eg. file not found).	Eg: mocha --grep 123 assigns "123" to flag. Appropriate action is taken.
boolean value	Eg: mocha true: "true" is treated like a positional value for filename.	Eg: mocha --retries true: "NaN" is assigned to flag. This value is then ignored.	Eg: mocha --delay true: works same as mocha --delay. true is assigned to flag.	Eg mocha --spec true: stringified "true" is added to array of flag values. Appropriate action is taken.	Eg mocha --ui true. Stringified "true" is assigned to flag.
array value	Eg: mocha v1,v2: "v1,v2" is treated like a positional value for a filename	🟠 Eg: mocha --retries v1,v2 "NaN" is assigned to flag. This value is then ignored.	Eg: mocha --delay v1,v2: value true is assigned to flag. "v1,v2" is treated like a positional filename.	Eg: mocha --spec v1,v2 Flag has value ["v1", "v2"]	Eg: mocha --grep v1,v2 String "v1,v2" is assigned to flag
string value	Eg: mocha abc: "abc" is treated like a positional value for a filename.	🟠 Eg: mocha --retries abc: "NaN" is assigned to flag. This value is then ignored.	Eg: mocha --delay abc: value true is assigned to flag. "abc" is treated like positional value for a filename.	Eg: mocha --spec abc. "abc" is added to array of flag values.	Eg: mocha --grep abc: Works as expected.
No value	-	Eg: mocha --retries. Error Not enough arguments following: retries is thrown	Eg: mocha --delay: Flag has value true	Eg: mocha --spec Error Not enough arguments following: spec is thrown	Eg: mocha grep: Error Not enough arguments following: grep is thrown

Based on the table above, only numeric values (or flags) seem to be causing the problem right now. That being said, I don't mind creating a more generic error handling approach. Lemme know if you prefer that - I'll make the changes.

[JoshuaKGoldberg]

What a great table 😄. Very helpful. I think the 🟠 ones can be ignored for now, agreed.

[JoshuaKGoldberg]

🚀 Wonderful! No more notes from me, this is all looking great. Thanks!

Since it's a nontrivial change, leaving it open for a bit in case others in @mochajs/maintenance-crew have time to review.

[JoshuaKGoldberg]

[Dinika]

Yayy!! Thanks for the thorough and quick review @JoshuaKGoldberg 😊