-
Notifications
You must be signed in to change notification settings - Fork 13
Fix error-handling to use new gRPC exceptions #131
Conversation
Previously, we were catching and wrapping AbortionError, which is no longer used by gRPC. Instead, use RpcError.
Current coverage is 98.02% (diff: 100%)@@ master #131 diff @@
==========================================
Files 8 8
Lines 608 608
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 591 596 +5
+ Misses 17 12 -5
Partials 0 0
|
Surprised how easy this fix was! |
2bb75ae
to
dbf531f
Compare
dbf531f
to
69efb57
Compare
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.
Just one comment. Otherwise LGTM.
self.assertEqual(code, grpc.StatusCode.UNKNOWN) | ||
self.assertEqual(code, grpc.STATUS_CODE_NAMES['UNKNOWN']) | ||
self.assertIsNone(grpc.exc_to_code(Exception)) | ||
self.assertIsNone(grpc.exc_to_code(grpc.RpcError())) |
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.
Should probably also test an RpcError
that contains a code.
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.
It already does: MyError
is an RpcError
that contains a code (StatusCode.UNKNOWN
).
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.
Oh, got it!
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.
LGTM
self.assertEqual(code, grpc.StatusCode.UNKNOWN) | ||
self.assertEqual(code, grpc.STATUS_CODE_NAMES['UNKNOWN']) | ||
self.assertIsNone(grpc.exc_to_code(Exception)) | ||
self.assertIsNone(grpc.exc_to_code(grpc.RpcError())) |
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.
Oh, got it!
Previously, we were catching and wrapping AbortionError, which is no
longer used by gRPC. Instead, use RpcError.
/cc @dhermes
Fixes #129 and #130