CHAD-14927: Driver fixes for lifecycle changes #2081
Conversation
aleclorimer
requested review from
greens,
varzac,
cjswedes,
ctowns,
hcarter-775 and
nickolas-deboom
|
|
Test Results 67 files 426 suites 0s ⏱️ For more details on these failures, see this check. Results for commit 2eef231. ♻️ This comment has been updated with latest results. |
|
matter-button_coverage.xml
matter-lock_coverage.xml
matter-sensor_coverage.xml
matter-switch_coverage.xml
matter-thermostat_coverage.xml
matter-window-covering_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 2eef231 |
There was a problem hiding this comment.
Can you add to the description of the PR when you expect these changes to land, so external developers dont think this is something live. Also can you add a test with test.enable_startup_messages(true) so there is an example in the repo of what that looks like?
| @@ -74,6 +74,7 @@ local CLUSTER_SUBSCRIBE_LIST ={ | |||
| } | |||
|
|
|||
| local function test_init() | |||
| test.enable_startup_messages(false) | |||
There was a problem hiding this comment.
Yes. I would expect these changes to land along with the 0.58 release and the lua-libs update after that.
Should I change it to test.disable_startup_messages() and make a test.enable_startup_messages() even though that one shouldn't be necessary?
There was a problem hiding this comment.
Having it be enabled by default and then including a test.disable_startup_messages() would be my preferred approach.
|
With the default RPC being set to 9999 can the RPC version still be overridden within test files? Some test cases rely on it to test certain functionality |
Yes. It should now work better than before as previously the RPC version could get set to a different value based on the order of initialization of the files within the script. |
|
Is this only expected to affect matter drivers? How come? |
It affects all drivers, though matter driver tests were the only tests that needed adjustments to the changes. |
| @@ -219,6 +219,11 @@ test.register_message_test( | |||
| clusters.Thermostat.server.attributes.OccupiedHeatingSetpoint:build_test_report_data(mock_device, 1, 40*100) | |||
| } | |||
| }, | |||
| { | |||
| channel = "capability", | |||
There was a problem hiding this comment.
Just for clarity - is this (among others) just included now that the RPC is defaulting to 9999, so these RPC dependent changes are now included? @nickolas-deboom
|
What is the reason only these specific matter tests needed the update? Why wouldn't all tests now need to have the |
A number of the matter tests are following a pattern that is not used by other tests. Specifically setting up test init functions that send |
|
Invitation URL: |
…deprecated state with set RPC version
Check all that apply
Type of Change
Checklist
Description of Change
Because the underlying behavior of drivers is changing and
initmessages are not synthetically generated from within the driver, some tests needed thisinitmessage to come from theDeviceLifecycleChannelin order to have the driver proceed properly.Secondly, now that drivers have a strict startup process with requiring to wait for an
initmessage for each device within the driver, some tests needed to toggle off this functionality withtest.enable_startup_messages(false).Lastly, the internal RPC version for all tests has been set to
9999to ensure that we are always testing the latest features based on RPC version. These changes fix a few tests where a change in driver behavior from this versioning caused it to fail.