Add additional sensors for Arlo Baby camera by lukiffer · Pull Request #15074 · home-assistant/core

lukiffer · 2018-06-21T06:32:45Z

Description:

Adds additional temperature, humidity, and air quality sensors for Arlo Baby cameras implemented in pyarlo==0.1.8.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5583

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

sensor:
  - platform: arlo
    monitored_conditions:
      - temperature
      - humidity
      - air_quality

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

houndci-bot · 2018-06-21T06:32:53Z

+            for base_station in arlo.base_stations:
+                if ((sensor_type == 'temperature' or \
+                        sensor_type == 'humidity' or \
+                        sensor_type == 'air_quality') and \


the backslash is redundant between brackets

houndci-bot · 2018-06-21T06:32:53Z


+            for base_station in arlo.base_stations:
+                if ((sensor_type == 'temperature' or \
+                        sensor_type == 'humidity' or \


the backslash is redundant between brackets

houndci-bot · 2018-06-21T06:32:53Z

                sensors.append(ArloSensor(name, camera, sensor_type))

+            for base_station in arlo.base_stations:
+                if ((sensor_type == 'temperature' or \


the backslash is redundant between brackets

houndci-bot · 2018-06-21T06:32:53Z

+                   sensor_type == 'humidity' or \
+                   sensor_type == 'air_quality':
+                    continue
+


blank line contains whitespace

MartinHjelmare

@tchellomello could you have a look and review?

MartinHjelmare · 2018-06-21T07:35:57Z

            try:
                self._state = self._data.battery_level
-            except TypeError:
+            except (AttributeError, TypeError):


Why would there be an AttributeError?

Yeah I don't think the attribute error should be necessary since all cameras have the battery_level

tchellomello · 2018-06-24T02:40:36Z

It looks good to me. I don't have a baby monitor but code looks good!

lukiffer · 2018-06-25T07:18:10Z

This PR may have been slightly premature. I’ve since discovered an issue with stats not updating. I’m working on resolving this before removing the WIP designation. If it would be preferable to close this PR and reopen one once this issue is resolved, feel free to close this.

tchellomello

@lukiffer could you bump the Pyarlo module to pyarlo==0.1.9 .

https://pypi.org/project/pyarlo/0.1.9/

Thanks!

…rlo-baby

houndci-bot · 2018-07-02T07:30:45Z

+        })
+        sensor = TestArloSensor._get_mock_sensor('test', 'humidity', data)
+        attrs = sensor.device_state_attributes
+        self.assertEqual(attrs.get(ATTR_ATTRIBUTION), 'Data provided by arlo.netgear.com')


line too long (90 > 79 characters)

houndci-bot · 2018-07-02T07:30:45Z

+        data = TestArloSensor._get_named_tuple({
+            'captured_today': [0, 0, 0, 0, 0]
+        })
+        sensor = TestArloSensor._get_mock_sensor('Captured Today', 'captured_today', data)


line too long (90 > 79 characters)

houndci-bot · 2018-07-02T07:30:46Z

+        data = TestArloSensor._get_named_tuple({
+            'cameras': [0, 0]
+        })
+        sensor = TestArloSensor._get_mock_sensor('Arlo Cameras', 'total_cameras', data)


line too long (87 > 79 characters)

houndci-bot · 2018-07-02T07:30:46Z

+        """Test the unit_of_measurement property."""
+        sensor = TestArloSensor._get_mock_sensor()
+        self.assertIsNone(sensor.unit_of_measurement)
+        sensor = TestArloSensor._get_mock_sensor('Battery Level', 'battery_level')


line too long (82 > 79 characters)

houndci-bot · 2018-07-02T07:30:46Z

+            'battery_level': 50
+        })
+
+        sensor = TestArloSensor._get_mock_sensor('Battery Level', 'battery_level', data)


line too long (88 > 79 characters)

houndci-bot · 2018-07-02T07:30:46Z

+        self.assertEqual(sensor.name, 'Last')
+
+    @asyncio.coroutine
+    @patch('homeassistant.helpers.dispatcher.async_dispatcher_connect', MagicMock())


line too long (84 > 79 characters)

houndci-bot · 2018-07-02T07:30:46Z

+TEST_USERNAME = 'test@domain.com'
+TEST_PASSWORD = 'password'
+
+class TestArloSensor(unittest.TestCase):


expected 2 blank lines, found 1

lukiffer · 2018-07-02T15:27:39Z

@tchellomello @MartinHjelmare above comments addressed and ready for review.

MartinHjelmare

I think the tests would be cleaner if they would be converted to pytest standalone test functions and using pytest fixtures. But it's not a requirement from my side.

MartinHjelmare · 2018-07-02T21:36:03Z

    @callback
    def _update_callback(self):
        """Call update method."""
+        _LOGGER.debug('update')


Remove this. The state update will be logged by the core at info level.

MartinHjelmare · 2018-07-02T21:38:04Z

-           self._sensor_type == 'captured_today' or \
-           self._sensor_type == 'battery_level' or \
-           self._sensor_type == 'signal_strength':
+        known_sensor_types = [


This could be moved to a constant at module level.

This was a lazy workaround for address a maximum conditions in an if statement on my part. There's a dictionary with keys of known types already at the module level, I'll use that.

MartinHjelmare · 2018-07-02T21:47:26Z

+        elif self._sensor_type == 'temperature':
+            try:
+                self._state = self._data.ambient_temperature
+            except (AttributeError, TypeError):


Why would there be these errors? The library shouldn't let a type error bubble up from a simple attribute access. All attributes should be initialized after init so attribute error shouldn't be possible either. If it is, the library should be updated to handle that better.

I had these in there because the library was in some cases bubbling up an AttributeError but in further testing, it appears to be the result of an issue that was fixed in pyarlo==0.1.9 where the method to get the ambient sensor data was not being called on update. I've removed them and will push.

MartinHjelmare · 2018-07-02T21:51:34Z

+        sensor.update()
+        self.assertEqual(sensor.state, 2)
+
+    def test_update_caputred_today(self):


MartinHjelmare · 2018-07-02T21:55:14Z

+
+    def test_setup_with_no_data(self):
+        """Test setup_platform with no data."""
+        result = arlo.setup_platform(self.hass, None, None)


setup_platform shouldn't return anything. Please fix that in the module and update this test.

MartinHjelmare · 2018-07-02T22:03:42Z

+
+    def test_sensor_name(self):
+        """Test the name property."""
+        sensor = TestArloSensor._get_mock_sensor()


The method is weirdly named since it doesn't seem to return a mock sensor but an actual instance of an arlo sensor.

Access methods using self..

Will correct the method name and change to self access.

Also, just for reference as I'm coming back to python several years away, is it not optimal to use the @staticmethod decorator and treat these as "static" as opposed to members? Or is this just convention? Or is this an artifact of having this in a class as opposed to just standalone functions? Thanks for the help.

Static methods can still be accessed on the instance, ie using self. inside instance methods. It's only inside the static method that the instance isn't available.

Yeah, the static methods are part of what could be cleaned up if we would move to standalone pytest tests.

MartinHjelmare · 2018-07-02T22:04:33Z

+        sensor = TestArloSensor._get_mock_sensor()
+        self.assertEqual(sensor.name, 'Last')
+
+    @asyncio.coroutine


Remove this.

Again, having just come back to python (after several years and from python2), async methods in python3 have come quite a long way, and I'm still getting accustomed to the new stuff:

When I omit this, it complains that the function is never awaited. In tests for other HA components, I noticed that standalone functions omit this, but functions declared in classes have it. Is this correlation true or just coincidence? If it does in fact need to be removed, what's the appropriate way to address the no-await issue?

We're already declaring the method a coroutine by saying async def. This is the new async syntax in Python 3.5. So we shouldn't add the coroutine decorator on top of that. Class based unittest tests don't support coroutines as test methods. Here we have to move to standalone pytest tests.

MartinHjelmare · 2018-07-02T22:07:53Z

+        """Test dispatcher called when added."""
+        sensor = TestArloSensor._get_mock_sensor()
+        await sensor.async_added_to_hass()
+        dispatcher.async_dispatcher_connect.assert_called_once()


We prefer not to use assert_called_once but instead use assert len(mock.calls) == 1. A typo on the mock method would pass silently.

MartinHjelmare · 2018-07-02T22:09:27Z

+
+    def test_sensor_state_default(self):
+        """Test the state property."""
+        sensor = TestArloSensor._get_mock_sensor()


Use self._get_mock_sensor().

MartinHjelmare · 2018-07-02T22:13:11Z

+    sensors = None
+
+    def _add_devices(self, sensors, boolean):
+        if boolean:


Why would we only add the sensors when passing True as the second argument? That's not how the actual code works.

Not actually sure what the second param does -- just noticed that it's hardcoded True in the sensor code. But this does make the test rather brittle and potentially (probably) incorrect. I've removed the condition for now and will take another pass after some discovery.

Passing True to add_devices calls entity.update during entity addition.

lukiffer · 2018-07-03T22:25:02Z

@MartinHjelmare thanks for the feedback - super helpful insights. I converted to standalone pytest functions and cleaned up some of the other stuff you mentioned.

MartinHjelmare · 2018-07-04T09:18:28Z

-           self._sensor_type == 'signal_strength':
+        known = False
+        for sensor_type in SENSOR_TYPES:
+            if self._sensor_type == sensor_type:


We already validate this in the config schema. We can remove this check completely here.

MartinHjelmare · 2018-07-04T09:28:02Z

+def test_async_added_to_hass(default_sensor):
+    """Test dispatcher called when added."""
+    yield from default_sensor.async_added_to_hass()
+    assert len(dispatcher.async_dispatcher_connect.calls) == 1


We should check that the calls on the mock object is of correct length.

MartinHjelmare · 2018-07-04T09:28:37Z

+
+@patch('homeassistant.helpers.dispatcher.async_dispatcher_connect',
+       MagicMock())
+def test_async_added_to_hass(default_sensor):


Define this as a coroutine with async def.

MartinHjelmare · 2018-07-04T09:29:46Z

+       MagicMock())
+def test_async_added_to_hass(default_sensor):
+    """Test dispatcher called when added."""
+    yield from default_sensor.async_added_to_hass()


Use await instead of yield from.

MartinHjelmare · 2018-07-04T09:30:33Z

+    assert default_sensor.name == 'Last'
+
+
+@patch('homeassistant.helpers.dispatcher.async_dispatcher_connect',


Move this to a fixture in which we yield the mock.

houndci-bot · 2018-07-05T06:52:34Z

+    """Test dispatcher called when added."""
+    with patch(
+        'homeassistant.components.sensor.arlo.async_dispatcher_connect',
+        MagicMock()) as mock:


visually indented line with same indent as next logical line

lukiffer · 2018-07-05T07:31:31Z

@MartinHjelmare thanks again for the feedback. I fixed a problem where I was patching the wrong namespace -- this helped make sense of a lot of the other feedback you gave. I did however still have one thing I was stuck on -- not sure if it's a blocker though:

I removed the @patch() decorator from the test method intending to move it onto a fixture, but am having an issue with passing the mock by reference on the fixture. When using the with patch() as syntax in the test method, the test works correctly and I've added checks for the call_args.

However, when I pass the sensor and the mock as a fixture, e.g.:

class DispatcherFixture():
    def __init__(self, sensor, mock):
        self.sensor = sensor
        self.mock = mock

@patch('homeassistant.components.sensor.arlo.async_dispatcher_connect',
        MagicMock())
@pytest.fixture()
def dispatcher_fixture(default_sensor, hass):
    # ...
    return DispatcherFixture(default_sensor, arlo.async_dispatcher_connect)

I get dispatcher_fixture.mock as an instance of MagicMock but the only call is [call.__eq__(sentinel.DEFAULT)]. Not sure what's going awry here, but again the test is functional as committed and I don't foresee the fixture being reused for another test.

As an aside, this may be getting a bit out of my league, and given that the code under test (for this specific test) isn't part of the scope of the initial feature addition, perhaps it would be appropriate to redact this test from this PR and file a separate PR with only the appropriate/pythonic implementation of this test. What are your thoughts?

MartinHjelmare · 2018-07-05T08:54:34Z

It's ok to keep the test as is.

This is how you could have applied the patch within a fixture. Note the yield.

@pytest.fixture
def mock_dispatch():
    with patch('path.to.dispatch') as _mock:
        yield _mock

MartinHjelmare

It's easier to catch bugs if we set up the complete platform, while patching appropriately, and run all checks on the entity using the home assistant core, eg get the state from the state machine and check that it's ok. Ie write the tests more as integration tests than isolated unit tests.

MartinHjelmare · 2018-07-05T09:15:16Z

-           self._sensor_type == 'battery_level' or \
-           self._sensor_type == 'signal_strength':
-            attrs['model'] = self._data.model_id
+        attrs['model'] = self._data.model_id


Add a test or update the test where all sensor types are set up, and test that attributes return ok also for the total cameras type. I think we need to add a check here where we don't add the model attribute for that sensor type.

lukiffer · 2018-07-05T16:54:31Z

@MartinHjelmare I've added the additional test cases you mentioned and updated the dispatcher test to use a fixture -- thanks for giving me the last piece of the puzzle on that one (again). I also added the check for total_cameras sensor type -- good catch.

I'm pretty sure that what you mentioned about having fully-setup integration tests as opposed to unit tests is not satisfied by the latest commits. I can definitely see the benefit of this approach (especially given the total_cameras example you highlighted). I would like to potentially focus on that a bit more in a separate branch/PR given the scope of those changes. Is that acceptable or should I proceed in this branch?

MartinHjelmare

Looks good!

lukiffer · 2018-07-06T02:48:33Z

@MartinHjelmare thanks again for all your help with this. @tchellomello could you also give these changes another look? Thanks!

tchellomello

Looks good @lukiffer!! Thanks for your contribution!!

Thanks @MartinHjelmare for the code review!!

* Add additional sensors for Arlo Baby camera * Fix linter errors * Fix linter error * Add tests for Arlo sensors * Fix linter errors * Bump pyarlo dependency to 0.1.9 * Remove unnecessary AttributeError except * Fix module reference error in py35 * Fix test * Address PR review concerns * Convert to standalone pytest methods * Fix linter errors * Fix linter errors * Fix linter errors * Fix test * Remove redundant check, fix async test * Fix linter error * Added check for total_cameras sensor, added additional attribute tests * Add missing docstring

Add additional sensors for Arlo Baby camera

88a0a43

homeassistant added cla-signed platform: sensor.arlo labels Jun 21, 2018

ghost added the in progress label Jun 21, 2018

houndci-bot reviewed Jun 21, 2018

View reviewed changes

Fix linter errors

ac179ca

homeassistant added the cla-signed label Jun 21, 2018

lukiffer changed the title ~~Add additional sensors for Arlo Baby camera~~ WIP Add additional sensors for Arlo Baby camera Jun 21, 2018

MartinHjelmare reviewed Jun 21, 2018

View reviewed changes

This was referenced Jun 25, 2018

Add docs for additional Arlo Baby sensors home-assistant/home-assistant.io#5583

Merged

Fix issue where ambient sensor data is never updated tchellomello/python-arlo#90

Merged

tchellomello suggested changes Jul 2, 2018

View reviewed changes

lukiffer added 3 commits July 1, 2018 22:16

Fix linter error

a6ce54f

Add tests for Arlo sensors

35e7541

Merge branch 'dev' of github.com:home-assistant/home-assistant into a…

9c07039

…rlo-baby

homeassistant added the cla-signed label Jul 2, 2018

houndci-bot reviewed Jul 2, 2018

View reviewed changes

lukiffer added 5 commits July 2, 2018 02:36

Fix linter errors

fe94826

Bump pyarlo dependency to 0.1.9

5890a18

Remove unnecessary AttributeError except

fe532ed

Fix module reference error in py35

c15964a

Fix test

597904a

lukiffer changed the title ~~WIP Add additional sensors for Arlo Baby camera~~ Add additional sensors for Arlo Baby camera Jul 2, 2018

MartinHjelmare reviewed Jul 2, 2018

View reviewed changes

Address PR review concerns

046c62b

homeassistant added the cla-signed label Jul 3, 2018

lukiffer changed the title ~~Add additional sensors for Arlo Baby camera~~ WIP Add additional sensors for Arlo Baby camera Jul 3, 2018

lukiffer added 3 commits July 3, 2018 13:20

Fix linter errors

4178527

Fix linter errors

61a5821

Fix test

7f3a096

lukiffer changed the title ~~WIP Add additional sensors for Arlo Baby camera~~ Add additional sensors for Arlo Baby camera Jul 3, 2018

MartinHjelmare reviewed Jul 4, 2018

View reviewed changes

Remove redundant check, fix async test

39c8639

homeassistant added the cla-signed label Jul 5, 2018

houndci-bot reviewed Jul 5, 2018

View reviewed changes

Fix linter error

d4610af

MartinHjelmare reviewed Jul 5, 2018

View reviewed changes

Added check for total_cameras sensor, added additional attribute tests

07a1ebf

homeassistant added the cla-signed label Jul 5, 2018

Add missing docstring

b994e56

MartinHjelmare approved these changes Jul 5, 2018

View reviewed changes

tchellomello approved these changes Jul 6, 2018

View reviewed changes

MartinHjelmare merged commit 0f1bcfd into home-assistant:dev Jul 6, 2018

ghost removed the in progress label Jul 6, 2018

lukiffer deleted the arlo-baby branch July 9, 2018 20:10

grantclem mentioned this pull request Jul 16, 2018

Arlo errors occurring after 0.74.0b0 update #15486

Closed

balloob mentioned this pull request Jul 20, 2018

0.74 #15570

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018

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

		assert default_sensor.name == 'Last'


		@patch('homeassistant.helpers.dispatcher.async_dispatcher_connect',

Uh oh!

Conversation

lukiffer commented Jun 21, 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!

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!

tchellomello Jun 24, 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!

tchellomello commented Jun 24, 2018

Uh oh!

lukiffer commented Jun 25, 2018

Uh oh!

tchellomello left a comment • 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!

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!

lukiffer commented Jul 2, 2018

Uh oh!

MartinHjelmare left a comment • 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!

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

lukiffer commented Jun 21, 2018 •

edited

Loading

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

tchellomello Jun 24, 2018 •

edited

Loading

tchellomello left a comment •

edited

Loading

MartinHjelmare left a comment •

edited

Loading

lukiffer commented Jul 5, 2018 •

edited

Loading