fixes in playback test#14040
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses fixes in the playback test to improve iteration handling and status verification during a stress test run. Key changes include replacing a hard-coded iteration count with the variable number_of_iterations and updating the status verification mechanism from a blocking wait to a timed sleep plus explicit status checking.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| unit-tests/live/rec-play/test-playback-stress.py | Refactored iteration count and status check logic in playback test |
| unit-tests/live/rec-play/playback_helper.py | Added collection of playback statuses for more detailed verification |
| # We allow 10 seconds to each iteration to verify the playback_stopped event. | ||
| psv.wait_for_status( 10, rs.playback_status.stopped ) | ||
| time.sleep(10) | ||
|
|
||
| statuses = psv.get_statuses() |
There was a problem hiding this comment.
Using a fixed sleep time may lead to nondeterministic test behavior if the playback status update is delayed. Consider using a more robust status waiting mechanism.
|
|
||
| # We allow 10 seconds to each iteration to verify the playback_stopped event. | ||
| psv.wait_for_status( 10, rs.playback_status.stopped ) | ||
| time.sleep(10) |
There was a problem hiding this comment.
Before this change we waited 15 sec between starting the sensor and stopping it.
Now it's 10 sec, will we finish the playback in 10 seconds?
You still check the status is stop and than start,
Can you explain the change and what does it try to solve?
There was a problem hiding this comment.
Do you think the start status override the stop before we managed to check it?
There was a problem hiding this comment.
- look at the updated code.
- the clip we run is very short (64 frames). It may happen that playback is stopped very fast after start so 'stop' status overriding 'start' status before we even test it. The change ensuring we get both start and stop in the proper order
There was a problem hiding this comment.
Pull Request Overview
This PR makes fixes and improvements to the playback stress test to enhance its robustness during continuous iterations. Key changes include introducing a variable for iteration count, updating the status check logic to ensure proper status transitions, and extending the PlaybackStatusVerifier functionality to capture multiple status changes.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| unit-tests/live/rec-play/test-playback-stress.py | Replaces a fixed iteration count with a variable and refines status checking using a new method in PlaybackStatusVerifier. |
| unit-tests/live/rec-play/playback_helper.py | Adds a statuses list to track playback status changes and introduces methods to wait for and retrieve multiple status changes. |
Comments suppressed due to low confidence (2)
unit-tests/live/rec-play/test-playback-stress.py:31
- [nitpick] Remove the unnecessary semicolon at the end of the line to adhere to Python coding style.
psv = PlaybackStatusVerifier( dev );
unit-tests/live/rec-play/playback_helper.py:73
- Timer is used here but there is no indication that it has been imported or defined in this diff. Please ensure Timer is properly imported or defined to avoid runtime errors.
wait_for_event_timer = Timer(timeout)
| required_status_detected = True | ||
| break | ||
| time.sleep( sample_interval ) | ||
| test.check(required_status_detected, description='Check failed, Timeout on waiting for ') |
There was a problem hiding this comment.
[nitpick] Consider enhancing the error description to specify which status transition failed or which statuses were expected, to make debugging easier.
alexkunin-gh
force-pushed
the
akunin/stress_test_relax
branch
from
8073569 to
da45ca1
Compare
| time.sleep( sample_interval ) | ||
| test.check(required_status_detected, description='Check failed, Timeout on waiting for ') | ||
|
|
||
| def reset_status_changes_cnt(self): |
There was a problem hiding this comment.
do we use this function? can we remove it?
There was a problem hiding this comment.
currently not in use but it make sense to have for future usages
| 'Multiple status changes detected, expecting a single change, got '+ str( self._status_changes_cnt - status_changes_cnt ) + | ||
| ' changes, consider lowering the sample interval' ) | ||
|
|
||
| def wait_for_status_changes( self, counter, timeout, sample_interval=0.01 ): |
There was a problem hiding this comment.
dose this replace wait_for_status function? if we use it in other places in the code can we replace it with the new function?
There was a problem hiding this comment.
i'm not sure yet. original wait_for_status() is fine for cases where we waiting for specific status and i see it's usefull in other test cases.
| file_name = os.path.join(repo.build, 'unit-tests', 'recordings', 'all_combinations_depth_color.bag' ) | ||
| log.d( 'deadlock file:', file_name ) | ||
| frames_in_bag_file = 64 | ||
| number_of_iterations = 150 |
| for i in range(250): | ||
| for i in range(number_of_iterations): | ||
| try: | ||
| log.d("Test - Starting iteration # " , i) |
There was a problem hiding this comment.
can we keep this log? I think it helps with debugging
3 participants
No description provided.