Move shell connector into core #1147
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1147 +/- ##
==========================================
+ Coverage 99.91% 99.92% +<.01%
==========================================
Files 46 47 +1
Lines 2490 2558 +68
==========================================
+ Hits 2488 2556 +68
Misses 2 2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This is really awesome many thanks! Thanks for also taking the time to update the docs. I only have one comment about the ordering of an index that you changed.
Just to check as you didn't mention it in the testing. Did you try it by just running opsdroid with the shell connector and talking to it? Running tox and building the container is great, but I'd like to hear that you actually used it too.
Do my tests really test or are they only walking thru all lines of code?
There is definitely a difference between 'testing that the code behaves as expected' and 'testing that the code does exactly what the code says it does'. This project definitely has some bad tests that check the code too ridigly, this may be my fault for insisting on high coverage.
The fact that you are asking this question is a good thing and shows you are aware of this problem. Your assertions look good and that's the best way to know if your tests are effective. If you are asserting things that the user will see (does the prompt look as expected, does the bot respond to mocked stdin, etc) then you're doing fine. If you call a function and then check that the function was called, not so fine. I'm generally happy with these tests though.
Add a deprecation notice to the old connector. (See the slack connector) ?? not really sure what you mean ??
In the legacy slack connector README there is a warning message that it is deprecated. We need to add that to the shell one now too.
| - 'Redis': 'databases/redis.md' | ||
| - 'SQLite': 'databases/sqlite.md' | ||
| - 'Matchers': |
There was a problem hiding this comment.
The matchers section here was actually in an intentional order. They were broken into three sections, 'Simple' (regex, parse), 'Third party NLU' (everything up until wit) and then 'Misc' (things that aren't chat related).
Perhaps we should make this more explicit with another level of nesting?
There was a problem hiding this comment.
Oh I didn't know, I just thought 'why not order them alphabetically' when i wanted to add the shellconnector to connectors and wasn't sure on wich position it should be added. Then looked at databases and matchers, 'why not reorder them too'.. I will add the extra nesting
Good point, I didn't. As it turns out I removed a return statement which I thought is not really needed which then broke the module, adding it back works. But somehow I can't get that return covered (yet).
So after this PR gets merged i just post a new PR to that repo to update the readme or do i miss something? |
Great thanks for checking. Shout if you need any help.
Yep a PR to that README is what I'm after. Feel free to set it up now, I can then merge them at the same time. |
jos3p
force-pushed
the
fix-667-shellconnector2core
branch
from
0741a71
to
01f3a98
Compare
|
after finally finding my problem with the not covered return (which was caused by not patching await self.loop.connect_read_pip(), i used wrong target all the time, took me more time than needed) i think i messed up the rebase but it still works |
|
This is amazing thank you so much for taking the time to fix this issue and merge the shell into the core - which is my favourite connector haha 馃槃 I've noticed that you addressed all the requests from Jacob, so thank you for working on this as well. I'm going to merge this PR now. We love to thank contributors by sending them opsdroid stickers! Fill in the claim form to get yours! |
|
thank you, i now have also filled in the form ;) |
Description
Hello again 馃帀 ,
this is the first time i take a look at testing so i appreciate tips+comments.
Do my tests really test or are they only walking thru all lines of code?
I took the test file from #735 and tried to expand on it with the help of googling and stackoverflow
from the issue thread:
changes:
mkdocs.yml
configuration-reference.md
example_config
Fixes #667
Status
READY
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist: