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

Add a code coverage recipe #444

Closed
wants to merge 1 commit into from
Closed

Conversation

ben-eb
Copy link
Contributor

@ben-eb ben-eb commented Jan 17, 2016

Just a quick note; I don't have experience of other CI services and hosted code coverage, so if anyone would like to add other services to the bottom of the guide then that'd be useful. 😄

`coverage` to your `.gitignore`.


## ES6 coverage
Copy link
Member

Choose a reason for hiding this comment

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

ES6 => ES2015

@ben-eb
Copy link
Contributor Author

ben-eb commented Jan 17, 2016

Updated. 👍

"test": "nyc --reporter=lcov --reporter=text ava test.js"
}
}
```
Copy link
Member

Choose a reason for hiding this comment

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

I generally prefer separating the coveralls stuff from the local test running. Meaning I don't see the point in having --reporter=lcov here.

Copy link
Member

Choose a reason for hiding this comment

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

So should look like:

"test": "nyc ava"

Copy link
Member

Choose a reason for hiding this comment

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

and in .travis.yml, you can do node_modules/.bin/nyc report --reporter=text-lcov

Then add the following to your `.travis.yml`:

```
after_script:
Copy link
Member

Choose a reason for hiding this comment

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

@ben-eb ben-eb force-pushed the coverage-recipe branch 2 times, most recently from 10f9dd1 to 155bb28 Compare January 17, 2016 11:22
@ben-eb
Copy link
Contributor Author

ben-eb commented Jan 17, 2016

Just tried the ES2015 version without the reporters, it needs at least one otherwise it throws, so I'll leave in the text reporter for now.

@sindresorhus
Copy link
Member

Just tried the ES2015 version without the reporters, it needs at least one otherwise it throws, so I'll leave in the text reporter for now.

Can you report that as a bug to nyc? It should support it as the default is text reporter:

  -r, --reporter  coverage reporter(s) to use                  [default: "text"]

@sindresorhus
Copy link
Member

Can you not hard wrap the text?

```json
{
"scripts": {
"report": "nyc report --reporter html"
Copy link
Member

Choose a reason for hiding this comment

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

--reporter=html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nyc report --reporter html works fine. Should I change anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I meant, for consistency, use = between key and value.

@sindresorhus
Copy link
Member

Add it to the recipe list in the readme: https://github.com/sindresorhus/ava#recipes

@sindresorhus sindresorhus changed the title Add a code coverage guide. Add a code coverage recipe Jan 17, 2016
@sindresorhus
Copy link
Member

Awesome! This will be helpful for a lot of users. Thanks @ben-eb :)

@ben-eb ben-eb deleted the coverage-recipe branch January 17, 2016 14:48
@ben-eb
Copy link
Contributor Author

ben-eb commented Jan 17, 2016

No problem. 😄

To see a HTML report for either the ES6 or ES5 coverage strategies we have outlined, do:

```
nyc report --reporter=html
Copy link
Contributor

Choose a reason for hiding this comment

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

should be ./node_modules/.bin/nyc unless installed globally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamestalmage
Copy link
Contributor

I know I'm a bit late to the party, but I had just a little bit of follow up.


I usually inline the text and lcov reporters:

{
  "test": "nyc --reporter=lcov --reporter=text ava"
}

istanbuljs/nyc#137 is part of the reason why. nyc report can fail if anything happens to the cache between running the tests and generating reports. Safer to just do it all at once.

Since the lcov reporter actually generates html reports in addition to lcov.info. I use the html report locally, and lcov.info in an after_script for Travis/Coveralls:

after_script:
 - 'cat ./coverage/lcov.info | ./node_modules/.bin/coveralls'

I prefer after_script to after_success because it's still helpful to have coverage info when reviewing WIP PR's that might still have a few failing tests (or if the project is plagued by a few finicky tests).


The --babel flag never made it into nyc. Instead we adopted a --require flag (very similar to AVA's).

In this case however, I wouldn't use it. Instead I would add it to AVA's config in package.json:

{
  "ava": {
    "require": "babel-core/register"
  }
}

The advantage of configuring for AVA instead of NYC is that you can just run ava from the command line (assuming a global install) - and it will pick up that config. Otherwise you would be required to add --require babel-core/register on the command line every time.

@ben-eb
Copy link
Contributor Author

ben-eb commented Jan 17, 2016

We used ./node_modules/.bin/nyc report --reporter=text-lcov | ./node_modules/.bin/coveralls because it outputs to stdout, so there's no need for saving it to a file and cating it back out to stdout again. Rest looks good to me but I can't spend more time on this.

This was referenced Jan 17, 2016
@sindresorhus
Copy link
Member

I usually inline the text and lcov reporters:

That adds bloat to your package.json with limited gain. The more boilerplate we add to package.json, the less likely it is people will bother using it. "People" here being me too.

istanbuljs/nyc#137 is part of the reason why. nyc report can fail if anything happens to the cache between running the tests and generating reports.

That should just be fixed in nyc.

I prefer after_script to after_success because it's still helpful to have coverage info when reviewing WIP PR's that might still have a few failing tests (or if the project is plagued by a few finicky tests).

You can still have that locally, but I don't think failing PRs should add noise to the Coveralls history.

In this case however, I wouldn't use it. Instead I would add it to AVA's config in package.json:

👍

@sindresorhus
Copy link
Member

@vdemedes Thoughts?

@vadimdemedes
Copy link
Contributor

I like this recipe, have nothing major to add or correct. The only thing I noted is that I usually prefix bash examples with $, but it's not that important.

@vadimdemedes
Copy link
Contributor

@jamestalmage As for me, I also prefer having only nyc && ava in npm test. I generate lcov and pipe it to coveralls right away. The same stuff as in AVA's config. It looks clean and simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants