Clean up e2e test scripts ahead of open-sourcing#1
Conversation
Artifacts were written to a non-existent repo-root e2e/ (missing tests/), bypassing .gitignore; smoke.sh ran sender.py which needs nvh264enc + a RealSense camera + pyrealsense2 (absent from pixi), so it failed on any clone. - smoke.sh/gpu_sink.sh: point E2E_DIR under tests/e2e (was repo-root e2e/) - smoke.sh: run sender_sw.py (software x264enc), drop dead E2E_HEADLESS var - receiver.py: decode via avdec_h264 and use fakesink for headless runs - ipc_two_procs/run.sh: fix temp-dir cleanup test (-f -> -d) - .gitignore: ignore stage-2 frames_msid/ output - README: fix command/artifact paths and sender script name
…cripts The manual e2e scripts accumulated copy-paste cruft: a never-called test source, noqa-suppressed dead imports, '# not used' comments on a TURN var that is actually passed to the constructor, and a sys.path hack pointing at the wrong dir. - sender.py: drop unused create_test_video_source() and wh/fr; trim dead imports - receiver.py/sender_sw.py: drop gi prelude (init_gst handles require_version) and dead imports - sender_sw.py: drop broken parents[1] sys.path hack; fix docstring path - receiver_msid_save.py: decode via avdec_h264 to match its CPU-decode comment; drop unused fmt; merge duplicate reshape branches - appsink_msid_e2e.py: drop redundant sys.path injection (editable install) - gpu_sink_save.py: drop unused h,w unpack; fix docstring sender path - inference_buffer_v2_ipc.py: surface child_error instead of crashing on assert
- gpu_sink/core.py: remove the copy_frame ctor param and its dead 'if not self.copy_frame' branch; frames were always copied regardless of the flag.
- sender/core.py: fix stale ICE comment ('>3s' -> '>1s') to match the 1.0s threshold the code and unit tests actually use.
WebRTCReceiver renames the appsink to the incoming stream's msid, so binding by GpuFrameSink's original name never matched and no frames were pulled (gpu_sink.sh timed out). Discover the appsink by element type instead, the way appsink_msid_e2e.py already does.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR removes the optional ChangesCore GStreamer behavior
E2E test infrastructure and decoder migration
🎯 3 (Moderate) | ⏱️ ~20 minutes
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4975ce571d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| output_format: str = "RGBA", | ||
| queue_size: int = 4, | ||
| drop_when_full: bool = True, | ||
| copy_frame: bool = True, | ||
| ) -> None: |
There was a problem hiding this comment.
The public design/usage docs still advertise GpuFrameSink(..., copy_frame=True) and explicitly say copy_frame=False is accepted for compatibility, but this signature now rejects that keyword with TypeError. Because the option was already a no-op, callers following the documented API (or existing external users) can be kept working by accepting and ignoring the parameter while continuing to always copy frames.
Useful? React with 👍 / 👎.
2 participants
What & why
We're getting the repo ready to open-source, so I went through
tests/— especially the manualtests/e2e/scripts — looking for dead code, comment/code mismatches, and logic that looks clever but doesn't actually do anything.The unit tests (
test_*.py+_fakes.py) turned out to be clean, so they're left untouched. The problems were concentrated intests/e2e/, where scripts had been copy-pasted and only half-edited — leaving behind wrong paths, contradictory comments, and dead functions. Most importantly,smoke.shcouldn't run on a fresh clone at all.Changes
Grouped into focused commits so they're easy to review one at a time:
fix(e2e)— correct output paths & make smoke runnable without GPU/cameraE2E_DIRpointed at a non-existent repo-roote2e/(missingtests/), so artifacts landed outside the.gitignored location and polluted the working tree. Now points attests/e2e.smoke.shransender.py, which needsnvh264enc(GPU) + a physical RealSense camera +pyrealsense2(not even in the pixi env). Switched tosender_sw.py(software x264enc).receiver.pynow decodes viaavdec_h264and usesfakesink, so it runs truly headless on any machine.ipc_two_procs/run.sh: temp-dir cleanup tested-fon a directory (always false, so it leaked) → fixed to-d.E2E_HEADLESSenv var.refactor(e2e)— remove dead code & misleading commentscreate_test_video_source(),# noqa-suppressed dead imports,# not usedcomments on aTURNvar that's actually passed to the constructor, and asys.pathhack pointing at the wrong dir.receiver_msid_save.pyclaimed# Keep CPU decodebut usednvh264dec; it now actually decodes on CPU.refactor(gpu_sink)— drop a no-op param & fix a stale commentGpuFrameSink'scopy_framector param never changed behavior (frames were always copied regardless); removed it and the dead branch.>3s→>1s) that contradicted both the code and the unit test.fix(e2e)— bind gpu_sink_save to the receiver's msid-renamed appsinkWebRTCReceiverrenames the appsink to the incoming stream's msid, butgpu_sink_save.pybound byGpuFrameSink's original name → never matched →gpu_sink.shalways timed out on any machine. It now discovers the appsink by element type, the wayappsink_msid_e2e.pyalready does.Verification
Also confirmed the repo root no longer grows a stray
e2e/directory.Notes
_fakes.pyare intentionally untouched — they're clean.sender.pyis kept as the RealSense hardware demo (only its dead code was removed), not wired into the headless smoke flow.Summary by CodeRabbit
Bug Fixes
Documentation
Tests