SmartThingsCommunity / SmartThingsEdgeDrivers

Apache License 2.0
263 stars 452 forks source link

Revert dynamic constraint updates for matter-thermostat and matter-sensor #1455

Closed nickolas-deboom closed 2 months ago

nickolas-deboom commented 3 months ago

Support for temperature conversion for the temperatureRange, coolingSetpointRange, and heatingSetpointRange capabilites was added by this PR . This change introduced a bug that causes an error when temperature conversion is attempted for these capabilites. See the Temperature Conversion Issue for more background information. Also see section Reverting Dynamic Constraints Changes for information about testing this change.

This PR will revert the dynamic constraint updates originally made in matter-thermostat (PR 1223) and matter-sensor (PR 1224) until the bug is fixed in the next firmware release. After that point, a new PR will be created to restore these changes.

Jira ticket: CHAD-13444

github-actions[bot] commented 3 months ago

Channel deleted.

github-actions[bot] commented 3 months ago

Test Results

   60 files    375 suites   0s :stopwatch: 1 821 tests 1 821 :white_check_mark: 0 :zzz: 0 :x: 3 169 runs  3 169 :white_check_mark: 0 :zzz: 0 :x:

Results for commit df4e941b.

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

github-actions[bot] commented 3 months ago

matter-sensor_coverage.xml

File Coverage
All files 86% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-sensor/src/air-quality-sensor/init.lua 91% :white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-sensor/src/init.lua 91% :white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-sensor/src/smoke-co-alarm/init.lua 83% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-sensor/src/embedded-cluster-utils.lua 42% :x:

matter-thermostat_coverage.xml

File Coverage
All files 80% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-sensor/src/air-quality-sensor/init.lua 91% :white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-sensor/src/init.lua 91% :white_check_mark:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-sensor/src/smoke-co-alarm/init.lua 83% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-sensor/src/embedded-cluster-utils.lua 42% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/embedded-cluster-utils.lua 42% :x:
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-thermostat/src/init.lua 82% :x:

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against df4e941b3acba85512db4e1c50ecff6693f66f32

github-actions[bot] commented 2 months ago

Duplicate profile check: Passed - no duplicate profiles detected.

hcarter-775 commented 2 months ago

yeah this looks good to me as well! I like @ctowns idea to comment as well, but conditionally approved :+1:

nickolas-deboom commented 2 months ago

This looks pretty good to me! Quick question - was this a pure revert of the two original commits? If it was it might be a good idea to just document the commit SHAs that were reverted in the PR comment or in one of the commit messages.

Also, remember to squash your commits!

It was a revert of the two PRs (one commit for matter-sensor and several for matter-thermostat after not squashing those commits...) plus some additional changes were needed in the test cases from changes that took place after the original two PRs, in order to make sure we're not subscribing to the min/max setpoint and temperature range attributes in test cases developed since that time. Do you think I should still list the commits? Maybe listing the two PRs might be best since it would be a list of about 17 commits?

ctowns commented 2 months ago

This looks pretty good to me! Quick question - was this a pure revert of the two original commits? If it was it might be a good idea to just document the commit SHAs that were reverted in the PR comment or in one of the commit messages. Also, remember to squash your commits!

It was a revert of the two PRs (one commit for matter-sensor and several for matter-thermostat after not squashing those commits...) plus some additional changes were needed in the test cases from changes that took place after the original two PRs, in order to make sure we're not subscribing to the min/max setpoint and temperature range attributes in test cases developed since that time. Do you think I should still list the commits? Maybe listing the two PRs might be best since it would be a list of about 17 commits?

Oh yeah no worries, if it's 17 commits I wouldn't worry about listing them all out!

Just remember to squash the commits on this PR - you can do it in the GitHub GUI or in your local repo and just force push the squashed commits. The GitHub GUI makes it pretty straightforward though :)