Implemented support for RabbitMQ broker-initiated Basic.Cancel

agoragames / haigha

AMQP Python client

BSD 3-Clause "New" or "Revised" License

161 stars 41 forks source link

Implemented support for RabbitMQ broker-initiated Basic.Cancel #82

Closed vitaly-krugl closed 9 years ago

vitaly-krugl commented 9 years ago

Implemented support for RabbitMQ broker-initiated Basic.Cancel Implemented unit and integration tests for RabbitMQ broker-initiated Basic.Cancel.

vitaly-krugl commented 9 years ago

Hi @awestendorf, I completed my work on the RabbitMQ-specific broker-initiated Basic.Cancel, including unit and integration tests. Please review.

I also refactored scripts/rabbit_extensions into a unittest-based framework and added an integration test for the broker-initiated Basic.Cancel feature: https://github.com/vitaly-krugl/haigha/blob/consumer-cancel-notify/scripts/rabbit_extensions.

Best, Vitaly

vitaly-krugl commented 9 years ago

@awestendorf, would you mind taking a look at PR #74 as well? It has several stability fixes as well as test fixes. Thank you.

vitaly-krugl commented 9 years ago

Hi Aaron, I implemented integration tests for broker-initiated Basic.Return and Basic.Cancel temporarily under scripts/integration/. Please review. When you create the integration directory under haigha/tests/, I can move these tests there prior to merging of this PR.

To be precise, I refactored scripts/rabbit_extensions into unittest-based test and implemented the broker-initiated Basic.Cancel test along with my original Basic.Cancel work. I subsequently made the following changes:

Created scripts/integration/channel_basic_test.py for Channel.basic integration tests and implemented test_unroutable_message_is_returned.
Moved scripts/rabbit_extensions to scripts/integration/rabbit_extensions_test.py for RabbitMQ-specific integration tests, removed option parsing logic, and implemented additional test test_unroutable_message_is_returned_with_puback.

vitaly-krugl commented 9 years ago

@awestendorf: I added rabbitmq service to .travis.yml and moved scripts/integration/ to tests/. Awaiting travis test results now. F.Y.I.

vitaly-krugl commented 9 years ago

@awestendorf: the integration tests are now getting tripped up by "TypeError: getsockaddrarg() takes exactly 2 arguments (4 given)" when connecting to RabbitMQ broker. This should be addressed by @kevinconway's PR #69.

Aaron, I reviewed #69, and found only a few superficial issues with it. It should work as-is. Once you accept #69, I will rebase my Basic.Cancel PR against master.

vitaly-krugl commented 9 years ago

@awestendorf: as expected, the combination of this PR's changeset and #69 pass; see TEST PR #86 for test results.

vitaly-krugl commented 9 years ago

UPDATE: Hi Aaron, everything is in place for this PR, as you requested (integration tests and all). We've been running with the Basic.Cancel change in production problem-free for several months now. This PR now depends on @kevinconway's PR #69 in order to pass the integration tests (due to RabbitMQ having an IPv6 address on Travis). This is demonstrated by my passing test PR #86 that combines this PR with #69.

I think that both PRs (this and #69) are now ready to make the leap into master and release. Is there anything else that needs to be done? Thx.

awestendorf commented 9 years ago

@vitaly-krugl thank you, sorry for the delay. Between work and summer vacation these threads fell off my radar. I haven't been able to exhaustively test myself, but I reviewed a long time ago and I think everything is in place to ship these features.

vitaly-krugl commented 9 years ago

Thank you Aaron!