Fix WUnderground error handling, rework entity methods (home-assistan…

…t#10295)

* WUnderground sensor error handling and sensor class rework

* WUnderground error handling, avoid long state, tests

* Wunderground - add handling ValueError exception on parsing

* Changes to address review comments - part 1

* Tests lint

* Changes to address review comments - part 2

homeassistant/components/sensor/wunderground.py

@@ -17,6 +17,7 @@
     TEMP_FAHRENHEIT, TEMP_CELSIUS, LENGTH_INCHES, LENGTH_KILOMETERS,
     LENGTH_MILES, LENGTH_FEET, STATE_UNKNOWN, ATTR_ATTRIBUTION,
     ATTR_FRIENDLY_NAME)
+from homeassistant.exceptions import PlatformNotReady
 from homeassistant.helpers.entity import Entity
 from homeassistant.util import Throttle
 import homeassistant.helpers.config_validation as cv
@@ -638,11 +639,9 @@ def setup_platform(hass, config, add_devices, discovery_info=None):
     for variable in config[CONF_MONITORED_CONDITIONS]:
         sensors.append(WUndergroundSensor(rest, variable))
 
-    try:
-        rest.update()
-    except ValueError as err:
-        _LOGGER.error("Received error from WUnderground: %s", err)
-        return False
+    rest.update()
+    if not rest.data:
+        raise PlatformNotReady
 
     add_devices(sensors)
 
@@ -656,21 +655,49 @@ def __init__(self, rest, condition):
         """Initialize the sensor."""
         self.rest = rest
         self._condition = condition
+        self._state = None
+        self._attributes = {
+            ATTR_ATTRIBUTION: CONF_ATTRIBUTION,
+        }
+        self._icon = None
+        self._entity_picture = None
+        self._unit_of_measurement = self._cfg_expand("unit_of_measurement")
         self.rest.request_feature(SENSOR_TYPES[condition].feature)
 
     def _cfg_expand(self, what, default=None):
+        """Parse and return sensor data."""
         cfg = SENSOR_TYPES[self._condition]
         val = getattr(cfg, what)
+        if not callable(val):
+            return val
         try:
             val = val(self.rest)
-        except (KeyError, IndexError) as err:
-            _LOGGER.warning("Failed to parse response from WU API: %s", err)
+        except (KeyError, IndexError, TypeError, ValueError) as err:
+            _LOGGER.warning("Failed to expand cfg from WU API."
+                            " Condition: %s Attr: %s Error: %s",
+                            self._condition, what, repr(err))
             val = default
-        except TypeError:
-            pass  # val was not callable - keep original value
 
         return val
 
+    def _update_attrs(self):
+        """Parse and update device state attributes."""
+        attrs = self._cfg_expand("device_state_attributes", {})
+
+        self._attributes[ATTR_FRIENDLY_NAME] = self._cfg_expand(
+            "friendly_name")
+
+        for (attr, callback) in attrs.items():
+            if callable(callback):
+                try:
+                    self._attributes[attr] = callback(self.rest)
+                except (KeyError, IndexError, TypeError, ValueError) as err:
+                    _LOGGER.warning("Failed to update attrs from WU API."
+                                    " Condition: %s Attr: %s Error: %s",
+                                    self._condition, attr, repr(err))
+            else:
+                self._attributes[attr] = callback
+
     @property
     def name(self):
         """Return the name of the sensor."""
@@ -679,46 +706,44 @@ def name(self):
     @property
     def state(self):
         """Return the state of the sensor."""
-        return self._cfg_expand("value", STATE_UNKNOWN)
+        return self._state
 
     @property
     def device_state_attributes(self):
         """Return the state attributes."""
-        attrs = self._cfg_expand("device_state_attributes", {})
-        for (attr, callback) in attrs.items():
-            try:
-                attrs[attr] = callback(self.rest)
-            except TypeError:
-                attrs[attr] = callback
-            except (KeyError, IndexError) as err:
-                _LOGGER.warning("Failed to parse response from WU API: %s",
-                                err)
-
-        attrs[ATTR_ATTRIBUTION] = CONF_ATTRIBUTION
-        attrs[ATTR_FRIENDLY_NAME] = self._cfg_expand("friendly_name")
-        return attrs
+        return self._attributes
 
     @property
     def icon(self):
         """Return icon."""
-        return self._cfg_expand("icon", super().icon)
+        return self._icon
 
     @property
     def entity_picture(self):
         """Return the entity picture."""
-        url = self._cfg_expand("entity_picture")
-        if isinstance(url, str):
-            return re.sub(r'^http://', 'https://', url, flags=re.IGNORECASE)
+        return self._entity_picture
 
     @property
     def unit_of_measurement(self):
         """Return the units of measurement."""
-        return self._cfg_expand("unit_of_measurement")
+        return self._unit_of_measurement
 
     def update(self):
         """Update current conditions."""
         self.rest.update()
 
+        if not self.rest.data:
+            # no data, return
+            return
+
+        self._state = self._cfg_expand("value", STATE_UNKNOWN)
+        self._update_attrs()
+        self._icon = self._cfg_expand("icon", super().icon)
+        url = self._cfg_expand("entity_picture")
+        if isinstance(url, str):
+            self._entity_picture = re.sub(r'^http://', 'https://',
+                                          url, flags=re.IGNORECASE)
+
 
 class WUndergroundData(object):
     """Get data from WUnderground."""
@@ -758,6 +783,10 @@ def update(self):
                                  ["description"])
             else:
                 self.data = result
+                return True
         except ValueError as err:
             _LOGGER.error("Check WUnderground API %s", err.args)
             self.data = None
+        except requests.RequestException as err:
+            _LOGGER.error("Error fetching WUnderground data: %s", repr(err))
+            self.data = None

tests/components/sensor/test_wunderground.py

@@ -2,7 +2,10 @@
 import unittest
 
 from homeassistant.components.sensor import wunderground
-from homeassistant.const import TEMP_CELSIUS, LENGTH_INCHES
+from homeassistant.const import TEMP_CELSIUS, LENGTH_INCHES, STATE_UNKNOWN
+from homeassistant.exceptions import PlatformNotReady
+
+from requests.exceptions import ConnectionError
 
 from tests.common import get_test_home_assistant
 
@@ -38,6 +41,7 @@
 WEATHER = 'Clear'
 HTTPS_ICON_URL = 'https://icons.wxug.com/i/c/k/clear.gif'
 ALERT_MESSAGE = 'This is a test alert message'
+ALERT_ICON = 'mdi:alert-circle-outline'
 FORECAST_TEXT = 'Mostly Cloudy. Fog overnight.'
 PRECIP_IN = 0.03
 
@@ -163,6 +167,41 @@ def json(self):
         }, 200)
 
 
+def mocked_requests_get_invalid(*args, **kwargs):
+    """Mock requests.get invocations invalid data."""
+    class MockResponse:
+        """Class to represent a mocked response."""
+
+        def __init__(self, json_data, status_code):
+            """Initialize the mock response class."""
+            self.json_data = json_data
+            self.status_code = status_code
+
+        def json(self):
+            """Return the json of the response."""
+            return self.json_data
+
+    return MockResponse({
+        "response": {
+            "version": "0.1",
+            "termsofService":
+                "http://www.wunderground.com/weather/api/d/terms.html",
+            "features": {
+                "conditions": 1,
+                "alerts": 1,
+                "forecast": 1,
+            }
+        }, "current_observation": {
+            "image": {
+                "url":
+                    'http://icons.wxug.com/graphics/wu2/logo_130x80.png',
+                "title": "Weather Underground",
+                "link": "http://www.wunderground.com"
+            },
+        },
+    }, 200)
+
+
 class TestWundergroundSetup(unittest.TestCase):
     """Test the WUnderground platform."""
 
@@ -199,9 +238,9 @@ def test_setup(self, req_mock):
             wunderground.setup_platform(self.hass, VALID_CONFIG,
                                         self.add_devices, None))
 
-        self.assertTrue(
+        with self.assertRaises(PlatformNotReady):
             wunderground.setup_platform(self.hass, INVALID_CONFIG,
-                                        self.add_devices, None))
+                                        self.add_devices, None)
 
     @unittest.mock.patch('requests.get', side_effect=mocked_requests_get)
     def test_sensor(self, req_mock):
@@ -219,6 +258,7 @@ def test_sensor(self, req_mock):
                 self.assertEqual(1, device.state)
                 self.assertEqual(ALERT_MESSAGE,
                                  device.device_state_attributes['Message'])
+                self.assertEqual(ALERT_ICON, device.icon)
                 self.assertIsNone(device.entity_picture)
             elif device.name == 'PWS_location':
                 self.assertEqual('Holly Springs, NC', device.state)
@@ -234,3 +274,21 @@ def test_sensor(self, req_mock):
                 self.assertEqual(device.name, 'PWS_precip_1d_in')
                 self.assertEqual(PRECIP_IN, device.state)
                 self.assertEqual(LENGTH_INCHES, device.unit_of_measurement)
+
+    @unittest.mock.patch('requests.get',
+                         side_effect=ConnectionError('test exception'))
+    def test_connect_failed(self, req_mock):
+        """Test the WUnderground connection error."""
+        with self.assertRaises(PlatformNotReady):
+            wunderground.setup_platform(self.hass, VALID_CONFIG,
+                                        self.add_devices, None)
+
+    @unittest.mock.patch('requests.get',
+                         side_effect=mocked_requests_get_invalid)
+    def test_invalid_data(self, req_mock):
+        """Test the WUnderground invalid data."""
+        wunderground.setup_platform(self.hass, VALID_CONFIG_PWS,
+                                    self.add_devices, None)
+        for device in self.DEVICES:
+            device.update()
+            self.assertEqual(STATE_UNKNOWN, device.state)