-
-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
Refactor tests for Brother integration #117377
Conversation
with ( | ||
patch("brother.Brother.initialize"), | ||
patch("brother.Brother._get_data", side_effect=UnsupportedModelError("error")), | ||
patch("homeassistant.components.brother.Brother.initialize"), | ||
patch( | ||
"homeassistant.components.brother.Brother._get_data", | ||
side_effect=UnsupportedModelError("error"), | ||
), | ||
): |
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.
2 things, why isn't this patched in the config flow, and can't we use the new mock for this?
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.
This exception may occur in the Brother.create
method, and this method is used to create an instance of the Brother
class, I was unable to get AsyncMock to raise the exception as a side_effect
. Something like this doesn't work
mock_brother_client.create.side_effect = UnsupportedModelError("error")
Maybe there is a better way to achieve this?
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'm just going to throw some ideas, I will have some time later to actually test some
mock_client.create.return_value.side_effect = UnsupportedModelError("error")
mock_client.create.return_value = UnsupportedModelError("error")
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.
Unfortunately, these propositions also do not give the desired effect 😒
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.
Have you tried something like
core/tests/components/shelly/test_config_flow.py
Lines 867 to 869 in f23419e
patch( | |
"aioshelly.block_device.BlockDevice.create", | |
new=AsyncMock(side_effect=InvalidAuthError), |
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.
Yes, this simplifies tests a bit, but I still can't force a fixture mock to raise an exception for the create()
method.
with ( | ||
patch("brother.Brother.initialize"), | ||
patch("brother.Brother._get_data") as mock_get_data, | ||
patch("homeassistant.components.brother.Brother.initialize"), | ||
patch("homeassistant.components.brother.Brother._get_data") as mock_get_data, | ||
): |
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.
idem
unique_id="0123456789", | ||
data={CONF_HOST: "localhost", CONF_TYPE: "laser"}, | ||
) | ||
with patch("homeassistant.components.brother.Brother.create", side_effect=exc): |
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 can use the asyncmock here as well I think
ab73cf3
to
bb23428
Compare
Breaking change
Proposed change
Refactors tests and increases coverage.
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: