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

Fix: colorB override not working for [Github] Stars #2171

Merged
merged 2 commits into from Oct 21, 2018

Conversation

Projects
None yet
4 participants
@lentil1016
Copy link
Contributor

commented Oct 13, 2018

Fix #2170
Hi. I believe this is a Bug fix. So I'm not sure whether I should add a test, but added it anyway.

@shields-ci

This comment has been minimized.

Copy link

commented Oct 13, 2018

Messages
📖

Thanks for your contribution to Shields, @lentil1016!

Generated by 🚫 dangerJS

@lentil1016 lentil1016 changed the title Fix: colorB override not working for Github Stars [WIP] Fix: colorB override not working for Github Stars Oct 13, 2018

@lentil1016 lentil1016 changed the title [WIP] Fix: colorB override not working for Github Stars Fix: colorB override not working for Github Stars Oct 13, 2018

@platan
Copy link
Member

left a comment

I've left some comments. The new test is failing now. You can change it to (test for hex color and non-hex color):

t.create('Stars (named color override)')
  .get('/stars/badges/shields.json?colorB=yellow&style=_shields_test')
  .expectJSONTypes(
    Joi.object().keys({
      name: 'stars',
      value: Joi.string().regex(/^\w+$/),
      colorB: Joi.equal(colorsB.yellow).required(),
    })
  )

t.create('Stars (hex color override)')
  .get('/stars/badges/shields.json?colorB=abcdef&style=_shields_test')
  .expectJSONTypes(
    Joi.object().keys({
      name: 'stars',
      value: Joi.string().regex(/^\w+$/),
      colorB: Joi.equal('#abcdef').required(),
    })
  )

Finally, please change title to Fix: colorB override not working for [Github] Stars in order to run tests on CI
(https://github.com/badges/shields/blob/master/doc/service-tests.md#pull-requests)

@@ -300,6 +300,16 @@ t.create('Stars (repo not found)')
value: 'repo not found',
})

t.create('Stars (Color override)')
.get('/stars/badges/shilds.json?colorB=brightgreen&style=_shields_test')

This comment has been minimized.

Copy link
@platan

platan Oct 13, 2018

Member

Please change shilds to shileds. shilds does not exist. This is the first reason this test is failing.

@@ -300,6 +300,16 @@ t.create('Stars (repo not found)')
value: 'repo not found',
})

