Add tests for directv platform

[ehendrix23]

Description:

Create test for DirecTV platform.
Added media_stop to common for media player

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.

[MartinHjelmare]

We prefer standalone pytest test functions for new tests. See eg the deconz test suite.

[ehendrix23]

@MartinHjelmare , all changed. Let me know. :-)

[MartinHjelmare]

Use MockDependency in tests/common.py.

[ehendrix23]

Done. :-)

[MartinHjelmare]

Don't set up the platform directly. Set up the component with async_setup_component and pass the config with the media_player component and the directtv platform.

[ehendrix23]

Done :-)

[ehendrix23]

@MartinHjelmare ,

All changed accordingly, let me know how it looks. This is the 1st test I'm writing. :-)

[houndci-bot]

multiple spaces before operator

[MartinHjelmare]

Set up the component but without the platform config and then fire the correct discovery event to test discovery.

[MartinHjelmare]

We might need to set up discovery component and not the media_player component.

[MartinHjelmare]

Nah, let's just call async_load_platform from helpers.discovery.py.

[ehendrix23]

@MartinHjelmare

Done all the other ones, but this one I'm stuck at. Can you have a look on what I put there and see where I'm off.
It states that it requires a valid hass_config and right now not sure what is meant with that and where to get it. :-)

Thx!

[MartinHjelmare]

An empty dict as fifth argument to async_load_platform should work as hass config in this case.

[ehendrix23]

Just seeing your message. Not passing an empty dict but instead just one with component in it.
Can change to empty if you want me to. Let me know. :-)

[MartinHjelmare]

Why make a factory? Instead yield inside the context manager from the fixture to keep the patch in place during the test.

[ehendrix23]

I used it to be able to return the entity objects. Changed now.

[MartinHjelmare]

Don't use mock assert methods. A simple typo in the method name could make the assertion pass when it should fail. Import mock.call helper method and assert the call_args attribute on the mock instance.

[ehendrix23]

@MartinHjelmare , could you provide an example on what you mean. Not fully getting what the difference would be between"
mock_key_press.assert_called_with('poweron')
and
assert mock_key_press.call_args == call('poweron')

PS, did the changes though. :-)

[MartinHjelmare]

If we do a typo in the mock assert method name, it will be interpreted as a call to a mock mock method which will always pass. That can't happen in the latter case.

[ehendrix23]

Thus you mean if we do a typo within the test itself. For example:
mock_key_press.assert_caled_with('poweron')
Then the assert will pass no matter what.

I just tested this.
mock_key_press.assert_caled_with('poweron')
fails with the error:
AttributeError: assert_caled_with

And if I try:
mock_key_pres.assert_called_with('poweron')
then it fails with NameError: name 'mock_key_pres' is not defined

And type in the patch.object itself (using 'key_pres' instead of 'key_press' gives me:
AttributeError: <tests.components.media_player.test_directv.MockDirectvClass object at 0x106ddd898> does not have the attribute 'key_pres'

And if I try:
mock_key_press.asert_called_with('poweron')
then I get: AttributeError: 'function' object has no attribute 'asert_called_with'

Not sure if something has changed. I did find:
https://engineeringblog.yelp.com/2015/02/assert_called_once-threat-or-menace.html

But I tried what they did there and it failed (assert_called_once in their example does work but it is also a valid assert from Python 3.6 onwards).

[MartinHjelmare]

Don't return the entities. We should use the states from the state machine to assert state in the tests.

[ehendrix23]

I also require the entity to be able to do patching of certain items within a test. Changed coding to just get the entity object in the test where required instead.

[MartinHjelmare]

MockDIRECTV

https://www.python.org/dev/peps/pep-0008/#class-names

[ehendrix23]

Based on that, should be: MockDirectvClass?

FYI, reason why I called it mockDIRECTVClass is cause the name of the original class is DIRECTV :-)

[houndci-bot]

continuation line over-indented for visual indent

[ehendrix23]

@MartinHjelmare OK, did those updates as well. :-) Been quite a learning experience. .

Review and let me know if there is anything else. :-)

[houndci-bot]

'asyncio' imported but unused

[MartinHjelmare]

It's redundant to assert the entities after asserting the states. Remove all entity assertions. If needed add assertions about the states, eg what entity_id the existing states have.

The point is that entities are details of the platforms and the tests will be much more robust if we use the core to both set up and assert the state of the tests. We shouldn't rely on the platforms to do that.

[ehendrix23]

Changed to get the state for the entity and then assert if state exist. If it does then we know the entity was indeed created with the correct name.

[MartinHjelmare]

Instead of accessing the entity to modify that patched instance, add a side_effect to the patch of DirectPy.DIRECTV. We can make the side_effect parameter a function, where each call to the mock will return the next mocked dtv instance.

