Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Unlatch command and Unlatched status (Unbolt feature) #1756

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add Unlatch command and Unlatched status (Unbolt feature) #1756

wants to merge 4 commits into from
+887 −83

Conversation

HunsupJung
Copy link
Collaborator

@HunsupJung HunsupJung commented Nov 15, 2024 •
edited
Loading

Check all that apply

Type of Change

  • WWST Certification Request
    • If this is your first time contributing code:
      • I have reviewed the README.md file
      • I have reviewed the CODE_OF_CONDUCT.md file
      • 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
  • New feature
  • Refactor

Checklist

  • 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

To support the Unbolt feature
https://smartthings.atlassian.net/browse/CPBLTS-2481

Summary of Completed Tests

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 15, 2024 •
edited
Loading

Duplicate profile check: Passed - no duplicate profiles detected.

@github-actions GitHub Actions
Copy link

Invitation URL:
https://bestow-regional.api.smartthings.com/invite/Boj0AwxQP9MA

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 15, 2024 •
edited
Loading

Test Results

   64 files  ± 0    403 suites  +1   0s ⏱️ ±0s
2 002 tests + 2  2 002 ✅ + 2  0 💤 ±0  0 ❌ ±0 
3 462 runs  +10  3 462 ✅ +10  0 💤 ±0  0 ❌ ±0 

Results for commit 31a85c3. ± Comparison against base commit 77176f5.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 15, 2024 •
edited
Loading

File Coverage
All files 91% ✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-lock/src/init.lua 91% ✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-lock/src/lock_utils.lua 98% ✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-lock/src/new-matter-lock/init.lua 90% ✅

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 31a85c3

@HunsupJung HunsupJung force-pushed the feature/matter-lock-for-unlatch branch from 713486b to e2d8f58 Compare November 26, 2024 05:24
@HunsupJung HunsupJung force-pushed the feature/matter-lock-for-unlatch branch from e2d8f58 to 17ee108 Compare November 26, 2024 06:08
tpmanley
tpmanley reviewed Nov 26, 2024
View reviewed changes
drivers/SmartThings/matter-lock/profiles/lock-unlatch.yml Outdated
value: '{{i18n.attributes.lock.i18n.value.unlocked.label}}'
- key: unlatched
value: '{{i18n.attributes.lock.i18n.value.unlatched.label}}'
- key: unknown
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to include unknown or unlocked with timeout since these are not included in enabledValues

tpmanley
tpmanley reviewed Nov 26, 2024
View reviewed changes
drivers/SmartThings/matter-lock/src/test/test_new_matter_lock.lua Outdated
@@ -755,7 +758,7 @@ test.register_message_test(
direction = "send",
message = mock_device:generate_test_message(
"main",
capabilities.lock.lock.locked(
capabilities.lock.lock.unlatched(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to change the existing unit test. I suggest adding a new unit test file that sets the feature_map to include the UBOLT feature and then fully exercising the lock/unlock/unlatch functionality

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it. For that, we need to remove the tests related to unlatch from current test unit. I will move the unlatch test to new test file.

@tpmanley tpmanley requested a review from bflorian November 26, 2024 15:36
@HunsupJung
Add test file for unlatch
d5b1d44 
Signed-off-by: Hunsup Jung <[email protected]>
@HunsupJung HunsupJung force-pushed the feature/matter-lock-for-unlatch branch from 0613750 to d5b1d44 Compare November 28, 2024 06:07
tpmanley
tpmanley reviewed Dec 3, 2024
View reviewed changes
drivers/SmartThings/matter-lock/src/test/test_matter_lock_unlatch.lua Outdated
)

test.register_message_test(
"Handle Lock Operation event from Matter device.", {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Handle Lock Operation event from Matter device.", {
"Handle Unlatch Operation event from Matter device.", {

tpmanley
tpmanley approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@tpmanley tpmanley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor remaining comments but otherwise looks good. Thanks for adding the new unit tests.

@bflorian could you look over the embedded device configs? They look right to me but I'd appreciate a second look

@HunsupJung
Copy link
Collaborator Author

@bflorian ,
Could you review this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpmanley tpmanley tpmanley approved these changes

@lelandblue lelandblue Awaiting requested review from lelandblue

@hcarter-775 hcarter-775 Awaiting requested review from hcarter-775

@nickolas-deboom nickolas-deboom Awaiting requested review from nickolas-deboom

@bflorian bflorian Awaiting requested review from bflorian

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HunsupJung @tpmanley