-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
Fix MatrixBot not resolving room aliases per-command #106347
Conversation
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @PaarthShah 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comment in a new PR. Thanks!
|
||
await hass.async_start() | ||
assert len(command_events) == 0 | ||
await matrix_bot._handle_room_message(room, command_params.room_message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer patching the library and setting up the integration, then calling the (notify) service and assert mock call args. We shouldn't interact with integration details in the tests.
https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In test_notify
, that's exactly what's being done for the purpose of actually testing that the underlying implementation detail is working as expected in the context of an automation using the notify service to send a message.
However, using the notify service like that is actively incorrect for the purpose of testing commands: the notify service will cause a message to be sent by the user that the bot itself is logged in as.
The bot is not supposed to respond to otherwise-valid commands sent by its own user (whether in this current session as used by home assistant, or by another login on another device).
One of the new test cases even explicitly checks for this.
In summary:
_handle_send_message
is therefore being invoked directly because:
- we test its proper behavior with the notify service in a different set of unit tests
- using it is a convenience over having to completely re-implement sending a message within the test code just to be able to send a message as someone else (when that implementation would look nearly the same as the implementation detail, for good reason)
- also prevent us from having to unnecessarily mock away the other implementation detail of
AsyncClient
any further than it already has beenMockAsyncClient
inconftest
already mocks away the lower-level IO logic performed by the 3rd party dependencymatrix-nio
, but further mocking the actual result of sending a message would make it unsafe (in my view as the actual maintainer ofmatrix-nio
) since it'd obscure any changes that occur in its API
I'm not against trying to improve the test, just the opposite, and I appreciate you taking a look! But I'd need to find a way to preserve the total effectiveness/correctness of the tests while doing so, and the notify service simply would cause it to become incorrect: any ideas anyone may have on this are very welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the integration a bit more. _handle_room_message
is a callback passed to client.add_event_callback
. So instead of knowing about an integration detail we should access the callback via the mock_client.return_value.add_event_callback.call_args
attribute.
Please refactor away all direct usage of matrix_bot
and use mock_client
as needed instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding correctly, this would involve actually unittest.patch
ing add_event_callback
?
Because right now that method on MockAsyncClient
isn't patched or even overridden so grabbing its call_args
isn't possible; is it actually desirable/acceptable to do so for this purpose? (I vaguely remember getting some mixed feedback on this approach during my first implementation of this, but I could be mistaken).
But if that's all fine, then perfect I can take that on in a different PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend autospeccing the mock instead and setting return values on the mock methods that need that, eg the methods that should return responses. An autospecced mock will have all real methods mocked and using other mock methods will raise.
Currently we return a custom class that inherit the real class as mock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep sounds good 👌🏾
Proposed change
Restore the intended behavior of the
rooms
field incommands
as documented in https://www.home-assistant.io/integrations/matrix/#roomsAs the result of a previous refactor, this behavior regressed as described in the original linked issue.
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
..coveragerc
.To help with the load of incoming pull requests: