Chore: Add a script for testing with more control#12444
Chore: Add a script for testing with more control#12444platinumazure merged 2 commits intoeslint:masterfrom
Conversation
platinumazure
left a comment
There was a problem hiding this 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.
| "main": "./lib/api.js", | ||
| "scripts": { | ||
| "test": "node Makefile.js test", | ||
| "test:cli": "./node_modules/.bin/mocha", |
There was a problem hiding this comment.
Would it work to simply say "mocha" here?
There was a problem hiding this comment.
It takes me quite a while to think about the naming and I can't think of a good one.🤦♂️
Maybe mocha works better.
There was a problem hiding this comment.
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.
platinumazure
left a comment
There was a problem hiding this comment.
I apologize, I miscommunicated in my last review.
| "main": "./lib/api.js", | ||
| "scripts": { | ||
| "test": "node Makefile.js test", | ||
| "mocha": "./node_modules/.bin/mocha", |
There was a problem hiding this comment.
My apologies, I meant to say you could get rid of the path prefix due to how npm run works.
"test:cli": "mocha"
There was a problem hiding this comment.
No need to apologize- this was my fault. 😄 Thanks for your patience!
7ab55c1 to
b886043
Compare
platinumazure
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
kaicataldo
left a comment
There was a problem hiding this comment.
LGTM, though I think we can mark this as a chore since it's only for internal use.
|
Should we add |
I am slightly toward to "No" If the developers are split into two groups, familiar with the Mocha or not. But I believe you have much more context than I do so either way is fine for me. |
|
I'll go ahead and merge this as-is, without Thanks @fa93hws for contributing! |
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.jsonWhat 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.jsonas 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