Added support for new Code Climate scores #1236
Conversation
service-tests/codeclimate.js
Outdated
| }); | ||
|
|
||
| t.create('test coverage score') | ||
| .get('/tc/Nickersoft/dql.json') |
There was a problem hiding this comment.
For consistency with a couple other badges, could you change /tc/ to /c/ for coverage?
| badgeData.colorscheme = 'red'; | ||
| } | ||
| sendBadge(format, badgeData); | ||
| } catch(e) { |
There was a problem hiding this comment.
Is this the "project not found" case? Is it possible to test that more explicitly above, so you can reserve the catch for "something went wrong" and return invalid instead of not found?
There was a problem hiding this comment.
I'm not exactly sure... I copied these code blocks from the existing CodeClimate blocks. Could you elaborate on when "not found" vs. "invalid" vs. "unknown" should be displayed?
There was a problem hiding this comment.
- When someone is asking for a nonexistent project, we should display something really clear and helpful like project not found, which tells them they have spelled something wrong, the project isn't public, etc.
- On the other hand unknown would mean CodeClimate has the project but for some reason can't answer the question, like some kind of error occurred when trying to compute it.
- And invalid is like a 500 error: something went wrong on our side or their side when we tried to fetch the information.
There was a problem hiding this comment.
Ah, I gotcha. I tried to fixed the errors to be more appropriate, so take a look when you can.
I think you should be able to accomplish this by mocking both requests. You can either using nock chaining or make two calls to |
server.js
Outdated
| })); | ||
|
|
||
|
|
||
| // Code Climate maintainability |
There was a problem hiding this comment.
Could this be merged with the coverage badge above as there is only a few lines difference.
example
There was a problem hiding this comment.
Good call! I went ahead and merged the two :)
2) Corrected error messages 3) Merged two new CodeClimate methods
|
@paulmelnikow So I've been trying to nock requests, and while I can get the response JSON to match, it still keeps failing with the following error: I tried looking at the nock repo, but the top issues I found regarding this seemed to be from 2012. Any insight as to what causes this kind of error? |
|
I think that means is that you're setting up requests that aren't actually getting made. I've worked with Nock some, and recently started helping to maintain it… a common complaint is that it's not easy to understand when things have gone wrong. If you'd like to push your failing tests to a branch I'd be happy to look into it. |
|
@paulmelnikow Okay, just pushed those tests. I'd like if possible to keep the badge ID of my Nickersoft repo out of the equation, but theoretically, the server is making a request to that exact endpoint and nocking it still results in the error. Let me know whatcha think. |
|
I got the tests passing but I can't push my changes. Could you check the "Allow edits from maintainers" box so I can do that? |
|
@paulmelnikow Sorry, just seeing this now.. it looked like the box was already checked but I un-checked it and re-checked it just to be sure. Let me know if it still won't let you. |
|
Odd! I just tried it again and it's still telling me I can't push. I opened Nickersoft#1. Maybe it's easiest to merge them in that way. |
Switch to https + fix tests
|
@paulmelnikow Yup! It's merged into my branch now :) |
|
Thanks! |
One think to note: unlike the previous CC badges, the new API uses badge tokens associated with repos to retrieve SVG badges for users (as opposed to just the repo name). As a result, the new badges have to first make a request to the CC API to get the token associated with the repo, then make another request to get information regarding the badge. Also due to this, it was difficult to write tests that test for invalid response headers. Everything else should be covered though.