-
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
[Testing] Add a command line argument to the all-clusters-app to Configure the minimal commissioning timeout #17911
[Testing] Add a command line argument to the all-clusters-app to Configure the minimal commissioning timeout #17911
Conversation
d2426a5
to
b1c311a
Compare
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 is going to fail some of our tests, which check that a too-short commissioning window can't be opened....
More generally, we shouldn't be introducing non-spec-compliant behavior that holds across all tests. If we want it for specific test because it does not actually depend on the spec-compliance, can we figure out a way to do it for just the one test? For example, could we add a command-line arg for this thing and then restart the app with that arg in the tests that would like a shorter timeout? |
PR #17911: Size comparison from 917142a to b1c311a Increases above 0.2%:
Increases (22 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, nrfconnect, p6, telink)
Decreases (4 builds for cc13x2_26x2)
Full report (23 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, p6, telink)
|
b1c311a
to
e08d134
Compare
e08d134
to
aeeaa7e
Compare
I have updated the patch. It does not launch the all-clusters-app with a commissioning timeout that it not spec-compliant by default. Now (in an other PR) we need to decide if we want to add an commissioning window timeout to the list of arguments that are allowed when starting an application from YAML, or an other way could be to add a method to Any opinions ? |
aeeaa7e
to
40183e1
Compare
PR #17911: Size comparison from e4049f2 to 40183e1 Increases above 0.2%:
Increases (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Decreases (5 builds for cc13x2_26x2, mbed)
Full report (34 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, 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.
Fast tracking, given this is unblocking testing.
For the followup: I think just passing this arg via yaml when doing a |
…igure the minimal commissioning timeout
40183e1
to
8cd929c
Compare
PR #17911: Size comparison from df644b2 to 8cd929c Increases (23 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Decreases (5 builds for cc13x2_26x2, mbed)
Full report (25 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
Problem
The tests from #17837 are trying to use a delay for the minimum timeout of the commissioning window, which is currently 3 minutes per spec (and worse the test plan states 5 minutes). That is just lost time when it comes to our CI.
Change overview
all-clusters-app
to be 1 second.Testing
The command
./out/debug/standalone/chip-tool administratorcommissioning open-basic-commissioning-window 10 0x12344321 0 --timedInteractionTimeoutMs 1000
fails with anINVALID_COMMAND
without this patch.