ping: Suppress ProcessLookupError on timeout #134281
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @tswsl1989
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
bot
added
cla-needed
integration: ping
small-pr
|
Hey there @jpbede, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
home-assistant
bot
added
Quality Scale: internal
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
tswsl1989
force-pushed
the
ping-exceptions
branch
from
3965ecc to
3cf4a56
Compare
|
The change looks good. Would you please add a test to cover this case? |
|
I've added what I think are a pair of sensible tests for this, although I couldn't find similar examples in the tests folder. Let me know if you think there's a better approach. |
When using the ping command, the command is killed if the timeout limit is reached before output is returned. If the ping process has exited before the exception is handled, a ProcessLookupError is raised. This generates exceptions in the log, but adds no useful information. As the command is being killed here to ensure it's terminated as part of error handling, suppress the exception raised in this instance to reduce noise in the application log.
Tests that expected exceptions are handled, but unexpected exceptions are still raised. Patches out asyncio.create_subprocess_exec to generate exceptions, which also means that the external ping command is not called during these tests.
tswsl1989
force-pushed
the
ping-exceptions
branch
from
a30f380 to
5b01f31
Compare
|
Normally we would want an end to end test, but thats likely not possible here because you are mocking a race condition. So I think its fine to test the helper directly in this case |
| raise self.killsig | ||
|
|
||
|
|
||
| async def test_async_ping_expected_exceptions( |
There was a problem hiding this comment.
I'd use pytest.mark.parametrize to test TypeError as well
Proposed change
Suppress log messages when the ping command exits after the timeout is reached.
If the ping command reaches the timeout while Home Assistant is waiting for data, the command is killed as part of the clean up. In some cases, the command can apparently exit before the clean up code is reached, causing an additional unhandled error to occur.
Suppressing this specific exception prevents the extra error messages being emitted, and allows the remainder of the exception handling to proceed.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: