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

Chore: Add a script for testing with more control #12444

Merged
merged 2 commits into from Oct 17, 2019

Conversation

@fa93hws
Copy link
Contributor

@fa93hws fa93hws commented Oct 16, 2019

What is the purpose of this pull request? (put an "X" next to item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Add a script to package.json

What changes did you make? (Give an overview)
Issue link: #12442
Not sure whether I am the only one, I tends to not read the documentation page by page before the development and hence I neglect the fact that there is an option to have better control on the unit testing.
Also, the documentation in https://eslint.org/docs/developer-guide/unit-tests#running-individual-tests isn't complete as the developer can utilize all options from the mocha cli such as --watch.
I think it might better to have it in the package.json as a script so that it's easier for someone else in the future to find this option easier.

Is there anything you'd like reviewers to focus on?
N/A

@eslint-deprecated eslint-deprecated bot added the triage label Oct 16, 2019
Copy link
Member

@platinumazure platinumazure left a comment

Looks good to me, but I'd like another set of eyes on this. Thanks for contributing!

I did leave one question, but it's not a blocker.

@@ -9,6 +9,7 @@
"main": "./lib/api.js",
"scripts": {
"test": "node Makefile.js test",
"test:cli": "./node_modules/.bin/mocha",

This comment has been minimized.

@platinumazure

platinumazure Oct 16, 2019
Member

Would it work to simply say "mocha" here?

This comment has been minimized.

@fa93hws

fa93hws Oct 16, 2019
Author Contributor

It takes me quite a while to think about the naming and I can't think of a good one.🤦‍♂️
Maybe mocha works better.

This comment has been minimized.

@platinumazure

platinumazure Oct 16, 2019
Member

Ack, I apologize, I wasn't clear.

I meant to say that you shouldn't need to prefix the mocha command:

"test:cli": "mocha"

This is because npm scripts should have the ./node_modules/.bin added to the path environment variable.

I like the script name test:cli. It's generic, and we wouldn't need to change it if we moved away from mocha.

Copy link
Member

@platinumazure platinumazure left a comment

I apologize, I miscommunicated in my last review.

@@ -9,6 +9,7 @@
"main": "./lib/api.js",
"scripts": {
"test": "node Makefile.js test",
"mocha": "./node_modules/.bin/mocha",

This comment has been minimized.

@platinumazure

platinumazure Oct 16, 2019
Member

My apologies, I meant to say you could get rid of the path prefix due to how npm run works.

"test:cli": "mocha"

This comment has been minimized.

@fa93hws

fa93hws Oct 16, 2019
Author Contributor

Oh sorry, I see

This comment has been minimized.

@platinumazure

platinumazure Oct 16, 2019
Member

No need to apologize- this was my fault. 😄 Thanks for your patience!

ac
@fa93hws fa93hws force-pushed the fa93hws:eric/easier-testing branch from 7ab55c1 to b886043 Oct 16, 2019
@fa93hws fa93hws requested a review from platinumazure Oct 16, 2019
Copy link
Member

@platinumazure platinumazure left a comment

Looks good to me, thanks!

Copy link
Member

@kaicataldo kaicataldo left a comment

LGTM, though I think we can mark this as a chore since it's only for internal use.

@kaicataldo kaicataldo added chore and removed triage labels Oct 16, 2019
@fa93hws fa93hws changed the title New: Add a script for testing with more control Chore: Add a script for testing with more control Oct 16, 2019
@g-plane
Copy link
Member

@g-plane g-plane commented Oct 16, 2019

Should we add --reporter=progress?

@fa93hws
Copy link
Contributor Author

@fa93hws fa93hws commented Oct 16, 2019

Should we add --reporter=progress?

I am slightly toward to "No"

If the developers are split into two groups, familiar with the Mocha or not.
For those who familiar with the Mocha, there isn't much difference between having it or not.
For those who aren't familiar with it, chances that they are using it according to https://eslint.org/docs/developer-guide/unit-tests#running-individual-tests. The default reporter would be more suitable for few files.

But I believe you have much more context than I do so either way is fine for me.

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Oct 17, 2019

I'll go ahead and merge this as-is, without --reporter=progress. If we need to change this later, we can do so in a separate PR.

Thanks @fa93hws for contributing!

@platinumazure platinumazure merged commit fb633b2 into eslint:master Oct 17, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191016.5 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@fa93hws fa93hws deleted the fa93hws:eric/easier-testing branch Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants