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

Added Codecov integration #269

Closed
wants to merge 1 commit into from
Closed

Added Codecov integration #269

wants to merge 1 commit into from

Conversation

stevepeak
Copy link
Contributor

@cainus it would be my honor for you to proof this pr's validity. Thank you!

curl -X HEAD https://codecov.io/github/stevepeak/timestring/coverage.png -D -

HTTP/1.1 302 Found
X-Coverage: 54
Cache-Control: no-cache
Location: https://s3-us-west-2.amazonaws.com/codecov.io/media/images/coverage/png/54.png

@cainus
Copy link

cainus commented Sep 18, 2014

Looks good on inspection. It's a bit weird to use headers (X-Coverage) for this, but if it works, it works. ;) I have permissions to merge this, but I don't think I should be the guy to do it since I haven't written any code in this repo. I have no idea how it deploys anyway. @espadrine ?

@stevepeak
Copy link
Contributor Author

@cainus I thought it may be easier then snipping it from the image name. If that is preferable then its should be easy to implement. Thank you!

@espadrine
Copy link
Member

@stevepeak Thanks a lot!

Using X-Coverage is just fine if it is likely to survive for some time. If it starts to break, it will probably fall on me to fix it (or remove the support), but I have no insight about the continued support of X-Coverage by Codecov.

On the other hand, I can't risk having the server crash upon an invalid badge.

$ curl -I https://codecov.io/github/jekyll/jekyll/coverage.png
HTTP/1.1 302 Found
Server: Cowboy
Connection: keep-alive
Content-Length: 0
X-Coverage: unknown

(Btw, I did find this curl -I https://codecov.io/github/codecov/example-python/coverage.png that worked. I'll use this in the comment instead. Also, for the future's sake, I chose to test whether coverage was a number by attempting to convert it, and checking whether it is NaN by comparing to itself; NaN is never equal to itself. Also, apparently your comments indicate an expectation for this to be a 404 and show "n/a" instead of "unknown". I would love to know the background story on this.)

Not to worry, though: covering the sensitive part with this should fix it.

// Here be the part just in the request callback, just after the if (err != null)
try {
// Setting the badge information here.
} catch(e) {
  badgeData.text[1] = 'malformed';
  sendBadge(format, badgeData);
  return;
}

Beyond that, this line does not work, since node puts keys lowercase:

var coverage = res.headers['X-Coverage'];  // should be 'x-coverage'

Finally, this line cannot work either:

badgeData.colorscheme = coveragePercentageColor(percentage);  // First use of percentage.

Another nitpick, there is a trailing space on line 967.

It is obvious that this PR was not tested, which is why I took much more time to review it than I wanted, delaying other reviews and making my eyes sleepy this morning for work.

Is there anything I could have done to make it easier for you to test your PR?

Apart from that, I'd like to use the URL /codecov/c/github/codecov/example-python.svg. The /c/ part gives way for potential future features of Codecov. Is that ok on your side?

@espadrine
Copy link
Member

Some fixes: b935dad

@stevepeak
Copy link
Contributor Author

Thank you @espadrine for taking the time to look at this :) I'm the lead developer at Codecov, so any questions I can promptly answer for you.

As far as X-Coverage, I'm sure it will remain a feature to have this header, but I understand if you would prefer to use the other technique of parsing the url instead, which is fine by me. We only redirect to the following images: range of 0...100 and unknown (for both 401/404 requests)

Using /codecov/c/github/codecov/example-python.svg for shields is just fine as well. Funny you bring this up because our roadmap does have many more features.

Let me know what, if anything, you would like me to do next. Thanks again!

@cainus
Copy link

cainus commented Sep 27, 2014

Is there anything I could have done to make it easier for you to test your PR?

Well a couple of things:

  • have some directions on how to test (I'd write these but I really don't know, unless you just mean manually testing by running the server locally)
  • have a spec for any other service integration that contributors could copy/paste/modify (I didn't see one at least). Service logic would probably be best factored into a separate file that can be tested independently. (This I can help with if you want)
  • create a make test target that also runs jshint to eliminate some of the more minor nitpicks (I can help with that too, if you want)

Also if a failure in any service can take down the whole server, you might want to try using something like express-domain-middleware to get better isolation.

Anyway... just some ideas. I'm willing to help where I can too.

espadrine added a commit that referenced this pull request Sep 27, 2014
@espadrine
Copy link
Member

have some directions on how to test (I'd write these but I really don't know, unless you just mean manually testing by running the server locally)

Where would you write these? They are both in CONTRIBUTING.md and INSTALL.md, and there are links to those in README.md.

create a make test target that also runs jshint to eliminate some of the more minor nitpicks (I can help with that too, if you want)

ok

have a spec for any other service integration that contributors could copy/paste/modify (I didn't see one at least). Service logic would probably be best factored into a separate file that can be tested independently. (This I can help with if you want)

There are very varied techniques to scrape the information we need, so we can't create a unique spec to copy. That said, maybe my attempts were insufficient. Maybe someone else can do it. The flexibility that this part of the program requires makes this a challenging task.

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.

3 participants