Fix CNET-4.11/4.12 setup/teardown lifecycle and disable wildcard subscription - #74048
Fix CNET-4.11/4.12 setup/teardown lifecycle and disable wildcard subscription#74048khodya wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthrough
Suggested reviewers: Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The PR repairs test lifecycle initialization and avoids wildcard-subscription failures during Wi-Fi changes. Reported manual testing completed successfully, with no known merge-blocking risk. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The pull request addresses the missing framework initialization indirectly through the lifecycle fix and avoids the _cq_excluded_attr_ids failure by disabling the wildcard subscription. It does not address the linked issue's import compatibility problem or the missing step logging for Steps 1–19. Full details: Out of Scope Changes checkExplanation The timeout change in TC_CNET_4_11, the TC_CNET_4_12 change, and deletion of Test_TC_CNET_4_12.yaml are not required by linked issue
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
khodya
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The current teardown ordering and use of asyncio.run() in lifecycle hooks can prevent MatterBaseTest framework cleanup from running reliably and may break under an active event loop.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the TC_CNET_4_11 Python certification test to align with the MatterBaseTest setup/teardown lifecycle and disables the background wildcard subscription to reduce instability during repeated Wi‑Fi reassociation steps.
Changes:
- Converted
setup_class/teardown_classoverrides from@classmethodto instance methods and addedsuper()calls to re-enable framework initialization/cleanup. - Disabled the background wildcard subscription for this test via
disable_wildcard_subscription = True. - Switched route preservation/restoration state from class (
cls._original_routes) to instance (self._original_routes).
File summaries
| File | Description |
|---|---|
| src/python_testing/TC_CNET_4_11.py | Fixes test lifecycle integration with MatterBaseTest and disables wildcard subscription for stability during TH Wi‑Fi switching. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The teardown_class override currently calls super().teardown_class() before host-network restoration, conflicting with the documented base-class teardown contract and potentially breaking later event-loop usage in teardown.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #74048 +/- ##
==========================================
- Coverage 57.42% 56.83% -0.60%
==========================================
Files 1940 2002 +62
Lines 127712 130415 +2703
Branches 14502 15173 +671
==========================================
+ Hits 73338 74117 +779
- Misses 54374 56298 +1924 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🔵 Needs a closer look
teardown can exit early on cleanup exceptions and skip restoring the test host’s network state unless cleanup/restoration is guarded with a try/finally.
Review details
Suppressed comments (1)
src/python_testing/TC_CNET_4_11.py:1022
- If _run_framework_cleanup() raises (e.g. DUT unreachable during teardown), teardown_class will exit early and skip restoring LAN routes / the original Wi‑Fi network, leaving the test host in a modified network state. Wrap the cleanup + restoration logic in a try/finally so host network restoration (and super().teardown_class()) always runs.
# Run DUT-side framework cleanup (ACL/fabrics/failsafe) first, while the TH is
# still on whatever network reaches the DUT. _run_framework_cleanup is guarded
# by self._framework_cleanup_done, so super().teardown_class() below can safely
# run it again as a no-op once the base class's own teardown runs.
self.event_loop.run_until_complete(self._run_framework_cleanup())
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The updated teardown_class currently restores the TH’s Wi‑Fi/routing before running base-class teardown cleanup, likely making the DUT unreachable during _run_framework_cleanup() and causing cleanup/state leakage.
Review details
Suppressed comments (1)
src/python_testing/TC_CNET_4_11.py:1029
teardown_classrestores the TH back to the original Wi‑Fi network (and potentially changes routing) before callingsuper().teardown_class(). Since this test ends with the DUT on the 2nd AP (steps 16–20), restoring the TH to the 1st AP first is likely to make the DUT unreachable duringMatterBaseTest’s class-scoped cleanup, causing_run_framework_cleanup()to skip DUT cleanup and leak state. Run the base teardown first (while still on the last operational network), then restore routes/Wi‑Fi in afinallyblock so restoration still happens even if framework cleanup raises.
try:
asyncio.run(restore_original_network())
except Exception as e:
logger.error("teardown_class: Failed to restore original Wi-Fi network: %s", e)
super().teardown_class()
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes correctly align with the established MatterBaseTest setup/teardown and wildcard-subscription controls, and the timeout unit fix matches the existing timedRequestTimeoutMs usage.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup failures in teardown_class are currently logged but can be silently ignored, risking leaving the test host in a broken network state without failing the run.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/python_testing/TC_CNET_4_11.py:1021
- teardown_class logs cleanup failures (route/Wi-Fi restore) but continues, which can leave the host in a broken network state while still reporting the test run as successful. Consider capturing any cleanup exception, running super().teardown_class() for framework cleanup, then re-raising so the failure is visible.
src/python_testing/TC_CNET_4_11.py:46 - TIMED_REQUEST_TIMEOUT is now explicitly in milliseconds (passed to timedRequestTimeoutMs), but the name doesn’t indicate units while nearby timeout constants appear to be in seconds; this is easy to misuse later. Consider renaming to TIMED_REQUEST_TIMEOUT_MS (and updating its uses) to make the unit explicit and align with other tests (e.g., TC_CNET_4_23.py uses TIMED_REQUEST_TIMEOUT_MS).
src/python_testing/TC_CNET_4_11.py:1007
- setup_class uses asyncio.run() even though the Matter test runner provides self.event_loop for async operations; using the runner’s loop avoids failures if setup_class is ever invoked while another loop is active and keeps async work on the same loop as the rest of the framework.
if os_name == "linux":
try:
self._original_routes = asyncio.run(remove_lan_routes())
except Exception as e:
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
khodya
changed the title
There was a problem hiding this comment.
🔵 Needs a closer look
TC_CNET_4_11 uses asyncio.run() in framework lifecycle hooks, which can conflict with the framework-managed event loop and should be switched to self.event_loop.run_until_complete(...).
Review details
Suppressed comments (3)
src/python_testing/TC_CNET_4_11.py:1007
- Using asyncio.run() inside MatterBaseTest hooks can conflict with the framework-managed event loop (and will raise if an event loop is already running). Prefer running these coroutines on the test's existing loop via self.event_loop.run_until_complete(...).
if os_name == "linux":
try:
self._original_routes = asyncio.run(remove_lan_routes())
except Exception as e:
src/python_testing/TC_CNET_4_11.py:1022
- Using asyncio.run() in teardown_class() may create/close a separate event loop; use the framework-managed loop (self.event_loop.run_until_complete) for consistency and to avoid failures when another loop is active.
if os_name == "linux":
try:
original_routes = getattr(self, "_original_routes", [])
asyncio.run(restore_lan_routes(original_routes))
except Exception as e:
src/python_testing/TC_CNET_4_11.py:1027
- Using asyncio.run() here spins up a separate event loop; use self.event_loop.run_until_complete(...) to run the async restore on the existing framework loop.
# Always try to restore original Wi-Fi network
try:
asyncio.run(restore_original_network())
except Exception as e:
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Fixes
TC_CNET_4_11to properly integrate with theMatterBaseTestsetup/teardown lifecycle and disable the background wildcard subscription.Problem
Fixes #72993
TC_CNET_4_11.setup_class/teardown_classwere declared as@classmethodoverrides that never calledsuper().setup_class()/super().teardown_class().MatterBaseTest.setup_class()initializesself.cluster_mapper, step tracking, and theFabricAdminnew-controller hook used during teardown cleanup — none of this ran.MatterBaseTest.teardown_class()runs_run_framework_cleanup()— this was also skipped, leaking framework state.TC_CGEN_2_2([CERT-TEST-FAILURE] Base-Wildcard-Subscription-Check from #43274 is based on wrong assumptions and btreaks tests #72732).Test_TC_CNET_4_12.yamlSolution
setup_class/teardown_classfrom@classmethodto regular instance methods matchingMatterBaseTest's contract, callingsuper().setup_class()first andsuper().teardown_class()last.disable_wildcard_subscription = Trueon the class to skip the background wildcard subscription for the duration of this test.Testing
Manually ran
TC_CNET_4_11.pyon Linux against an ESP32lighting-appDUT, confirming setup/teardown correctly manage LAN routes and the original Wi-Fi network, and that the test completes cleanly across the Wi-Fi reassociation steps with the wildcard subscription disabled.