[Create Test] Create TC_ACL_2_10 persistence test with reboot DUT functionality #39945
+515
−748
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
…ctionality: - Add TC_ACL_2_10 Python test module for ACL persistence testing - Remove legacy ACL_2_10 YAML script (replaced by Python implementation) - Add reboot functionality to run_python_test python3 test runner - Implement flag file mechanism for app restart signaling (/tmp/chip_test_restart_app) - Add environment variable support for app restart configuration - Add proper process management and cleanup for restarted apps - Ensure backward compatibility with existing tests The reboot functionality allows testing data persistence across device reboots, which is essential for validating AccessControl cluster behavior.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Generate unique test run ID using UUID to avoid conflicts in concurrent test runs
- Replace hardcoded /tmp paths with unique paths: /tmp/chip_test_restart_app_{test_run_id}
- Add environment variables CHIP_TEST_RESTART_FLAG_FILE and CHIP_TEST_STOP_FLAG_FILE
- Update monitor function to accept flag file paths as parameters
- Maintain backward compatibility with existing tests
This prevents race conditions when multiple test instances run concurrently
on the same machine, eliminating test flakiness caused by file path conflicts.
|
PR #39945: Size comparison from 0c0e2b9 to 04bfd28 Full report (3 builds for cc32xx, stm32)
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new persistence test for ACL and adds the necessary DUT reboot functionality to the Python test runner. My feedback focuses on improving the robustness of the new reboot mechanism, particularly around file cleanup and preventing race conditions in concurrent test runs.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
PR #39945: Size comparison from 0c0e2b9 to 85a1888 Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
…les: - Updated test code in test steps 12-16 to resolve some redundancy when there is only one entry - Added in a finally block so that we make sure that any flag file is removed at the end of tests that call for reboots - Added some try-catch logic around the cleanup operations - Moved import uuid to top of test runner script - Enforced that flag files are set in test environment when running TC_ACL_2_10 test step 9
|
PR #39945: Size comparison from 0c0e2b9 to 3cad3cf Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
reviewed
Jul 11, 2025
| while True: | ||
| try: | ||
| # Check if we should stop monitoring | ||
| if os.path.exists(stop_flag_file): |
There was a problem hiding this comment.
this file gets created only by this script right? If so, can you please change this over to use events rather than file existence? or terminate/sigterm
| logging.info("Restarting app process...") | ||
|
|
||
| A93C | # Stop the existing app process | |
| if hasattr(app_process, 'terminate'): |
There was a problem hiding this comment.
This gets started by Subprocess, so I think this should always exist, right? Are you finding places where it doesn't?
| os.environ['CHIP_TEST_APP_READY_PATTERN'] = app_ready_pattern.pattern.decode() if app_ready_pattern else "" | ||
| # Signal that the app restart capability is available to the test script | ||
| os.environ['CHIP_TEST_APP_RESTART_AVAILABLE'] = '1' | ||
| os.environ['CHIP_TEST_RESTART_FLAG_FILE'] = restart_flag_file |
There was a problem hiding this comment.
if you're putting this in an environment variable, that removes the differentiation you created with the uuid 'cause there can only be one.
might just make more senses to pass this to the test file directly like we do with the named pipe files.
| os.environ['CHIP_TEST_APP_ARGS'] = app_args | ||
| os.environ['CHIP_TEST_APP_READY_PATTERN'] = app_ready_pattern.pattern.decode() if app_ready_pattern else "" | ||
| # Signal that the app restart capability is available to the test script | ||
| os.environ['CHIP_TEST_APP_RESTART_AVAILABLE'] = '1' |
There was a problem hiding this comment.
This kind of makes sense, but I think you're better off passing a command line arg to the test script so it can check internally that the arg is passed correctly if it requires this functionality.
| # Set environment variables for app restart capability | ||
| if app: | ||
| os.environ['CHIP_TEST_APP_PATH'] = app | ||
| os.environ['CHIP_TEST_APP_ARGS'] = app_args |
There was a problem hiding this comment.
rather than pulling these from environment vars, it makes more sense to store these internally - they are passed to this script, so they're already available.
| time.sleep(1) | ||
|
|
||
| # Start a new app process | ||
| new_app_process = Subprocess(app, *shlex.split(app_args), |
There was a problem hiding this comment.
Do you need to do anything here to hook up the stdin processing thread?
There was a problem hiding this comment.
actually, do you know where that gets used or why? It seems like it would clash with the testing framework.
| time.sleep(0.5) # Check every 500ms | ||
| except Exception as e: | ||
| logging.error(f"Error in app restart monitor: {e}") | ||
| time.sleep(1) |
| # Get the current app process (which might have been restarted) | ||
| current_app_process = app_process_ref[0] if app_process_ref else app_process | ||
|
|
||
| if current_app_process: |
There was a problem hiding this comment.
This is pretty similar to the code above - can you pop this into a function so it can be re-used? I think this will also get the stdin pipes hooked correctly.
| app_process_ref = None | ||
| if app: | ||
| # Create a reference to app_process so it can be updated by the monitor thread | ||
| app_process_ref = [app_process] |
There was a problem hiding this comment.
so this is doing thread sync by shared mem - suggesting adding a lock. Maybe? Maybe it's not strictly necessary here since your monitor thread should be killed before the reference is used again. Maybe I'm being paranoid.
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.
Summary
Creates ACL_2_10 python3 test module for ACL persistence testing, also adds in reboot logic for rebooting DUT/app during the test, key implementations include:
Related issues
Testing