Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change CLI file parsing errors to use an error code #4502

Merged
merged 4 commits into from
May 5, 2021

Conversation

evaline-ju
Copy link
Contributor

Description of the Change

After the implementations of #3467 and #3666 and #4464, there are a few instances in the code that do not use error codes yet.

Alternate Designs

Open to other names for this error code

Why should this be in core?

Using codes is a best practice like in Node.js, codes help identify errors as mocha errors at a glance

Benefits

Using codes is a best practice like in Node.js, codes help identify errors as mocha errors at a glance

Possible Drawbacks

None I'm aware of

Applicable issues

Contributes to #3656, semver-minor

@coveralls
Copy link

coveralls commented Nov 10, 2020

Coverage Status

Coverage increased (+0.008%) to 94.209% when pulling 3c81ed8 on evaline-ju:issue/codes-3656-2 into d7ed5c2 on mochajs:master.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

thanks Evaline! I have some comments/suggestions... please let me know what you think.

lib/errors.js Outdated Show resolved Hide resolved
lib/cli/options.js Outdated Show resolved Hide resolved
test/node-unit/cli/config.spec.js Outdated Show resolved Hide resolved
@boneskull boneskull added semver-minor implementation requires increase of "minor" version number; "features" area: usability concerning user experience or interface labels Nov 13, 2020
@boneskull
Copy link
Contributor

There's a conflict in lib/errors.js because I added a TIMEOUT error... should be able to just use both sets of changes

@boneskull boneskull added the area: node.js command-line-or-Node.js-specific label Nov 13, 2020
@boneskull
Copy link
Contributor

in case anyone else was wondering, I think "unparsable" is correct and "unparseable" is not. my spell checker dislikes both 😄

@evaline-ju
Copy link
Contributor Author

I'm not really sure if there is/was a temporary problem with the browser tests since I saw at least another PR with the same failure. At the same time, I don't know how to just re-run the test. 🤔

@boneskull
Copy link
Contributor

hmm. I'll look into that error. some saucelabs weirdness.. hope it's just temporary.

this seems to have a conflict now--could you please rebase onto master?

@evaline-ju
Copy link
Contributor Author

evaline-ju commented Nov 21, 2020

hmm I'm not seeing a conflict currently, after I merged from master 2 days ago?

Screen Shot 2020-11-20 at 4 40 52 PM

@evaline-ju
Copy link
Contributor Author

evaline-ju commented Apr 2, 2021

After a long hiatus I have tried to formally rebase - looks like the browser test on forked PRs problem is known and still existing

@outsideris
Copy link
Contributor

You can ignore the browser test. We are working on #4553 .

@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label May 5, 2021
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label May 5, 2021
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@evaline-ju thank you for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific area: usability concerning user experience or interface semver-minor implementation requires increase of "minor" version number; "features"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants