Skip to content

Add default value to timeout on SamsungTV Device #17221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
anapaulagomes wants to merge 1 commit into from
Closed

Add default value to timeout on SamsungTV Device #17221

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion homeassistant/components/media_player/samsungtv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def setup_platform(hass, config, add_entities, discovery_info=None):
class SamsungTVDevice(MediaPlayerDevice):
"""Representation of a Samsung TV."""

def __init__(self, host, port, name, timeout, mac):
def __init__(self, host, port, name, mac, timeout=0):
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There already is a default timeout defined. Why is this change needed?

Copy link
Copy Markdown
Contributor Author

@anapaulagomes anapaulagomes Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a default but when the config entry doesn't exist this value is None.
https://github.com/home-assistant/home-assistant/blob/400658f23649d22c7214c8e4a88127119d653612/homeassistant/components/media_player/samsungtv.py#L63

This is causing the issue presented in the description. I added the default timeout in the line above (timeout = config.get(CONF_TIMEOUT, DEFAULT_TIMEOUT)) and a test for it but the problem remains when testing in a real device. Any thoughts?

This test reproduces the problem:

    @MockDependency('samsungctl')
    @MockDependency('wakeonlan')
    def test_setup_without_timeout(self, samsung_mock, wol_mock):
        """Testing setup of platform to unset timeout."""
        with mock.patch(
                'homeassistant.components.media_player.samsungtv.socket'):
            add_entities = mock.Mock()

            WORKING_CONFIG_WITHOUT_TIMEOUT = WORKING_CONFIG.copy()
            WORKING_CONFIG_WITHOUT_TIMEOUT.pop(CONF_TIMEOUT)

            setup_platform(
                self.hass, WORKING_CONFIG_WITHOUT_TIMEOUT, add_entities)

            assert add_entities.called
            device = add_entities.call_args[0][0][0]
            assert device._config['timeout'] == DEFAULT_TIMEOUT

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a default defined in the config schema. When setting up via config the platform schema will populate default values in the config.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your test example is incomplete. We need to set up the platform via the core with setup_component to have config validation be done.

"""Initialize the Samsung device."""
from samsungctl import exceptions
from samsungctl import Remote
Expand Down