Add an improve_contrast switch #230

hawkeye217 · 2022-03-25T21:50:46Z

To be paired with the PR I submitted for frigate for toggling improve_contrast via mqtt.

I'm not sure if adding even more entities is something you want for the HA integration, but it was an easy addition so I figured I'd submit this PR for your consideration. I'm happy to keep some manually added mqtt switch entities in HA if you don't want to add this to the official integration.

Thanks again for all the amazing work on Frigate!

NickM-27 · 2022-03-25T21:53:27Z

I don't speak for the others, but personally I think spamming entities is bad, but this is certainly one that many would find useful. Also in general, entities can be disabled if not used so not a huge issue. We are also looking to add a generic motion sensor soon for actual motion detection as that has been requested often.

codecov · 2022-03-25T22:16:53Z

Codecov Report

Merging #230 (03cf64b) into master (68882b4) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #230   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         1318      1322    +4     
=========================================
+ Hits          1318      1322    +4

Impacted Files	Coverage Δ
custom_components/frigate/const.py	`100.00% <100.00%> (ø)`
custom_components/frigate/switch.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68882b4...03cf64b. Read the comment docs.

dermotduffy · 2022-03-26T22:30:17Z

Similar to @NickM-27 suggests: I would suggest we add this but have it disabled by default. Users who want it, can enable it and get the extra functionality. To do this, I'd add another variable to the FrigateSwitch constructor (default_enabled) and set _attr_entity_registry_enabled_default to that value in the constructor. That should automatically disable the switch unless the user explicit enables it.

Please also add a test that verifies the disable-by-default works, you can follow the example @NickM-27 set in this test.

hawkeye217 · 2022-03-26T23:22:02Z

Thanks for the feedback, it's a great idea. I have a lot of python experience but no experience with HA integrations - but I'll do what I can. I may need some questions answered!

hawkeye217 · 2022-03-26T23:52:06Z

@dermotduffy @NickM-27 Updated the constructor and added some tests... Since I have little idea what I am doing, please help if it's way off :)

hawkeye217 · 2022-03-26T23:57:25Z

@dermotduffy @NickM-27 I added the new improve_contrast switch to the SWITCH_ENTITY_IDS in test_switch.py. Not sure if that should have been added there if it's disabled by default...

NickM-27 · 2022-03-27T00:03:44Z

Could maybe adjust it to be assert switch or (not _attr_entity_registry_enabled_default) but not 100% if that is a good solution for the test

dermotduffy · 2022-03-27T15:57:47Z

I think maybe break apart SWITCH_ENTITY_IDS into enabled and disabled sets. I do something like that in the motionEye tests.

dermotduffy · 2022-03-27T15:59:19Z

PS: Make sure to run the pre-commit locally, it'll make sure you find the formatting/typing issues without needing to wait for the Github build.

hawkeye217 · 2022-03-27T16:31:15Z

Appreciate the help and advice @dermotduffy. Let me know how this looks now.

dermotduffy · 2022-03-27T18:18:55Z

tests/test_switch.py

@@ -154,3 +164,23 @@ async def test_switch_unique_id(hass: HomeAssistant) -> None:
    assert (
        registry_entry.unique_id == f"{TEST_CONFIG_ENTRY_ID}:switch:front_door_detect"
    )
+
+
+async def test_switch_improve_contrast_can_be_enabled(hass: HomeAssistant) -> None:


Even better would be to use @pytest.mark.parametrize here so that this test gets run for all members of DISABLED_SWITCH_ENTITY_IDS (so that when we add more switches there this test will "just work").

Take a look at the sensor test for inspiration: https://github.com/blakeblackshear/frigate-hass-integration/blob/master/tests/test_sensor.py#L92

Great idea. So, something like this? Not sure if I've got the syntax right.

@pytest.mark.parametrize("disabled_switch_name", DISABLED_SWITCH_ENTITY_IDS) async def test_disabled_switch_can_be_enabled( disabled_switch_name: Any, hass: HomeAssistant ) -> None: """Verify disabled switches can be enabled.""" await setup_mock_frigate_config_entry(hass) entity_registry = er.async_get(hass) # Test original entity is disabled as expected entry = entity_registry.async_get(disabled_switch_name) assert entry assert entry.disabled assert entry.disabled_by == er.DISABLED_INTEGRATION entity_state = hass.states.get(disabled_switch_name) assert not entity_state # Update and test that entity is now enabled updated_entry = entity_registry.async_update_entity( disabled_switch_name, disabled_by=None ) assert not updated_entry.disabled

Really appreciate your patience as I learn this!

Yeah, exactly.

Minor:

I'd call disabled_switch_name as disabled_entity_id instead (slightly clearer)

I think you should be able to type annotate it with str instead of Any

dermotduffy · 2022-03-27T18:20:19Z

Looking great @hawkeye217 ! One improvement suggestion added above that could make this even better I think. Also need to fix the tests/formatting (see failed CI build). Thanks!

hawkeye217 · 2022-03-27T18:38:41Z

Looking great @hawkeye217 ! One improvement suggestion added above that could make this even better I think. Also need to fix the tests/formatting (see failed CI build). Thanks!

% pre-commit run
pyupgrade................................................................Passed
black....................................................................Passed
codespell................................................................Passed
flake8...................................................................Passed
isort....................................................................Passed
Check JSON...........................................(no files to check)Skipped
yamllint.............................................(no files to check)Skipped
pylint...................................................................Passed
mypy.....................................................................Passed

Sorry about all the failed tests. I've run pre-commit and it passes, but Is there a way I can run all of them locally, easily?

dermotduffy · 2022-03-27T18:44:04Z

Sorry about all the failed tests. I've run pre-commit and it passes, but Is there a way I can run all of them locally, easily?

(~/src/frigate-hass-integration) $ pre-commit run -a
pyupgrade................................................................Passed
black....................................................................Passed
codespell................................................................Passed
flake8...................................................................Passed
isort....................................................................Passed
Check JSON...............................................................Passed
yamllint.................................................................Passed
pylint...................................................................Passed
mypy.....................................................................Passed

(~/src/frigate-hass-integration) $ pytest
Test session starts (platform: linux, Python 3.8.10, pytest 6.2.5, pytest-sugar 0.9.4)
rootdir: /home/src/frigate-hass-integration, configfile: pyproject.toml, testpaths: tests
plugins: freezegun-0.4.2, homeassistant-custom-component-0.5.0, aiohttp-0.3.0, respx-0.19.0, cov-2.12.1, test-groups-1.0.3, forked-1.4.0, sugar-0.9.4, requests-mock-1.9.2, socket-0.4.1, xdist-2.4.0, anyio-3.5.0, timeout-2.0.1
timeout: 10.0s
timeout method: signal
timeout func_only: False
collecting ... 
 tests/test_api.py ✓✓✓✓✓✓✓✓                                                                                                7% ▊         
 tests/test_binary_sensor.py ✓✓✓✓✓✓✓                                                                                      14% █▍        
 tests/test_camera.py ✓✓✓✓✓✓✓✓                                                                                            21% ██▎       
 tests/test_config_flow.py ✓✓✓✓✓✓✓                                                                                        28% ██▊       
 tests/test_init.py ✓✓✓✓✓✓✓✓✓                                                                                             36% ███▋      
 tests/test_media_source.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                           53% █████▍    
 tests/test_sensor.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                                72% ███████▎  
 tests/test_switch.py ✓✓✓✓✓✓                                                                                              77% ███████▊  
 tests/test_views.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                           100% ██████████

