Codeception / module-amqp

AMQP module for Codeception
MIT License
3 stars 6 forks source link

Not acking or acking message in seeMessageInQueueContainsText causes random test results #11

Closed renq closed 3 years ago

renq commented 3 years ago

Hi,

in my company we use codeception for end-to-end testing and we found one nasty bug in the AMQP module. Messages that are read in the seeMessageInQueueContainsText method using the basic_get function are not acked or nacked which has strange side effects.

The seeMessageInQueueContainsText assertion reads a message, but does not cosume it or return it to the queue. In our test scenario, which took a few minutes in total, it turned out that we get success or failure depending on how fast the test scenario go. When our CI pipeline was running slower, the tests always fail because the assertion that the queue is empty was not met. It turned out that basic_get reads the message and then message has "unacked" state. The message returns back to the queue after a few minutes. That's why I think this behaviour is a bug.

I considered which way to go with the pull request - whether to do ACK or NACK? ACK would be more or less backward compatible, but the fact that an assertion consumes a message is not obvious. Comparing it to DB codeception module, lt would be as if the function that checks that the row exists at the end would delete it.

On the other hand, NACK with requeue option is not backward compatible, because the assertion checks the message and after that the message is returned to the queue. The user has to clear the queue themselves in this case.

I also wondered about the third parameter to the seeMessageInQueueContainsText method, so that you can decide whether you want to consume the message or return it. I also considered completely new method, but to be honest, I think it's the worst solution.

I would really appreciate your help on how best to solve this problem.

Here is my proposition of fixing that bug: https://github.com/Codeception/module-amqp/pull/10

Thanks!

renq commented 3 years ago

I decided to change nack with requeue to ack to make my change backward compatible.

Naktibalda commented 3 years ago

Released as 1.1.1