search

shelljs / shx

Portable Shell Commands for Node
MIT License
1.73k stars 45 forks source link

Lowering test coverage in order to test README badge #52

Closed nfischer closed 8 years ago

nfischer commented 8 years ago

Do not merge

This is just a test to verify #51 is actually ok.

levithomason commented 8 years ago

You're correct, the badge is reporting the 42% even though it is pointed to master. Hm.

master https://img.shields.io/codecov/c/github/shelljs/shx/master.svg

default https://img.shields.io/codecov/c/github/shelljs/shx.svg

levithomason commented 8 years ago

Looks like it is an issue with codecov itself? This PR shows 42% right now, but clicking branches > master (https://codecov.io/gh/shelljs/shx/branch/master) also shows 42%.

nfischer commented 8 years ago

Hmmm... that definitely seems strange. But yeah, I can't seem to find a way to point at the real branch of master on Github.

This may be due to how code coverage is set up. Perhaps it's because it gets triggered during CI, which is after it merges into master on the CI machine (but not on the remote repo)?

This isn't the biggest deal. I just thought I'd point it out since it could be misleading if we're unaware.

levithomason commented 8 years ago

Agree on all counts. I spent some time in their interface trying to see how to separate the tested PR merge commit from the actual master commit to no avail.

Codecov is still pretty new and has a few issues. We use it at work and have run into a few other reported coverage level problems.

If you'd like to dig on this more, we could investigate use of a codecov.yml and see if one of the settings there resolves this. Otherwise, feel free to close these out for now.

nfischer commented 8 years ago

This appears to be an issue for others, as seen in codecov/support#191. Closing this PR, since we obviously wouldn't want to merge it and the issue appears to be out of our hands.

levithomason commented 8 years ago

Want to kill the branch as well I assume?

nfischer commented 8 years ago

Just to document the results we saw, coverage went down from 84% to 42% by removing test cases. This was visible in the README badge, despite the fact that we hadn't merged this PR. I'm including a screenshot of badge (in the comments of this PR) just in case the badge updates as master branch changes:

screenshot from 2016-05-10 16-33-01

ariporad commented 8 years ago

Hmm... I don't really like this (the bug, not the PR). What are your thoughts on considering coveralls? They've worked pretty well for me in the past, and I know that they do support this.

levithomason commented 8 years ago

i'd like to give them a little while and see how it goes. if they are responsive and quick on fixes, I say stay. otherwise, a switch is needed. Thoughts?

ariporad commented 8 years ago

SGTM