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

Add test for handling partial data with error(s) #15

Merged
merged 4 commits into from
Apr 5, 2019

Conversation

TimonVS
Copy link
Contributor

@TimonVS TimonVS commented Apr 4, 2019

Ref: #14

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

as part of this Pull Request we should also add a section for Partial responses to the README.md


.then(result => {
throw new Error('Should not resolve')
})
Copy link
Contributor

Choose a reason for hiding this comment

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

if you agree, we could add a .catch() statement similar to the test above, only also check for error.data. Would that be a desired behavior for partial responses?

Suggested change
})
})
.catch(error => {
expect(error.message).to.equal('Field \'bioHtml\' doesn\'t exist on type \'User\'')
expect(error.errors).to.deep.equal(mockResponse.errors)
expect(error.request.query).to.equal(query)
expect(error.data).to.deep.equal(mockResponse.data)
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that would make the most sense!

@TimonVS
Copy link
Contributor Author

TimonVS commented Apr 5, 2019

@gr2m I updated the test and made it pass, I also added a section to the readme. Let me know what you think :)

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Fabulous! Thank you Timon 💝

@gr2m gr2m merged commit c7ffda6 into octokit:master Apr 5, 2019
@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants