zulip: Reraise exceptions in do_api_query.

[eeshangarg]

There are cases where the call to get_server_settings returns a JSON response with a connection error. In the case of such an unsuccessful response, we should raise an appropriate exception instead of parsing the response as though it was successful.

@timabbott Could you please take a look at this? Not sure if this is the right fix. Also, wondering if I should put a test for this somewhere in zulip_botserver?

[timabbott]

This looks correct to me. @andersk FYI.

For testing, I would prefer a test for the main zulip module, since the code is for that module. I'm not sure we are setup to make it easy to write such a test, but at the very least I'd like to see the output from trying to connect to a non Zulip server with this.

[timabbott]

@neiljp I was realizing it'd be helpful to have your review on this, since zulip-terminal probably cares about this aspect of error handling.

[eeshangarg]

@andersk I just addressed your feedback. Thanks!

@timabbott I am honestly not sure how to write a test for this within the module. Seems like it might be better to write a test for this server-side? Should we maybe write some sort of a mocking framework that mimics a Zulip server and raises exceptions at certain points?

[timabbott]

A server-side test that we run alongside test-api in CI could work. If we want to write proper tests, we need to write a bit of a test framework, which is probably out of scope for this issue.

Grepping for connection-error call_on_each_event makes use of the old error handling convention, and needs to be ported to handle exceptions. We can test the ported version manually by just pointing a bot at the development environment and trying things like ctrl+C on the development server to generate connection errors.

(It's an important part of the call_on_each_event interface that it will continue running even if you get connection errors once it has started, so that your bot using that API will continue working even if the server has temporary downtime)

[eeshangarg]

@timabbott This PR is mostly ready for review, just needs to be tested manually. Thanks!

[eeshangarg]

A summary of next steps or what needs to be done:

• This PR needs to be manually tested.
• As for automated testing, we need to write a test framework that mimics a Zulip server to be able to test this in the same repository.
• In the absence of such a test framework, a server-side test that we run in something like tools/test-api would probably work.

Thanks!

[eeshangarg]

Forgot to mention that the previous feedback has all been resolved on thsi PR! :)

[timabbott]

Merged, thanks @eeshangarg for your work on this.