Make non-mocked message publishing fail when running tests in bcd environment #5633
Comments
AdamWill
added a commit
to AdamWill/bodhi
that referenced
this issue
Apr 4, 2024
In working on fedora-infra#5630 I found that what happens when you don't properly mock out message publish attempts in tests isn't great. In CI, the test will hang for several minutes, then eventually proceed. Obviously we don't want to slow the tests down, and there is a 30 minute time limit on the test, so if you do this more than twice or so, the tests will time out. It's not very clear why the tests are timing out and failing - you have to guess that it's message publishing causing the problem. In bcd, the test doesn't hang, it just silently proceeds (I guess the message gets published to the rabbitmq instance in the bcd environment, I haven't checked). So you will not notice that you got it wrong until you submit a PR. Neither of those behaviors is great, so instead, let's mock out the 'real' message publishing function in fedora_messaging in a fixture that's automatically used by every test. We replace it with a mock that raises an exception with a hopefully- useful message. We have to mock _twisted_publish, not publish, because mock_sends also mocks _twisted_publish; if we mocked publish, we would break mock_sends. This way mock_sends gets to override us and work OK. Unfortunately this usually causes a database rollback error so you have to scroll back through some confusing errors to find the 'real' problem, but it's still an improvement, I think. Fixes: fedora-infra#5633 Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill
added a commit
to AdamWill/bodhi
that referenced
this issue
Apr 8, 2024
In working on fedora-infra#5630 I found that what happens when you don't properly mock out message publish attempts in tests isn't great. In CI, the test will hang for several minutes, then eventually proceed. Obviously we don't want to slow the tests down, and there is a 30 minute time limit on the test, so if you do this more than twice or so, the tests will time out. It's not very clear why the tests are timing out and failing - you have to guess that it's message publishing causing the problem. In bcd, the test doesn't hang, it just silently proceeds (I guess the message gets published to the rabbitmq instance in the bcd environment, I haven't checked). So you will not notice that you got it wrong until you submit a PR. Neither of those behaviors is great, so instead, let's mock out the 'real' message publishing function in fedora_messaging in a fixture that's automatically used by every test. We replace it with a mock that raises an exception with a hopefully- useful message. We have to mock _twisted_publish, not publish, because mock_sends also mocks _twisted_publish; if we mocked publish, we would break mock_sends. This way mock_sends gets to override us and work OK. Unfortunately this usually causes a database rollback error so you have to scroll back through some confusing errors to find the 'real' problem, but it's still an improvement, I think. Fixes: fedora-infra#5633 Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill
added a commit
to AdamWill/bodhi
that referenced
this issue
Apr 9, 2024
In working on fedora-infra#5630 I found that what happens when you don't properly mock out message publish attempts in tests isn't great. In CI, the test will hang for several minutes, then eventually proceed. Obviously we don't want to slow the tests down, and there is a 30 minute time limit on the test, so if you do this more than twice or so, the tests will time out. It's not very clear why the tests are timing out and failing - you have to guess that it's message publishing causing the problem. In bcd, the test doesn't hang, it just silently proceeds (I guess the message gets published to the rabbitmq instance in the bcd environment, I haven't checked). So you will not notice that you got it wrong until you submit a PR. Neither of those behaviors is great, so instead, let's mock out the 'real' message publishing function in fedora_messaging in a fixture that's automatically used by every test. We replace it with a mock that raises an exception with a hopefully- useful message. We have to mock _twisted_publish, not publish, because mock_sends also mocks _twisted_publish; if we mocked publish, we would break mock_sends. This way mock_sends gets to override us and work OK. Unfortunately this usually causes a database rollback error so you have to scroll back through some confusing errors to find the 'real' problem, but it's still an improvement, I think. Fixes: fedora-infra#5633 Signed-off-by: Adam Williamson <awilliam@redhat.com>
|
Fixed by #5634 which was just merged. |
mergify bot
pushed a commit
that referenced
this issue
Apr 15, 2024
In working on #5630 I found that what happens when you don't properly mock out message publish attempts in tests isn't great. In CI, the test will hang for several minutes, then eventually proceed. Obviously we don't want to slow the tests down, and there is a 30 minute time limit on the test, so if you do this more than twice or so, the tests will time out. It's not very clear why the tests are timing out and failing - you have to guess that it's message publishing causing the problem. In bcd, the test doesn't hang, it just silently proceeds (I guess the message gets published to the rabbitmq instance in the bcd environment, I haven't checked). So you will not notice that you got it wrong until you submit a PR. Neither of those behaviors is great, so instead, let's mock out the 'real' message publishing function in fedora_messaging in a fixture that's automatically used by every test. We replace it with a mock that raises an exception with a hopefully- useful message. We have to mock _twisted_publish, not publish, because mock_sends also mocks _twisted_publish; if we mocked publish, we would break mock_sends. This way mock_sends gets to override us and work OK. Unfortunately this usually causes a database rollback error so you have to scroll back through some confusing errors to find the 'real' problem, but it's still an improvement, I think. Fixes: #5633 Signed-off-by: Adam Williamson <awilliam@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Working on #5630 made me realize that, when running tests in the bcd environment, non-mocked attempts to publish messages 'work', silently and speedily. This is a problem, because they don't do that in the CI environment. They take a very long time and eventually time out; this doesn't usually cause the test to fail, but there is a 30 minute time limit so if you have more than a couple of cases of this, the tests will time out (and obviously we don't want even one, really).
Ideally, we should find a clean way to make non-mocked message publish attempts fail when testing - without breaking message publishing in the bodhi development environment itself. This may not be possible, but hey, we can try.
I don't think we can do this by fiddling with Bodhi's
notifications.pybecause the tests frequently use fedora-messaging's ownmock_sendsconvenience thingy, which requires us to publish 'as normal' all the way out to fedora-messaging. If you e.g. mock out_publish_with_retry, a lot of the tests fail because of this.I found a dirty way to make 'real' publishing fail - edit
fedora_messaging/api.pyin the bcd env and maketwisted_publishimmediately raise an exception when called. That's the hack I used to find all the cases in #5630 and fix them. But that's obviously not something we can bake in.The text was updated successfully, but these errors were encountered: