- Notifications
You must be signed in to change notification settings - Fork 1.4k
Docs: Recipes / Code-Coverage - AVA+NYC on watch using nodemon #1379
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
Conversation
… nodemon. Also, fixed few grammar errors in the original text.
novemberborn left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the grammar fixes 👍
This doesn't really integrate with AVA's watch mode though: nodemon will cause all tests to be rerun. That's useful of course if you want up-to-date coverage but I wouldn't recommend it for regular usage.
Perhaps this can be changed into more of a "Tip: Re-run nyc using nodemon" section? That could suggest installing nodemon globally and invoking nodemon --quiet --watch ./ --exec npm run coverage directly.
We may also want to update this recipe to use nyc's configuration options, to avoid having to specify various --reporter values in the nyc invocation.
docs/recipes/code-coverage.md Outdated
| ### Use the Babel require hook | ||
| | ||
| To use the Babel require hook, add `babel-core/register` to the `require` section of you AVA config in `package.json`. | ||
| To use the Babel `require` hook, add `babel-core/register` to the `require` section of you AVA config in `package.json`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're updating this though, we should recommend using babel-register, not babel-core/register which is undocumented (and, without verifying, might very well be going away in Babel 7).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I extended the tick to include the word hook too. Let's do babel-register as a separate commit, let's get nodemon example sorted first. What do you think?
| | ||
| ```json | ||
| "scripts": { | ||
| "coverage": "nyc report --reporter=text-lcov | coveralls", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to what you're trying to show.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered your point, slept over it and considered again. This line is called by .travis.yml. Without it, Travis won't be able to ping Coveralls. We can omit it, but then we should note aside that scripts snippet is incomplete. Some people might think it's complete, paste gung-ho and their Coveralls won't update. Being a lazy person, I like complete snippets, ones without missing bits.
I'm happy to remove this line, just say no and I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used by .travis.yml. Read the above .travis.yml code.
docs/recipes/code-coverage.md Outdated
| ```json | ||
| "scripts": { | ||
| "coverage": "nyc report --reporter=text-lcov | coveralls", | ||
| "test": "standard && nyc --reporter=html --reporter=text ava", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
standard && too.
And put the reporter option as package.json config instead CLI flags. (See the nyc docs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Please review. I tested the separated setup on one of my libraries, seems fine on mine. Thank you, that's a new thing learned. 👍
docs/recipes/code-coverage.md Outdated
| "scripts": { | ||
| "coverage": "nyc report --reporter=text-lcov | coveralls", | ||
| "test": "standard && nyc --reporter=html --reporter=text ava", | ||
| "watch": "nodemon --quiet --watch ./ --exec npm run test" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./ => .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done good spot. Please review.
docs/recipes/code-coverage.md Outdated
| | ||
| First, install `nodemon` as a dev dependency: | ||
| | ||
| ```bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bash => console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, good spot. I remember some of my editors had issues in the past recognising console but it's probably an old reflex. Thank you. Please review.
docs/recipes/code-coverage.md Outdated
| | ||
| Your coverage report will then appear on coveralls shortly after Travis completes. | ||
| | ||
| ## Integrating with AVA's Watch mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watch => watch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworded the title completely. Like Mark noted, it was not accurate actually, we can't say "AVA's watch" because it's "nodemon's watch". Please review.
| "watch": "nodemon --quiet --watch . --exec npm run test" | ||
| }, | ||
| "nyc": { | ||
| "reporter": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect indentation here.
| Hi Sindre, guys, I'll take down the PR, but feel free to amend, reuse or replace the text whichever way you want (or drop it completely). |
| @revelt Why? |
| @sindresorhus that's fine, feel free to fix it and publish. It will be more effective, less backwards forwards. Seriously, no hurt feelings. I'll help all I can and keep contributing. |
| No, I mean, why not just finish it? You only had a couple of super minor changes left. I'm curious, as I often see people just not finishing almost done PRs and I still don't understand why. |
| You know Monopoly, it was a popular game but its rules inadvertently alienated the families. It was people winning against people, not a family winning together against imaginary "Monopoly". I find GitHub a bit of a similar thing. Documentation editing on OS projects should be like a happy Google Docs editing session: somebody writes up something new, others jump in and everybody edit the damn thing TOGETHER, quickly, happily and in live mode. Like a public Google Doc. If admin/maintainer does not like certain character, he just edits it over and happy days. As opposed to formal "requests" and rubbing in to original poster's ego (no offence, I'm talking in general). I know I was not entirely exact on some places, but "requests" to amend make it public which hurts. It's like Monopoly, giving away your albeit imaginary money and failing in front of siblings. Also, you know, any change, even to readme, triggers CI on GitHub. They should review the whole documentation concept a bit - how people work together. |
But open-source is "a family winning together"! 🤗
I find it 😔 that you consider a PR review "rubbing it in". Rather look at it as an opportunity to learn or see other ways to solve problems. Your PR was actually correct, it was just not according to our preferences and required some minor tweaks, which you couldn't have known, and it's as expected. Very few, or close to no PRs are merged without changes. I could have done the changes myself of course. It would have taken much less time. But I value pull request reviews, both as a submitter and as a reviewer. I often learn things as a reviewer on PRs. I suggest some change and the submitter elaborates on why the existing way is better. I don't always request the best changes, but by requesting something, I start a conversation about it. I would recommend leaving out "ego" and "emotions" when doing open-source. Yes, I know it's hard. |
| linters often prevent hurt feelings over formatting rules |
Let's add some examples of how to run both AVA and NYC on watch mode. Also, I fixed few English grammar errors in the original copy.