-
Notifications
You must be signed in to change notification settings - Fork 240
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 title to ManageIncidentOptions #372
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution. I do have one requested change to the tests.
setup() | ||
defer teardown() | ||
|
||
mux.HandleFunc("/incidents", func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to assert that Title
is serialized in the JSON and sent in the body. Can you update this handler to return an http.StatusBadRequest
if the title
JSON key is not the expected value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning http.StatusBadRequest
result in unclear errors HTTP response with status code 422 does not contain Content-Type: application/json
Having clear errors will require to use an helper function in order to set proper headers and serialize the error in the right format: {"error":{"message": "my error", "code": 2100}}
. This ends-up in mocking the api errors and testing the error handling code path which may complicate test maintenance in the future.
Looking at the code I found https://github.com/PagerDuty/go-pagerduty/blob/master/business_service_test.go#L100
This look like how http method test are handled https://github.com/PagerDuty/go-pagerduty/blob/master/client_test.go#L55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I get that. I've a desire to rework the tests to more clearly assert that we're matching the API's expectations, so I was trying to avoid having more places where I need to worry about testing for that in the future. To your point, there are no patterns of how to really do that well yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! I'll get it merged in shortly.
Looking at PagerDuty API doc it seems that the
title
attribute is missing fromManageIncidentOptions
https://developer.pagerduty.com/api-reference/b3A6Mjc0ODEzOQ-manage-incidents