Make WUnderground async by OttoWinter · Pull Request #12385 · home-assistant/core

OttoWinter · 2018-02-13T18:17:22Z

Description:

Makes WUnderground sensor platform async, using aiohttp's HTTP client.

Previous tests had to be updated quite significantly to use the aiohttp_mock fixture. During that process, the JSON payloads were moved to file fixtures. I didn't know how a unittest.TestCase class could be transformed to handle async tests, so I had to move the tests to top-level coroutines and change the self.assert* calls.

If someone knows how to fix the tests/make them aiohttp-aware without changing the whole test_wunderground.py file and making reviewing more difficult, please tell me.

Example entry for `configuration.yaml` (if applicable):

sensor:
  - platform: wunderground
    api_key: !secret wunderground_api_key
    monitored_conditions:
      - weather
      - weather_1d_metric
      - weather_1n_metric

Checklist:

The code change is tested and works locally.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass

houndci-bot · 2018-02-13T18:17:30Z

tests/components/sensor/test_wunderground.py

+            assert "Weather Summary" == friendly_name
+        elif entity_id == 'sensor.pws_alerts':
+            assert 1 == device.state
+            assert 'This is a test alert message' == device.device_state_attributes['Message']


line too long (94 > 79 characters)

houndci-bot · 2018-02-13T18:17:30Z

tests/components/sensor/test_wunderground.py

+        friendly_name = device.name
+
+        if entity_id == 'sensor.pws_weather':
+            assert 'https://icons.wxug.com/i/c/k/clear.gif' == device.entity_picture


line too long (84 > 79 characters)

houndci-bot · 2018-02-13T18:17:31Z

tests/components/sensor/test_wunderground.py

+    """Test that the component is loaded if passed in PWS Id."""
+    aioclient_mock.get(URL, text=load_fixture('wunderground-valid.json'))
+    aioclient_mock.get(PWS_URL, text=load_fixture('wunderground-valid.json'))
+    aioclient_mock.get(INVALID_URL, text=load_fixture('wunderground-error.json'))


line too long (81 > 79 characters)

OttoWinter · 2018-02-13T18:18:52Z

homeassistant/components/sensor/wunderground.py

    def _build_url(self, baseurl=_RESOURCE):
        url = baseurl.format(
-            self._api_key, "/".join(self._features), self._lang)
+            self._api_key, '/'.join(sorted(self._features)), self._lang)


self._features needs to be sorted in order to make the generated URL stable for tests - i.e. so that the generated URL doesn't depend on the order of execution.

MartinHjelmare

Please also remove the use of STATE_UNKNOWN from this module. Default unknown state should be Nonein the entities. The base entity class will handle this and return the correct unknown state.

MartinHjelmare · 2018-02-14T01:39:59Z

homeassistant/components/sensor/wunderground.py

-    add_devices(sensors)
+    async_add_devices(sensors)

    return True


Please remove this. Nothing is checking this return value.

Fixed. Side note: Does this comment apply to the whole codebase? Was wondering about that in #12274

MartinHjelmare · 2018-02-14T01:43:31Z

homeassistant/components/sensor/wunderground.py

        """Get the latest data from WUnderground."""
        try:
-            result = requests.get(self._build_url(), timeout=10).json()
+            websession = async_get_clientsession(self._hass)


I think the session is persistent, so you should not do this in the update method but in async_setup_platform and pass it to the instance and store the session as an instance attribute.

Sure. Fixed.

MartinHjelmare · 2018-02-14T02:01:06Z

homeassistant/components/sensor/wunderground.py

-                self.data = result
-                return True
+            self.data = result
+            return True


Nothing is checking this return value.

MartinHjelmare · 2018-02-14T02:01:17Z

homeassistant/components/sensor/wunderground.py

            _LOGGER.error("Error fetching WUnderground data: %s", repr(err))
-            self.data = None
+        self.data = None
+        return False


Same as above.

MartinHjelmare · 2018-02-14T02:05:31Z

tests/components/sensor/test_wunderground.py

+    aioclient_mock.get(INVALID_URL,
+                       text=load_fixture('wunderground-error.json'))

+    result = yield from wunderground.async_setup_platform(


Instead setup the component and assert that a platform was setup with assert_setup_component.

MartinHjelmare · 2018-02-14T02:08:35Z

tests/components/sensor/test_wunderground.py

+
+    result = yield from wunderground.async_setup_platform(hass, VALID_CONFIG,
+                                                          async_add_devices)
+    assert result


Assert that async_add_devices was called once.

MartinHjelmare · 2018-02-14T02:20:05Z

tests/components/sensor/test_wunderground.py

-
-    def add_devices(self, devices):
+    @callback
+    def async_add_devices(new_devices):


If you don't want to mock this function, append each call to this function to a list that is accessible from the outer scope, similar to devices. This will allow you to check that this function was called.

I know the difference makes it difficult to see the changes ... but that's exactly what I'm doing at the moment 👀?

I think you're only appending the devices currently, correct?

Ah, you're right. Didn't understand your comment completely there 🙈

OttoWinter · 2018-02-14T06:16:00Z

homeassistant/components/sensor/wunderground.py

        self._unit_of_measurement = self._cfg_expand("unit_of_measurement")
        self.rest.request_feature(SENSOR_TYPES[condition].feature)
-        self.entity_id = generate_entity_id(
+        self.entity_id = async_generate_entity_id(


All other platforms using async_generate_entity_id this way, but should we really be using an event loop callback from within __init__? Would async_added_to_hass also work or is the entity_id required before that method is called?

Why not? It's not doing any I/O, so should be ok.

In order to call an async_* function in Home Assistant we need to guarantee that the caller is inside the event loop. If we can't guarantee that, we usually wrap the async call inside a hass.add_job().

So, because of our direct call to async_generate_entity_id() we'd also need to guarantee that __init__ is executed from within the event loop. Can we guarantee that though? In this case, it's only used in asyn_setup_platform (therefore inside the event loop), but I mean the object could also be initialized from outside the event loop.

Entities are supposed to be initialized in setup_platform/async_setup_platform. Also the caller is responsible to properly handle the objects it calls. So it's fine like this.

async_added_to_hass is executed after entity_id is assigned. This is fine since the platform uses async_setup_platform

OttoWinter · 2018-02-14T12:33:35Z

I've updated the code according to your comments now, though I've kind of hit a wall and need your advise:

For updating the tests to use assert_setup_component and async_setup_component, I'm now using the state machine to check the states of all sensors in test_sensor. We're relying on the entities to poll the WUndergroundData every so often for updates. Because we don't actually receive the entity objects through async_setup_component, we can't manually schedule the state updates anymore, therefore breaking the tests. So in my view we can:
- Figure out a potentially ugly solution for the tests so that they're able to call async_update() (or maybe I'm mistaken and this is simple)
- Move WUnderground to use the dispatcher to distribute state updates from WUndergroundData -> WUndergroundSensor. If you're willing to go that way: Should that be in this PR?
Also: We have a test that's checking whether PlatformNotReady gets thrown in async_setup_platform if no connection to the WUnderground API can be made. I have not been able to migrate this to assert_setup_component since that doesn't tell you whether that exception has been raised.

MartinHjelmare · 2018-02-14T13:47:59Z

If you add a second argument True to async_add_devices in the code, async_update will be called during entity addition. Then you should be able to get the state from the state machine in the tests and assert that the state is correct.

~~Does it work to patch PlatformNotReady and check that it's called?~~
You don't need to migrate the test of PlatformNotReady. I think it's fine to keep that as is.

OttoWinter · 2018-02-14T13:50:23Z

Thank you very much! That's very good to know, and patching PlatformNotReady should definitely work 👍 Will update the code in the afternoon.

balloob · 2018-02-16T06:00:35Z

Just drop support for assert_setup_component. It's not needed as you will test functionality right after you have set it up. I've never been a fan of that assertion.

OttoWinter · 2018-02-16T09:06:14Z

@balloob I've now removed assert_setup_component where we're testing the states right afterwards. For some other test cases where the test is about whether setup works/doesn't work I've however left it in.

balloob · 2018-02-16T18:32:19Z

homeassistant/components/sensor/wunderground.py

        sensors.append(WUndergroundSensor(hass, rest, variable))

-    rest.update()
+    yield from rest.async_update()


If you do this update before initializing the sensors, you will not do unnecessary work. It also means that you don't have to pass True to async_add_devices because the sensors have access to data the moment it is initialized.

If you do this update before initializing the sensors, you will not do unnecessary work.

With the current design of this platform (which isn't ideal, imho), when a sensor entity is created it tells the data holder that its "feature" should be monitored, so doing WUndergroundData#async_update() before the sensors have added their features would result in an empty query. We could however instead just pass the features into the data holder directly and not through the sensors, but I tried to limit this PR for now as this platform has many improvements that could be made (using the dispatcher to not rely on polling, ...)

It also means that you don't have to pass True to async_add_devices

The reason I'm passing True to async_add_devices is that the only way for sensors to get new data is through their own WUndergroundSensor#async_update. They don't get notified by WUndergroundData that a request has been made. Instead, they periodically poll data from the data holder, which also takes care of throttling the request. Therefore the only way to have some data in the entities before async_update is automatically called is by passing True to async_add_devices. Again, this could be fixed by completely reworking this platform.

okay, fine fine 👍

I'm sorry if this was perceived as being a bit rude - that was not my intention. I wanted to explain why I did this so that you, and anybody attempting to change this part of the codebase in the future, can follow what's going on. It's always better to ask when some code snippet "smells wrong" 👍.

Anyway, the underlying issue of this component being weird with it using polling still persists. But I think changing that would make the most sense during a transition to the weather component (btw, I'm not using this platform too much, so I wouldn't be the one who would like to do that 🤓).

🐎 Async WUnderground

cecdc4a

homeassistant added platform: sensor.wunderground cla-signed labels Feb 13, 2018

houndci-bot reviewed Feb 13, 2018

View reviewed changes

OttoWinter commented Feb 13, 2018

View reviewed changes

OttoWinter added 2 commits February 13, 2018 19:19

∞ Them lines be too long

c546d8e

Fix pylint warnings

371767d

MartinHjelmare requested changes Feb 14, 2018

View reviewed changes

OttoWinter commented Feb 14, 2018

View reviewed changes

OttoWinter added 2 commits February 14, 2018 13:17

Changes according to comments

52000ca

Remove STATE_UNKNOWN

988e907

OttoWinter changed the title ~~Make WUnderground async~~ WIP: Make WUnderground async Feb 14, 2018

🔬 Fix tests

20bcc44

homeassistant added the cla-signed label Feb 14, 2018

OttoWinter changed the title ~~WIP: Make WUnderground async~~ Make WUnderground async Feb 14, 2018

OttoWinter closed this Feb 14, 2018

OttoWinter reopened this Feb 14, 2018

Improve tests

2b37fcf

homeassistant added the cla-signed label Feb 16, 2018

balloob reviewed Feb 16, 2018

View reviewed changes

balloob approved these changes Feb 16, 2018

View reviewed changes

balloob merged commit fe5626b into home-assistant:dev Feb 16, 2018

balloob mentioned this pull request Feb 18, 2018

Reset Wunderground alerts on API update #11527

Closed

3 tasks

balloob mentioned this pull request Feb 22, 2018

0.64.0 #12609

Merged

arsaboo mentioned this pull request Mar 9, 2018

Wunderground not working since 0.65 #13012

Closed

OttoWinter deleted the async-wunderground branch March 13, 2018 20:19

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost added integration: wunderground and removed platform: sensor.wunderground labels Mar 21, 2019

Uh oh!

Conversation

OttoWinter commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OttoWinter Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OttoWinter commented Feb 14, 2018

Uh oh!

MartinHjelmare commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OttoWinter commented Feb 14, 2018

Uh oh!

balloob commented Feb 16, 2018

Uh oh!

OttoWinter commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

OttoWinter commented Feb 13, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

OttoWinter Feb 13, 2018 •

edited

Loading

MartinHjelmare commented Feb 14, 2018 •

edited

Loading