Add support for ZHA door locks #2

[presslab-us]

Description:

Adds support for ZHA door locks. Supports lock and unlock.

This is my second try, maybe it will work this time. Last try is here: #24126

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9522

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist

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

• Documentation added/updated in home-assistant.io

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

• The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

If the code does not interact with devices:

• Tests have been added to verify that the new code works.

[ghost]

Hey there @dmulcahey, @Adminiuga, mind taking a look at this pull request as its been labeled with a integration (zha) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

[Adminiuga]

Looks good on my side.
@dmulcahey would you want 6ac908b as a separte PR?

[presslab-us]

@Adminiuga thanks for the review. It looks like zigpy `t.Optional' has broken the tests. How do you recommend I fix that? I mean I guess it should have still worked because it's "optional" but the type is different so the test fails.

[Adminiuga]

>           assert cluster.request.call_args == call(
                False, LOCK_DOOR, (), expect_reply=True, manufacturer=None)
E           AssertionError: assert call(False, 0...facturer=None) == call(False, 0,...facturer=None)
E           AssertionError: assert call(False, 0...facturer=None) == call(False, 0,...facturer=None)
E             At index 0 diff: (False, 0, (<class 'zigpy.types.basic.Optional.<locals>.Optional'>,)) != ''
E             Right contains one more item: {'expect_reply': True, 'manufacturer': None}
E             Full diff:
E             - call(False, 0, (<class 'zigpy.types.basic.Optional.<locals>.Optional'>,), expect_reply=True, manufacturer=None)
E             + call(False, 0, (), expect_reply=True, manufacturer=None)

third param of a Cluster.request() method is schema. I think the test broke once the zigpy DoorLock cluster schema was updated for that command, regardless of schema content the assertion would fail, since schema for the command is not empty anymore.

I'm personally (cause that how I learnt it from bellows) in favor in checking just the required args, thus no need to update the test in the underlying API changes.
eg make it into two assertions, and check command_id and command_args (if any), lile:

assert cluster.request.call_args[0][0] is False  # Cluster specific command
assert cluster.request.call_args[0][1] == LOCK_DOOR  # Cluster command id

and since no args were given to command, no *args to check. HA tests should not be concerned that request() method gets the right schema or any params internal to zigpy.

Also, specifically for this case IMO it is better to mock the command() method of the cluster class and check call_args for this method, to keep tests from breaking if zigpy api changes

[presslab-us]

I'm personally (cause that how I learnt it from bellows) in favor in checking just the required args, thus no need to update the test in the underlying API changes.

Great, thanks for your help. Yes this is working fine. If you are okay with just doing that I will commit these changes.

Also, specifically for this case IMO it is better to mock the command() method of the cluster class and check call_args for this method, to keep tests from breaking if zigpy api changes

This seems like a bigger change to how the ZHA tests are done, probably should be a different pull request at least?

[presslab-us]

While I do appreciate the tests, they certainly make for better code. But man they take hours to run and are so cumbersome to use. And while I did figure out how to run a limited test it seems that it doesn't really catch things. Is it even possible to run codecov locally?

[dmulcahey]

@Adminiuga I am ok with it in 1 PR... I just want to poke through things once to make sure there aren't other wrapped command calls that need to be updated. I'll try to get to that today. @presslab-us Thanks a bunch for the contribution. This is going to make a lot of folks happy!

[presslab-us]

Thanks @dmulcahey. This codecov thing is killing me, I guess the other tests were grandfathered in.

[dmulcahey]

@presslab-us Don't sweat the patch failure. We got permission to ignore it.

[presslab-us]

Thanks, guys!

[Hedda]

@presslab-us Does the zha component description/documentation in zha.markdown need updating?

https://www.home-assistant.io/components/zha/

https://github.com/home-assistant/home-assistant.io/blob/current/source/_components/zha.markdown

Compare to zwave component description/documentation:

https://www.home-assistant.io/components/zwave/

[presslab-us]

@Hedda Something needed other than this pull request? home-assistant/home-assistant.io#9522