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

[cocoapods] fix Metric regexes #1713

Merged
merged 1 commit into from
Jun 7, 2018
Merged

Conversation

chris48s
Copy link
Member

There are a couple of cocoapods tests failing because the package we are testing against has been getting 0 downloads/week (how sad 😞 ) and the isMetricOverTimePeriod regex didn't allow 0/week. In principle though, it should also be valid to have 0 open issues + so on, so I've changed the others

@shields-ci
Copy link

shields-ci commented May 30, 2018

Messages
📖

✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS

Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see one potential issue with the revised regexes: "numbers" with leading zeros will now be valid, for instance 000002/week would pass, which is a bit strange. Maybe something along the lines of (0|[1-9][0-9]*) would do the trick?

@chris48s
Copy link
Member Author

chris48s commented Jun 1, 2018

good call - cheers

Copy link
Member

@RedSparr0w RedSparr0w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good 👍

@@ -50,11 +50,11 @@ const isPhpVersionReduction = withRegex(/^((>= \d+(\.\d+)?)|(\d+\.\d+(, \d+\.\d+
const isStarRating = withRegex(/^(?=.{5}$)(\u2605{0,5}[\u00BC\u00BD\u00BE]?\u2606{0,5})$/);

// Required to be > 0, because accepting zero masks many problems.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update this comment too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. Now you've pointed out this comment, I've gone and looked back at the history. I probably should have done this before submitting this PR:

It looks like this was introduced in response to a particular problem, where an error condition was manifesting as a zero value: #1201

That seems like a bit of an edge case, but I now wonder if there are other cases like this I'm not thinking of 🤔

So maybe would be better to leave the regexes requiring a non-zero value and just switch this test to a more active repo. Then I tried to find one and realised that while the badge is correctly reporting the number from the CocoaPods API, CocoaPods seems to be incorrectly reporting all (or many) packages as having zero downloads per week. For example, this is one of the most popular packages on cocoapods: http://cocoapods.org/pods/Alamofire its unlikely that this has really had >300,000 downloads this month with 0 of them in the last week. This seems to be a reported issue which has been open for some time :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we be better to show an error message rather than 0?
Although it won't always be valid, neither will 0 and if it is actually 0, it would be less likely to go noticed that the message is incorrect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris48s good investigation. #1201 was a really ugly bug.
@RedSparr0w 0 is OK for me, better than unavailable. Stats are available and are equal 0.

In my opinion this kind of bugs can be avoided when we have tests which stubs and verifies exact value.
What do you think about replacing tests for weekly downloads and weekly apps with?

t.create('downloads (valid, weekly)')
  .get('/dw/AFNetworking.json')
  .intercept(nock => nock('https://metrics.cocoapods.org')
    .get('/api/v1/pods/AFNetworking')
    .reply(200, { stats: { download_week: 2 }}))
  .expectJSON({
    name: 'downloads',
    value: '2/week'
  });

t.create('apps (valid, weekly)')
  .get('/aw/AFNetworking.json')
  .intercept(nock => nock('https://metrics.cocoapods.org')
    .get('/api/v1/pods/AFNetworking')
    .reply(200, { stats: { app_week: 2 }}))
  .expectJSON({
    name: 'apps',
    value: '2/week'
  });

@chris48s chris48s force-pushed the fix_metric_regexes branch from e9439af to 9cb5978 Compare June 4, 2018 20:14
@chris48s
Copy link
Member Author

chris48s commented Jun 4, 2018

Having chewed this over a bit, I think the best solution here is to:

  • Keep hitting the live endpoint in tests
  • Specifically allow 0 in the regex for cocoapods weekly downloads to account for this condition
  • More generally require >0 in the metric regexes to avoid allowing unwanted bugs to creep in

@chris48s chris48s merged commit fa52c04 into badges:master Jun 7, 2018
chris48s added a commit to chris48s/shields that referenced this pull request Jun 10, 2018
It appears this does not only
happen on the weekly stats.
refs badges#1713
chris48s added a commit that referenced this pull request Jun 13, 2018
It appears this does not only
happen on the weekly stats.
refs #1713
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.

5 participants