CHAD-14284: Test zigbee health monitoring opt out with zigbee-motion and zigbee-contact drivers

[kiera-robinson-st]

Check all that apply

Type of Change

• [ ] WWST Certification Request
  • If this is your first time contributing code:
    • [x] I have reviewed the README.md file
    • [x] I have reviewed the CODE_OF_CONDUCT.md file
    • [x] I have signed the CLA
  • [ ] I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
• [ ] Bug fix
• [x] New feature
• [ ] Refactor

Checklist

• [x] I have performed a self-review of my code
• [ ] I have commented my code in hard-to-understand areas
• [ ] I have verified my changes by testing with a device or have communicated a plan for testing
• [ ] I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

Changed health check action for 2 drivers and their associated tests: zigbee-motion and zigbee-contact

Summary of Completed Tests

This is not needed to keep device online, and so its being removed to gain the small memory/latency savings of having it disabled. Overnight tests were done for both zigbee-motion and zigbee-contact drivers.

[github-actions[bot]]

Test Results

   67 files    444 suites   0s ⏱️ 2 260 tests 2 259 ✅ 0 💤 1 ❌ 3 842 runs  3 841 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 352a863b.

:recycle: This comment has been updated with latest results.

[github-actions[bot]]

zigbee-contact_coverage.xml

File	Coverage
All files	99%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-contact/src/init.lua	98%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-contact/src/multi-sensor/init.lua	97%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-contact/src/frient/init.lua	98%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-contact/src/smartsense-multi/init.lua	95%	:white_check_mark:

zigbee-motion-sensor_coverage.xml

File	Coverage
All files	98%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-contact/src/init.lua	98%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-contact/src/multi-sensor/init.lua	97%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-contact/src/frient/init.lua	98%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-contact/src/smartsense-multi/init.lua	95%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-motion-sensor/src/gatorsystem/init.lua	98%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-motion-sensor/src/thirdreality/init.lua	95%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-motion-sensor/src/aqara/aqara_utils.lua	93%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-motion-sensor/src/aqara/init.lua	94%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-motion-sensor/src/ikea/init.lua	97%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-motion-sensor/src/frient/init.lua	98%	:white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-motion-sensor/src/init.lua	98%	:white_check_mark:

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against 352a863be83b3d832ef0f2660d58f04462778676

[cjswedes]

Looks like there is a test for zigbee-motion that was actually testing the healthcheck functionality. Since we are removing the functionality, we can just remove the test. https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/blob/0ccdd939fc904f90798c370ed675b1803d880c84/drivers/SmartThings/zigbee-motion-sensor/src/test/test_all_capabilities_zigbee_motion.lua#L175

[cjswedes]

FYI, I think we should hold off merging this for a little bit, so that it ends up going to production after the new year. I will have more time to monitor the heavily used drivers then. cc: @varzac

[kiera-robinson-st2]

Merging is held off as per @varzac and @cjswedes call.

TS: Wednesday, December 11, 2024 at 2:40:48 PM ET

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[cjswedes]

@kiera-robinson-st2 @kiera-robinson-st We would like to merge this soon. In order for it to be merged the following needs to be done:

• Remove commented out test code that is no longer necessary
• Rebase this on top of the main branch to update to the latest and let CI validation occur.

[kiera-robinson-st2]

@kiera-robinson-st2 @kiera-robinson-st We would like to merge this soon. In order for it to be merged the following needs to be done:

• Remove commented out test code that is no longer necessary
• Rebase this on top of the main branch to update to the latest and let CI validation occur.

Hi @cjswedes

I am following up on your comment. In our email communication regarding this Pull Request, you had stated that there was another directive shared by Zach Varberg on December 11th 2024.

I am asking for clarification.

Was the above mentioned directive something to be done and is still missing from the other tests, or are the tests where I have commented out the line --zigbee_test_utils.init_noop_health_check_timer() removal due to change to stop health checks going forward the only instances where changes are to be made?

I just want to be sure that the PR is modified to meet the ask.

As for the frequent rebase to the target branch you shared in your email, what is the frequency one should rebase PRs with the target branch when the PR is on hold - for a long duration of time - from merging?

Your prior communication to me conflicted with an earlier directive - that code need only to be rebased at the time of merging unless instructed otherwise - so I want to be sure I am doing what is expected in the future.

Lastly, on this team, for a case like this where we are testing things and the merging will be paused for some long duration of time, is it preferred that the code is deleted and left in that state during the wait for future direction, or is commented out code also appropriate?

Thank you for any clarification.

TS: Monday, June 30, 2025 at 4:57:44 PM ET

[kiera-robinson-st2]

Hi @varzac

I have requested further information from @cjswedes on a directive you had stated on December 11th 2024 that @cjswedes had shared that I did not complete, that is "Since the health check disable is happening in the base driver, we will need to remove this test config from all of the tests in the test/ folder, not just this test. The same for the motion driver." https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/1816/files#r1880202335

I believe that that I modified the relevant instance and just need to delete them but it has become uncertain as to whether all instances that need to be removed are removed.

I have asked for clarity but have not been able to get responses to that nature. Given the expressed importance of these changes, can you confirm that ll instances that were commented out are the only instances of changes that need to be made?

This is besides deleting in place of commenting out the already marked lines in the PR in the state that it is at the time of writing, and allowing the branch to have no conflicts with main

At the moment, this last step presents a blocker, especially given the expressed significance of these changes.

I am hoping that some clarity on that question can be provided soon so that I can move forward with bring the PR to a mergeable state that is satisfactory.

TS: Tuesday, July 1, 2025 ET

[varzac]

Hi @varzac

I have requested further information from @cjswedes on a directive you had stated on December 11th 2024 that @cjswedes had shared that I did not complete, that is "Since the health check disable is happening in the base driver, we will need to remove this test config from all of the tests in the test/ folder, not just this test. The same for the motion driver." https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/1816/files#r1880202335

I believe that that I modified the relevant instance and just need to delete them but it has become uncertain as to whether all instances that need to be removed are removed.

I have asked for clarity but have not been able to get responses to that nature. Given the expressed importance of these changes, can you confirm that ll instances that were commented out are the only instances of changes that need to be made?

This is besides deleting in place of commenting out the already marked lines in the PR in the state that it is at the time of writing, and allowing the branch to have no conflicts with main

At the moment, this last step presents a blocker, especially given the expressed significance of these changes.

I am hoping that some clarity on that question can be provided soon so that I can move forward with bring the PR to a mergeable state that is satisfactory.

TS: Tuesday, July 1, 2025 ET

Yes, you will need to delete all the commented out lines, and resolve the merge conflicts. Looking at the history of the test folder I don't see any added tests since December, so the conflicts from the frient sensor test should be the only new factor causing conflicts/complications. We should be able to validate then that all tests pass. When searching here: https://github.com/search?q=repo%3ASmartThingsCommunity%2FSmartThingsEdgeDrivers+path%3A%2F%5Edrivers%5C%2FSmartThings%5C%2Fzigbee-motion-sensor%5C%2Fsrc%5C%2Ftest%5C%2F%2F+init_noop_health_check_timer&type=code it shows 18 files doing the noop timer in the test folder, so that should line up with the files edited in this PR. And there are 3 instances here: https://github.com/search?q=repo%3ASmartThingsCommunity%2FSmartThingsEdgeDrivers+path%3A%2F%5Edrivers%5C%2FSmartThings%5C%2Fzigbee-contact%5C%2Fsrc%5C%2Ftest%5C%2F%2F+init_noop_health_check_timer&type=code

[kiera-robinson-st2]

@varzac,

Communication from @greens indicates that this work was done as part of work in merged PR (LINK: https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/2231)

For this reason, I am closing this PR without merging.

TS: Monday, July 7, 2025 at 2:07:41 PM ET

[kiera-robinson-st2]

Code changes in this PR is included in work done by @greens. PR Link: https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/2231

[varzac]

@varzac,

Communication from @greens indicates that this work was done as part of work in merged PR (LINK: #2231)

For this reason, I am closing this PR without merging.

TS: Monday, July 7, 2025 at 2:07:41 PM ET

Understood. It looks like Chris' team chose to go with a broad application of the changes instead of individual device types. Closing is the right choice.