Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Adding coverage report for server-side tests using istanbul #811

Merged
merged 2 commits into from
Sep 1, 2015
Merged

Adding coverage report for server-side tests using istanbul #811

merged 2 commits into from
Sep 1, 2015

Conversation

lirantal
Copy link
Member

Code coverage is now built-in to our project and grunt tasks!

meanjs-code-coverage-2

and further results:

meanjs-code-coverage-1

To run the code coverage simply execute:

grunt coverage

Addresses:

@lirantal lirantal self-assigned this Aug 15, 2015
@lirantal lirantal added this to the 0.4.x milestone Aug 15, 2015
@lirantal lirantal mentioned this pull request Aug 15, 2015
Closed
@lirantal
Copy link
Member Author

@ilanbiala please go ahead and commit your coveralls to this PR

@mleanos
Copy link
Member

mleanos commented Aug 16, 2015

LGTM. Really slick! I really like the initializing status 👍

@lirantal
Copy link
Member Author

lol cool

@ilanbiala
Copy link
Member

@lirantal I won't be able to test anything until the weekend, so until then if you want to take a crack at it with Coveralls' guide, go for it.

@lirantal
Copy link
Member Author

@ilanbiala so let's merge this PR and we can later create a new one to extend the local coverage we're running with an integration to coveralls.

@lirantal
Copy link
Member Author

@codydaig @mleanos do you guys want to review this PR? I thought maybe you have some experience with code coverage tools

@lirantal lirantal mentioned this pull request Aug 17, 2015
Closed
@lirantal
Copy link
Member Author

@sielay @abhijt-zanak maybe you also have something to add?

@lirantal lirantal mentioned this pull request Aug 17, 2015
Closed
@ilanbiala
Copy link
Member

@lirantal I'd rather add it all at once. I don't think people are really gonna benefit from just having coverage in the terminal compared to having Coveralls running for them.

@lirantal lirantal force-pushed the feature/code-coverage branch from f7a20d0 to 8e2cda1 Compare August 19, 2015 19:02
@lirantal
Copy link
Member Author

@ilanbiala WDYT?
we must merge this first to master for coveralls.io to actually pick it up and show information about code coverage

codydaig
codydaig reviewed Aug 19, 2015
View reviewed changes
README.md
@@ -3,6 +3,7 @@
[![Gitter](https://badges.gitter.im/Join Chat.svg)](https://gitter.im/meanjs/mean?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge)
[![Build Status](https://travis-ci.org/meanjs/mean.svg?branch=master)](https://travis-ci.org/meanjs/mean)
[![Dependencies Status](https://david-dm.org/meanjs/mean.svg)](https://david-dm.org/meanjs/mean)
[![Coverage Status](https://coveralls.io/repos/meanjs/mean/badge.svg?branch=master&service=github)](https://coveralls.io/github/meanjs/mean?branch=coveralls-code-coverage)
Copy link
Member

Choose a reason for hiding this comment

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

@lirantal The .svg is displaying from the master branch but the link is going to the coveralls-code-coverage branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks! that's probably a leftover from taking README update off of ilan's branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

there's also a missing service=github in the actual link?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure about that one, I just noticed the odd ball branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

nm, looked at their website, it's not required

@lirantal lirantal force-pushed the feature/code-coverage branch from 8e2cda1 to 260b8d6 Compare August 19, 2015 19:25
@mleanos
Copy link
Member

mleanos commented Aug 19, 2015

@lirantal LGTM... tested and verified the coverage outputs the logs correctly. Everything looks good.

And STILL like the Initializing status :)

@lirantal
Copy link
Member Author

lol @mleanos :)
Awesome, thanks!

ilanbiala
ilanbiala reviewed Aug 20, 2015
View reviewed changes
gruntfile.js
coverage: true,
require: 'test.js',
coverageFolder: 'coverage',
reportFormats: ['cobertura','lcovonly'],
Copy link
Member

Choose a reason for hiding this comment

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

What's cobertura?

@rhutchison rhutchison mentioned this pull request Aug 20, 2015
Closed
15 tasks
@lirantal lirantal force-pushed the feature/code-coverage branch from 260b8d6 to 7c286b0 Compare August 21, 2015 11:59
@lirantal
Copy link
Member Author

@ilanbiala per your questions:

  1. Cobertura is a code coverage tool which is commonly used in Java project. the grunt-mocha-istanbul plugin supports exports to the cobertura reporter format to integrate with hudson/jenkins. We use in our enterprise both Coberutra and Jenkins so I figure others will find this useful too without requiring extra tweaks.
  2. About the check limits - for now we should keep that threshold because we don't have much more than that covered, so until we have more code coverage let's keep it on a 'sane' threshold. (I don't see a reason to keep a 90 threshold to show the project as not reaching it's goal and keeping that for who knows how long...
  3. Moved coveralls to devDep
  4. Corrected indentations
  5. Fixed

@codydaig
Copy link
Member

LGTM

@ilanbiala
Copy link
Member

LGTM, @lirantal do we already see some coverage reports for this PR or not until we merge?

@lirantal
Copy link
Member Author

After we'll merge (I'll do it later), and then we can also tweak and take care of areas for more test as we wish.

@ilanbiala
Copy link
Member

Sounds good.

@ilanbiala
Copy link
Member

@lirantal are we merging this or is there something left to add?

@ilanbiala ilanbiala modified the milestones: 0.4.1, 0.4.x Aug 31, 2015
@lirantal
Copy link
Member Author

lirantal commented Sep 1, 2015

Merging! :)

lirantal added a commit that referenced this pull request Sep 1, 2015
@lirantal
Merge pull request #811 from lirantal/feature/code-coverage
7051345 
Adding coverage report for server-side tests using istanbul
@lirantal lirantal merged commit 7051345 into meanjs:master Sep 1, 2015
@ilanbiala
Copy link
Member

@lirantal Coveralls still doesn't show anything https://coveralls.io/github/meanjs/mean when I log in and see the repo status. Is there something else we have to add?

@lirantal
Copy link
Member Author

lirantal commented Sep 2, 2015

I'll check this

@lirantal
Copy link
Member Author

lirantal commented Sep 3, 2015

@ilanbiala ok so I put a specific grunt task called coverage and we aren't calling it as part of the travis ci tests today, and I don't think we should. Instead, we should add another task to the Travis CI to run the coverage. I'll work it out and let you know.

@ilanbiala
Copy link
Member

@lirantal 👍

@lirantal lirantal mentioned this pull request Sep 4, 2015
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@lirantal lirantal

Labels
enhance:feature
Projects
None yet
Milestone
0.4.1
Development

Successfully merging this pull request may close these issues.
4 participants
@lirantal @mleanos @ilanbiala @codydaig