CHAD-14284: Test zigbee health monitoring opt out with zigbee-motion and zigbee-contact drivers #1816
Conversation
kiera-robinson-st
requested review from
cjswedes,
NoahCornell,
tandres and
varzac
Test Results 67 files 444 suites 0s ⏱️ For more details on these failures, see this check. Results for commit 352a863. ♻️ This comment has been updated with latest results. |
|
zigbee-contact_coverage.xml
zigbee-motion-sensor_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 352a863 |
|
Looks like there is a test for |
| @@ -31,7 +31,7 @@ local mock_device = test.mock_device.build_test_zigbee_device( | |||
| zigbee_test_utils.prepare_zigbee_env_info() | |||
| local function test_init() | |||
| test.mock_device.add_test_device(mock_device) | |||
| zigbee_test_utils.init_noop_health_check_timer() | |||
| --zigbee_test_utils.init_noop_health_check_timer() removal due to change to stop health checks going forward | |||
There was a problem hiding this comment.
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.
|
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 @kiera-robinson-st We would like to merge this soon. In order for it to be merged the following needs to be done:
|
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 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 |
|
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 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 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 |
…and zigbee-contact drivers
kiera-robinson-st2
force-pushed
the
CHAD-14284-health-monitoring-opt-out-with-zigbee-motion-and-zigbee-contact-drivers
branch
from
2c85ffe to
7f8de49
Compare
…gbee-motion-and-zigbee-contact-drivers # Conflicts: # drivers/SmartThings/zigbee-contact/src/init.lua # drivers/SmartThings/zigbee-contact/src/test/test_zigbee_contact.lua # drivers/SmartThings/zigbee-contact/src/test/test_zigbee_contact_battery.lua # drivers/SmartThings/zigbee-contact/src/test/test_zigbee_contact_tyco.lua # drivers/SmartThings/zigbee-motion-sensor/src/init.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_all_capabilities_zigbee_motion.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_aqara_high_precision.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_aqara_motion_illuminance.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_aurora_motion.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_battery_voltage_motion.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_centralite_motion.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_compacta_motion.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_frient_sensor.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_gator_motion.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_ikea_motion.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_samjin_sensor.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_smartthings_motion.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_thirdreality_sensor.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_zigbee_motion_iris.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_zigbee_motion_nyce.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_zigbee_motion_orvibo.lua # drivers/SmartThings/zigbee-motion-sensor/src/test/test_zigbee_plugin_motion_sensor.lua
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. |
Check all that apply
Type of Change
Checklist
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.