[Tests] Properly clean up test suite tests on KeyboardInterrupt #42261
+186
−153
Conversation
This file contains hidden or 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
MarekPikula
force-pushed
the
keyboardinterrupt-tests
branch
from
93b213f to
1b7e384
Compare
Copilot finished reviewing on behalf of
MarekPikula
December 3, 2025 15:15
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the test suite infrastructure by implementing proper cleanup mechanisms for KeyboardInterrupt signals (Ctrl+C) and improving type safety throughout the codebase. The changes ensure that resources such as temporary files, sockets, and Linux network namespaces are properly cleaned up when test execution is interrupted, preventing resource leaks and system state corruption.
Key Changes:
- Implemented try-except-finally pattern with KeyboardInterrupt handling to ensure cleanup on interruption
- Refactored
SubprocessInfo.kindfrom string literals toSubprocessKindenum for type safety - Migrated
AppsRegistermethods from camelCase to snake_case naming convention - Added comprehensive type hints across multiple modules for better IDE support and type checking
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/tests/run_test_suite.py | Added KeyboardInterrupt handling, cleanup mechanism via try-finally block, type hints for CLI functions, and network namespace index option for shell command |
| scripts/tests/run_darwin_framework_ota_test.py | Updated to use SubprocessKind enum and snake_case AppsRegister methods |
| scripts/tests/chipyaml/paths_finder.py | Updated return type annotation to allow None |
| scripts/tests/chiptest/test_definition.py | Changed TestTag to StrEnum, added comprehensive type hints, added assertions for safety, updated to use snake_case AppsRegister methods |
| scripts/tests/chiptest/runner.py | Introduced SubprocessKind enum, refactored dataclass modifications to use replace(), added comprehensive type annotations |
| scripts/tests/chiptest/linux.py | Added type hints, refactored methods to use variadic arguments, made setup_app_link_up public, added wait_for_dad parameter |
| scripts/tests/chiptest/darwin.py | Added type hints to executor methods |
| scripts/tests/chiptest/accessories.py | Refactored all methods to snake_case, added comprehensive type hints, added terminate() method for cleanup protocol |
| scripts/tests/chiptest/init.py | Updated type hints to use modern syntax |
| src/python_testing/matter_testing_infrastructure/mypy.ini | Removed (moved to pyproject.toml) |
| pyproject.toml | Added mypy and pyright configuration |
| .vscode/settings.json | Added Python ruler configuration |
| .vscode/extensions.json | Added recommended extensions for Python development (ruff, mypy, etc.) |
| .github/workflows/mypy-validation.yml | Updated to check additional files and use centralized mypy configuration |
|
PR #42261: Size comparison from 1b2ac27 to 1b7e384 Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
MarekPikula
force-pushed
the
keyboardinterrupt-tests
branch
from
1b7e384 to
7c72f13
Compare
|
PR #42261: Size comparison from 1b2ac27 to 7c72f13 Full report (30 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
MarekPikula
force-pushed
the
keyboardinterrupt-tests
branch
from
7c72f13 to
3f8fbda
Compare
5 tasks
|
PR #42261: Size comparison from 8dd3e20 to 3f8fbda Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
MarekPikula
changed the title
MarekPikula
force-pushed
the
keyboardinterrupt-tests
branch
2 times, most recently
from
577fae8 to
4222664
Compare
|
PR #42261: Size comparison from fe96e6d to 4222664 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Makes it easier to experiment with environment within Linux namespace. Signed-off-by: Marek Pikuła <[email protected]>
Reduces code clutter. Signed-off-by: Marek Pikuła <[email protected]>
Signed-off-by: Marek Pikuła <[email protected]>
Before when stopping execution with KeyboardInterrupt (i.e., Ctrl+C) there was no mechanism to properly clean up the state (e.g., remove temporary files, close sockets, remove Linux network namespaces). Now, KeyboardInterrupt is caught in the main loop, making the cleanup more seamless and less error-prone. Signed-off-by: Marek Pikuła <[email protected]>
This allows to perform all termination commands, even if some fail making it more likely that we leave a clean state. Signed-off-by: Marek Pikuła <[email protected]>
In case any exception is encountered during namespace setup we ensure that, before rethrowing the exception, we properly clean up the state. Signed-off-by: Marek Pikuła <[email protected]>
MarekPikula
force-pushed
the
keyboardinterrupt-tests
branch
from
4222664 to
5ebec2e
Compare
Copilot finished reviewing on behalf of
MarekPikula
December 4, 2025 18:00
Signed-off-by: Marek Pikuła <[email protected]>
|
PR #42261: Size comparison from e333758 to d47c4bb Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Signed-off-by: Marek Pikuła <[email protected]>
|
PR #42261: Size comparison from e333758 to 146dd90 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
arkq
approved these changes
Dec 5, 2025
scripts/tests/chiptest/linux.py
Outdated
| try: | ||
| self._run(cmd) | ||
| except Exception as e: | ||
| log.warning("Encountered error during namespace termination: %s", e, exc_info=True) |
Contributor
There was a problem hiding this comment.
Do we really need stack trace here? Also, in other similar place you are logging exceptions as error while here as warning.
Author
There was a problem hiding this comment.
I was debating it myself how should we treat an exception during cleanup. On one hand it's indeed an error, on the other it's not critical, so I opted for warning. About the traceback, I prefer to have a traceback where possible, but maybe indeed it should go on a debug level. What do you think?
Contributor
There was a problem hiding this comment.
Seeing a call trace on terminal triggers "there is a bug" reaction (at least that's my case). If such trace is not a bug but simply some runtime error which should be handled "somehow", by showing traces in random places we are training ourselves to treat traces as a noise, which is not good in my opinion.
Author
There was a problem hiding this comment.
What about the warning part?
Contributor
There was a problem hiding this comment.
If the program can recover form here onward, then it's a warning, if it cannot recover (functionality will be degraded) then it's an error, if it can not function correctly, then it's a critical error. That's how I see it. So, in this particular case it seems like a warning - it's a termination logic anyway... The same applies to the def cleanup() -> None in the test suite code, IMHO.
Contributor
There was a problem hiding this comment.
Fixed
You did not fixed the def cleanup() -> None in the test suite code....
Signed-off-by: Marek Pikuła <[email protected]>
|
PR #42261: Size comparison from 4579de1 to b6d1654 Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
Signed-off-by: Marek Pikuła <[email protected]>
Signed-off-by: Marek Pikuła <[email protected]>
Signed-off-by: Marek Pikuła <[email protected]>
Signed-off-by: Marek Pikuła <[email protected]>
|
PR #42261: Size comparison from 4579de1 to 77c5aac Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
Signed-off-by: Marek Pikuła <[email protected]>
|
PR #42261: Size comparison from 4579de1 to 244dec3 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Before when stopping execution with KeyboardInterrupt (i.e., Ctrl+C) there was no mechanism to properly clean up the state (e.g., remove temporary files, close sockets, remove Linux network namespaces).
Now, KeyboardInterrupt is caught in the main loop, making the cleanup more seamless and less error-prone.
In addtion to this, there are a few small changes:
Related issues
Related to #42204
Testing
Local tests with
run_test_suite.pyand sending Ctrl+C at various points in time.Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines