Log a connector exit

[Cadair]

Description

If a connector exits, especially if it exits with an error we should log this. This adds a done callback on the listener task which logs the exception is one is raised. If the task was cancelled we do not log it as if the task is cancelled opsdroid is shutting down.

I am not sure how to write a unit test for this?

Status

READY

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• Test A

Make a connector raise an exception which is not caught, i.e. to exit the coroutine with an exception.

• Test B

Make a connector where the listen coroutine returns a value.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[coveralls]

Coverage decreased (-0.6%) to 96.76% when pulling f386b57 on Cadair:log_connector_exit into d71873c on opsdroid:master.

[codecov]

Codecov Report

Merging #348 into master will decrease coverage by 0.56%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
- Coverage     100%   99.43%   -0.57%     
==========================================
  Files          17       17              
  Lines         877      886       +9     
==========================================
+ Hits          877      881       +4     
- Misses          0        5       +5

Impacted Files	Coverage Δ
opsdroid/core.py	97.23% <44.44%> (-2.77%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11f130a...4ab32ae. Read the comment docs.

[coveralls]

Coverage decreased (-0.6%) to 96.76% when pulling 0d34746 on Cadair:log_connector_exit into d71873c on opsdroid:master.

[coveralls]

Coverage decreased (-0.6%) to 96.763% when pulling 70b1925 on Cadair:log_connector_exit into d71873c on opsdroid:master.

[coveralls]

Coverage decreased (-0.6%) to 96.763% when pulling 1544424 on Cadair:log_connector_exit into d71873c on opsdroid:master.

[FabioRosado]

I believe that You could test this by creating a side_effect and set it to future.exception() (like I’ve done when testing for the aiohttp.ClientOsError() on the parsers), then mock the logging call and see if the logging function was called.

I’m on the phone now so I can’t really give you links, but I’ll edit this post once I’m at home 👍

--EDIT--
As promised here's the code to give you an idea how to test for those things:

Mocking ClientOSError()

[...]
with amock.patch.object(luisai, 'call_luisai') as mocked_call:
    mocked_call.side_effect = ClientOSError()

    # Call the parser, this will make the side_effect run and return a ClientOSError()
    await luisai.parse_luisai(opsdroid, message, opsdroid.config['parsers'][0])

    # Assert if the call was successful, the test should show the ClientOSError message when it passes
    self.assertTrue(mocked_call.called)

Logging call test

def test_welcome_message(self):
    config = {"welcome-message": True}
    with mock.patch('opsdroid.__main__._LOGGER.info') as logmock:
    opsdroid.welcome_message(config)
    self.assertTrue(logmock.called)

The logging call test is pretty much similar to the clientOSError one. Hope this helps you to create your tests, let me know if you need any further help 👍

[jacobtomlinson]

👍 this is a great idea.

Agree with @FabioRosado on testing methods.

Also a couple of questions:

• Why is this function in the core? Would it not be better in the connector base class?
• Why are you using partials?

Also I wonder if it would be useful for connectors to have enter and exit methods and to be instantiated with an async with instead. Then we could handle the close better.

[Cadair]

@jacobtomlinson

Why is this function in the core? Would it not be better in the connector base class?

The issue is if the listen method exits, as core expects it to be an infinite loop. So the natural place for this felt like core as that is where the Task is created. I am not sure the context manager protocol would help without some major refactoring as again, this is to catch the listen method not the whole object. I do wonder as I am typing this if we should call the disconnect method when the listen method exits.

Why are you using partials?

The Python documentation on add_done_callback explicitly tells you to use partials to pass parameters to the callback here.

[jacobtomlinson]

@Cadair happy with your comments.

Could you please look at adding tests so I can get this merged.
Cheers!

[FabioRosado]

@Cadair I've updated my post, if you still need help creating tests let me know and I'll give you a hand 👍

[jacobtomlinson]

@Cadair any update on this?

[Cadair]

Sorry, other things have stolen my attention. I will try to get back to it asap.

[jacobtomlinson]

It's been more than 30 days since the last activity on this PR. I'm going to close it but please feel free to reopen it if you wish to continue this work.