search

ruby-amqp / hutch

A system for processing messages from RabbitMQ.
https://gocardless.com/blog/hutch-inter-service-communication-with-rabbitmq/
MIT License
857 stars 137 forks source link

Add error handler class #302

Closed valentin-krasontovitsch closed 6 years ago

valentin-krasontovitsch commented 6 years ago

To handle setup exceptions, a method was added to each error handler.

Said method is subsequently called when setup of the client fails.

This is to ensure that a failed setup is not only logged, but also reported to e.g. sentry.

Furthermore, a base class indicating the interface for error handlers is added.

Closes #288

valentin-krasontovitsch commented 6 years ago

I can’t say that I didn’t look at adding a test, but I was a wee bit stumped as to what it might look like. You got me ^^

I’ll take another look and try adding a test : )

On Wed, 6 Dec 2017 at 16:28, Michael Klishin notifications@github.com wrote:

@michaelklishin requested changes on this pull request.

Please add a test case, as requested in the comments.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gocardless/hutch/pull/302#pullrequestreview-81559350, or mute the thread https://github.com/notifications/unsubscribe-auth/APMdBdy8YREX9HLVW7nKZJvMg-IL2Kcwks5s9rK4gaJpZM4Q2Z9z .

michaelklishin commented 6 years ago

@valentin-krasontovitsch thank you.

valentin-krasontovitsch commented 6 years ago

So I just pushed a commit that introduces a test case, making sure that if Hutch.connect throws an error, that error is passed to all backends.

I'm not super happy with the test, as it relies on the implementation of the tested method: start_work_loop is tested, and we assume that the method calls Hutch.connect, which in turn is used to raise an exception.

If you have any pointers or ideas about how this could be tested in a less brittle way, let me know.

Maybe the coupling to the implementation is OK in this case...

valentin-krasontovitsch commented 6 years ago

So I'm basically happy with the other merge request merged in, but just for completeness sake: Any new thought / comments on the test I wrote?

michaelklishin commented 6 years ago

@valentin-krasontovitsch sorry, this slipped through the cracks. What's the other PR you are referring to? Is this one still relevant and would you like us to review it?

valentin-krasontovitsch commented 6 years ago

no worries : ) the other PR was #301, included the first two commits of this merge request, and got merged immediately - while here (rightfully) the lacking unit tets was pointed out (and mitigated).

I guess as you've merged this one now, i don't need to answer your second question anymore ^^