From 25408941dea4954ed5e55f9cc6091c474adb9a1c Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Sun, 19 Feb 2017 22:43:10 -0800 Subject: [PATCH] Mqtt fixes (#6116) * Fix MQTT stop * MQTT: Make sure to have connection setup at end of MQTT setup. * Fix MQTT connect --- homeassistant/components/mqtt/__init__.py | 55 +++++++++----- tests/common.py | 1 + tests/components/mqtt/test_init.py | 92 +++++++++++++---------- tests/components/mqtt/test_server.py | 10 ++- tests/conftest.py | 5 +- 5 files changed, 100 insertions(+), 63 deletions(-) diff --git a/homeassistant/components/mqtt/__init__.py b/homeassistant/components/mqtt/__init__.py index bb5990baeb5..2d2a77bfc48 100644 --- a/homeassistant/components/mqtt/__init__.py +++ b/homeassistant/components/mqtt/__init__.py @@ -226,10 +226,6 @@ def _async_setup_server(hass, config): """ conf = config.get(DOMAIN, {}) - # Only setup if embedded config passed in or no broker specified - if CONF_EMBEDDED not in conf and CONF_BROKER in conf: - return None - server = yield from async_prepare_setup_platform( hass, config, DOMAIN, 'server') @@ -272,7 +268,11 @@ def async_setup(hass, config): client_id = conf.get(CONF_CLIENT_ID) keepalive = conf.get(CONF_KEEPALIVE) - broker_config = yield from _async_setup_server(hass, config) + # Only setup if embedded config passed in or no broker specified + if CONF_EMBEDDED not in conf and CONF_BROKER in conf: + broker_config = None + else: + broker_config = yield from _async_setup_server(hass, config) if CONF_BROKER in conf: broker = conf[CONF_BROKER] @@ -329,6 +329,11 @@ def async_setup(hass, config): hass.bus.async_listen_once(EVENT_HOMEASSISTANT_START, async_start_mqtt) + success = yield from hass.data[DATA_MQTT].async_connect() + + if not success: + return False + @asyncio.coroutine def async_publish_service(call): """Handle MQTT publish service calls.""" @@ -375,6 +380,9 @@ class MQTT(object): import paho.mqtt.client as mqtt self.hass = hass + self.broker = broker + self.port = port + self.keepalive = keepalive self.topics = {} self.progress = {} self.birth_message = birth_message @@ -412,8 +420,6 @@ class MQTT(object): will_message.get(ATTR_QOS), will_message.get(ATTR_RETAIN)) - self._mqttc.connect_async(broker, port, keepalive) - def async_publish(self, topic, payload, qos, retain): """Publish a MQTT message. @@ -422,6 +428,21 @@ class MQTT(object): return self.hass.loop.run_in_executor( None, self._mqttc.publish, topic, payload, qos, retain) + @asyncio.coroutine + def async_connect(self): + """Connect to the host. Does not process messages yet. + + This method must be run in the event loop and returns a coroutine. + """ + result = yield from self.hass.loop.run_in_executor( + None, self._mqttc.connect, self.broker, self.port, self.keepalive) + + if result != 0: + import paho.mqtt.client as mqtt + _LOGGER.error('Failed to connect: %s', mqtt.error_string(result)) + + return not result + def async_start(self): """Run the MQTT client. @@ -434,7 +455,7 @@ class MQTT(object): This method must be run in the event loop and returns a coroutine. """ - def stop(self): + def stop(): """Stop the MQTT client.""" self._mqttc.disconnect() self._mqttc.loop_stop() @@ -477,14 +498,11 @@ class MQTT(object): Resubscribe to all topics we were subscribed to and publish birth message. """ - if result_code != 0: - _LOGGER.error('Unable to connect to the MQTT broker: %s', { - 1: 'Incorrect protocol version', - 2: 'Invalid client identifier', - 3: 'Server unavailable', - 4: 'Bad username or password', - 5: 'Not authorised' - }.get(result_code, 'Unknown reason')) + import paho.mqtt.client as mqtt + + if result_code != mqtt.CONNACK_ACCEPTED: + _LOGGER.error('Unable to connect to the MQTT broker: %s', + mqtt.connack_string(result_code)) self._mqttc.disconnect() return @@ -574,7 +592,10 @@ class MQTT(object): def _raise_on_error(result): """Raise error if error result.""" if result != 0: - raise HomeAssistantError('Error talking to MQTT: {}'.format(result)) + import paho.mqtt.client as mqtt + + raise HomeAssistantError( + 'Error talking to MQTT: {}'.format(mqtt.error_string(result))) def _match_topic(subscription, topic): diff --git a/tests/common.py b/tests/common.py index 16cc580a5e1..6dd1ecd4586 100644 --- a/tests/common.py +++ b/tests/common.py @@ -248,6 +248,7 @@ def mock_http_component_app(hass, api_password=None): def mock_mqtt_component(hass): """Mock the MQTT component.""" with patch('homeassistant.components.mqtt.MQTT') as mock_mqtt: + mock_mqtt().async_connect.return_value = mock_coro(True) setup_component(hass, mqtt.DOMAIN, { mqtt.DOMAIN: { mqtt.CONF_BROKER: 'mock-broker', diff --git a/tests/components/mqtt/test_init.py b/tests/components/mqtt/test_init.py index 2f96ada94ae..18510dd2ff3 100644 --- a/tests/components/mqtt/test_init.py +++ b/tests/components/mqtt/test_init.py @@ -27,9 +27,11 @@ def mock_mqtt_client(hass, config=None): } with mock.patch('paho.mqtt.client.Client') as mock_client: - yield from async_setup_component(hass, mqtt.DOMAIN, { - mqtt.DOMAIN: config - }) + mock_client().connect = lambda *args: 0 + result = yield from async_setup_component(hass, mqtt.DOMAIN, { + mqtt.DOMAIN: config + }) + assert result return mock_client() @@ -66,41 +68,6 @@ class TestMQTT(unittest.TestCase): self.hass.block_till_done() self.assertTrue(self.hass.data['mqtt'].async_stop.called) - @mock.patch('paho.mqtt.client.Client') - def test_setup_fails_if_no_connect_broker(self, _): - """Test for setup failure if connection to broker is missing.""" - test_broker_cfg = {mqtt.DOMAIN: {mqtt.CONF_BROKER: 'test-broker'}} - - with mock.patch('homeassistant.components.mqtt.MQTT', - side_effect=socket.error()): - self.hass.config.components = set() - assert not setup_component(self.hass, mqtt.DOMAIN, test_broker_cfg) - - # Ensure if we dont raise it sets up correctly - self.hass.config.components = set() - assert setup_component(self.hass, mqtt.DOMAIN, test_broker_cfg) - - @mock.patch('paho.mqtt.client.Client') - def test_setup_embedded(self, _): - """Test setting up embedded server with no config.""" - client_config = ('localhost', 1883, 'user', 'pass', None, '3.1.1') - - with mock.patch('homeassistant.components.mqtt.server.async_start', - return_value=mock_coro( - return_value=(True, client_config)) - ) as _start: - self.hass.config.components = set() - assert setup_component(self.hass, mqtt.DOMAIN, - {mqtt.DOMAIN: {}}) - assert _start.call_count == 1 - - # Test with `embedded: None` - _start.return_value = mock_coro(return_value=(True, client_config)) - self.hass.config.components = set() - assert setup_component(self.hass, mqtt.DOMAIN, - {mqtt.DOMAIN: {'embedded': None}}) - assert _start.call_count == 2 # Another call - def test_publish_calls_service(self): """Test the publishing of call to services.""" self.hass.bus.listen_once(EVENT_CALL_SERVICE, self.record_calls) @@ -253,8 +220,8 @@ class TestMQTTCallbacks(unittest.TestCase): """Setup things to be run when tests are started.""" self.hass = get_test_home_assistant() - with mock.patch('paho.mqtt.client.Client'): - self.hass.config.components = set() + with mock.patch('paho.mqtt.client.Client') as client: + client().connect = lambda *args: 0 assert setup_component(self.hass, mqtt.DOMAIN, { mqtt.DOMAIN: { mqtt.CONF_BROKER: 'mock-broker', @@ -354,6 +321,51 @@ class TestMQTTCallbacks(unittest.TestCase): test_handle.output[0]) +@asyncio.coroutine +def test_setup_embedded_starts_with_no_config(hass): + """Test setting up embedded server with no config.""" + client_config = ('localhost', 1883, 'user', 'pass', None, '3.1.1') + + with mock.patch('homeassistant.components.mqtt.server.async_start', + return_value=mock_coro( + return_value=(True, client_config)) + ) as _start: + yield from mock_mqtt_client(hass, {}) + assert _start.call_count == 1 + + +@asyncio.coroutine +def test_setup_embedded_with_embedded(hass): + """Test setting up embedded server with no config.""" + client_config = ('localhost', 1883, 'user', 'pass', None, '3.1.1') + + with mock.patch('homeassistant.components.mqtt.server.async_start', + return_value=mock_coro( + return_value=(True, client_config)) + ) as _start: + _start.return_value = mock_coro(return_value=(True, client_config)) + yield from mock_mqtt_client(hass, {'embedded': None}) + assert _start.call_count == 1 + + +@asyncio.coroutine +def test_setup_fails_if_no_connect_broker(hass): + """Test for setup failure if connection to broker is missing.""" + test_broker_cfg = {mqtt.DOMAIN: {mqtt.CONF_BROKER: 'test-broker'}} + + with mock.patch('homeassistant.components.mqtt.MQTT', + side_effect=socket.error()): + result = yield from async_setup_component(hass, mqtt.DOMAIN, + test_broker_cfg) + assert not result + + with mock.patch('paho.mqtt.client.Client') as mock_client: + mock_client().connect = lambda *args: 1 + result = yield from async_setup_component(hass, mqtt.DOMAIN, + test_broker_cfg) + assert not result + + @asyncio.coroutine def test_birth_message(hass): """Test sending birth message.""" diff --git a/tests/components/mqtt/test_server.py b/tests/components/mqtt/test_server.py index f017d257a26..cfef8ebcc16 100644 --- a/tests/components/mqtt/test_server.py +++ b/tests/components/mqtt/test_server.py @@ -26,23 +26,25 @@ class TestMQTT: @patch('homeassistant.components.mqtt.MQTT') def test_creating_config_with_http_pass(self, mock_mqtt): """Test if the MQTT server gets started and subscribe/publish msg.""" + mock_mqtt().async_connect.return_value = mock_coro(True) self.hass.bus.listen_once = MagicMock() password = 'super_secret' self.hass.config.api = MagicMock(api_password=password) assert setup_component(self.hass, mqtt.DOMAIN, {}) assert mock_mqtt.called - assert mock_mqtt.mock_calls[0][1][5] == 'homeassistant' - assert mock_mqtt.mock_calls[0][1][6] == password + assert mock_mqtt.mock_calls[1][1][5] == 'homeassistant' + assert mock_mqtt.mock_calls[1][1][6] == password mock_mqtt.reset_mock() + mock_mqtt().async_connect.return_value = mock_coro(True) self.hass.config.components = set(['http']) self.hass.config.api = MagicMock(api_password=None) assert setup_component(self.hass, mqtt.DOMAIN, {}) assert mock_mqtt.called - assert mock_mqtt.mock_calls[0][1][5] is None - assert mock_mqtt.mock_calls[0][1][6] is None + assert mock_mqtt.mock_calls[1][1][5] is None + assert mock_mqtt.mock_calls[1][1][6] is None @patch('tempfile.NamedTemporaryFile', Mock(return_value=MagicMock())) @patch('hbmqtt.broker.Broker.start', return_value=mock_coro()) diff --git a/tests/conftest.py b/tests/conftest.py index d408ed254f2..1e987a5f0a2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,7 +10,7 @@ from homeassistant import util, bootstrap from homeassistant.util import location from homeassistant.components import mqtt -from .common import async_test_home_assistant +from .common import async_test_home_assistant, mock_coro from .test_util.aiohttp import mock_aiohttp_client logging.basicConfig() @@ -66,7 +66,8 @@ def aioclient_mock(): def mqtt_mock(loop, hass): """Fixture to mock MQTT.""" with patch('homeassistant.components.mqtt.MQTT') as mock_mqtt: - loop.run_until_complete(bootstrap.async_setup_component( + mock_mqtt().async_connect.return_value = mock_coro(True) + assert loop.run_until_complete(bootstrap.async_setup_component( hass, mqtt.DOMAIN, { mqtt.DOMAIN: { mqtt.CONF_BROKER: 'mock-broker',