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.

Sign up for GitHub

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

Jump to bottom

[CircleCI] Extended existing try-catch block #1614

Merged
merged 3 commits into from
Mar 30, 2018
Merged

[CircleCI] Extended existing try-catch block #1614

merged 3 commits into from
Mar 30, 2018

Conversation

PyvesB
Copy link
Member

@PyvesB PyvesB commented Mar 27, 2018

This pull request should resolve issue #1608.

@shields-ci
Copy link

shields-ci commented Mar 27, 2018 •
edited
Loading

Messages
📖

✨ Thanks for your contribution to Shields, @PyvesB!

Generated by 🚫 dangerJS

@PyvesB
Copy link
Member Author

PyvesB commented Mar 27, 2018

I believe the CI failure is unrelated to this PR.

@RedSparr0w
Copy link
Member

RedSparr0w commented Mar 27, 2018 •
edited
Loading

May be worth adding the checkErrorResponse() helper function,
As it looks like data is undefined TypeError: Cannot read property 'message' of undefined,
So i'm assuming this is happening when the request is returning a 404 or !=200 status code possibly?

@paulmelnikow
Copy link
Member

By the way, to re-run a test in CI, log in to Circle using your Github account. Look for the Workflow on-commit link and click it. From the dropdown, choose Rerun failed jobs.

@PyvesB
Copy link
Member Author

PyvesB commented Mar 28, 2018

@RedSparr0w I've added the checkErrorResponse helper function. Not entirely sure why data would be undefined. Status codes should now be handled by the helper, I've added a test where we get a 200 but no response data at all. 😉

@paulmelnikow thanks for the tip!

@PyvesB PyvesB added the service-badge New or updated service badge label Mar 28, 2018
paulmelnikow
paulmelnikow reviewed Mar 29, 2018
View reviewed changes
services/circleci/circleci.tester.js Outdated
@@ -25,7 +25,7 @@ t.create('circle ci (valid, with branch)')

t.create('circle ci (not found)')
.get('/project/github/PyvesB/EmptyRepo.json')
.expectJSON({name: 'build', value: 'Project not found'});
.expectJSON({name: 'build', value: 'not found'});
Copy link
Member

Choose a reason for hiding this comment

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

I like the explicit message. You can pass a custom "not found" message to checkErrorResponse to preserve the old behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted. I've nevertheless switched to a lower case P to be more consistent with what we use elsewhere.

@RedSparr0w
Copy link
Member

Not entirely sure why data would be undefined.

Actually think i was partially wrong about the status code being the problem with it, Think it may have to do with {json:true} in the request, if the data that is returned is not JSON then it probably just returns undefined.
(just a guess, haven't had a chance to test it out yet)

@PyvesB PyvesB merged commit 66c5d91 into badges:master Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RedSparr0w RedSparr0w RedSparr0w approved these changes

@paulmelnikow paulmelnikow paulmelnikow approved these changes

Assignees
No one assigned
Labels
service-badge New or updated service badge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PyvesB @shields-ci @RedSparr0w @paulmelnikow