-
Notifications
You must be signed in to change notification settings - Fork 247
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
chore(DataPlanePublicAPI): Error message handling #4678
base: main
Are you sure you want to change the base?
chore(DataPlanePublicAPI): Error message handling #4678
Conversation
Changed error handling method to be returned directly as list, instead of being concatenated first and then wrapped to the list
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.
We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contributors manual, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.
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.
Please provide unit tests
Minor adjustment in returned value of should_returnInternalServerError_if_transferFails test, adding test case for should_returnListOfErrorsAsAResponse_if_anythingFails
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 PR does not properly test the changes. Please fix this.
.then() | ||
.statusCode(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()) | ||
.contentType(JSON) | ||
.body("errors", is(List.class)); |
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 test does not properly verify the change. Compare it to how the original test verifies the error messages.
Fix in unit test matcher
.then() | ||
.statusCode(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()) | ||
.contentType(JSON) | ||
.body("errors", isA(List.class)); |
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 still does not verify multiple messages are included. The test is only verifying a list type, which has not changed.
Added detailed assertions to an unit test should_returnListOfErrorsAsAResponse_if_anythingFails
|
||
baseRequest() | ||
var jsonPath = baseRequest() |
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.
Why is this called jsonPath?
In the future, please:
Thx |
Code cleanup, renamed variable
This pull request is stale because it has been open for 7 days with no activity. |
This pull request was closed because it has been inactive for 7 days since being marked as stale. |
…sponse_refactor' into DataplaneControllerAPIV2_errorResponse_refactor
…sponse_refactor' into DataplaneControllerAPIV2_errorResponse_refactor
…sponse_refactor' into DataplaneControllerAPIV2_errorResponse_refactor
What this PR changes/adds
Changed error handling method to be returned directly as list, instead of being concatenated first and then wrapped to the list
Why it does that
In order to make an error response being more readable
Further notes
N/A
Linked Issue(s)
Closes #4670