---------- coverage: platform linux, python 3.8.10-final-0 -----------
Name                                         Stmts   Miss  Cover   Missing
--------------------------------------------------------------------------
custom_components/frigate/__init__.py          178      0   100%
custom_components/frigate/api.py                57      0   100%
custom_components/frigate/binary_sensor.py      46      0   100%
custom_components/frigate/camera.py             84      0   100%
custom_components/frigate/config_flow.py        54      0   100%
custom_components/frigate/const.py              33      0   100%
custom_components/frigate/media_source.py      441      0   100%
custom_components/frigate/sensor.py            165      0   100%
custom_components/frigate/switch.py             55      0   100%
custom_components/frigate/views.py             205      0   100%
--------------------------------------------------------------------------
TOTAL                                         1318      0   100%
Coverage XML written to file coverage.xml

Required test coverage of 100% reached. Total coverage: 100.00%

Results (15.50s):
     109 passed

hawkeye217 · 2022-03-27T18:52:47Z

Great, thanks! I must be missing some dependencies because I'm getting errors on files I didn't modify.

Let me push what I have and I'll work through that issue separately.

dermotduffy · 2022-03-27T18:55:30Z

Yeah, that may be a sign your development environment isn't quite right. I'd recommend using a dedicated or throwaway Python virtual environment, and making sure you have all the dependencies installed:

(~/src/frigate-hass-integration) $ pip install -r requirements_dev.txt

Getting it to pass cleanly prior to your changes is a good sign you have everything setup right.

hawkeye217 · 2022-03-27T21:07:40Z

@dermotduffy Got the dev environment set up right and I can finally see what test was failing. Thanks for the help.

The test_switch_icon is failing for the new improve_contrast switch. How would you address that one? Should the icon be tested for a disabled switch?

dermotduffy · 2022-03-27T21:48:40Z

@hawkeye217 Yes, I think so, the point of that test is to verify switch icons -- so whether enabled/disable we should probably do that test. For disabled switches, enable them first ... then check the icon? If you want to test the icons as part of some other test (and remove the dedicated test for it) that'd be fine also, whatever works out cleanest in the test code.

hawkeye217 · 2022-03-27T22:45:36Z

@hawkeye217 Yes, I think so, the point of that test is to verify switch icons -- so whether enabled/disable we should probably do that test. For disabled switches, enable them first ... then check the icon? If you want to test the icons as part of some other test (and remove the dedicated test for it) that'd be fine also, whatever works out cleanest in the test code.

A separate test to check disabled switch icons is probably the simplest/cleanest since the default-enabled ones are already hard coded in the test. But that does mean that the enabling test code is redundant.

And here's where my unfamiliarity with HA integrations comes in - is there something else I need to call to enable an entity? The last assertion is failing:

async def test_disabled_switch_icon(hass: HomeAssistant) -> None:
    """Verify icons for disabled switches by enabling them."""
    await setup_mock_frigate_config_entry(hass)
    entity_registry = er.async_get(hass)

    expected_results = {
        TEST_SWITCH_FRONT_DOOR_IMPROVE_CONTRAST_ENTITY_ID: "mdi:contrast-circle",
    }

    for disabled_entity_id, icon in expected_results.items():
        updated_entry = entity_registry.async_update_entity(
            disabled_entity_id, disabled_by=None
        )
        assert not updated_entry.disabled
        entity_state = hass.states.get(disabled_entity_id)
        assert entity_state
        assert entity_state.attributes["icon"] == icon

% pytest tests/test_switch.py::test_disabled_switch_icon   

Test session starts (platform: darwin, Python 3.9.10, pytest 6.2.5, pytest-sugar 0.9.4)
rootdir: /Users/jhawkins/Desktop/Life/Github contributions/Frigate HA integration/frigate-hass-integration.venv, configfile: pyproject.toml
plugins: cov-2.12.1, respx-0.19.0, sugar-0.9.4, freezegun-0.4.2, anyio-3.5.0, forked-1.4.0, requests-mock-1.9.2, homeassistant-custom-component-0.5.0, socket-0.4.1, xdist-2.4.0, timeout-2.0.1, aiohttp-0.3.0, test-groups-1.0.3
timeout: 10.0s
timeout method: signal
timeout func_only: False
collecting ... 

――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― test_disabled_switch_icon[pyloop] ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

hass = <homeassistant.core.HomeAssistant object at 0x109232550>

    async def test_disabled_switch_icon(hass: HomeAssistant) -> None:
        """Verify icons for disabled switches by enabling them."""
        await setup_mock_frigate_config_entry(hass)
        entity_registry = er.async_get(hass)
    
        expected_results = {
            TEST_SWITCH_FRONT_DOOR_IMPROVE_CONTRAST_ENTITY_ID: "mdi:contrast-circle",
        }
    
        for disabled_entity_id, icon in expected_results.items():
            updated_entry = entity_registry.async_update_entity(
                disabled_entity_id, disabled_by=None
            )
            assert not updated_entry.disabled
            entity_state = hass.states.get(disabled_entity_id)
>           assert entity_state
E           assert None

tests/test_switch.py:206: AssertionError

Would you do this differently?

dermotduffy · 2022-03-27T22:49:22Z

No, that's roughly what I would have done. How is it failing? If I had to guess, I'd say the state machine may not yet have processed the re-enable, so maybe something like this is necessary between calling async_update_entity and getting the state from hass.states

# async_update_entity
await hass.async_block_till_done()
# Now check hass.states

hawkeye217 · 2022-03-27T22:53:42Z

Makes sense, I figured it could have been that. Unfortunately putting an await hass.async_block_till_done() after the async_update_entity doesn't seem to work either :(

dermotduffy · 2022-03-27T22:54:46Z

After the re-enable & async_block_till_done, what is the contents of the entity_state variable?

hawkeye217 · 2022-03-27T22:58:19Z

It's None. Weird...

I do see this from HA, however:

DEBUG:homeassistant.core:Bus:Handling <Event entity_registry_updated[L]: action=update, entity_id=switch.front_door_improve_contrast, changes=disabled_by=integration>

dermotduffy · 2022-03-27T23:03:59Z

I knew this problem seemed familiar, I ran into this with the Hyperion integration, this was my fix.

hawkeye217 · 2022-03-27T23:17:52Z

Legend. That's awesome.

I see tests.common is defined in HA core, are there any particular changes I need to be able to import it in this test environment?

from tests.common import async_fire_time_changed E ModuleNotFoundError: No module named 'tests.common'

dermotduffy · 2022-03-27T23:21:02Z

Yeah, this is a weird one. The HA Core tests functionality is bundled separately for use in custom integrations like this, it's bundled into this package. We already use this package in these tests. Try:

from pytest_homeassistant_custom_component.common import async_fire_time_changed

The test_sensor.py file already does similar, you can take a peep at that.

hawkeye217 · 2022-03-27T23:29:29Z

Hmm. Still no-go with the icon, the final assertion fails. Any other ideas?

    for disabled_entity_id, icon in expected_results.items():
        updated_entry = entity_registry.async_update_entity(
            disabled_entity_id, disabled_by=None
        )
        assert not updated_entry.disabled
        await hass.async_block_till_done()

        async_fire_time_changed(hass, dt_util.utcnow() + SCAN_INTERVAL)

        await hass.async_block_till_done()

        entity_state = hass.states.get(disabled_entity_id)
        assert entity_state
        assert entity_state.attributes["icon"] == icon

dermotduffy · 2022-03-27T23:31:08Z

As in the Hyperion example, you specifically need to "fast-forward" time by RELOAD_AFTER_UPDATE_DELAY seconds:

            async_fire_time_changed(
                hass,
                dt.utcnow() + timedelta(seconds=RELOAD_AFTER_UPDATE_DELAY + 1),
            )

SCAN_INTERVAL may be different.

hawkeye217 · 2022-03-27T23:35:40Z

Yeah, I tried that first but got this:

DEBUG:homeassistant.core:Bus:Handling <Event state_changed[L]: entity_id=binary_sensor.steps_person_motion, old_state=<state binary_sensor.steps_person_motion=unavailable; device_class=motion, friendly_name=Steps Person Motion @ 2022-03-27T23:33:22.793204+00:00>, new_state=<state binary_sensor.steps_person_motion=unavailable; restored=True, device_class=motion, friendly_name=Steps Person Motion, supported_features=0 @ 2022-03-27T23:33:22.793204+00:00>>
ERROR:custom_components.frigate:Unexpected error fetching frigate data: A test tried to use socket.socket.connect() with host "93.184.216.34" (allowed: "127.0.0.1").
Traceback (most recent call last):
  ...
