Merge Shell Connector into Core

[FabioRosado]

Description

Since we have a PR #725 waiting for the shell connector to be merged into core I decided to take up the task to do it. @jacobtomlinson I know that you were thinking to do this yourself eventually, but perhaps you were thinking to modify some things on this connector?

I have made little changes to the connector, mostly some refactoring in order to get linting happy and merged the external functions into the connector class. Everything seems to be working ok as opsdroid works fine with this connector now.

I'm having an issue with testing the async_input method as I don't fully understand what is happening there. Also, line 49 is not being tested, but I assume that once the async_input method is tested properly this will pass.

If possible could you give me a hand testing this method in order to get Travis happy? 😄

Fixes #667

Status

READY | UNDER DEVELOPMENT | ON HOLD

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update (Google style docstrings still need to be implemented)
• Documentation (fix or adds documentation)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

• Tox all green - but coverage is reduced to 98%
• Opsdroid runs without any issues

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

[jacobtomlinson]

It looks like travis is happy but appveyor isn't. I imagine this is an issue to do with pipes on Windows. I would recommend exploring how to capture stdin with asyncio on Windows.

Also looking back through the code we seem to set up a reader and writer. However we only use the reader as we are writing using print. I expect this is probably because this code is copied from stack overflow 😄. You may want to remove the writing code to simplify things, that may also help with your testing.

[FabioRosado]

Haha okay I will work on this more tomorrow and try to figure out how to solve some of these issues 👍

[codecov]

Codecov Report

Merging #735 into master will decrease coverage by 0.21%.
The diff coverage is 93.1%.

@@            Coverage Diff             @@
##           master     #735      +/-   ##
==========================================
- Coverage     100%   99.78%   -0.22%     
==========================================
  Files          27       28       +1     
  Lines        1804     1862      +58     
==========================================
+ Hits         1804     1858      +54     
- Misses          0        4       +4

Impacted Files	Coverage Δ
opsdroid/connector/shell/__init__.py	93.1% <93.1%> (ø)

---

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 e1f004e...7e72875. Read the comment docs.

[FabioRosado]

I've updated the stdio method by removing all the writer references, I tried to bump the coverage but I kept getting a lot of issues when trying to test this method so I am still at loss as to how to properly test it.

I get issues with readlines, then with replace, I tried to mock the readlines from asyncio.StreamReader and I guess I could move the decode('utf8').replace(...) into an helper function and then mock that as well, but that causes the with statement to be massive in the tests.

Any idea how to test this bit, because I have no idea haha

I will fix the linting issues on the next commit

[jacobtomlinson]

Yeah I would just mock that stuff out, don't worry too much about a large with statement. You could always use the patch decorator if you prefer.

I wouldn't move the string cleaning stuff out, that shouldn't be causing an issue.

[FabioRosado]

I'm pondering to go and just close this PR. I have been trying for the whole week to figure out how to test these two functions without any success. This last commit bumps the coverage but its nothing but a hack to do it.

With the async_input I either get an issue on the readline, or i get an issue with that return statement due to the replace() chaining

Also, this connector will never work on windows as it is. I tried to play around with it in order to make it work on windows, after long hours reading stackoverflow the solution might pass to go and use run_in_executor method, but that only the rabbit hole further as I get another exception DoNotReadFromInput with every little change i try to make

[jacobtomlinson]

Perhaps we should scrap the shell connector all together and use websockets and opsdroid desktop as the default.

[jacobtomlinson]

As discussed on matrix we could also move this into a separate application (called opsdroid-client or similar) and connect using websockets like opsdroid desktop does. That way we could avoid all the asyncio stdin/stdout issues we are having and implement it more simply.

[FabioRosado]

I'm giving this bit a test (even if its buggy to sigkill opsdroid) I'm curious to see if things will okay okay-ish on windows, if they do i'll worry about the rest later.

On the testing bit, I am still failing to see how to test the return statements properly since tox keeps complaining about it.

Since this PR has caused a bit of a headache i might just update the shell connector until we decide to implement that client idea.

[FabioRosado]

AppVeyor is calling the wrong bit of code, but yeah I guess this wont work either.