ustreamer: video streaming - #872
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver_test.py (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the actual
prctlcontract, not just callability.This test would pass even if the hook used the wrong signal/option or stopped calling
prctl. Patch_IS_LINUXandctypes.CDLLso both platform branches and the Linux call contract are deterministic.Suggested test expansion
+import signal @@ def test_get_preexec_fn_linux(): - if not sys.platform.startswith("linux"): + if not sys.platform.startswith("linux"): assert _get_preexec_fn() is None else: assert callable(_get_preexec_fn()) + + +def test_get_preexec_fn_non_linux(): + with patch("jumpstarter_driver_ustreamer.driver._IS_LINUX", False): + assert _get_preexec_fn() is None + + +def test_get_preexec_fn_sets_pdeathsig_on_linux(): + with ( + patch("jumpstarter_driver_ustreamer.driver._IS_LINUX", True), + patch("jumpstarter_driver_ustreamer.driver.ctypes.CDLL") as mock_cdll, + ): + libc = mock_cdll.return_value + libc.prctl.return_value = 0 + + preexec_fn = _get_preexec_fn() + assert callable(preexec_fn) + preexec_fn() + + libc.prctl.assert_called_once_with(1, signal.SIGTERM, 0, 0, 0)As per coding guidelines,
**/jumpstarter-driver-*/jumpstarter_driver_*/*_test.py: “Add comprehensive tests indriver_test.pyfile within the driver package.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver_test.py` around lines 22 - 27, The `_get_preexec_fn` test only checks callability, so it can miss regressions in the Linux `prctl` setup. Update `test_get_preexec_fn_linux` in `driver_test.py` to deterministically cover both branches by patching `_IS_LINUX`, and mock `ctypes.CDLL` so you can assert the Linux path actually calls `prctl` with the expected option and signal. Keep the non-Linux branch asserting `None`, and make the Linux branch verify the returned hook invokes `prctl` rather than only being callable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/client.py`:
- Around line 44-70: The _run_server helper is catching all BaseException
failures and masking real startup errors with the generic stopping message.
Update the exception handling around client.portal.call(serve) to catch only
KeyboardInterrupt, and let other exceptions from serve(), runner.setup(), or
site.start() propagate so actual failures remain visible; keep the cleanup in
the finally block via runner.cleanup.
- Around line 50-54: Use the public TCPSite API instead of reaching into aiohttp
internals when building the URL in the client startup flow. In the section that
starts the site and computes actual_port in client.py, replace the direct use of
site._server.sockets[0] with site.port after await site.start(), and keep the
url construction based on that value.
In
`@python/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver.py`:
- Around line 46-56: Keep _get_preexec_fn() minimal by removing libc loading and
logger.warning calls from set_pdeathsig; that forked-child setup should only
perform the prctl(PR_SET_PDEATHSIG, signal.SIGTERM, ...) call and avoid any
logging or other risky work before exec. Change set_pdeathsig to raise an
exception on failure instead of swallowing errors, and handle/report the failure
in the parent side where the subprocess is created so the child path stays safe
and simple.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver_test.py`:
- Around line 22-27: The `_get_preexec_fn` test only checks callability, so it
can miss regressions in the Linux `prctl` setup. Update
`test_get_preexec_fn_linux` in `driver_test.py` to deterministically cover both
branches by patching `_IS_LINUX`, and mock `ctypes.CDLL` so you can assert the
Linux path actually calls `prctl` with the expected option and signal. Keep the
non-Linux branch asserting `None`, and make the Linux branch verify the returned
hook invokes `prctl` rather than only being callable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9e07d8f-a7b9-439e-9bd0-14cc15dae188
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/client.pypython/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver.pypython/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver_test.py
e5c4b02 to
f12e774
Compare
j video Usage: j video [OPTIONS] COMMAND [ARGS]... Video capture and streaming Options: --help Show this message and exit. Commands: snapshot Save a single snapshot to file state Show video source state stream Start local MJPEG streaming server Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: grok-4.5
Uh oh!
There was an error while loading. Please reload this page.