fix: improve default error handling #1379 #1409#1439
Conversation
Alexj9837
force-pushed
the
chore/improve-default-error-handling-1379-1409
branch
from
accdd8c to
c5ab6b3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1439 +/- ##
==========================================
+ Coverage 95.19% 95.26% +0.07%
==========================================
Files 43 43
Lines 3119 3125 +6
==========================================
+ Hits 2969 2977 +8
+ Misses 150 148 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Alexj9837
force-pushed
the
chore/improve-default-error-handling-1379-1409
branch
from
c5ab6b3 to
883b09e
Compare
Alexj9837
force-pushed
the
chore/improve-default-error-handling-1379-1409
branch
from
883b09e to
05150ae
Compare
| class BlueskyRemoteControlError(BlueskyRequestError): | ||
| pass |
There was a problem hiding this comment.
This can stay as Exception
Because you are not using the code part of it in the 3 exception that are changed above.
| class NotFoundError(BlueskyRequestError): | ||
| pass |
There was a problem hiding this comment.
I coundnt understand what peter said in the issue but it
look like the issue was to change the name from KeyError to
NotFoundError which make sense in more case ?
| return BlueskyRequestError(code, str(response)) | ||
| else: | ||
| return BlueskyRequestError(code, response.text) | ||
| return BlueskyRequestError(code, str(response)) |
There was a problem hiding this comment.
These can stay as response.text
and same for the above
| def test_get_non_existent_plan(rest_client: BlueapiRestClient): | ||
| with pytest.raises(KeyError, match="{'detail': 'Item not found'}"): | ||
| with pytest.raises(NotFoundError): |
There was a problem hiding this comment.
Can the match string be added back ?
it is good to check the type of error but if we have match details we should check for them as well
|
|
||
| def test_create_task_validation_error(rest_client: BlueapiRestClient): | ||
| with pytest.raises(BlueskyRequestError, match="Internal Server Error"): | ||
| with pytest.raises(BlueskyRequestError): |
| isinstance(result.exception, BlueskyRemoteControlError) | ||
| and result.exception.args == ("Response [450]",) | ||
| and result.exception.args[1] == "Response [450]" |
There was a problem hiding this comment.
This can stay same as before and if other new errors are added
We can check for them
| ) | ||
| assert result.exception.args[0] == "Failed to tear down the environment" | ||
| assert result.exception.args[1] == "Failed to tear down the environment" |
There was a problem hiding this comment.
What exception is in args[0] ?
| (401, None, UnauthorisedAccessError(401, "")), | ||
| (403, None, UnauthorisedAccessError(403, "")), | ||
| (404, None, UnknownPlanError(404, "")), |
There was a problem hiding this comment.
nit: Content can also be checked
ZohebShaikh
left a comment
ZohebShaikh
left a comment
There was a problem hiding this comment.
Looks good only few things can be tidied up
2 participants
Fixes #1379
Fixes #1409
Both issues related to error handling in the REST client.
1379 - Catches 'UnauthorisedAccessError
explicitly instead of string-matching onBlueskyRemoteControlError`.1409 - Expands the exception hierarchy so each
HTTP status code raises a distinct typed exception rather than bundling
everything into
BlueskyRemoteControlError.Tests
runcommand