t.create('Stars (Color override)')
.get('/stars/badges/shilds.json?colorB=brightgreen&style=_shields_test')
.expectJSON(

This comment has been minimized.

Copy link
@platan

platan Oct 13, 2018

Member

expectJSONTypes should be used instead of expectJSON. Joi.object().keys cannot be used with expectJSON (https://github.com/IcedFrisby/IcedFrisby/blob/master/API.md#expectjsonpath-json).

Joi.object().keys({
name: 'stars',
value: Joi.string().regex(/^\w+$/),
colorB: colorsB.brightgreen,

This comment has been minimized.

Copy link
@platan

platan Oct 13, 2018

Member

This test will pass if colorB is missing in response. We should use Joi.equal(colorsB.brightgreen).required() to be sure that colorB is in response.

@@ -35,7 +35,7 @@ module.exports = class GithubStars extends LegacyService {
try {
badgeData.text[1] = metric(JSON.parse(buffer).stargazers_count)
badgeData.colorscheme = null
badgeData.colorB = '#4183C4'
badgeData.colorB = data.colorB || '#4183C4'

This comment has been minimized.

Copy link
@platan

platan Oct 13, 2018

Member

This will not handle http://localhost:8080/github/stars/badges/shields.svg?colorA=abcdef&colorB=abcdef. But we can use this instead:

setBadgeColor(badgeData, data.colorB || '#4183C4')

@lentil1016 lentil1016 changed the title Fix: colorB override not working for Github Stars Fix: colorB override not working for [Github] Stars Oct 13, 2018

@lentil1016 lentil1016 changed the title Fix: colorB override not working for [Github] Stars [WIP] Fix: colorB override not working for [Github] Stars Oct 13, 2018

@lentil1016 lentil1016 changed the title [WIP] Fix: colorB override not working for [Github] Stars Fix: colorB override not working for [Github] Stars Oct 14, 2018

@lentil1016

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2018

@platan Sorry for bothering. I have handled my careless mistake in this PR, but I hit some wired phenomena when running test on CircleCI. Every time I rerun the test, there is a great chance that multiple timeout failure will be reported in services-pr@node-latest and services-pr. The most frequently failed test cases are:

  • Github (open pull requests by label (raw)) [ GET /issues-pr-raw/badges/shields/service-badge.json ]
  • Github (hit counter) [ GET /search/torvalds/linux/goto.json ]
  • Github (hit counter for nonexistent repo) [ GET /search/torvalds/not-linux/goto.json ]

And:

  1. There is a chance that some failure goes away at every rerunning.
  2. These failures seems not quite relative to the fix I've done.
  3. These failed cases works fine on my machine with curl, not likely to exceed the time limit.
  4. I used to doubled the timeout values, but failure remain the same on CircleCI. Then I changed it back.
  5. I've checked the build history of #2157. He meet the same phenomena, but not as many times as I did.

I guess this is probably caused by my ignorance or mistake. Luckily it passed the last time.

@platan

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

@lentil1016 thanks for this PR, for the latest improvements and for thorough description of problems with running tests on CI.
There is a room for improvement for this issue. Currently we have 22 GitHub services (.service.js files in services/github) but only one file with service tests and all of them (near 120) have to pass. It is something we should improve.

@chris48s

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

Is there a reason to implement this manually in this badge? If we set .colorscheme, wouldn't this be handled elsewhere?

@lentil1016

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

@chris48s Hi. Sorry for I can't understand that clearly, I'm not quite familiar with both JS and this project. It's that mean I should set .colorscheme instead of .colorB there? I've checked how others use .colorscheme, and still have no clue how to use it there.

@platan

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@chris48s Could you please explain more precisely how would you like to solve this? BaseService has defaultBadgeData which is used in _makeBadgeData. But github-stars is a legacy service which do not use _makeBadgeData. I think we have to handle this that way.

@@ -35,7 +36,7 @@ module.exports = class GithubStars extends LegacyService {
try {
badgeData.text[1] = metric(JSON.parse(buffer).stargazers_count)
badgeData.colorscheme = null

This comment has been minimized.

Copy link
@platan

platan Oct 16, 2018

Member

This can be removed, setBadgeColor will unset colorscheme.

Suggested change
badgeData.colorscheme = null
@chris48s

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Yep no probs. Apologies for being a bit vague. If you look at another legacy badge (e.g: GH size), we only set colorscheme:

badgeData.colorscheme = 'green'

and we can override the colours e.g:

https://img.shields.io/github/size/webcaetano/craft/build/phaser-craft.min.js.svg?colorB=orange&colorA=green

I think the error on this badge is we're setting colorB directly (which overrides whatever we pass in the URL params). Where we find badges setting colorB directly (I accept there are probably a buch of these that we need to fix) I think the correct fix in these cases is to set:

badgeData.colorscheme = 'blue'

(or whatever). That allows the URL params to override in makeBadge() even if its a legacy service.

@lentil1016

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Thanks. I think I've got the point. It's very kind of you.
Considering the previous commit I do set default value to .colorB, which makes .colorscheme default value trivial. I'll redo my commits.

@platan

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@chris48s thanks for explanation. I just thought we don't want to change default color value (#4183C4), but is OK to change it to blue and it simplifies code.

badgeData.colorscheme = null
badgeData.colorB = '#4183C4'
badgeData.colorscheme = 'blue'
setBadgeColor(badgeData, data.colorB)

This comment has been minimized.

Copy link
@platan

platan Oct 17, 2018

Member

Please remove this line (setBadgeColor(badgeData, data.colorB)). It causes a problem when you do not override colorB. In this case data.colorB is not defined and setBadgeColor sets color to red
https://github.com/badges/shields/blob/master/lib/badge-data.js#L64

This comment has been minimized.

Copy link
@lentil1016

lentil1016 Oct 17, 2018

Author Contributor

Please remove this line (setBadgeColor(badgeData, data.colorB)).

I'm not sure that's the way to fix this. Maybe I shouldn't set .colorscheme directly because setBadgeColor() will actually do that for me. I think the first version might be a better way. And I think I should remove badgeData.colorscheme = null, because setBadgeColor() will handle it in every case.

-            badgeData.colorscheme = null
-            badgeData.colorB = '#4183C4'
+            setBadgeColor(badgeData, data.colorB || 'blue')

This comment has been minimized.

Copy link
@lentil1016

lentil1016 Oct 19, 2018

Author Contributor

😓 I just realized I've misunderstood something. It's ashamed. Sorry.

@lentil1016

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

@platan Hi. I've fix this bug by setting .colorscheme as suggested. Any chance to provide me some feedback? Thanks ❤️

@platan

platan approved these changes Oct 20, 2018

@platan

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

@lentil1016 now it looks good to me :-)
@chris48s could you please look at this small change again?

@chris48s
Copy link
Member

left a comment

cheers 👍

@chris48s chris48s merged commit 88f10f8 into badges:master Oct 21, 2018

8 checks passed

WIP ready for review
Details
ci/circleci: danger Your tests passed on CircleCI!
Details
ci/circleci: frontend Your tests passed on CircleCI!
Details
ci/circleci: main Your tests passed on CircleCI!
Details
ci/circleci: main@node-latest Your tests passed on CircleCI!
Details
ci/circleci: npm-install Your tests passed on CircleCI!
Details
ci/circleci: services-pr Your tests passed on CircleCI!
Details
ci/circleci: services-pr@node-latest Your tests passed on CircleCI!
Details
@shields-deployment

This comment has been minimized.

Copy link

commented Oct 21, 2018

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.