Show new errors from the MotionMount #137006

RJPoelstra · 2025-01-31T13:07:41Z

Proposed change

This change updates the 'error sensor' that can now show the new errors that are generated by the MotionMount

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

andrewsayre

Looks good, however, would you please add some tests to cover these changes? You'll need to cover around 82% for the checks to pass, which you should be able to do by testing the entity state.

home-assistant · 2025-01-31T16:46:31Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

RJPoelstra · 2025-01-31T17:14:52Z

Hi @andrewsayre, thanks for your review.
Is it oké if I add the tests later? There are currently only the required tests for the config flow, but I intend to add 100% coverage after getting the Bronze quality scale. I’ve already prepared a PR for bronze, so I think full test coverage will only take a week or 2.
Adding all required boiler plate for the entity tests feels a bit out of scope for this simple PR.

andrewsayre · 2025-01-31T17:42:08Z

I'd rather see the start of testing here vs. it all coming in (a presumably large PR) later, even if it requires adding test setup/mocking code now. I don't consider that out of scope. 👍

andrewsayre · 2025-01-31T18:01:59Z

To clarify, for the CI to pass, you only need to add tests for the changes, not the entire integration. A test that asserts the entity state should be all that's needed. 🙂

RJPoelstra · 2025-01-31T18:56:55Z

As of yet I’ve no idea how to add any entity test, so this is going to take some considerable time to add.

andrewsayre · 2025-01-31T19:26:40Z

Yeah, I hear it can feel daunting when it's new. Would you like to take a stab at it first? Otherwise I'd be happy to get a test started.

Here’s an example test for the state: https://github.com/home-assistant/core/blob/dev/tests/components/heos/test_media_player.py#L73

The most work is behind the scenes mocking out the API calls with data. The config flow tests should already do this though.

RJPoelstra · 2025-01-31T19:29:29Z

I'll give it a try.
If I'm stuck I'll let you know :)

homeassistant/components/motionmount/sensor.py

andrewsayre

Great job on the tests! A few comments to look at! Let me know if you have any questions!

homeassistant/components/motionmount/sensor.py

andrewsayre · 2025-02-03T18:50:00Z

tests/components/motionmount/test_sensor.py

+    type(mock_motionmount_config_flow).name = PropertyMock(return_value=ZEROCONF_NAME)
+    type(mock_motionmount_config_flow).mac = PropertyMock(return_value=MAC)
+    type(mock_motionmount_config_flow).is_authenticated = PropertyMock(
+        return_value=True
+    )


Instead of patching the type, patch the location where motionmount is imported so new instances are MagicMocks. Then you can use the mock object to set the values needed directly. Something similar to this:

with patch( "homeassistant.components.motionmount.motionmount.MotionMount", autospec=True, ) as motionmount_mock: motionmount_mock.name.return_value = ZEROCONF_NAME ... assert await hass.config_entries.async_setup(mock_config_entry.entry_id)

I think most integrations move this into a fixture generator, similar to mock_motionmount_config_flow.

I've got it working using the patch.
I tried updating the generator, but I can't wrap my head around it.

tests/components/motionmount/test_sensor.py

andrewsayre

Looking good! A few minor change and this will be ready!

andrewsayre · 2025-02-04T15:41:12Z

homeassistant/components/motionmount/sensor.py

@@ -9,6 +10,14 @@
 from . import MotionMountConfigEntry
 from .entity import MotionMountEntity

+error_messages = {


Module-level consts should be all-caps (and optionally denoted as Final):

Suggested change

error_messages = {

from typing import Final

ERROR_MESSAGES: Final = {

andrewsayre · 2025-02-04T15:41:26Z

homeassistant/components/motionmount/sensor.py


-            return "internal"
+        for error, message in error_messages.items():


Suggested change

for error, message in error_messages.items():

for error, message in ERROR_MESSAGES.items():

andrewsayre · 2025-02-04T15:42:53Z

tests/components/motionmount/test_sensor.py

+
+
+@pytest.mark.parametrize(
+    "system_status",


Parameterize supports multiple arguments and will split the tuple into the args for you automatically:

Suggested change

"system_status",

("system", "status"),

andrewsayre · 2025-02-04T15:43:46Z

tests/components/motionmount/test_sensor.py

+async def test_error_status_sensor_states(
+    hass: HomeAssistant,
+    mock_config_entry: MockConfigEntry,
+    system_status: (MotionMountSystemError, str),


Suggested change

system_status: (MotionMountSystemError, str),

status: MotionMountSystemError,

state: str,

andrewsayre · 2025-02-04T15:43:55Z

tests/components/motionmount/test_sensor.py

+    system_status: (MotionMountSystemError, str),
+) -> None:
+    """Tests the state attributes."""
+    (status, state) = system_status


Suggested change

(status, state) = system_status

home-assistant bot added cla-signed integration: motionmount new-feature small-pr PRs with less than 30 lines. by-code-owner Quality Scale: No score labels Jan 31, 2025

RJPoelstra marked this pull request as ready for review January 31, 2025 13:13

andrewsayre requested changes Jan 31, 2025

View reviewed changes

home-assistant bot marked this pull request as draft January 31, 2025 16:46

RJPoelstra added 2 commits February 3, 2025 14:12

Show the new errors from the MotionMount

0f3e6ef

Add tests

965424c

RJPoelstra force-pushed the show-new-errors branch from 8d7c6d2 to 965424c Compare February 3, 2025 13:13

RJPoelstra marked this pull request as ready for review February 3, 2025 13:19

home-assistant bot requested a review from andrewsayre February 3, 2025 13:19

joostlek reviewed Feb 3, 2025

View reviewed changes

homeassistant/components/motionmount/sensor.py Outdated Show resolved Hide resolved

RJPoelstra added 2 commits February 3, 2025 16:34

Iterate over dict to get error message

d61a9be

Reduce namespace prefixes

a3aa454

joostlek reviewed Feb 3, 2025

View reviewed changes

homeassistant/components/motionmount/sensor.py Outdated Show resolved Hide resolved

joostlek reviewed Feb 3, 2025

View reviewed changes

homeassistant/components/motionmount/sensor.py Outdated Show resolved Hide resolved

Fix casing

a5a07fd

andrewsayre requested changes Feb 3, 2025

View reviewed changes

home-assistant bot marked this pull request as draft February 3, 2025 18:56

Move errors to module level

7c47cdf

RJPoelstra added 2 commits February 4, 2025 10:12

Use parameter for testing various system states

61d3c72

Use patch to create mock

344d3a9

RJPoelstra marked this pull request as ready for review February 4, 2025 12:12

home-assistant bot requested a review from andrewsayre February 4, 2025 12:12

andrewsayre requested changes Feb 4, 2025

View reviewed changes

home-assistant bot marked this pull request as draft February 4, 2025 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show new errors from the MotionMount #137006

Show new errors from the MotionMount #137006

RJPoelstra commented Jan 31, 2025

andrewsayre left a comment

home-assistant bot commented Jan 31, 2025

RJPoelstra commented Jan 31, 2025 •

edited

Loading

andrewsayre commented Jan 31, 2025 •

edited

Loading

andrewsayre commented Jan 31, 2025

RJPoelstra commented Jan 31, 2025

andrewsayre commented Jan 31, 2025

RJPoelstra commented Jan 31, 2025

andrewsayre left a comment

andrewsayre Feb 3, 2025 •

edited

Loading

RJPoelstra Feb 4, 2025

andrewsayre left a comment

andrewsayre Feb 4, 2025

andrewsayre Feb 4, 2025

andrewsayre Feb 4, 2025

andrewsayre Feb 4, 2025

andrewsayre Feb 4, 2025

-error_messages = {
+from typing import Final
+ERROR_MESSAGES: Final = {


		return "internal"
		for error, message in error_messages.items():

	system_status: (MotionMountSystemError, str),
	status: MotionMountSystemError,
	state: str,



		@pytest.mark.parametrize(
		"system_status",

Show new errors from the MotionMount #137006

Are you sure you want to change the base?

Show new errors from the MotionMount #137006

Conversation

RJPoelstra commented Jan 31, 2025

Proposed change

Type of change

Additional information

Checklist

andrewsayre left a comment

Choose a reason for hiding this comment

home-assistant bot commented Jan 31, 2025

RJPoelstra commented Jan 31, 2025 • edited Loading

andrewsayre commented Jan 31, 2025 • edited Loading

andrewsayre commented Jan 31, 2025

RJPoelstra commented Jan 31, 2025

andrewsayre commented Jan 31, 2025

RJPoelstra commented Jan 31, 2025

andrewsayre left a comment

Choose a reason for hiding this comment

andrewsayre Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

RJPoelstra Feb 4, 2025

Choose a reason for hiding this comment

andrewsayre left a comment

Choose a reason for hiding this comment

andrewsayre Feb 4, 2025

Choose a reason for hiding this comment

andrewsayre Feb 4, 2025

Choose a reason for hiding this comment

andrewsayre Feb 4, 2025

Choose a reason for hiding this comment

andrewsayre Feb 4, 2025

Choose a reason for hiding this comment

andrewsayre Feb 4, 2025

Choose a reason for hiding this comment

RJPoelstra commented Jan 31, 2025 •

edited

Loading

andrewsayre commented Jan 31, 2025 •

edited

Loading

andrewsayre Feb 3, 2025 •

edited

Loading