-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Submit handles non 2xx responses #793
Conversation
cmd/submit.go
Outdated
@@ -261,6 +261,10 @@ func (s *submitCmdContext) submit(metadata *workspace.ExerciseMetadata, docs []w | |||
return fmt.Errorf(jsonErrBody.Error.Message) | |||
} | |||
|
|||
if resp.StatusCode < 200 || resp.StatusCode > 299 { | |||
return fmt.Errorf("submission unsuccessful (%s)", resp.Status) |
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.
I'm guessing we want this to be more useful; what should this say?
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.
The API should respond with {"error": {"type": "some_value", "message": "A human-readable message"}}
in the case of an error. Let's use the message
if it has something available in it, otherwise let's use the default that you have here.
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.
Actually, you wrote code that fixed it. I've extracted that here:
#814
8f6551f
to
4b23582
Compare
@jdsutherland Now that we've gotten some of the helpers into master, would it make sense to rework this to use the helpers? |
1c3d8a9
to
4b23582
Compare
Previously, 404s silently continue to misleading success output
4b23582
to
10aea95
Compare
This is ready for another look. |
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 looks great! Thanks for your patience here 💙
@@ -611,6 +611,44 @@ func TestSubmitServerErr(t *testing.T) { | |||
assert.Regexp(t, "test error", err.Error()) | |||
} | |||
|
|||
func TestHandleErrorResponse(t *testing.T) { |
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 adding this test!
Previously, 404s silently continue to misleading success output. This change throws an error for all non 2xx responses.
Closes #790.