Refactor connector opsdroid pointer #749

zonagilreath · 2018-11-21T16:58:02Z

Description

Slight update to previous request

Fixes #725

Status

READY

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Documentation (fix or adds documentation)

How Has This Been Tested?

Tox tests passing

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

… core OpsDroid

codecov · 2018-11-21T17:04:55Z

Codecov Report

Merging #749 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #749   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          28     28           
  Lines        1856   1850    -6     
=====================================
- Hits         1856   1850    -6

Impacted Files	Coverage Δ
opsdroid/connector/github/__init__.py	`100% <100%> (ø)`	⬆️
opsdroid/connector/facebook/__init__.py	`100% <100%> (ø)`	⬆️
opsdroid/connector/rocketchat/__init__.py	`100% <100%> (ø)`	⬆️
opsdroid/core.py	`100% <100%> (ø)`	⬆️
opsdroid/connector/slack/__init__.py	`100% <100%> (ø)`	⬆️
opsdroid/connector/websocket/__init__.py	`100% <100%> (ø)`	⬆️
opsdroid/connector/__init__.py	`100% <100%> (ø)`	⬆️
opsdroid/connector/telegram/__init__.py	`100% <100%> (ø)`	⬆️

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 62513dc...decd15e. Read the comment docs.

FabioRosado · 2018-11-21T17:14:21Z

I've created a PR in your fork with the changes that I tried to push 👍

zonagilreath · 2018-11-27T15:55:04Z

@FabioRosado I'm sorry I didn't see that before, last week was pretty busy with work and the holiday. Do I still need to merge that PR, or is there anything else I need to do now?

FabioRosado · 2018-11-28T08:08:21Z

Hello @zonagilreath my apologies for not replying yesterday. Yeah that PR still needs to be merged into your opsdroid fork - for what tox told me everything should pass once that PR is merged and I'm sure this one will be updated as soon as you merge my PR into yours then we can see if we can get this merged without any issues 😄 👍

Update opsdroid pointer

zonagilreath · 2018-11-28T14:43:00Z

Okay, done. But there's a conflict on the listen method of each of the connectors, and in a few of the tests and I'm not sure how they should be resolved.

FabioRosado · 2018-11-28T14:46:09Z

Yeah this week we changed a few things related to the logging and connectors, I have fixed the conflicts for you 😄 👍

zonagilreath · 2018-11-28T14:48:16Z

Great, hope everything is resolved now

FabioRosado · 2018-11-28T14:53:06Z

Hmmm this is odd everything is broken now but my tox was showing green o.O

zonagilreath · 2018-11-28T14:58:34Z

Yeah, it looks like one issue is that the connector.listen() is still getting a None argument, which I thought I had fixed. Should I get that fixed?

FabioRosado · 2018-11-28T14:59:54Z

I didn't have time to check exactly what was wrong here but if you have the time that would be great 👍

zonagilreath · 2018-11-28T15:40:24Z

I'm about to test the rest, but there's one error I don't understand:

self = <test_connector_slack.TestConnectorSlackAsync testMethod=test_listen_loop>
    async def test_listen_loop(self):
        """Test that listening consumes from the socket."""
        connector = ConnectorSlack({"api-token": "abc123"}, opsdroid=OpsDroid())
        connector.receive_from_websocket = amock.CoroutineMock()
        connector.receive_from_websocket.side_effect = Exception()
        with self.assertRaises(Exception):
            await connector.listen(amock.CoroutineMock())
