Add connection error support#377
Conversation
…nd each clients tests class extends from it
…comodate connection errors handling)
sjaensch
left a comment
There was a problem hiding this comment.
Nice, this looks good to me! One small change and then 🛳!
| ), | ||
| ) | ||
| timeout_errors = tuple(self.future.timeout_errors or ()) | ||
| connection_errors = tuple(self.future.connection_errors or ()) |
There was a problem hiding this comment.
Could we make this code backwards compatible? I think something like
connection_errors = getattr(self.future, 'connection_errors', ())
I don't think it's a problem to use whatever the value of that attribute is if it is defined.
There was a problem hiding this comment.
Sure ... keeping backward compatibility is something to aim for :)
I'm not against using getattr but I would understand how the change could provide backward incompatible changes (so next time I'll think to it too)
There was a problem hiding this comment.
We talked about it and well the change is backward compatible as self.future in an instance of bravado.http_future. FutureAdapter which defines those attributes.
So even if http clients are not defining them the attribute will be available with an empty tuple as default value
The main objective of this PR is to provide support to
ConnectionErrors as is currently provided forTimeoutErrors.This aims to address also @bplotnick suggestions in PR #346.
Details of this PR code changes
TestServerRequestsClient. This is intended done in order to move the base integration tests suite intobravado.testingmodule so external http clients (ie. bravado-asyncio) could be able to run the same tests that we run for the http clients provided by bravado.NOTE: moving integration tests into
bravado.testingis not addressed here, a new PR will be provided in the futuretimeout_errorscontained a single item, but with connection errors (at least with fido client) we have more that one exception class