-
Notifications
You must be signed in to change notification settings - Fork 5
Add a modifier requires_rti to cut down duplicated code in unit tests #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other tests also have the RTI logic, please search it, for example:
https://github.com/isaac-for-healthcare/i4h-workflows/blob/main/workflows/robotic_ultrasound/tests/test_simulation/test_ultrasound.py
and
https://github.com/isaac-for-healthcare/i4h-workflows/blob/main/workflows/robotic_ultrasound/tests/test_simulation/test_sim_with_dds.py
Thanks.
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
|
test log after refractor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new decorator, @requires_rti, to remove duplicated RTI availability checks from our unit tests. The changes replace the inline try/except blocks and skipUnless decorators with a single, reusable decorator imported from helpers.py.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| workflows/robotic_ultrasound/tests/test_utils/test_visualization.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_simulation/test_ultrasound.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_simulation/test_target.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_simulation/test_sim_with_dds.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_simulation/test_franka.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_simulation/test_camera.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_simulation/test_base.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_policy_runner/test_pi0.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_holoscan_apps/test_realsense_camera.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_dds/test_subscriber.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/test_dds/test_publisher.py | Replaced inline RTI check with @requires_rti |
| workflows/robotic_ultrasound/tests/helpers.py | Added the helper decorator to encapsulate RTI checks |
Comments suppressed due to low confidence (1)
workflows/robotic_ultrasound/tests/helpers.py:20
- [nitpick] Consider caching the result of os.getenv('RTI_LICENSE_FILE') in a local variable to avoid calling it twice, improving readability and potentially performance.
def requires_rti(func):
KumoLiu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, LGTM.
Fixes #32
Description
Add the modifier
@requires_rtiso that we don't need to duplicate the following snippet