https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock

To be able to modify the side_effect on a test basis, add a pytest fixture that returns the function that we want to use for the side_effect. Pass this fixture to the fixture here, and use it as side_effect argument. Pytest fixtures can be overridden per test.

https://docs.pytest.org/en/latest/fixture.html#override-a-fixture-with-direct-test-parametrization

Also create different fixtures that return the different DirectPy.DIRECTV mocked instances that we need to use in the tests. This will allow us to easily access them in the tests to assert calls to the instances. And make sure to have the MockDirectvClass class return a mock with the correct spec, so we can assert calls on instances of that class directly without doing further patching in the tests.

@pytest.fixture
def client_dtv():
    mocked_dtv = MockDirectvClass('mock_ip')
    mocked_dtv.attributes = RECORDING
    mocked_dtv._standby = False
    return mocked_dtv


@pytest.fixture
def main_dtv():
    return MockDirectvClass('mock_ip')


@pytest.fixture
def dtv_side_effect(client_dtv, main_dtv):
    def mock_dtv(ip, port, client_addr):
        mocked_dtv = next(iter([main_dtv, client_dtv]))
        mocked_dtv._host = ip
        mocked_dtv._port = port
        mocked_dtv._device = client_addr
        return mocked_dtv
    return mock_dtv


@pytest.fixture
def platforms(hass, dtv_side_effect):
    ...
    with MockDependency('DirectPy'), \
            patch('DirectPy.DIRECTV', side_effect=dtv_side_effect):
        hass.loop.run_until_complete(async_setup_component(
            hass, mp.DOMAIN, config))
        hass.loop.run_until_complete(hass.async_block_till_done())
        yield


async def test_main_services(hass, platforms, main_dtv):
    ...
    assert main_dtv.get_standby.call_count == 6

In the end, this should allow us to totally avoid accessing and modifying the entities of the tests directly, making the tests more robust.

[ehendrix23]

Made the changed and been going over it for a while to understand. However this is not working right now.
I get the following error:

ERROR:homeassistant.helpers.entity:Update for media_player.main_dvr fails
Traceback (most recent call last):
  File "/Users/ehendrix/Documents/GitHub/home-assistant/homeassistant/helpers/entity.py", line 221, in async_update_ha_state
    await self.async_device_update()
  File "/Users/ehendrix/Documents/GitHub/home-assistant/homeassistant/helpers/entity.py", line 349, in async_device_update
    await self.hass.async_add_executor_job(self.update)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/ehendrix/Documents/GitHub/home-assistant/homeassistant/components/media_player/directv.py", line 148, in update
    self._is_standby = self.dtv.get_standby()
AttributeError: 'NoneType' object has no attribute 'get_standby'

Still trying to figure this out. Looks to me that what is provided is just a MockClass and not the actual MockDirectvClass.

[MartinHjelmare]

I pushed some fixes.

[ehendrix23]

@MartinHjelmare

Back. :-) Was spending some time with family over the break and was "banned" from my computer. :-)

Thank you SO much for your help in this. Been going through the changes you've made to ensure I understand as well what you did and I believe I pretty much get it. :-)
I do see that you execute async_fire_time_changed before each test. Is that to ensure that HASS does a poll for status update?

I had the asserts for hass.data[DATA_DIRECTV] in there to ensure that the information for this is added correctly as it is then used at other times to prevent duplicate entities from being created.
Thus there is the test that makes sure that no duplicates are created based on the data in hass.data[DATA_DIRECTV] and then there are the asserts to make sure that hass.data[DATA_DIRECTV] is updated accurately. My personal preference would be that those are in there. :-)

Let me know on the asserts. Test here ran fine. Guessing I will need to do a small commit/push to remove the "Changes Requested".

Thx.

[MartinHjelmare]

Yes, we fire a time changed event to make the scheduled polling trigger.

The state machine is the truth of the entities. Only case I've come across where we need to test hass.data when testing platforms is leftover dispatcher signals. That's not the case here I believe.

[ehendrix23]

As you state, the state machine is the truth of the entities. Yet within the platform we use hass.data to determine if the device (i.e. discovered) is already created or not.

Would it thus be better to include the data required to check if a duplicate exist as attributes of the entity and then to retrieve those attributes for all entities of the platform? And thus not relying on hass.data information?

[MartinHjelmare]

If we think this is good, just remove WIP from PR title and we can merge.

[ehendrix23]

Thanks again for all the help. Will be using what was done here as examples for any next test I would write. :-)

[MartinHjelmare]

If we would create a duplicate entity, there would be two states right? We can test that using the state machine. We shouldn't change the code under test. Using hass.data in the code under test is fine. Not in the tests.