TC-SU-2.2 python test #40366
TC-SU-2.2 python test #40366
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the basic structure for the TC_SU_2_2 test script. The initial commit is a good start and follows the established patterns of the repository. My review includes suggestions for improving code quality by removing unused code, adding necessary documentation like a class docstring, and enhancing the readability of test step definitions. These changes will help ensure the code is maintainable and easy to understand as the implementation progresses. The comments reference the Gemini Code Review Instructions style guide, specifically the rule to only comment when a change is probably desirable.
|
PR #40366: Size comparison from 7b8639e to 602584c Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #40366: Size comparison from 7b8639e to 3be2b76 Increases above 0.2%:
Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40366 +/- ##
==========================================
+ Coverage 50.81% 50.94% +0.13%
==========================================
Files 1365 1372 +7
Lines 100227 100459 +232
Branches 13015 13016 +1
==========================================
+ Hits 50928 51183 +255
+ Misses 49299 49276 -23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nModelError: UnsupportedEndpoint (0x7f).
…nModelError: UnsupportedEndpoint (0x7f) v3.
src/python_testing/TC_SU_2_2.py
Outdated
| logger.info(f'Step #1.0 - Provider NodeID: {provider_node_id}, FabricId: {fabric_id}') | ||
|
|
||
| # ------------------------------------------------------------------------------------ | ||
| # Step 1.1 - Open commissioning window on DUT (via TH1) |
There was a problem hiding this comment.
why are you opening the commissioning window on the DUT here? It should be commissioned already according to the comment in step 1.0.
There was a problem hiding this comment.
Refactored: The initial approach assumed the DUT was already commissioned, but that has been updated. Now run_ota_step handles all prerequisites per step (launch, commission, ACLs, etc.) to keep each step isolated and reproducible.
src/python_testing/TC_SU_2_2.py
Outdated
| cmd_announce = Clusters.OtaSoftwareUpdateRequestor.Commands.AnnounceOTAProvider( | ||
| providerNodeID=0x0000000000000001, # Provider | ||
| vendorID=0xFFF1, | ||
| announcementReason=Clusters.OtaSoftwareUpdateRequestor.Enums.AnnouncementReasonEnum.kSimpleAnnouncement, |
There was a problem hiding this comment.
This isn't specified in the test plan - is this just to trigger the QueryImage command? If so, the announcement should be UrgentUpdateAvailable in order to get the best change at triggering the QueryImage. Even then it's not guaranteed, but let's cross that bridge a bit later with a fallback.
There was a problem hiding this comment.
Based on feedback from Andy Salisbury, the automatic update triggered by SimpleAnnouncement is consistent with the OTA Requestor sample app behavior in the SDK. While the spec indicates that only UpdateAvailable or UrgentUpdateAvailable should trigger an update, the current behavior is allowed, as the spec does not forbid the Requestor from immediately checking. This explains why the update starts automatically even without --autoApplyImage.
Refactored: For Step 1, the Provider is now launched with queue="updateAvailable" and the announcement reason changed to kUpdateAvailable to align with the test plan.
There was a problem hiding this comment.
You're not writing a test for the SDK though - you're writing a test for any compliant device.
The difference between urgent and normal update available is the recommendation on when the query should be done.
For UpdateAvailable
A receiving OTA Requestor SHOULD only query the indicated OTA Provider at the ProviderLocation at its next upcoming OTA Provider query.
For UrgentUpdateAvailable:
A receiving OTA Requestor SHOULD query the indicated OTA Provider at the ProviderLocation after a random jitter delay between 1 and 600 seconds.
src/python_testing/TC_SU_2_2.py
Outdated
| expected_attribute=Clusters.OtaSoftwareUpdateRequestor.Attributes.UpdateStateProgress | ||
| ) | ||
|
|
||
| # Start subscription (async) |
There was a problem hiding this comment.
The subscription here should be started before the announcement is sent, or you risk missing the report.
There was a problem hiding this comment.
Based on the current implementation and timing, the subscription setup after sending the AnnounceOTAProvider does not risk missing any reports, since the OTA update takes 1-3 seconds to start. All relevant UpdateState and UpdateStateProgress events are captured correctly. We’ve verified that the observed sequences match the expected OTA flow, so no functional changes are required.
Example for execution ran
[MatterTest] 08-22 00:35:50.782 INFO Step #1.3 - Full OTA state sequence observed: [<UpdateStateEnum.kDownloading: 4>, <UpdateStateEnum.kApplying: 5>, <UpdateStateEnum.kIdle: 1>]
[MatterTest] 08-22 00:35:50.783 INFO Step #1.3 - Progress values observed: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99]
Would you like me to adjust the sequence to start the subscriptions before the announcement, or keep the current approach?
There was a problem hiding this comment.
Just because you're currently winning the race does not mean there is no race. If you could change the order, that would be preferable.
…UpdateState and UpdateStateProgress.
- Provider is commissioned and ACLs only once at the start of the test. - Provider process is launched per step but reuses the same KVS/data and ACLs. - Cleanup (ACLs, sessions, KVS deletion) is no longer performed after each step.
There was a problem hiding this comment.
Some changes/refactor has been made:
Refactor OTA Provider setup:
- Reuse commissioned provider across steps
- Provider is commissioned and ACLs only once at the start of the test.
- Provider process is launched per step but reuses the same KVS/data and ACLs.
- Cleanup (ACLs, sessions, KVS deletion) is no longer performed after each step.
- In the OTA wrapper (
apps.py) file updated the functionOTAProviderSubprocessto accept custom path, if not passed relies on fixed temporary files.
STEP_7:
- When closing the OTA Provider, the stack prints the following:
src/messaging/ExchangeMgr.cpp:185: CHIP Error 0x00000007: No unsolicited message handler at src/protocols/bdx/BdxTransferServer.cpp:55
- This occurs because the BDX Transfer Server no longer has an active handler when the provider process is terminated. It does not affect test outcomes and can be safely ignored.
|
Summary / Status Update New Refactor / Updates:
|
- Move OTA provider logic: setup_provider (Provider launch, Provider commissioning, configure ACLs, and add_single_ota_provider), cleanup_provider. - Add helpers: read_single_attribute, read_single_attribute_check_success. - Update test to use new OTAHelper.
- Attribute subscriptions: start() updated to use keepSubscriptions=False for each step after step 1. Step 1 still uses True due to parallel asyncio.gather to start multiple subscriptions. - Event subscriptions: integrate cancel() method in EventSubscriptionHandler to properly terminate active subscriptions instead of using reset(). - Ensures clean state for each step and avoids lingering events or INVALID_ACTION errors.
…ODE_ID, FABRIC_ID,and CONTROLLER.
- Added clear_ota_providers helper function to OTA wrapper - Updated test Step 7.5 to clear DefaultOTAProviders for a clean state
- Step 3.2: Track Idle > Querying > Idle flow; collect unexpected states during 120s - Step 3.3: Handle subscription properly - Step 3.4: Assert full sequence matches expected flow; ensure no unexpected states occurred - Improve logic and logging
- Step 2.2: Matcher now tracks DelayedOnQuery → Downloading → Idle flow - Observes the 120s Busy interval and captures any unexpected states - Step 2.4: Validates full sequence and delay between DelayedOnQuery and Downloading - Logic and logger improved
There was a problem hiding this comment.
I have completed all the changes, refactors, and updates based on Cecille and Andy feedback. Here is the link with the summary: project-chip/matter-test-scripts#650 (comment)
| ) | ||
| logging.info(f'Step #1.0 - cmd AnnounceOTAProvider response: {resp_announce}.') | ||
|
|
||
| # ------------------------------------------------------------------------------------ |
There was a problem hiding this comment.
Refactored: setup_provider in the OTA wrapper already handles launch, commissioning, ACLs, and add_single_ota_provider produces no logs to validate, so keeping it before starting subscriptions is ok.
src/python_testing/TC_SU_2_2.py
Outdated
| def matcher_busy_state(report): | ||
| """ | ||
| Step #2.2 matcher function to track OTA UpdateState (Busy sequence). | ||
| Tracks state transitions: DelayedOnQuery > Downloading > Idle. |
There was a problem hiding this comment.
Refactor matcher logic for DelayedOnQuery:
- Matcher now tracks
DelayedOnQuery > Downloading > Idle. - Observes the 120s
Busyinterval and captures any unexpected states. - Step 2.4: Validates full sequence and delay between
DelayedOnQueryand Downloading. - Logic and logger improved.
src/python_testing/TC_SU_2_2.py
Outdated
| return False | ||
|
|
||
| current_time = time.time() | ||
|
|
There was a problem hiding this comment.
Refactored: Now matcher on step 3 flow:
- Step 3.2:
Track Idle > Querying > Idle. - Collect unexpected states during 120s.
- Step 3.3: Handle subscription properly.
- Step 3.4: Assert full sequence matches expected, ensure no unexpected states occurred.
- Improve logic and logging
src/python_testing/TC_SU_2_2.py
Outdated
| step_number_s1 = "[STEP_1]" | ||
|
|
||
| # Prerequisite #1.0 - Provider_S1 info | ||
| provider_node_id_s1 = 1 |
There was a problem hiding this comment.
Refactored: Now provider using the same data and KVS to avoid re-commissioning and full cleanup in each step and cleanup will only be done once at the end of the test.
src/python_testing/TC_SU_2_2.py
Outdated
| # Commission the DUT (Requestor) with the TH/OTA-P (Provider) | ||
|
|
||
| # Prerequisite #1.0 - Requestor (DUT) info | ||
| CONTROLLER = self.default_controller |
There was a problem hiding this comment.
Refactored: The values (REQUESTOR_NODE_ID, FABRIC_ID, CONTROLLER) updated to snake_case naming conventions.
src/python_testing/TC_SU_2_2.py
Outdated
| fabric_filtered=False, | ||
| min_interval_sec=1, | ||
| max_interval_sec=1, | ||
| keepSubscriptions=True |
There was a problem hiding this comment.
Refactored subscriptions for attributes and events:
- Integrate cancel() for event subscription on EventSubscriptionHandler
- Update the start call for attribute subscriptions to use keepSubscriptions=False
src/python_testing/TC_SU_2_2.py
Outdated
| subprocess.run("rm -rf /tmp/chip_kvs /tmp/chip_kvs-shm /tmp/chip_kvs-wal", shell=True) | ||
| await asyncio.sleep(1) | ||
| subprocess.run("rm -rf /tmp/chip_kvs_provider*", shell=True) | ||
| await asyncio.sleep(1) |
There was a problem hiding this comment.
Refactored: Cleanup of DefaultOTAProviders at the end of the test:
- Added
clear_ota_providershelper function to OTA wrapper - Updated test Step 7.5 to clear
DefaultOTAProvidersfor a clean state
src/python_testing/TC_SU_2_2.py
Outdated
| logger.info(f'Prerequisite #4.0 - Write DefaultOTAProviders response: {resp}') | ||
| asserts.assert_equal(resp[0].Status, Status.Success, "Failed to write DefaultOTAProviders attribute") | ||
|
|
||
| async def setup_provider( |
There was a problem hiding this comment.
Refactored: Create OTAHelper class:
- Move OTA provider logic:
setup_provider(Provider launch,Provider commissioning,configure ACLs, andadd_single_ota_provider), cleanup_provider - Add helpers:
read_single_attribute,read_single_attribute_check_success - Update test to use new
OTAHelper
| @@ -1,4 +1,4 @@ | |||
| # Copyright (c) 2024 Project CHIP Authors | |||
| # Copyright (c) 2025 Project CHIP Authors | |||
There was a problem hiding this comment.
| # Copyright (c) 2025 Project CHIP Authors | |
| # Copyright (c) 2024-2025 Project CHIP Authors |
if you're going to change these, the original year should stay. Or you can just leave them with the original year.
There was a problem hiding this comment.
The apps.py in my current PR will no longer be used, since we are moving to the new/update OTA wrapper approach. I suggest pausing all review and refactors on this branch related to apps.py for now. Once the new wrapper is ready, I will refactor my test accordingly.
| except Exception: | ||
| # Do not leak KVS file descriptor on failure | ||
| os.close(self.kvs_fd) | ||
| if self.kvs_fd is not None: |
There was a problem hiding this comment.
how would this be None if you set it at the start?
There was a problem hiding this comment.
The apps.py in my current PR will no longer be used, since we are moving to the new/update OTA wrapper approach. I suggest pausing all review and refactors on this branch related to apps.py for now. Once the new wrapper is ready, I will refactor my test accordingly.
| secured_device_port: Port for provider process. | ||
| queue: Optional queue name. | ||
| timeout: Optional timeout in seconds. | ||
| override_image_uri: Optional ImageURI override. |
There was a problem hiding this comment.
what is this overriding? Is there a default? how is this different than the constructed uri from the ota_file?
There was a problem hiding this comment.
As mentioned above, the current apps.py won't be updated further; the new OTA wrapper will be used once it is ready.
| self, | ||
| requestor_node: int, | ||
| provider_node: int, | ||
| fabric_index: int, |
There was a problem hiding this comment.
You shouldn't need to pass a fabric index explicitly - the fabric index of a write is the fabric index of the node doing the writing. You can use 0 for writes.
There was a problem hiding this comment.
same case as mentioned above, the current apps.py won't be updated further; the new OTA wrapper will be used once it is ready.
| requestor_node: int, | ||
| provider_node: int, | ||
| fabric_index: int, | ||
| original_requestor_acls: list |
There was a problem hiding this comment.
why are these passed in vs. kept as part of the class? The function description says this is read from the node. You need this later for cleanup too, right?
There was a problem hiding this comment.
same case as mentioned above, the current apps.py won't be updated further; the new OTA wrapper will be used once it is ready.
|
|
||
| assert resp[0].Status == Status.Success, "Failed to clear DefaultOTAProviders" | ||
|
|
||
| async def setup_provider( |
There was a problem hiding this comment.
a lot of the parameters being passed into this function feel like they're not actually something the caller should need to know about.
ex. the fabric ID - why is that necessary if the controller is passed in? The provider node id is something the class should maintain internally. The discriminator is fully transient.
Need to reconsider the class structure here - the point of having a class is to abstract away some of the complexity from the callers, not just to put stuff into a collection.
There was a problem hiding this comment.
same case as mentioned above, the current apps.py won't be updated further; the new OTA wrapper will be used once it is ready.
| ) | ||
| logging.info(f'Step #1.0 - cmd AnnounceOTAProvider response: {resp_announce}.') | ||
|
|
||
| # ------------------------------------------------------------------------------------ |
There was a problem hiding this comment.
Events that happen before AnnounceOTAProvier shouldn't affect your test since you're only subscribing to the events you care about.
src/python_testing/TC_SU_2_2.py
Outdated
| def matcher_busy_state(report): | ||
| """ | ||
| Step #2.2 matcher function to track OTA UpdateState (Busy sequence). | ||
| Tracks state transitions: DelayedOnQuery > Downloading > Idle. |
There was a problem hiding this comment.
Andy's concern is that if the device doesn't transition to downloading, which is spec compliant, then the test will fail even though it shouldn't. "Real timing behaviour" is not something you can model from the examples in the SDK.
There was a problem hiding this comment.
Please find below a summary based on the latest feedback:
apps.pyrom my branch in this PR will no longer be used once we move to the new OTA wrapper; please pause any review or refactors on this branch.- Step 2: The DUT transitions to
Downloadingafter 120 s. During the 120 s interval, onlyQueryingandDelayedOnQueryare observed, and no newQueryImagecommands are sent.
src/python_testing/TC_SU_2_2.py
Outdated
| def matcher_busy_state(report): | ||
| """ | ||
| Step #2.2 matcher function to track OTA UpdateState (Busy sequence). | ||
| Tracks state transitions: DelayedOnQuery > Downloading > Idle. |
There was a problem hiding this comment.
@cecille and @harimau-qirex, I agree that technically the DUT could remain in DelayedOnQuery for longer than 120 s and still comply with the spec. However, in this test, the DUT transitions to Downloading after 120 s because we launch the OTA provider with Busy and delayedActionTime=60s, and the DUT applies the spec's minimum delay of 120 s.
Since we cannot cancel the OTA mid-download until task # 685 is implemented, the test completes the full flow through Downloading > Idle to validate the sequence correctly.
During the 120 s interval after the first DelayedOnQuery, the test only validates that the DUT reports Querying and DelayedOnQuery. No new Downloading should occur, and no additional QueryImage requests are sent.
QueryImageResponse logs confirm this expected flow:
[SWU] QueryImageResponse:
[SWU] status: 1
[SWU] delayedActionTime: 60 seconds
[SWU] userConsentNeeded: 0
[SWU] metadataForRequestor: 0
[SWU] Scheduling a retry; delay: 120
This ensures the test validates the sequence correctly according to the spec.
This confirms that in our current flow the DUT always transitions to Downloading after 120 s. The test also validates that during the 120 s interval only Querying and DelayedOnQuery are present, ensuring no new QueryImage commands are sent before the minimum wait time.
|
PR #40366: Size comparison from 5d63878 to 43a3b2c Full report (5 builds for cc32xx, realtek, stm32)
|
|
PR #40366: Size comparison from e2f86ee to bcf21d3 Full report (5 builds for cc32xx, realtek, stm32)
|
- Refactor based on latest versions of TC_SUTestBase.py - Refactor matcher logic steps 2, 3, 4 - Adjust await_all_expected_report_matches durations
- Extracted OTA provider launch into a fn setup_provider. - Extracted ACL into a fn setup_acl. - Extracted and Refactored OTA UpdateState matcher for steps 2, 3, and 4 into a fn matcher_ota_updatestate, using small timing tolerance to handle scheduling drifts (~0.008s) and avoid false unexpected state detections. - Improved interval asserts 120s, 180s in Steps 2 and 4.
There was a problem hiding this comment.
New refactors based on code review:
- Base alignment files (
TC_SUTestBase.py,apps.py,apps.pyi,test_metadata.yaml) updated for structure/compatibility. - Extracted OTA provider launch (
setup_provider) and ACL setup (setup_acl) into functions. - Refactored OTA UpdateState matcher (
matcher_ota_updatestate) for Steps 2, 3, and 4: added small timing tolerance (~0.008 s) to avoid false unexpected state detections and stabilize 120 s / 180 s interval checks. - Updated
timeout_secinawait_all_expected_report_matches.
Additional Notes:
- Step 2–4 matcher logic refined based on recent OTA flows; allowed states enforced during intervals.
Important Note:
The base alignment refactor files (TC_SUTestBase.py, apps.py, apps.pyi, test_metadata.yaml) should be reviewed in their dedicated PRs, not in this PR, to avoid duplicate review.
|
The SU test logic looks good to me. Approval pending due to in-progress dependencies. |
Summary
This PR is focused on implementing and validating the TC-SU-2.2
Fixes: project-chip/matter-test-scripts#650
Test plan: 3.2.2. [TC-SU-2.2] Handling Different QueryImageResponse Scenarios on Requestor
Scope
Related Issues & PRs
TC-SU-all: Consolidate updates #654
OTA: Provider cannot gracefully cancel a BDX transfer during OTA update #685
OTA provider wrapper #649
Umbrealla: #648 TC-SU and TC-BDX: Automate and re-think (umbrella)
yaml: https://github.com/project-chip/connectedhomeip/blob/master/src/app/tests/suites/certification/Test_TC_SU_2_2.yaml
matter OTA: https://github.com/project-chip/connectedhomeip/blob/e560a107149315e2b8da09dc8e53afc3deba5a4d/docs/platforms/esp32/ota.md
Bug: [BUG] WriteAttribute updates DefaultOTAProviders attribute despite ConstraintError in DefaultOTAProviders cluster #40294
Testing
To run the automated TC-SU-2.2 test, follow these steps:
Reproduce Test
To reproduce the issue, please follow the prerequisites and test steps described below.
Prerequisite: Build and run Matter Project
Prerequisite: OTA
Build Components:
Create OTA Images:
The script requires OTA images with different version numbers. To generate a new OTA image (example: version 2.0 to the requestor app): # Open examples/ota-requestor-app/linux/include/CHIPProjectAppConfig.h# Add or update the following definitions at the end of the file:Required OTA Images
Run Test