CHAD-14927: Driver fixes for lifecycle changes #2081
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aleclorimer
requested review from
greens,
varzac,
cjswedes,
ctowns,
hcarter-775 and
nickolas-deboom
|
|
Test Results 68 files 447 suites 0s ⏱️ For more details on these failures, see this check. Results for commit ec40623. ♻️ 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
zigbee-contact_coverage.xml
zigbee-humidity-sensor_coverage.xml
zigbee-motion-sensor_coverage.xml
zigbee-power-meter_coverage.xml
zigbee-smoke-detector_coverage.xml
zigbee-sound-sensor_coverage.xml
zigbee-switch_coverage.xml
zigbee-thermostat_coverage.xml
zigbee-water-leak-sensor_coverage.xml
zwave-switch_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against ec40623 |
cjswedes
reviewed
Apr 17, 2025
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?
| } | ||
|
|
||
| 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. |
ctowns
reviewed
Apr 18, 2025
| } | ||
| }, | ||
| { | ||
| 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
aleclorimer
force-pushed
the
CHAD-14927-device-driver-lifecycle-rework
branch
from
2eef231 to
ec40623
Compare
|
Revisiting this set of changes for the 0.58 release. These changes have transitioned over to a new PR. I will close this PR once that new PR gets merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.