pytest_socket.SocketConnectBlockedError: A test tried to use socket.socket.connect() with host "93.184.216.34" (allowed: "127.0.0.1").
DEBUG:custom_components.frigate:Finished fetching frigate data in 0.006 seconds (success: False)
WARNING:homeassistant.config_entries:Config entry 'http://example.com' for frigate integration not ready yet: A test tried to use socket.socket.connect() with host "93.184.216.34" (allowed: "127.0.0.1").; Retrying in background

So I looked at the similar calls in test_sensor.py and it used SCAN_INTERVAL there.

dermotduffy · 2022-03-27T23:47:57Z

As a silver lining, aren't you getting a nice tour of HA integration testing? ;-)

Somehow your test is trying to talk to the outside world. Would you be able to commit whatever code you have, and I'll take a look?

hawkeye217 · 2022-03-27T23:51:43Z

Haha, yes! This is actually really helpful (and fun) and you've been super patient with a HA integration newbie. Thank you!

I'll commit what I have so you can take a look.

dermotduffy · 2022-03-28T00:06:23Z

The data coordinator (the code that runs every X seconds to poll Frigate stats) was trying to reload its data during the time you were 'fast-forwarding time'. Since the API patch wasn't in place during this fetch, it was trying to talk to the Frigate server at example.com which obviously isn't a real Frigate server. The fix is to keep the patch firmly in place during this whole thing, so that any attempts to create a new Frigate API instance will always use the mock version.

This test passes for me:

async def test_disabled_switch_icon(hass: HomeAssistant) -> None:
    """Verify icons for disabled switches by enabling them."""
    client = create_mock_frigate_client()
    await setup_mock_frigate_config_entry(hass, client=client)

    entity_registry = er.async_get(hass)
    expected_results = {
        TEST_SWITCH_FRONT_DOOR_IMPROVE_CONTRAST_ENTITY_ID: "mdi:contrast-circle",
    }

    # Keep the patch in place to ensure that coordinator updates that are
    # scheduled during the reload period will use the mocked API.
    with patch(
        "custom_components.frigate.FrigateApiClient",
        return_value=client,
    ):
        for disabled_entity_id, icon in expected_results.items():
            updated_entry = entity_registry.async_update_entity(
                disabled_entity_id, disabled_by=None
            )
            assert not updated_entry.disabled
            await hass.async_block_till_done()

            async_fire_time_changed(hass, dt_util.utcnow() + timedelta(seconds=RELOAD_AFTER_UPDATE_DELAY + 1))
            # async_fire_time_changed(hass, dt_util.utcnow() + SCAN INTERVAL)

            await hass.async_block_till_done()

            entity_state = hass.states.get(disabled_entity_id)
            assert entity_state
            assert entity_state.attributes["icon"] == icon

hawkeye217 · 2022-03-28T00:19:08Z

Awesome! It passes now for me as well, and what you said makes sense. Thank you!

dermotduffy · 2022-03-28T00:38:50Z

Great! Think we're good to go here. I'll merge it in after the parent PR has been merged (blakeblackshear/frigate#3011).

PS: Thank you for the contribution, using your new experience and finding more opportunities to contribute would be really appreciated. Bugs that need help.

hawkeye217 · 2022-03-28T00:43:44Z

I'll certainly look for more opportunities. I've learned a lot from you here and it's made the contribution process seem much less intimidating. Frigate is an amazing piece of software (and so is HA!) and I'd love help to improve it wherever I can.

Thanks again!

NickM-27 · 2022-04-21T01:04:44Z

@dermotduffy also just a heads up the related frigate PR was merged so this can be merged as well 👍

hawkeye217 added 2 commits March 25, 2022 16:46

Add improve_contrast switch

bbacc8c

Add switch to README

5d56d7e

dermotduffy added the pending label Mar 26, 2022

dermotduffy added the enhancement New feature or request label Mar 26, 2022

Disable by default and add tests

97231e6

Add newline at eof

81aaff6

NickM-27 approved these changes Mar 26, 2022

View reviewed changes

Add test for disabled improve_contrast switch

71b436b

dermotduffy reviewed Mar 27, 2022

View reviewed changes

Parametrize disabled switch test

382c63d

contrast icon

0d31de3

Non-working disabled switch icon test

612cb90

Fix test comment

a761fbf

Keep patch in place

33f7d36

hawkeye217 added 2 commits March 27, 2022 19:23

Forgot to pre-commit

b549cee

Forgot to pre-commit

03cf64b

dermotduffy removed the pending label Apr 21, 2022

dermotduffy changed the title ~~Add improve_contrast switch~~ Add an improve_contrast switch Apr 21, 2022

dermotduffy merged commit 4ec0eb3 into blakeblackshear:master Apr 21, 2022

hawkeye217 deleted the improve-contrast branch May 2, 2022 02:49

Add an improve_contrast switch #230

Add an improve_contrast switch #230

Conversation

hawkeye217 commented Mar 25, 2022

NickM-27 commented Mar 25, 2022 • edited Loading

codecov bot commented Mar 25, 2022 • edited Loading

Codecov Report

dermotduffy commented Mar 26, 2022

hawkeye217 commented Mar 26, 2022

hawkeye217 commented Mar 26, 2022

hawkeye217 commented Mar 26, 2022

NickM-27 commented Mar 27, 2022

dermotduffy commented Mar 27, 2022

dermotduffy commented Mar 27, 2022

hawkeye217 commented Mar 27, 2022

dermotduffy Mar 27, 2022

Choose a reason for hiding this comment

hawkeye217 Mar 27, 2022

Choose a reason for hiding this comment

dermotduffy Mar 27, 2022 • edited Loading

Choose a reason for hiding this comment

dermotduffy commented Mar 27, 2022

hawkeye217 commented Mar 27, 2022

dermotduffy commented Mar 27, 2022 • edited Loading

hawkeye217 commented Mar 27, 2022

dermotduffy commented Mar 27, 2022

hawkeye217 commented Mar 27, 2022

dermotduffy commented Mar 27, 2022

hawkeye217 commented Mar 27, 2022 • edited Loading

dermotduffy commented Mar 27, 2022

hawkeye217 commented Mar 27, 2022 • edited Loading

dermotduffy commented Mar 27, 2022 • edited Loading

hawkeye217 commented Mar 27, 2022 • edited Loading

dermotduffy commented Mar 27, 2022

hawkeye217 commented Mar 27, 2022

dermotduffy commented Mar 27, 2022 • edited Loading

hawkeye217 commented Mar 27, 2022

dermotduffy commented Mar 27, 2022 • edited Loading

hawkeye217 commented Mar 27, 2022 • edited Loading

dermotduffy commented Mar 27, 2022

hawkeye217 commented Mar 27, 2022

dermotduffy commented Mar 28, 2022

hawkeye217 commented Mar 28, 2022

dermotduffy commented Mar 28, 2022

hawkeye217 commented Mar 28, 2022

NickM-27 commented Apr 21, 2022

NickM-27 commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading

dermotduffy Mar 27, 2022 •

edited

Loading

dermotduffy commented Mar 27, 2022 •

edited

Loading

hawkeye217 commented Mar 27, 2022 •

edited

Loading

hawkeye217 commented Mar 27, 2022 •

edited

Loading

dermotduffy commented Mar 27, 2022 •

edited

Loading

hawkeye217 commented Mar 27, 2022 •

edited

Loading

dermotduffy commented Mar 27, 2022 •

edited

Loading

dermotduffy commented Mar 27, 2022 •

edited

Loading

hawkeye217 commented Mar 27, 2022 •

edited

Loading