Feat/idsse 424/publish confirm start callback

[mackenzie-grimes-noaa]

Note: this is dependent on #26 being merged first <-- Merged now

Linear Issue

IDSSE-424

Changes

• Add optional callback to PublishConfirm.start()
  • Callback will be invoked after all RabbitMQ setup is complete and confirm delivery is enabled, which means messages can be published to the channel
  • If callback not provided, sleep() briefly before returning to mitigate the risk of race conditions

Previous usage of PublishConfirm all had a manual sleep between start() and usage, to ensure pika had usable RabbitMQ connection and channel, enabled delivery confirmations via RPC, etc.

class PubConfUsage:
    def __init__(self):
        self.pub_conf = PublishConfirm(...)
        self.pub_conf.start()
        sleep(1)
        self.pub_conf.publish_message({'data': 100})

Now, with these changes, the sleep() call above can be dropped, or the callback arg can be used like this:

class PubConfUsage:
    def __init__(self):
        self.pub_conf = PublishConfirm(...)
        self.pub_conf.start(callback=self.send_data)

    def send_data(self):
        print('Connected to PublishConfirm, starting to send data now')
        success = self.pub_conf.publish_message({'data': 100})

[Geary-Layne]

Looks like pika support callbacks on start, that's great. Paul should definitely take a look at this when he is back working on idsse. I have just the one comment on the last new test. We now need a task to remove the sleep() call after start in the DAS/EventPortMan code.

[Geary-Layne]

I don't have any ideas for improvements, but this doesn't look like a great test. As I understand it, the mock sleep is possibly called by code other than PublishConfirm.start(), so I don't see how you know that 'mock_sleep.assert_called()' is testing that start() actually called sleep, just that mock_sleep is called at least once.

[mackenzie-grimes-noaa]

Yeah, I went back and forth about this test.

I originally asserted that sleep was called just once (the sleep call I'm targeting inside PublishConfirm.start()), but then it only passed some of the time--probably a race condition between my unit test and internal Thread sleeps, or other unit test mechanics. The mock said it was either called once, or called 10000 times.

I'll revert to my previous attempt to see if it's more consistent

[mackenzie-grimes-noaa]

Ok, I updated the test now to manually count sleep calls only if it's for 0.2 seconds. It's ugly, and I don't like business logic being influenced by its testability, but that's the best way I found to uniquely pick out the sleep() call from PublishConfirm.start() vs others.

Running in debug mode, I saw my mock catching sleep(5) (from PublishConfirm.run()) and sleep(0.1) (probably from python Thread or unit test operations)

[Geary-Layne]

I'm with you, I don't know what way to go.
The previous test could pass if PublishConfirm.start() is never called.
This test could fail if something else calls sleep(0.2) in addition to PublishConfirm.start().
I think I'd go with too lenient and let it pass even if maybe it shouldn't. So I'd use something like assert.called_with(.2), which check that sleep is called with the same length of time that PublishConfirm.start() would call it with, but would still pass if something else also calls sleep(0.2).
Also is there a reason you didn't use assert.called_once_with(.2) vs the custom counter in the current code?
I don't want to hold this PR up anymore so do what you think is best and merge.

[mackenzie-grimes-noaa]

assert_called_once_with() just had a race condition where the unit test would sometimes pass, but sometimes fail with

 AssertionError: Expected 'mock' to be called once. Called 2 times.
E           Calls: [call(5), call(0.2)].

My latest code asserts that sleep was at least called with 0.2 seconds, but doesn't fail if an additional sleep call of 5 seconds was captured (from PublishConfirm.run()).

[rabellino-noaa]

Thanks for incorporating a callback approach, no further suggestions on my end