[BitBucket GitHub] fixes and tests for BitBucket service integration #1315

chris48s · 2017-12-03T18:37:55Z

Add test suite for BitBucket service integration
Present BitBucket issues as a metric (for consistency with BitBucket PR endpoint and GitHub endpoints)
Factor out a shared regex
Fail cleanly if count is undefined

i.e: do this:
not this:

Other similar endpoints e.g: BitBucket PRs, GitHub PRs, Github Issues etc present this as a metric. BitBucket issues should behave consistently

chris48s · 2017-12-03T18:46:43Z

I expect the GH failures are caused by #979 rather than stuff I've broken. If it makes it easier to review, I could back out 3611916 so only the BitBucket tests need to be run.

paulmelnikow

These are awesome! You're on fire! 🔥

paulmelnikow · 2017-12-04T02:04:54Z

service-tests/helpers/validators.js

@@ -37,6 +37,8 @@ const isStarRating = withRegex(/^(?=.{5}$)(\u2605{0,5}[\u00BC\u00BD\u00BE]?\u260
 // Required to be > 0, beacuse accepting zero masks many problems.
 const isMetric = withRegex(/^[1-9][0-9]*[kMGTPEZY]?$/);

+const isMetricOpenIssues = withRegex(/^[0-9]+[kMGTPEZY]? open$/);


Could we make sure this is nonzero, as with isMetric? We've had broken code which displayed 0 on a malformed response.

Alternatively we could add a test with API mock which will allow expect specific value in result.

It is legitimate for this to be zero if there are actually zero open issues though. In the GitHub tests (on master), you've got a mix of tests that are looking for [0-9]+... (e.g: https://github.com/badges/shields/blob/master/service-tests/github.js#L69-L74 ) and others looking for [1-9][0-9]+... (e.g: https://github.com/badges/shields/blob/master/service-tests/github.js#L80-L85 ).

Do you think we should extract both regexes and add tests for both cases, or should we always validate a non-zero value?

True, though as long as we pick a project that is likely to stay around, there will always be open issues. I'm always fine with additional tests using mocks if the integrated tests don't seem satisfactory.

I think using /^[1-9][0-9]*[kMGTPEZY]? open$/ would be fine for both open issues and open issues by label. Does that answer your question?

paulmelnikow · 2017-12-04T02:08:37Z

I expect the GH failures are caused by #979 rather than stuff I've broken. If it makes it easier to review, I could back out 3611916 so only the BitBucket tests need to be run.

No need, I'll just run the Github tests locally before merging. They are green right now. ✅

platan · 2017-12-04T19:56:23Z

service-tests/bitbucket.js

+
+t.create('issues-raw (invalid)')
+  .get('/issues-raw/atlassian/not-a-repo.json')
+  .expectJSON({ name: 'issues', value: 'invalid' });


How about displaying repo not found instead of invalid? Someone asked for repo which does not exist.

This catch block might potentially catch errors other than repo doesn't exist

This behaviour is consistent with the output of other endpoints. e.g:

https://img.shields.io/cpan/v/not-a-package.svg

https://img.shields.io/pypi/v/blablabla/not-a-package.svg

https://img.shields.io/packagist/v/blablabla/not-a-package.svg

Okay, though is there another way to detect the "not found" case? Agree with @platan that it would be better to detect it and return a helpful message. It lets the user know where the problem lies.

Ah, didn't see this: #1315 (comment)

platan · 2017-12-04T20:03:30Z

server.js

@@ -4277,6 +4280,9 @@ cache(function(data, match, sendBadge, request) {
    try {
      var data = JSON.parse(buffer);
      var pullrequests = data.size;
+      if (typeof pullrequests === 'undefined') {


In tests I can see that this code is used to handle case, when repo if not found (please correct me if I'm wrong). In this case API will return 404 status code (e.g. https://bitbucket.org/api/2.0/repositories/atlassian/python-bitbucket/pullrequests_NOT_FOUND/?limit=0&state=OPEN). How about checking status code before parsing response body?
Have you know a case when status code is 200 and data.size is undefined?

chris48s · 2017-12-04T21:17:00Z

@platan - I can't think of another case where should be valid for count/size to be undefined with a 200 OK response so I have changed the condition to check for if (res.statusCode !== 200) 👍

other invalid status codes or exceptions will be caught with genreric 'invalid'

chris48s · 2017-12-05T19:18:15Z

updated based on latest review comments

paulmelnikow · 2017-12-05T20:27:20Z

Github service tests passing locally 👍

Thanks!

platan · 2017-12-05T21:42:43Z

server.js

@@ -4246,11 +4246,18 @@ cache(function(data, match, sendBadge, request) {
    try {
      var data = JSON.parse(buffer);
      var issues = data.count;
-      badgeData.text[1] = issues + (isRaw? '': ' open');
+      if (res.statusCode !== 200) {


It's already merged, but I think next time we could check status code before parsing response body.

chris48s added 4 commits December 3, 2017 17:08

add test suite to formalise existing behaviour of bitbucket integration

67bede3

present bitbucket issues as a metric

71f3df6

Other similar endpoints e.g: BitBucket PRs, GitHub PRs, Github Issues etc present this as a metric. BitBucket issues should behave consistently

factor out shared regex

3611916

fail gracefully if issue/pr count is undefined

6250087

paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Dec 3, 2017

paulmelnikow reviewed Dec 4, 2017

View reviewed changes

platan reviewed Dec 4, 2017

View reviewed changes

check for non-200 status instead of undefined size/length

e6635b6

chris48s added 2 commits December 5, 2017 18:19

always require open issues metric to be >0

f65f12a

add special case for 404 status code

ea677fd

other invalid status codes or exceptions will be caught with genreric 'invalid'

paulmelnikow merged commit 433d69b into badges:master Dec 5, 2017

platan reviewed Dec 5, 2017

View reviewed changes

This was referenced Dec 6, 2017

[BitBucket] check status code before parsing body #1343

Merged

The Great Service Rewrite 🏆 #1358

Closed

snyk-bot mentioned this pull request May 28, 2021

[Snyk] Upgrade ioredis from 4.14.1 to 4.27.2 andruszd/shields#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BitBucket GitHub] fixes and tests for BitBucket service integration #1315

[BitBucket GitHub] fixes and tests for BitBucket service integration #1315

chris48s commented Dec 3, 2017

chris48s commented Dec 3, 2017

paulmelnikow left a comment

paulmelnikow Dec 4, 2017

platan Dec 4, 2017

chris48s Dec 4, 2017

paulmelnikow Dec 5, 2017

paulmelnikow commented Dec 4, 2017

platan Dec 4, 2017

chris48s Dec 4, 2017 •

edited

Loading

paulmelnikow Dec 5, 2017

paulmelnikow Dec 5, 2017

platan Dec 4, 2017 •

edited

Loading

chris48s commented Dec 4, 2017

chris48s commented Dec 5, 2017

paulmelnikow commented Dec 5, 2017

platan Dec 5, 2017

[BitBucket GitHub] fixes and tests for BitBucket service integration #1315

[BitBucket GitHub] fixes and tests for BitBucket service integration #1315

Conversation

chris48s commented Dec 3, 2017

chris48s commented Dec 3, 2017

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented Dec 4, 2017

Choose a reason for hiding this comment

chris48s Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platan Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

chris48s commented Dec 4, 2017

chris48s commented Dec 5, 2017

paulmelnikow commented Dec 5, 2017

Choose a reason for hiding this comment

chris48s Dec 4, 2017 •

edited

Loading

platan Dec 4, 2017 •

edited

Loading