-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[DISHM]-Initial add of yaml test scripts for dishwasher mode cluster #27946
Conversation
abpoth
commented
Jul 13, 2023
•
edited
Loading
edited
PR #27946: Size comparison from d9a6f2e to ad3b8a9 Increases (1 build for cyw30739)
Decreases (5 builds for bl602, bl702l, cyw30739, psoc6)
Full report (29 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, k32w, linux, mbed, nrfconnect, psoc6, qpg)
|
PR #27946: Size comparison from 5edf049 to a285535 Increases (11 builds for cyw30739, esp32, linux, qpg, telink)
Decreases (14 builds for cc32xx, cyw30739, esp32, k32w, linux, psoc6, telink)
Full report (49 builds for cc32xx, cyw30739, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to checking against the test plan, these tests should actually be enabled. Specifically:
- Add them to
src/app/tests/suites/ciTests.json
- Rerun codegen (
scripts/tools/zap_regen_all.py --type tests
)
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
PR #27946: Size comparison from 7a39e50 to 1eb8df7 Decreases (3 builds for efr32)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
PR #27946: Size comparison from 6268fbb to bcfd6c8 Decreases (1 build for efr32)
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
"4. TH reads optional attribute (DISHM.S.F00(DEPONOFF)) in | ||
AttributeList" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this in the test plan... Did it get changed after the last update to this PR?
"4. TH reads optional attribute (DISHM.S.F00(DEPONOFF)) in | ||
AttributeList" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, don't see this in the test plan.
|
||
asserts.assert_greater_equal(len(supported_modes), 2, "SupportedModes must have at least two entries!") | ||
|
||
supported_modes_dut = [m.mode for m in supported_modes] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test plan says to verify that self.modeFail
is in supported_modes_dut
, no? Though it says that before it sets the value of supported_modes_dut
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test plan for this one says "REMOVED". Should we still have this?
(I did review it, and the code matches the claimed-removed test plan.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, says "REMOVED".
self.print_step(5, "Read CurrentMode attribute") | ||
|
||
old_current_mode_dut = await self.read_mode_attribute_expect_success(endpoint=self.endpoint, attribute=attributes.CurrentMode) | ||
self.default_controller.ExpireSessions(self.dut_node_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this ExpireSessions here? I don't see it in the test plan.... If the idea is to save time or avoid errors, when the device is power-cycled, shouldn't this be at the point where the power-cycle happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one also seems to be removed?
That said, I reviewed this, and it looks good modulo the documented issue from https://github.com/CHIP-Specifications/chip-test-plans/issues/3478
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
PR #27946: Size comparison from 6268fbb to 8921f91 Increases (56 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
Decreases (23 builds for cyw30739, efr32, esp32, linux, nrfconnect, telink)
Full report (67 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
PR not updated in 1 year, has merge conflicts, CI failures and changes requested tags. Closing as stale. |