-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[npm bountysource cdnjs circleci clojars docker gem homebrew itunes microbadger nexus requires shippable suggest uptimerobot] Service tests for NPM total downloads + invalidJSON helper #1471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
service-tests/helpers/mocks.js
Outdated
@@ -0,0 +1,13 @@ | |||
'use strict'; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would response-fixtures
be a clearer name?
service-tests/helpers/mocks.js
Outdated
const invalidJSON = function() { | ||
return [ | ||
200, | ||
'invalid json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something a little stranger here, like 'this string <here> is [definitly.not] valid json'
make it clearer that this is the json string, and not an error message that gets passed in?
Added: Some of the other tests use "{{{{{invalid json}}"
which I think is a good one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've change it to {{{{{invalid json}}
.
.reply(200, 'invalid json'), { | ||
'Content-Type': 'application/json' | ||
}) | ||
.reply(invalidJSON)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay for refactoring. This looks great!
Would you be willing to go through the other service tests and update them to use your new fixture as well?
I seems that I have to check/fix 2 tests tests not connected with my change. |
module.exports = t; | ||
|
||
t.create('image size without a specified tag') | ||
.get('/image-size/_/hello-world.json') | ||
.get('/image-size/_/centos.json') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a quick fix. For hello-world
badge returned error
, because image.DownloadSize
is undefined
in
Line 6479 in 8ad176d
const size = prettyBytes(parseInt(image.DownloadSize)); |
@goozler Do you have any idea why it changed?
There is no
DownloadSize
property in main object but there is "DownloadSize": 0
in Versions[0]
. We can replace error
in this case with 0
, but I'm not sure it's the best solution (and I would like to fix this in a separate PR).
curl "https://api.microbadger.com/v1/images/library/hello-world" -s | jq .
{
"LatestSHA": "690d802025313dfb6400e3872bcbf3ab5e486c5160b89fa0b3edf0332bfa4cf1",
"WebhookURL": "https://hooks.microbadger.com/images/library/hello-world/6Yx3YymZyiVymVHeR6XIH_Yzy2s=",
"UpdatedAt": "2018-01-28T12:50:00.104461Z",
"Description": "Hello World! (an example of minimal Dockerization)",
"LastUpdated": "2018-01-08T20:43:23.336375Z",
"PullCount": 128583318,
"StarCount": 431,
"Versions": [
{
"SHA": "690d802025313dfb6400e3872bcbf3ab5e486c5160b89fa0b3edf0332bfa4cf1",
"Tags": [
{
"tag": "linux"
},
{
"tag": "latest"
}
],
"ImageName": "hello-world",
"Author": "",
"LayerCount": 2,
"DownloadSize": 0,
"Created": "2017-11-21T00:23:18.797568Z"
}
],
"Id": "690d802025313dfb6400e3872bcbf3ab5e486c5160b89fa0b3edf0332bfa4cf1",
"ImageName": "hello-world",
"ImageURL": "https://hub.docker.com/r/library/hello-world/",
"LayerCount": 2,
"LatestVersion": "linux"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it could be Docker or Microbadger related issue. They had some issues with their service and the support suggested to make a POST request to the WebhookURL to recalculate statistics. I think the error message in that case is fine.
I have my own docker hub image and the size of it was reset several times but I didn't change anything. I called the webhook url manually and it solved the issue temporarily. Last two months everything has been fine, so, I assume they fixed it. But for some images the problem still exists.
I think the size may be broken after new Docker version is released or something like that.
I agree with changing the image for tests if it's broken. That makes sense for me. Also on the demo page the httpd
image doesn't have the size too and the webhook url doesn't help and it could be changed too, I guess.
I chose hello-world
for tests because I thought it wouldn't be changed and it should work all the time :-)
In general, I don't know how to solve it because it's on the Microbadger side.
})); | ||
|
||
t.create('Uptime Robot: Percentage (valid, with numberOfDays param)') | ||
.get('/ratio/7/m778918918-3e92c097147760ee39d02d36.json') | ||
.expectJSONTypes(Joi.object().keys({ | ||
name: 'uptime', | ||
value: isDecimalPercentage, | ||
value: isPercentage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently API returns custom_uptime_ratio
equal 100.000
which is changed to 100
in
Line 7370 in 8ad176d
var percent = parseFloat(json.monitors[0].custom_uptime_ratio); |
isPercentage
.
@paulmelnikow All issues fixed (I hope). |
Thanks for this @platan, all the changes look good to me. |
No description provided.