Add default value to timeout on SamsungTV Device#17221
Add default value to timeout on SamsungTV Device#17221anapaulagomes wants to merge 1 commit intohome-assistant:devfrom
Conversation
|
This fixes the issue...thanks for the quick fix 👍 |
MartinHjelmare
left a comment
There was a problem hiding this comment.
Update the argument order when instantiating the entity too.
| """Representation of a Samsung TV.""" | ||
|
|
||
| def __init__(self, host, port, name, timeout, mac): | ||
| def __init__(self, host, port, name, mac, timeout=0): |
There was a problem hiding this comment.
There already is a default timeout defined. Why is this change needed?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
There's a default defined in the config schema. When setting up via config the platform schema will populate default values in the config.
There was a problem hiding this comment.
Your test example is incomplete. We need to set up the platform via the core with setup_component to have config validation be done.
|
Turns out we found out that changing DEFAULT_TIMEOUT to 1 would do the trick. @arsaboo is gonna open a PR. I'm closing this one. Thanks you all. |
Description:
Add default value to timeout. cc @arsaboo
Related issue (if applicable): fixes #17203
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: