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.

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

add thirdreality vibration sensor #781

Merged
merged 11 commits into from
Oct 11, 2023

Conversation

weihuan1111
Copy link
Contributor

Change-Id: Idc4f20f4041dfa416b3cccd4063e52669e078740

Change-Id: Idc4f20f4041dfa416b3cccd4063e52669e078740
@github-actions
Copy link

Duplicate profile check: Passed - no duplicate profiles detected.

@github-actions
Copy link

github-actions bot commented Jun 21, 2023

Channel deleted.

@github-actions
Copy link

github-actions bot commented Jun 21, 2023

Test Results

     52 files  ±0     345 suites  +1   0s ⏱️ ±0s
1 622 tests ±0  1 622 ✔️ ±0  0 💤 ±0  0 ±0 
2 842 runs  +3  2 842 ✔️ +3  0 💤 ±0  0 ±0 

Results for commit 4a6a30b. ± Comparison against base commit 38c1a61.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jun 21, 2023

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

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 4a6a30b

@lelandblue lelandblue requested review from greens and ctowns June 26, 2023 12:34
@lelandblue
Copy link
Contributor

Hello, @weihuan1111 Could you please add some unit tests to this? We see that is not included and would appreciate your adding coverage for the code that is being saught to be added.

Copy link
Contributor

@ctowns ctowns left a comment

Choose a reason for hiding this comment

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

Besides the one little change and adding units tests, everything looks good to me. Thanks for defining the constants at the top of the file!

@weihuan1111
Copy link
Contributor Author

weihuan1111 commented Jun 27, 2023

Hello, @weihuan1111 Could you please add some unit tests to this? We see that is not included and would appreciate your adding coverage for the code that is being saught to be added.

@lelandblue How to add unit tests? And how to test the unit tests? And is test units neccessary?

@lelandblue
Copy link
Contributor

Hello @weihuan1111 .

Thank you for your continued communication. I also left a comment on the other pull request but wanted to leave one here to to help support.

We do require unit tests for this addition to the repo. For examples of other tests in this repo, please feel free to review these examples :https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/tree/main/drivers/SmartThings/zigbee-contact/src/test

Also, here is our documentation on writing tests. https://developer.smartthings.com/docs/devices/hub-connected/test-your-driver

Thank you.

@weihuan1111
Copy link
Contributor Author

weihuan1111 commented Jun 28, 2023

@lelandblue Hi, i have two questions.

  1. Can we start certification first, and simultaneously we add the test unit?
  2. When we complete add the test unit, how to confirm it is work without error?

@greens
Copy link
Contributor

greens commented Jun 28, 2023

@weihuan1111 Please write the tests first. There are plenty of existing examples in the code that you can modify lightly.

If you first follow the guide here: https://developer.smartthings.com/docs/devices/hub-connected/set-up-dev-env , you will be able to run the unit tests locally to verify they are passing.

@weihuan1111
Copy link
Contributor Author

@lelandblue @greens Test units has been added.

Copy link
Contributor

@greens greens left a comment

Choose a reason for hiding this comment

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

Aside from the copyright, everything looks good.

@weihuan1111
Copy link
Contributor Author

@greens The copyright has been added.

@weihuan1111
Copy link
Contributor Author

@Brianj94 Hi, now, we have not receive the e-mail from UL to do WWST certification. When will you finish the code review process and create the necessary tickets on your side to bring in UL

@Brianj94
Copy link
Contributor

Hi @weihuan1111 , I was speaking with the certification team today about the most recent certification requests submitted for Third Reality. They are concerned with the missing Firmware Version value as well as the missing Product Category fields. Could you please re-submit these three certification requests (Multi-Function Night Light, Vibration Sensor, Smart Button) with the Firmware version for each device listed, and the Product Category listed?

This should help us avoid any additional issues with the basic information and ensuring the "Firmware Update" capability can be tested correctly at UL.

Please let me know if you have any questions. Thank you!

@weihuan1111
Copy link
Contributor Author

@Brianj94 If we fill in the firmware version, but after that, we update the firmware version, and send the the newest firmware version to test, will it affect the test results?

@Brianj94
Copy link
Contributor

Hi @weihuan1111 , if you update the firmware version after the device is sent to UL just add a comment on this thread, and I will add that change to the ticket so that UL knows the correct version. That should not cause any issues.

@weihuan1111
Copy link
Contributor Author

@Brianj94 Thank you, i have add the Firmware version and Product Category for the device in this pull request.

@Brianj94
Copy link
Contributor

Hi @weihuan1111 , thanks for the update. I was just looking at this certification request and I believe the product category is not listed correctly. Could you please change that to "Multi-Function Sensor" and resubmit?

Everything else looks good and we should be able to proceed after that change.

Thank you.

@weihuan1111
Copy link
Contributor Author

@Brianj94 I have change the product category to "Multi-Function Sensor" and resubmit. And we still have #790 this product, it has not complete the review.

@Brianj94
Copy link
Contributor

Hi @weihuan1111 , thank you for making those changes. At this time all three certification requests have completed the checks and code review. The UL tickets have been created as well. Once the UL team has reviewed the scope of work they will reach out to provide quotes on these three devices.

Thank you.

@weihuan1111
Copy link
Contributor Author

@Brianj94 Hi, now, we still have not received the e-mail from UL to do WWST certification test for our 3 products. Is there any problems?

@Brianj94
Copy link
Contributor

Hi @weihuan1111 , These are no problems that I am aware of. I can see that the tasks for UL have been created, and at this time we are awaiting the UL team to review and move forward with these devices. If I hear of any issues I will let you know. Thanks!

@lelandblue
Copy link
Contributor

Hey @weihuan1111 There are conflicts on this PR, Can you please resolve them?

@Brianj94
Copy link
Contributor

Brianj94 commented Oct 4, 2023

Hi @weihuan1111 , I received an update today that this device has passed certification. In order for our team to merge this PR could you please resolve the conflicts on this request? Thank you!

@weihuan1111
Copy link
Contributor Author

@lelandblue @Brianj94 The conflicts have been resolved.

@lelandblue lelandblue merged commit e672912 into SmartThingsCommunity:main Oct 11, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants