TC-TCTL-2.3 python test #36703
Open
TC-TCTL-2.3 python test #36703
+102
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with Changed Files
|
|
|
github-actions
bot
added
github
workflows
tests
matter-1.4-te2-script-change
|
PR #36703: Size comparison from 2bc3251 to 71708d1 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
reviewed
Dec 4, 2024
src/python_testing/TC_TCTL_2_3.py
Outdated
| ] | ||
| return steps | ||
|
|
||
| @async_test_body |
There was a problem hiding this comment.
This is correct as of today, but it's something we want to change. The ability to change this is going to be dependent on this PR landing #36727. So nothing to do right now, but either a follow up or something that can be addressed in this PR if the other PR lands before you can undraft.
Right now, the purpose of the pics_ function above is to allow the test harness to select the tests used for certification. This is not used at all in CI, and the CI works because we know the all-clusters-app happens to have this feature enabled.
The PICS come from an external file (actually set of files) that describe what's on the device at a certain endpoint. The PICS in this test say that there is a temperature control server on the endpoint (TCTL.S) and the temperature control server supports feature 1 (TCTL.S.F01). Both of these are things that we can read directly from the device - we don't need to be going out to an external file for this information, and it's preferable not to since that's more error prone (file might be incorrect, incorrect file might have been loaded, maybe no file was supplied etc.)
We recently started testing with self-selecting test. We have a set of decorators that will only run the test if certain conditions hold. However, as noted in this PR description (#36727), we still need to keep this top level pics_ method for the test harness because even just starting tests from the test harness is slow.
So, once that PR lands, I'm going to suggest changing @async_test_body to
@run_if_endpoint_matches(has_feature(Clusters.TemperatureControl, Clusters.TemperatureControl.Bitmaps.Feature.kTemperatureLevel))
That will allow the automation that GD is writing to automatically select this test based on the device composition.
TL;DR - nothing to do now, if that PR lands before this goes in, let's update
There was a problem hiding this comment.
Dependency is merged so I have decorated with @run_if_endpoint_matches
src/python_testing/TC_TCTL_2_3.py
Outdated
|
|
||
| # Step 2: Read SelectedTemperatureLevel attribute | ||
| self.step(2) | ||
| if self.check_pics("TCTL.S.A0004"): # SelectedTemperatureLevel attribute |
There was a problem hiding this comment.
Similar to above, this PICS just indicates whether a attribute 4 is supported on this cluster / endpoint and that information is available from the device. Getting this information directly from the device rather than from the PICS file is less error prone.
I've asked one of the CSA folks working on test automation to add in a guard function to just read this from the device. In other tests, we've done this by reading the attribute list directly in the test, but since we do this a lot it makes sense to centralize this. The PR to add that is here: #34290.
As above, if that PR happens to land before this gets undrafted and goes for review, it makes sense to switch over to use that, otherwise it can be done as a followup.
the change here would be something like
if attribute_guard(Clusters.TemperatureControl.Attributes.SelectedTemperatureLevel):
also below.
There was a problem hiding this comment.
Dependency is merged so I'm using attribute_guard here
…ttribute_check_success()
|
PR #36703: Size comparison from 2bc3251 to cb1fdfc Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36703: Size comparison from af8d173 to 5777abd Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36703: Size comparison from b0d0614 to 7fa71d7 Full report (19 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
|
PR #36703: Size comparison from b0d0614 to 1a8d6b2 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
reviewed
Dec 18, 2024
There was a problem hiding this comment.
Left two suggestions. That second one is totally my fault - should have realized that earlier. My apologies.
With those changes, this PR looks good, so just ping me once those changes are done and I can checkmark this.
The other thing that will need to happen here is an update to the test plan to remove the PICS markings. Once you add the expected outcome strings, we have a script that can dump the test plan table, so it's just a copy-paste, open PR thing.
The test plan generation script is here: https://github.com/project-chip/connectedhomeip/blob/master/src/python_testing/test_plan_table_generator.py
You just give the file name, class name and test name. So that would be
python src/python_testing/test_plan_table_generator.py TC_TCTL_2_3.py TC_TCTL_2_3 test_TC_TCTL_2_3
A bit redundant, but oh well.
The test plan is here: https://github.com/CHIP-Specifications/chip-test-plans/blob/master/src/cluster/temperaturecontrol.adoc#tc-tctl-2-3-optional-temperature-level-attributes-with-dut_server
You would just replace the table in the "Test Procedure" section with the one that's generated, and open a PR saying that you're removing the PICS markers because those attributes are mandatory when the TL feature is supported.
src/python_testing/TC_TCTL_2_3.py
Outdated
| steps = [ | ||
| TestStep(1, "Commissioning, already done", is_commissioning=True), | ||
| TestStep(2, "TH reads from the DUT the SelectedTemperatureLevel attribute"), | ||
| TestStep(3, "TH reads from the DUT the SupportedTemperatureLevels attribute and verifies string lengths"), |
There was a problem hiding this comment.
Would you mind adding in the expected outcome strings here too? It's not a huge deal, but it makes it much easier to update the test plan.
src/python_testing/TC_TCTL_2_3.py
Outdated
|
|
||
| # Step 2: Read SelectedTemperatureLevel attribute | ||
| self.step(2) | ||
| if self.attribute_guard(endpoint=self.endpoint, attribute=attributes.SelectedTemperatureLevel): |
There was a problem hiding this comment.
I'm sorry - I know I commented earlier that these should be changed to attribute_guard (and this is totally correct, btw - thank you). But I also just realized that these attributes are mandatory when this feature is enabled. In other words, I believe the test plan is incorrect here and you've accidentally uncovered a test bug. You can just assume these two attributes are there. In other words, just remove this attribute guard, those attributes are there, and if they're not, then the test will throw an exception, and that exception is correct and expected because it means the device configuration is incorrect.
There was a problem hiding this comment.
Got it, really useful input for understanding this functionality. I've removed attribute guard.
|
PR #36703: Size comparison from b0d0614 to 0daf2a1 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
I've made a requested changes and asked Juan to raise a PR to a test plan repo https://github.com/CHIP-Specifications/chip-test-plans/pull/4834 as I still have to get access to CHIP-Specifications organization |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Category:
Functional
Description:
Verifies that the DUT can respond to Temperature Control Cluster attribute read commands. This test case is required only if the TL feature is supported (TCTL.S.F01(TL)). Some check examples:
Test plan for more details: https://github.com/CHIP-Specifications/chip-test-plans/blob/master/src/cluster/temperaturecontrol.adoc#tc-tctl-2-3-optional-temperature-level-attributes-with-dut_server