Repository navigation
Migrate LibCI dds tests to pytest - #15104
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
Nir-Az
left a comment
Nir-Az
left a comment
There was a problem hiding this comment.
Auto-generated review by rs-agentic bot
Overall this is a solid migration of the DDS test suite from the legacy test-*.py runner to pytest. The fixture patterns, pytestmark usage, pytest_check soft-assertions, and pytest.raises replacements are well done. A few issues are flagged inline below.
| remote.wait_until_ready() | ||
|
|
||
| with test.closure( '1W:1R, 1 message' ): | ||
| remote.run( 'topic = flexible.writer( participant, "1w1r" )' ) |
There was a problem hiding this comment.
remote.run is given a dynamically constructed code string (concatenating JSON). Avoid executing constructed code strings; use explicit RPC/callable interfaces instead.
Details
✨ AI Reasoning
The change constructs Python code as strings and sends them to be executed remotely. Dynamically built code strings can hide behavior, making it harder to review and potentially enabling execution of unintended or malicious payloads. Even in tests, explicit function calls or structured RPC would be clearer and safer than composing code snippets. This concern arises from the added use of remote.run with concatenated strings that embed JSON payloads, which increases opacity of executed actions and could mask side effects.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| topic = flexible.reader( participant, "1w1r" ) | ||
| topic.wait_for_writers() | ||
| remote.run( 'topic.wait_for_readers()' ) | ||
| remote.run( 'topic.write( """' + sample1_json + '""" )' ) |
There was a problem hiding this comment.
remote.run executes a dynamically constructed string including sample1_json. Prefer explicit call interfaces to avoid hidden behavior.
Details
✨ AI Reasoning
The added code builds code snippets by concatenating JSON into triple-quoted Python source strings and executes them remotely. This dynamic execution pattern obscures the exact operations performed and is commonly used to hide malicious behavior, making review and auditing harder.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| topic.wait_for_writers() | ||
| remote.run( 'topic.wait_for_readers()' ) | ||
| remote.run( 'topic.write( """' + sample1_json + '""" )' ) | ||
| remote.run( 'topic.write( """' + sample2_json + '""" )' ) |
There was a problem hiding this comment.
remote.run executes constructed code containing sample2_json; prefer structured remote invocation instead of executing assembled source.
Details
✨ AI Reasoning
Another remote.run call is created by concatenating sample2_json into a string to be executed remotely. Dynamically executing such constructed source increases opacity and may hide unintended actions.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| r1.wait_for_writers() | ||
| r2.wait_for_writers() | ||
| remote.run( 'topic.wait_for_readers( 2 )' ) | ||
| remote.run( 'topic.write( """' + sample1_json + '""" )' ) |
There was a problem hiding this comment.
remote.run executes constructed code containing sample1_json for write; pass payloads as data to a remote API instead of embedding them into source strings.
Details
✨ AI Reasoning
The remote.run call composes a topic.write call with sample1_json embedded into the executed string. Embedding data into executable source increases the risk of hiding operations and makes intention less explicit during review.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
21e8fa4 to
44d657f
Compare
| with test.remote.fork( script=__file__, nested_indent=' S' ) as remote: | ||
| yield remote | ||
|
|
||
| def test_device_discovery(remote_server): |
There was a problem hiding this comment.
test_device_discovery is very long and mixes many responsibilities; split into smaller tests/fixtures to improve readability and reduce cognitive load.
Details
✨ AI Reasoning
1. Identify the code element: a single test function was added that contains many different responsibilities: setup of DDS topic reader, callback handlers, watcher setup, numerous broadcast/add/remove scenarios, and multiple inline helper classes and functions.
2. Does it harm maintainability? Yes — the function is long and mixes setup, orchestration, multiple test scenarios and detailed checks, increasing cognitive burden.
3. Is it appropriate here? Test functions benefit from clarity and small focused scopes; this one bundles many flows.
4. Would fixing meaningfully improve codebase? Yes — splitting into smaller fixtures/helpers and separate tests would improve readability and reduce fragility.
5. Is it a common pattern? Large tests exist but this exceeds a reasonable test size threshold.
6. Fix scope: can be addressed within PR by extracting helpers/fixtures and breaking into smaller tests.
Confidence rationale: computed per provided rubric (base 0.5, +0.4 length, +0.4 complexity), resulting confidence 1.0.
🔧 How do I fix it?
Break down long functions into smaller helper functions. Aim for functions under 60 lines with fewer than 10 local variables.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: out of scope for migration PR
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
out of scope for migration PR
| # underlying rs.context can be torn down (lets DDS tests have a clean | ||
| # FastDDS state). If the cache is gone, re-create a transient context | ||
| # and look up by serial number on first access. | ||
| if self._dev is None: |
There was a problem hiding this comment.
Device.handle re-acquire duplicates transient-context device-enumeration logic; consolidate into a shared helper to avoid duplicated enumeration loops.
Details
✨ AI Reasoning
A new lazy re-acquire block was added to Device.handle that constructs an rs.context and iterates ctx.query_devices() to find a device by serial number. That repeats the same device-enumeration logic used elsewhere (transient context + device loop) to locate devices. Duplication increases maintenance burden because future changes to enumeration (filters, settings, error handling) would need parallel updates in both places. The new block is narrowly scoped and not trivial logging, so consolidation into a shared helper would be beneficial.
🔧 How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| // Held by shared_ptr so the watcher's device-added/removed callbacks (which run on detached | ||
| // threads) keep the signal alive for their duration: a callback may still be in flight when this | ||
| // singleton is destroyed, and dereferencing it through a raw `this` would be a use-after-free. | ||
| std::shared_ptr< signal > const _callbacks = std::make_shared< signal >(); |
There was a problem hiding this comment.
Can you explain this part?
Real bug fixed? how discovered?
There was a problem hiding this comment.
Yes, this is a fix for a 'use-after-free' - the watcher's device-added/removed callbacks ran on a detached thread that captured this, if one is running when the watcher died, it touched freed memory and crashed
Real bug, discovered when running tests as part of this PR
| @@ -1181,10 +1181,11 @@ PYBIND11_MODULE(NAME, m) { | |||
| py::class_< dds_stream, std::shared_ptr< dds_stream > > stream_client_base( m, "stream", stream_base ); | |||
| stream_client_base // | |||
| .def( "open", &dds_stream::open ) | |||
There was a problem hiding this comment.
This doesn't need a GIL release?
why only close?
There was a problem hiding this comment.
when close runs it needs to join the callback thread, but if the callback thread is running, it needs GIL to finish the python callback - so it's stuck because close is holding it
this would deadlock as:
close holding GIL and waiting for reader thread
reader thread won't exit and wait for GIL
open does not wait for a thread join so not needed there
| // GIL release needed: close/stop_streaming join the reader thread and hang if it's inside a callback | ||
| .def( "close", &dds_stream::close, py::call_guard< py::gil_scoped_release >() ) | ||
| .def( "is_open", &dds_stream::is_open ) | ||
| .def( "start_streaming", &dds_stream::start_streaming ) |
There was a problem hiding this comment.
Not needed, and not really needed on close_streaming too - just added it for consistency with close
|
Sorry wrong pr |
Dismissing the review as accidently approved, please review when possible
| check.is_true( len(dropped_metadata) > 0 ) | ||
|
|
||
| with test.closure( 'Two threads, slow callback on image thread -> metadata builds up with eventual drops' ): | ||
| def test_two_threads_slow_callback_image_thread_metadata_buildup(): |
There was a problem hiding this comment.
Function names shorten the old names, but perhaps too short. Add comments explaining the expected logic, at least for multithreaded tests, as it is not always trivial
There was a problem hiding this comment.
added some docstrings
| def test_color_and_od_streams_have_distinct_types(remote_and_streams): | ||
| _, od_stream, color_stream, _, _ = remote_and_streams | ||
| # Verify the color stream is not mistaken for an object_detection stream | ||
| check.is_true( color_stream.type_string() != od_stream.type_string(), |
There was a problem hiding this comment.
If this will be tested in the fixture it can simplify the return values
There was a problem hiding this comment.
Also would diff less than original :-)
There was a problem hiding this comment.
moved to test in fixture, test_color_and_od_streams_have_distinct_types removed
| device = dds.device( participant, device_info ) | ||
| device.wait_until_ready() | ||
| try: | ||
| yield remote, device |
28f10e3 to
afdb6a2
Compare
|
|
||
| ############################################################################################# | ||
| # | ||
| def test_d435i_extrinsics(remote_and_context): |
There was a problem hiding this comment.
test_d435i_extrinsics is large and mixes discovery, profile selection, extrinsics calculation and many assertions; split responsibilities into smaller helper functions to reduce complexity.
Details
✨ AI Reasoning
1. The function's goal is to discover a broadcast device, gather sensor stream profiles, compute extrinsics between several profile pairs, and assert many numerical expectations. 2. It mixes device discovery, profile selection loops, extrinsics computation, and multiple detailed numeric assertions, increasing cognitive load. 3. This is test code but still a single test function with many responsibilities and branches that make it harder to scan and maintain. 4. Splitting responsibilities (discovery, profile-selection, and per-extrinsics assertions) into smaller helpers would improve readability and reduce the chance of errors when tests are extended.
🔧 How do I fix it?
Break down long functions into smaller helper functions. Aim for functions under 60 lines with fewer than 10 local variables.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
@AikidoSec ignore: Out of scope for migration PR
There was a problem hiding this comment.
✅ Based on your feedback, we ignored this issue because of the following reason:
Out of scope for migration PR
sysrsbuild-gh-agentic
left a comment
sysrsbuild-gh-agentic
left a comment
There was a problem hiding this comment.
Auto-generated review by rs-agentic bot
Reviewed the squashed/rebased commits on afdb6a28c4956551ad2203b492f0f48c05027e1f. The sentinel-nonce improvement in rspy/test.py, the use-after-free fix in rsdds-device-factory.cpp, the GIL-release fix in pyrealdds.cpp, and the --not-live guard on devices.query are all solid changes.
However, the squash/rebase has introduced a copyright-year regression and also revealed that many new files (listed below) still carry their legacy years. Per .github/copilot-instructions.md, every new source file must use Copyright(c) <current year> (2026).
Files still needing Copyright(c) 2026:
| File | Current year |
|---|---|
unit-tests/dds/adapter/pytest-depth.py |
2024 |
unit-tests/dds/pytest-control-reply.py |
2023-4 ← regression (was fixed in a pre-squash commit) |
unit-tests/dds/pytest-guid.py |
2023 |
unit-tests/dds/pytest-librs-connections.py |
2022-4 |
unit-tests/dds/pytest-librs-device-properties.py |
2022-4 |
unit-tests/dds/pytest-librs-extrinsics.py |
2022-4 |
unit-tests/dds/pytest-librs-formats-conversion.py |
2023-4 |
unit-tests/dds/pytest-librs-intrinsics.py |
2022-4 |
unit-tests/dds/pytest-librs-options.py |
2024 |
unit-tests/dds/pytest-no-metadata.py |
2023 |
unit-tests/dds/pytest-option-value.py |
2024 |
unit-tests/dds/pytest-options.py |
2024 |
unit-tests/dds/pytest-participant.py |
2022-4 |
unit-tests/dds/pytest-query-filter.py |
2025 |
unit-tests/dds/pytest-query-option.py |
2024 |
unit-tests/dds/pytest-streaming.py |
2022 |
unit-tests/dds/pytest-time.py |
2022 |
Note: renamed files (pytest-librs-context.py, pytest-md-syncer.py, pytest-stream-sensor-bridge.py) keep their original year — that is correct.
Change approved (from previous rs-agentic review): The TimeoutError fix in pytest-device-discovery.py (devices_added/devices_removed in the message) is present and correct. Copyright years for pytest-device-init.py, pytest-dds-metadata.py, and pytest-device-discovery.py were updated to 2026 and retained — change approved for those.
OhadMeir
left a comment
OhadMeir
left a comment
There was a problem hiding this comment.
Great work, lots of tests that needed to be migrated.
Tracked on: [RSDSO-21466]