Unit test on jwt exception type instead of string #3417
Conversation
| assert ( | ||
| str(exc.value.__cause__) == "Signature has expired" | ||
| ), f"{exc.value.__cause__}" | ||
| assert exc.value.exc_type == jwt.ExpiredSignatureError, f"{exc.value.exc_type}" |
There was a problem hiding this comment.
assert isinstance(exc.value.__cause__, jwt.ExpiredSignatureError) would allow you to keep using the raise [] from e.
And in any case, the comparison should be is rather than ==.
lib/pbench/server/auth/__init__.py
Outdated
| class OpenIDTokenInvalid(Exception): | ||
| pass | ||
| def __init__(self, exc_type: Exception): | ||
| self.exc_type = exc_type | ||
|
|
||
| def __str__(self) -> str: | ||
| return str(f"Token invalid with exception: {self.exc_type}") |
There was a problem hiding this comment.
While I like the idea of having "our exception" be a wrapper for the underlying exception (e.g., having the stringification of our exception include the stringification of the underlying exception), the argument to the c'tor should be an exception instance, not an exception type -- when we stringify the underlying exception, any arguments with which it was created should be reflected in the string, and that won't happen if we reference the class instead of the instance.
Also, in general, when you derive a subclass from a superclass, you should invoke the superclass c'tor in the subclass c'tor (this is implicit in many languages, but not in Python). Moreover, the Exception class already provides a __str__() method which we can make use of, here. E.g.:
class OpenIDTokenInvalid(Exception):
def __init__(self, exc: Exception):
super().__init__(f"Token invalid with exception: {exc}")That is, str(OpenIDTokenInvalid(e)) will use Exception.__str__() and it will return Token invalid with exception: followed by the stringification of e, and you don't need any more code.
There was a problem hiding this comment.
I just reverted this change. If we use isinstance to check the type of the cause in the unit test, we don't need to touch this exception class.
There was a problem hiding this comment.
That's a good sign that using isinstance() is the right path. 😁
However, doing the super() call is "more correct" than the current code, and it enables direct (and useful) stringification of the resulting instance. (The current implementation will inherit the __str__() method, but, since it doesn't call the superclass c'tor, the method will produce an empty string, which is quite unsatisfying.)
There was a problem hiding this comment.
OK, this change is fine (even "good"), but it would be "better" if we took the opportunity to improve the implementation of OpenIDTokenInvalid (but, I'm not going to block this PR on that basis).
…istributed-system-analysis#3417) * Check jwt exception type instead of string in the unit tests Jwt exception (specifically jwt.exception.InvalidAudienceError) has changed its string message from `Invalid Audience` to `Audience doesn't match`. In the unit test instead of checking the string message check the type of the exception.
…3417) (#3424) * Check jwt exception type instead of string in the unit tests Jwt exception (specifically jwt.exception.InvalidAudienceError) has changed its string message from `Invalid Audience` to `Audience doesn't match`. In the unit test instead of checking the string message check the type of the exception. Co-authored-by: Nikhil Palaskar <[email protected]>
Jwt exception (specifically
jwt.exception.InvalidAudienceError) has changed its string message fromInvalid AudiencetoAudience doesn't match. In the unit test instead of checking the string message check the type of the exception so we don't run into these problems again.