>       self.assertTrue(connector.receive_from_websocket.called)
E       AssertionError: False is not true
tests/test_connector_slack.py:102: AssertionError```

FabioRosado · 2018-11-28T15:43:20Z

That amock.CoroutineMock() shouldnt be there. Im assuming that if we take that bit out it will work.

Usually we either call opsdroid with with OpsDroid or use opsdroid = amock.CoroutineMock() so that test sort of uses a short hand for opsdroid

zonagilreath · 2018-11-28T15:49:58Z

Oh okay. I was wrong earlier about having fixed the arguments for listen() in all the tests. I hadn't changed the signature of listen() in the first place, I think that came in when I merged your PR.

zonagilreath · 2018-11-28T15:58:34Z

Now everything is passing but this:

____________________ TestConnectorGitHubAsync.test_connect ____________________

self = <test_connector_github.TestConnectorGitHubAsync testMethod=test_connect>

    async def test_connect(self):
        opsdroid_mock = amock.CoroutineMock()
        with amock.patch('aiohttp.ClientSession.get') as patched_request:
            mockresponse = amock.CoroutineMock()
            mockresponse.status = 200
            mockresponse.json = amock.CoroutineMock(return_value={
                "login": 'opsdroid'
            })
            patched_request.return_value = asyncio.Future()
            patched_request.return_value.set_result(mockresponse)
            await self.connector.connect()
            self.assertEqual(self.connector.github_username, "opsdroid")
>           self.assertTrue(opsdroid_mock.web_server.web_app.router.add_post.called)
E           AssertionError: False is not true

tests\test_connector_github.py:61: AssertionError

FabioRosado · 2018-11-28T15:58:58Z

Yeah I changed the listen method since opsdroid was being passed as an argument for the class, I thought I had changed all the listen texts since tox didn't complain on my end but I'm not sure now

zonagilreath · 2018-11-28T16:32:27Z

Yeah that's weird that they weren't caught by the tests before. Got them fixed now, but I don't understand what's supposed to be happening with the above test well enough to debug. I'll go ahead and push what I have, but if you see what needs to be fixed I'm happy to do it.

FabioRosado · 2018-11-30T10:03:28Z

Just to let you know that I'm off today and I plan to spend some time with this PR to see why that test is now failing, will let you know if I can fix or offer any sort of solution for it 👍

FabioRosado · 2018-11-30T17:25:41Z

@jacobtomlinson perhaps you could give us a hand here, I am trying to figure out why the github.connect test is failing. If we replace the line opsdroid = amock.CoroutineMock() with opsdroid = OpsDroid() or use with OpsDroid() as opsdroid the test will still fail since web_app is None and we get AttributeError: 'NoneType' object has no attribute 'web_app'.

Should web_app be mocked in order to get this test passing or is there another way to do this?

jacobtomlinson · 2018-12-03T13:33:51Z

I would check the Facebook connector tests to see what we did there, as they are both adding routes to the web server. Otherwise I'll try and take a look soon.

zonagilreath · 2018-12-03T20:12:49Z

I got it, I think.

zonagilreath · 2018-12-03T20:37:40Z

Still failing on docker commands, but everything else is working now.

FabioRosado

Thanks for working on this. I have noticed that you commited files within .vs folder, I am going to assume that we don't really need these and should be removed?

.vs/VSWorkspaceState.json

FabioRosado · 2018-12-03T20:52:13Z

opsdroid/connector/websocket/__init__.py

        _LOGGER.debug("Starting Websocket connector")
        self.name = "websocket"
        self.config = config
-        self.opsdroid = None


Do you know this is the reason why we are getting the NoneType issue within docker?

I bet that wherever ConnectorWebsocket is being initialized it's not being given the opsdroid argument that it needs now, so it's defaulting to None.

I think I found it in core

FabioRosado · 2018-12-03T21:29:17Z

I checked docker when you changed the core. It only showed an issue with the listen method perhaps because we need to update the code further down? Also the only reason why the other tests were failing was due the tests in core not being updated with that change 👍

zonagilreath · 2018-12-03T21:42:56Z

Ah, I thought it was OpsDroid.start_connectors() that wasn't liking the new argument but I see now. Updating the mock connectors, and will test again.

…atch original refactoring

zonagilreath · 2018-12-03T22:02:06Z

Woo!

FabioRosado · 2018-12-03T22:07:26Z

This is an amazing work! Congratulations 🎉
Also well done finding those buggers that were making the tests complaining 😄 👍

I am going to merge this now!

We also give stickers to contributors so if you would like them DM your home address to opsdroid twitter account and Jacob will send them to you 👍

zonagilreath and others added 6 commits November 5, 2018 22:05

Refactored connector opsdroid pointer

5be7937

updated references to Connector class(es) in all tests, as well as in…

8849e65

… core OpsDroid

removed packing in Connector.__init__, added opsdroid=None default

2c48505

Merge branch 'master' into master

e8fe504

Refacor opsdroid connectors to remove opsdroid param. Update tests

7cf8fb0

Merge branch 'master' into master

c87e08a

FabioRosado changed the title ~~Reopening same request~~ Refactor connector opsdroid pointer Nov 21, 2018

Merge branch 'master' into update-opsdroid-pointer

44815f9

This was referenced Nov 27, 2018

shell connector fails on start #761

Closed

Revert opsdroid pointer changes opsdroid/connector-shell#9

Merged

Merge pull request #1 from FabioRosado/update-opsdroid-pointer

51cce09

Update opsdroid pointer

zonagilreath closed this Nov 28, 2018

zonagilreath reopened this Nov 28, 2018

Merge branch 'master' into master

835c1b5

fixed method calls in tests

1a3ee3c

zonagilreath and others added 2 commits December 3, 2018 14:19

fixed bug in async github connector test

9d3a949

Merge branch 'master' into master

9ec9cf7

FabioRosado requested changes Dec 3, 2018

View reviewed changes

.vs/VSWorkspaceState.json Outdated Show resolved Hide resolved

zonagilreath added 2 commits December 3, 2018 14:48

removed .vs directory

77deafd

Merge branch 'master' of https://github.com/zonagilreath/opsdroid

e7733c8

FabioRosado reviewed Dec 3, 2018

View reviewed changes

FabioRosado approved these changes Dec 3, 2018

View reviewed changes

zonagilreath added 2 commits December 3, 2018 15:13

checking fix of OpsDroid.start_connectors()

004efdc

undid change to core

fcc3719

zonagilreath and others added 2 commits December 3, 2018 15:46

changed start_connectors method in core, updated mock connectors to m…

d8348b2

…atch original refactoring

Update core.py

decd15e

FabioRosado merged commit dc2a079 into opsdroid:master Dec 3, 2018

This was referenced Dec 3, 2018

Remove self from disconnect() method when unload is called #775

Merged

Remove opsdroid pointer from methods, add opsdroid param to __init__ opsdroid/connector-matrix#40

Closed

Refactor database opsdroid pointer #777

Merged

FabioRosado pushed a commit to FabioRosado/opsdroid that referenced this pull request Dec 18, 2018

Refactor connector opsdroid pointer (opsdroid#749)

21c79c5

jacobtomlinson mentioned this pull request Jan 27, 2019

Fix broken websocket handler #819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor connector opsdroid pointer #749

Refactor connector opsdroid pointer #749

zonagilreath commented Nov 21, 2018 •

edited by FabioRosado

Loading

codecov bot commented Nov 21, 2018 •

edited

Loading

FabioRosado commented Nov 21, 2018

zonagilreath commented Nov 27, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018 •

edited

Loading

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018 •

edited

Loading

FabioRosado commented Nov 30, 2018

FabioRosado commented Nov 30, 2018

jacobtomlinson commented Dec 3, 2018

zonagilreath commented Dec 3, 2018

zonagilreath commented Dec 3, 2018

FabioRosado left a comment

FabioRosado Dec 3, 2018

zonagilreath Dec 3, 2018

zonagilreath Dec 3, 2018

FabioRosado commented Dec 3, 2018

zonagilreath commented Dec 3, 2018

zonagilreath commented Dec 3, 2018

FabioRosado commented Dec 3, 2018

Refactor connector opsdroid pointer #749

Refactor connector opsdroid pointer #749

Conversation

zonagilreath commented Nov 21, 2018 • edited by FabioRosado Loading

Description

Status

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Nov 21, 2018 • edited Loading

Codecov Report

FabioRosado commented Nov 21, 2018

zonagilreath commented Nov 27, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018 • edited Loading

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

zonagilreath commented Nov 28, 2018

FabioRosado commented Nov 28, 2018

zonagilreath commented Nov 28, 2018 • edited Loading

FabioRosado commented Nov 30, 2018

FabioRosado commented Nov 30, 2018

jacobtomlinson commented Dec 3, 2018

zonagilreath commented Dec 3, 2018

zonagilreath commented Dec 3, 2018

FabioRosado left a comment

Choose a reason for hiding this comment

FabioRosado Dec 3, 2018

Choose a reason for hiding this comment

zonagilreath Dec 3, 2018

Choose a reason for hiding this comment

zonagilreath Dec 3, 2018

Choose a reason for hiding this comment

FabioRosado commented Dec 3, 2018

zonagilreath commented Dec 3, 2018

zonagilreath commented Dec 3, 2018

FabioRosado commented Dec 3, 2018

zonagilreath commented Nov 21, 2018 •

edited by FabioRosado

Loading

codecov bot commented Nov 21, 2018 •

edited

Loading

zonagilreath commented Nov 28, 2018 •

edited

Loading

zonagilreath commented Nov 28, 2018 •

edited

Loading