New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented support for RabbitMQ broker-initiated Basic.Cancel #82
Implemented support for RabbitMQ broker-initiated Basic.Cancel #82
Conversation
Hi @awestendorf, I completed my work on the RabbitMQ-specific broker-initiated Basic.Cancel, including unit and integration tests. Please review. I also refactored Best, |
@awestendorf, would you mind taking a look at PR #74 as well? It has several stability fixes as well as test fixes. Thank you. |
3955646
to
99c0f2c
Compare
Hi Aaron, I implemented integration tests for broker-initiated Basic.Return and Basic.Cancel temporarily under To be precise, I refactored
|
Implemented unit and integration tests for RabbitMQ broker-initiated Basic.Cancel. Created integration/channel_basic_test.py for Channel.basic integration tests and implemented test_unroutable_message_is_returned. Moved rabbit_extensions.py from scripts to 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. Added rabbitmq service to .travis.yml Moved scripts/integration directory to tests/
99c0f2c
to
99b2432
Compare
@awestendorf: I added rabbitmq service to |
@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. |
@awestendorf: as expected, the combination of this PR's changeset and #69 pass; see TEST PR #86 for test results. |
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. |
@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. |
Implemented support for RabbitMQ broker-initiated Basic.Cancel
Thank you Aaron! |
Implemented support for RabbitMQ broker-initiated Basic.Cancel
Implemented unit and integration tests for RabbitMQ broker-initiated Basic.Cancel.