From d5813cf167dc4899e241c823d71243375c5d40c4 Mon Sep 17 00:00:00 2001 From: Malte Franken Date: Fri, 21 Sep 2018 23:24:50 +0930 Subject: [PATCH] Make rest sensor and binary sensor more efficient (#14484) * create binary sensor even if initial update fails * fixed broken test assertion * fixed broken test assertion * avoid fetching resource twice - manually in the setup_platform and then through add_devices * raising PlatformNotReady instead of creating the sensor if the initial rest call fails; throttling the update to avoid fetching the same resource again immediately after setting up sensor * rolled back throttling of the rest update call; can still avoid updating the binary sensor's rest resoure twice; fixed tests * typo --- .../components/binary_sensor/rest.py | 9 +- homeassistant/components/sensor/rest.py | 6 + tests/components/binary_sensor/test_rest.py | 117 ++++++++++-------- tests/components/sensor/test_rest.py | 100 ++++++++------- 4 files changed, 129 insertions(+), 103 deletions(-) diff --git a/homeassistant/components/binary_sensor/rest.py b/homeassistant/components/binary_sensor/rest.py index 412aeb46a3a..ac82ab126fd 100644 --- a/homeassistant/components/binary_sensor/rest.py +++ b/homeassistant/components/binary_sensor/rest.py @@ -18,6 +18,7 @@ from homeassistant.const import ( CONF_HEADERS, CONF_AUTHENTICATION, HTTP_BASIC_AUTHENTICATION, HTTP_DIGEST_AUTHENTICATION, CONF_DEVICE_CLASS) import homeassistant.helpers.config_validation as cv +from homeassistant.exceptions import PlatformNotReady _LOGGER = logging.getLogger(__name__) @@ -66,13 +67,13 @@ def setup_platform(hass, config, add_entities, discovery_info=None): rest = RestData(method, resource, auth, headers, payload, verify_ssl) rest.update() - if rest.data is None: - _LOGGER.error("Unable to fetch REST data from %s", resource) - return False + raise PlatformNotReady + # No need to update the sensor now because it will determine its state + # based in the rest resource that has just been retrieved. add_entities([RestBinarySensor( - hass, rest, name, device_class, value_template)], True) + hass, rest, name, device_class, value_template)]) class RestBinarySensor(BinarySensorDevice): diff --git a/homeassistant/components/sensor/rest.py b/homeassistant/components/sensor/rest.py index 53aab3f1ff7..f2fbe6cd191 100644 --- a/homeassistant/components/sensor/rest.py +++ b/homeassistant/components/sensor/rest.py @@ -18,6 +18,7 @@ from homeassistant.const import ( CONF_UNIT_OF_MEASUREMENT, CONF_USERNAME, CONF_VALUE_TEMPLATE, CONF_VERIFY_SSL, HTTP_BASIC_AUTHENTICATION, HTTP_DIGEST_AUTHENTICATION, STATE_UNKNOWN) +from homeassistant.exceptions import PlatformNotReady from homeassistant.helpers.entity import Entity import homeassistant.helpers.config_validation as cv @@ -76,7 +77,11 @@ def setup_platform(hass, config, add_entities, discovery_info=None): auth = None rest = RestData(method, resource, auth, headers, payload, verify_ssl) rest.update() + if rest.data is None: + raise PlatformNotReady + # Must update the sensor now (including fetching the rest resource) to + # ensure it's updating its state. add_entities([RestSensor( hass, rest, name, unit, value_template, json_attrs, force_update )], True) @@ -170,6 +175,7 @@ class RestData: def update(self): """Get the latest data from REST service with provided method.""" + _LOGGER.debug("Updating from %s", self._request.url) try: with requests.Session() as sess: response = sess.send( diff --git a/tests/components/binary_sensor/test_rest.py b/tests/components/binary_sensor/test_rest.py index d1c26624452..db70303e217 100644 --- a/tests/components/binary_sensor/test_rest.py +++ b/tests/components/binary_sensor/test_rest.py @@ -1,11 +1,13 @@ """The tests for the REST binary sensor platform.""" import unittest +from pytest import raises from unittest.mock import patch, Mock import requests from requests.exceptions import Timeout, MissingSchema import requests_mock +from homeassistant.exceptions import PlatformNotReady from homeassistant.setup import setup_component import homeassistant.components.binary_sensor as binary_sensor import homeassistant.components.binary_sensor.rest as rest @@ -18,9 +20,18 @@ from tests.common import get_test_home_assistant, assert_setup_component class TestRestBinarySensorSetup(unittest.TestCase): """Tests for setting up the REST binary sensor platform.""" + DEVICES = [] + + def add_devices(self, devices, update_before_add=False): + """Mock add devices.""" + for device in devices: + self.DEVICES.append(device) + def setUp(self): """Set up things to be run when tests are started.""" self.hass = get_test_home_assistant() + # Reset for this test. + self.DEVICES = [] def tearDown(self): """Stop everything that was started.""" @@ -45,76 +56,80 @@ class TestRestBinarySensorSetup(unittest.TestCase): side_effect=requests.exceptions.ConnectionError()) def test_setup_failed_connect(self, mock_req): """Test setup when connection error occurs.""" - self.assertFalse(rest.setup_platform(self.hass, { - 'platform': 'rest', - 'resource': 'http://localhost', - }, lambda devices, update=True: None)) + with raises(PlatformNotReady): + rest.setup_platform(self.hass, { + 'platform': 'rest', + 'resource': 'http://localhost', + }, self.add_devices, None) + self.assertEqual(len(self.DEVICES), 0) @patch('requests.Session.send', side_effect=Timeout()) def test_setup_timeout(self, mock_req): """Test setup when connection timeout occurs.""" - self.assertFalse(rest.setup_platform(self.hass, { - 'platform': 'rest', - 'resource': 'http://localhost', - }, lambda devices, update=True: None)) + with raises(PlatformNotReady): + rest.setup_platform(self.hass, { + 'platform': 'rest', + 'resource': 'http://localhost', + }, self.add_devices, None) + self.assertEqual(len(self.DEVICES), 0) @requests_mock.Mocker() def test_setup_minimum(self, mock_req): """Test setup with minimum configuration.""" mock_req.get('http://localhost', status_code=200) - self.assertTrue(setup_component(self.hass, 'binary_sensor', { - 'binary_sensor': { - 'platform': 'rest', - 'resource': 'http://localhost' - } - })) - self.assertEqual(2, mock_req.call_count) - assert_setup_component(1, 'switch') + with assert_setup_component(1, 'binary_sensor'): + self.assertTrue(setup_component(self.hass, 'binary_sensor', { + 'binary_sensor': { + 'platform': 'rest', + 'resource': 'http://localhost' + } + })) + self.assertEqual(1, mock_req.call_count) @requests_mock.Mocker() def test_setup_get(self, mock_req): """Test setup with valid configuration.""" mock_req.get('http://localhost', status_code=200) - self.assertTrue(setup_component(self.hass, 'binary_sensor', { - 'binary_sensor': { - 'platform': 'rest', - 'resource': 'http://localhost', - 'method': 'GET', - 'value_template': '{{ value_json.key }}', - 'name': 'foo', - 'unit_of_measurement': 'MB', - 'verify_ssl': 'true', - 'authentication': 'basic', - 'username': 'my username', - 'password': 'my password', - 'headers': {'Accept': 'application/json'} - } - })) - self.assertEqual(2, mock_req.call_count) - assert_setup_component(1, 'binary_sensor') + with assert_setup_component(1, 'binary_sensor'): + self.assertTrue(setup_component(self.hass, 'binary_sensor', { + 'binary_sensor': { + 'platform': 'rest', + 'resource': 'http://localhost', + 'method': 'GET', + 'value_template': '{{ value_json.key }}', + 'name': 'foo', + 'unit_of_measurement': 'MB', + 'verify_ssl': 'true', + 'authentication': 'basic', + 'username': 'my username', + 'password': 'my password', + 'headers': {'Accept': 'application/json'} + } + })) + self.assertEqual(1, mock_req.call_count) @requests_mock.Mocker() def test_setup_post(self, mock_req): """Test setup with valid configuration.""" mock_req.post('http://localhost', status_code=200) - self.assertTrue(setup_component(self.hass, 'binary_sensor', { - 'binary_sensor': { - 'platform': 'rest', - 'resource': 'http://localhost', - 'method': 'POST', - 'value_template': '{{ value_json.key }}', - 'payload': '{ "device": "toaster"}', - 'name': 'foo', - 'unit_of_measurement': 'MB', - 'verify_ssl': 'true', - 'authentication': 'basic', - 'username': 'my username', - 'password': 'my password', - 'headers': {'Accept': 'application/json'} - } - })) - self.assertEqual(2, mock_req.call_count) - assert_setup_component(1, 'binary_sensor') + with assert_setup_component(1, 'binary_sensor'): + self.assertTrue(setup_component(self.hass, 'binary_sensor', { + 'binary_sensor': { + 'platform': 'rest', + 'resource': 'http://localhost', + 'method': 'POST', + 'value_template': '{{ value_json.key }}', + 'payload': '{ "device": "toaster"}', + 'name': 'foo', + 'unit_of_measurement': 'MB', + 'verify_ssl': 'true', + 'authentication': 'basic', + 'username': 'my username', + 'password': 'my password', + 'headers': {'Accept': 'application/json'} + } + })) + self.assertEqual(1, mock_req.call_count) class TestRestBinarySensor(unittest.TestCase): diff --git a/tests/components/sensor/test_rest.py b/tests/components/sensor/test_rest.py index 4d40ad394cd..7f818193a29 100644 --- a/tests/components/sensor/test_rest.py +++ b/tests/components/sensor/test_rest.py @@ -1,11 +1,13 @@ """The tests for the REST sensor platform.""" import unittest +from pytest import raises from unittest.mock import patch, Mock import requests from requests.exceptions import Timeout, MissingSchema, RequestException import requests_mock +from homeassistant.exceptions import PlatformNotReady from homeassistant.setup import setup_component import homeassistant.components.sensor as sensor import homeassistant.components.sensor.rest as rest @@ -45,76 +47,78 @@ class TestRestSensorSetup(unittest.TestCase): side_effect=requests.exceptions.ConnectionError()) def test_setup_failed_connect(self, mock_req): """Test setup when connection error occurs.""" - self.assertTrue(rest.setup_platform(self.hass, { - 'platform': 'rest', - 'resource': 'http://localhost', - }, lambda devices, update=True: None) is None) + with raises(PlatformNotReady): + rest.setup_platform(self.hass, { + 'platform': 'rest', + 'resource': 'http://localhost', + }, lambda devices, update=True: None) @patch('requests.Session.send', side_effect=Timeout()) def test_setup_timeout(self, mock_req): """Test setup when connection timeout occurs.""" - self.assertTrue(rest.setup_platform(self.hass, { - 'platform': 'rest', - 'resource': 'http://localhost', - }, lambda devices, update=True: None) is None) + with raises(PlatformNotReady): + rest.setup_platform(self.hass, { + 'platform': 'rest', + 'resource': 'http://localhost', + }, lambda devices, update=True: None) @requests_mock.Mocker() def test_setup_minimum(self, mock_req): """Test setup with minimum configuration.""" mock_req.get('http://localhost', status_code=200) - self.assertTrue(setup_component(self.hass, 'sensor', { - 'sensor': { - 'platform': 'rest', - 'resource': 'http://localhost' - } - })) + with assert_setup_component(1, 'sensor'): + self.assertTrue(setup_component(self.hass, 'sensor', { + 'sensor': { + 'platform': 'rest', + 'resource': 'http://localhost' + } + })) self.assertEqual(2, mock_req.call_count) - assert_setup_component(1, 'switch') @requests_mock.Mocker() def test_setup_get(self, mock_req): """Test setup with valid configuration.""" mock_req.get('http://localhost', status_code=200) - self.assertTrue(setup_component(self.hass, 'sensor', { - 'sensor': { - 'platform': 'rest', - 'resource': 'http://localhost', - 'method': 'GET', - 'value_template': '{{ value_json.key }}', - 'name': 'foo', - 'unit_of_measurement': 'MB', - 'verify_ssl': 'true', - 'authentication': 'basic', - 'username': 'my username', - 'password': 'my password', - 'headers': {'Accept': 'application/json'} - } - })) + with assert_setup_component(1, 'sensor'): + self.assertTrue(setup_component(self.hass, 'sensor', { + 'sensor': { + 'platform': 'rest', + 'resource': 'http://localhost', + 'method': 'GET', + 'value_template': '{{ value_json.key }}', + 'name': 'foo', + 'unit_of_measurement': 'MB', + 'verify_ssl': 'true', + 'authentication': 'basic', + 'username': 'my username', + 'password': 'my password', + 'headers': {'Accept': 'application/json'} + } + })) self.assertEqual(2, mock_req.call_count) - assert_setup_component(1, 'sensor') @requests_mock.Mocker() def test_setup_post(self, mock_req): """Test setup with valid configuration.""" mock_req.post('http://localhost', status_code=200) - self.assertTrue(setup_component(self.hass, 'sensor', { - 'sensor': { - 'platform': 'rest', - 'resource': 'http://localhost', - 'method': 'POST', - 'value_template': '{{ value_json.key }}', - 'payload': '{ "device": "toaster"}', - 'name': 'foo', - 'unit_of_measurement': 'MB', - 'verify_ssl': 'true', - 'authentication': 'basic', - 'username': 'my username', - 'password': 'my password', - 'headers': {'Accept': 'application/json'} - } - })) + with assert_setup_component(1, 'sensor'): + self.assertTrue(setup_component(self.hass, 'sensor', { + 'sensor': { + 'platform': 'rest', + 'resource': 'http://localhost', + 'method': 'POST', + 'value_template': '{{ value_json.key }}', + 'payload': '{ "device": "toaster"}', + 'name': 'foo', + 'unit_of_measurement': 'MB', + 'verify_ssl': 'true', + 'authentication': 'basic', + 'username': 'my username', + 'password': 'my password', + 'headers': {'Accept': 'application/json'} + } + })) self.assertEqual(2, mock_req.call_count) - assert_setup_component(1, 'sensor') class TestRestSensor(unittest.TestCase):