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 Test Coverage to All Modules #966

Closed
Miserlou opened this issue Nov 1, 2016 · 6 comments
Closed

Add Test Coverage to All Modules #966

Miserlou opened this issue Nov 1, 2016 · 6 comments

Comments

@Miserlou
Copy link

@Miserlou Miserlou commented Nov 1, 2016

It looks like all of the modules in this stack don't have code coverage. This makes it difficult as a user/new contributor to see how well tested these modules are, and it makes it more difficult for you as maintainers to see if incoming pull requests are appropriately testing all the code paths that they may be introducing.

I use the coverage and coveralls services on all of my projects and have found them quite useful. It seems that the istanbul library is kind of the go-to Node alternative here, but I think that anything would be better than nothing here.

That being said, I understand if you have your own objections (ex, well covered != well integrated) about this and I can close this.

Just a suggestion that would be appreciated!

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Nov 1, 2016

I don't see the value in code coverage tools. You can run them locally if you care, but they've never been helpful to me.

@Miserlou

This comment has been minimized.

Copy link
Author

@Miserlou Miserlou commented Nov 2, 2016

I think there is pretty great value:

  • Easily see the general state of how healthy and tested a codebase is (for the parts I'm using, it's ~80% coverage, ~60% of paths, for the curious)
  • Get an at-a-glance view of which features are tested and which aren't
  • Track over-time if the reliability of the code is increasing or decreasing
  • Maintainers can easily see if PRs introduce untested code
  • Contributors can see which areas of the code need additional test coverage, which gives new contributors a starting point and existing contributors better ownership of their code.

I'm curious what other contributors think about this. Test coverage is a pretty standard part of modern FOSS tooling so I was surprised that this stack didn't use it, especially considering how many projects are starting to depend on it, my own included. ¯_(ツ)_/¯

@josephfrazier

This comment has been minimized.

Copy link
Member

@josephfrazier josephfrazier commented Nov 2, 2016

I think @Miserlou makes some good points. In any case, adding code coverage as part of CI doesn't seem like it would have any downsides (assuming a README badge and a PR comment/status don't count).

@Miserlou, maybe you could use your fork to show what this might look like? That way there would be something more concrete to discuss. For example, I did this on another project at nvbn/thefuck#566

P.S. It looks like istanbul is already used by zuul, so we might be able to have browser test coverage statistics as well!

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

@DiegoRBaquero DiegoRBaquero commented Nov 2, 2016

I like the idea, it adds more information. I don't see how it could impact negatively in any way.

@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jan 29, 2017

I don't have any immediate plans to work on this issue, so I'm going to close it.

@feross feross closed this Jan 29, 2017
@lock

This comment has been minimized.

Copy link

@lock lock bot commented May 4, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.