Repository navigation
fix: workaround HA discovery cache miss - #12
Conversation
jmattsson
left a comment
jmattsson
left a comment
There was a problem hiding this comment.
I think it makes sense? I'm mostly surprised this isn't handled automatically by HA, but I guess this would be nice to have for other users too.
|
|
||
| # How long to wait for a plug to answer when its records are not cached, | ||
| # matching Home Assistant's own zeroconf discovery. | ||
| _REQUEST_TIMEOUT_MS = 10000 |
There was a problem hiding this comment.
We have knowledge of the plug implementation, we should be able to drop this a lot. I'm seeing ~200ms response times on my network, fwiw. How much delay can we realistically expect on a bad WiFi network? 10 seconds seems outlandish - nobody is going to have buffers for that amount of data?
There was a problem hiding this comment.
I'm not sure what an appropriate timeout should be and I don't see any harm leaving it at 10s. I'm happy to change it to a different value, question is how many seconds is appropriate?
There was a problem hiding this comment.
I wouldn't go above five myself. Also, the comment is a recipe for becoming outdated 🙃
| seeded from cached PTR records, and the plug's SRV and A records | ||
| have much shorter lifetimes, so they may have expired. Waiting for | ||
| the plug to announce itself again can take a long time, so ask it | ||
| directly instead, as Home Assistant's own zeroconf discovery does. |
There was a problem hiding this comment.
Gah, why doesn't HA do this internally on a reload? It seems like this would be a common issue across all zeroconf users?
| the plug to announce itself again can take a long time, so ask it | ||
| directly instead, as Home Assistant's own zeroconf discovery does. | ||
| """ | ||
| info = AsyncServiceInfo(type_, name) |
There was a problem hiding this comment.
The AsyncServiceInfo looks to be an empty class deriving from the regular ServiceInfo. Weird.
| info = fake_service_info(cached=False, answers=True) | ||
| with patch.object(zd, 'AsyncServiceInfo', info), \ | ||
| patch.object(devices, handler, AsyncMock()) as on_resolved: | ||
| getattr(zd._Listener(devices), method)(MagicMock(), TYPE, NAME) |
There was a problem hiding this comment.
Again, we don't need to keep creating new mocks for the Zeroconf object
| async def test_add_service_uses_cached_records() -> None: | ||
| devices = PowersensorZeroconfDevices(zeroconf_instance=MagicMock()) | ||
| info = fake_service_info(cached=True, answers=False) | ||
| with patch.object(zd, 'AsyncServiceInfo', info), \ |
There was a problem hiding this comment.
Hang on, AsyncServiceInfo is in zeroconf.async, not powersensor_local.zeroconf_devices?
There was a problem hiding this comment.
When zeroconf_devices.py does from zeroconf.asyncio import AsyncServiceInfo, it copies the name into zeroconf_devices. When _resolve runs AsyncServiceInfo, it looks up the name in zeroconf_devices and not zeroconf.asyncio?
| ) | ||
| info = fake_service_info(cached=False, answers=False) | ||
| with caplog.at_level(logging.WARNING), \ | ||
| patch.object(zd, 'AsyncServiceInfo', info), \ |
There was a problem hiding this comment.
Hang on, AsyncServiceInfo is in zeroconf.async, not powersensor_local.zeroconf_devices?
| with caplog.at_level(logging.WARNING), \ | ||
| patch.object(zd, 'AsyncServiceInfo', info), \ | ||
| patch.object(devices, '_on_zc_add', AsyncMock()) as on_add: | ||
| zd._Listener(devices).add_service(MagicMock(), TYPE, NAME) |
| zd._Listener(devices).add_service(MagicMock(), TYPE, NAME) | ||
| await settle(devices) | ||
| on_add.assert_not_awaited() | ||
| assert f'No response from {NAME}' in caplog.text |
There was a problem hiding this comment.
Add test case for suggested new log entry too.
There was a problem hiding this comment.
Added test_invalid_record_is_ignored unit test to cover the log entries.
|
|
||
| def fake_service_info( | ||
| cached: bool, answers: bool, id_: bytes = b'aabbccddeeff', | ||
| ) -> type[Any]: |
There was a problem hiding this comment.
The hell?? Github lost my comment here?!
Can we tighten up the return type to an AsyncServiceInfo and have FakeServiceInfo derive from AsyncServiceInfo?
| async def test_malformed_service_name_is_ignored() -> None: | ||
| # Stands in for AsyncServiceInfo rejecting a name. The real constructor is | ||
| # not used because, under coverage, zeroconf's compiled modules can raise a | ||
| # copy of the exception class that is not zeroconf.BadTypeInNameException. |
There was a problem hiding this comment.
Huh? But we aren't instantiating a real Zeroconf object anywhere?
There was a problem hiding this comment.
I've added this comment on top of the file to explain the reason:
# The zeroconf instance is a MagicMock rather than a real Zeroconf. _Listener
# only hands it to AsyncServiceInfo, which the tests replace, and the tests call
# the listener directly instead of starting a browser, so nothing reads it. A
# real instance would also open multicast sockets on port 5353 and send queries
# onto the local network, making the tests slow and dependent on the host's
# network setup.
| # not used because, under coverage, zeroconf's compiled modules can raise a | ||
| # copy of the exception class that is not zeroconf.BadTypeInNameException. | ||
| def reject(type_: str, name: str) -> NoReturn: | ||
| raise zeroconf.BadTypeInNameException |
There was a problem hiding this comment.
This is the only mention of the zeroconf module I see in this test file; we could just from zeroconf import BadTypeInNameException?
There was a problem hiding this comment.
Now we have zeroconf.Zeroconf as well.
jmattsson
left a comment
jmattsson
left a comment
There was a problem hiding this comment.
Just need to resolve the merge conflict
No description provided.