From c603124f6487604baee5aab590e1432e99570ca8 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Fri, 6 Feb 2026 13:00:01 -0600 Subject: feat(recording): rewrite device setup, fix video stop, update modeline icons Video stop fix: kill wf-recorder (producer) first on Wayland so ffmpeg gets clean EOF, then signal process group. Replaces sit-for with poll-based wait-for-exit. Fixes zero-byte output files. Device selection: rewrite quick setup to show all available mics with PulseAudio descriptions, auto-detect default sink monitor for system audio. Skip confirmation dialog, add Cancel option to mic list. Modeline: replace red dot emoji with nerd font icons (mic/camcorder). Rename quick-setup-for-calls to quick-setup, rebind C-; r s / C-; r S. 173 recording tests pass (was 165). --- .../test-video-audio-recording-process-cleanup.el | 219 ++++++++++++++++----- 1 file changed, 173 insertions(+), 46 deletions(-) (limited to 'tests/test-video-audio-recording-process-cleanup.el') diff --git a/tests/test-video-audio-recording-process-cleanup.el b/tests/test-video-audio-recording-process-cleanup.el index 10b17f2c..d1cd442c 100644 --- a/tests/test-video-audio-recording-process-cleanup.el +++ b/tests/test-video-audio-recording-process-cleanup.el @@ -1,11 +1,14 @@ ;;; test-video-audio-recording-process-cleanup.el --- Tests for recording process cleanup -*- lexical-binding: t; -*- ;;; Commentary: -;; Tests that verify proper cleanup of recording processes, especially -;; wf-recorder on Wayland which can become orphaned if not explicitly killed. +;; Tests that verify proper cleanup of recording processes. ;; -;; Unit tests verify pkill is called at the right times. -;; Integration tests verify no orphan processes remain after stop. +;; Key behaviors tested: +;; - SIGINT is sent to the process group (negative PID), not just the shell +;; - Video stop waits for the process to actually exit before declaring done +;; - On Wayland, wf-recorder is killed AFTER ffmpeg exits (safety net) +;; - On X11, pkill is not called (no wf-recorder involved) +;; - Starting a recording cleans up orphan wf-recorder from previous crashes ;;; Code: @@ -38,29 +41,89 @@ (setq cj/recording-mic-device nil) (setq cj/recording-system-device nil)) -;;; Unit Tests - Verify pkill is called +;;; Unit Tests - Signal delivery -(ert-deftest test-video-recording-stop-wayland-calls-pkill-wf-recorder () - "Test that stopping video on Wayland calls pkill to kill wf-recorder." +(ert-deftest test-video-recording-stop-sends-sigint-to-process-group () + "Test that stopping sends SIGINT to the process group via negative PID." (test-cleanup-setup) (unwind-protect (let ((fake-process (make-process :name "test-video" :command '("sleep" "1000"))) - (pkill-called nil) - (pkill-args nil)) + (signaled-pid nil) + (signaled-sig nil)) + (setq cj/video-recording-ffmpeg-process fake-process) + (cl-letf (((symbol-function 'cj/recording--wayland-p) (lambda () nil)) + ((symbol-function 'signal-process) + (lambda (pid sig) + (setq signaled-pid pid) + (setq signaled-sig sig) + 0)) + ((symbol-function 'cj/recording--wait-for-exit) + (lambda (_proc _timeout) t))) + (cj/video-recording-stop) + ;; Should send signal 2 (SIGINT) to negative PID (process group) + (should signaled-pid) + (should (< signaled-pid 0)) + (should (= signaled-sig 2))) + (ignore-errors (delete-process fake-process))) + (test-cleanup-teardown))) + +;;; Unit Tests - Wayland wf-recorder cleanup + +(ert-deftest test-video-recording-stop-wayland-kills-wf-recorder-before-signal () + "Test that on Wayland, pkill wf-recorder is called BEFORE signal-process. +Orderly pipeline shutdown requires killing the producer (wf-recorder) first +so ffmpeg sees EOF on its video input pipe and starts finalizing the file." + (test-cleanup-setup) + (unwind-protect + (let ((fake-process (make-process :name "test-video" :command '("sleep" "1000"))) + (call-order nil)) (setq cj/video-recording-ffmpeg-process fake-process) (cl-letf (((symbol-function 'cj/recording--wayland-p) (lambda () t)) ((symbol-function 'call-process) (lambda (program &rest args) (when (equal program "pkill") - (setq pkill-called t) - (setq pkill-args args)) + (push (cons 'pkill args) call-order)) + 0)) + ((symbol-function 'signal-process) + (lambda (_pid _sig) + (push 'signal call-order) 0)) - ((symbol-function 'interrupt-process) (lambda (_proc) nil))) + ((symbol-function 'cj/recording--wait-for-exit) + (lambda (_proc _timeout) t))) (cj/video-recording-stop) - (should pkill-called) - ;; Should call: pkill -INT wf-recorder - (should (member "-INT" pkill-args)) - (should (member "wf-recorder" pkill-args))) + ;; call-order is reversed (push adds to front), so last element is first call + (let ((order (reverse call-order))) + ;; First call should be pkill (kill producer first) + (should (consp (car order))) + (should (eq 'pkill (caar order))) + ;; signal-process should come after pkill + (should (eq 'signal (cadr order))))) + (ignore-errors (delete-process fake-process))) + (test-cleanup-teardown))) + +(ert-deftest test-video-recording-stop-wayland-calls-pkill-with-correct-args () + "Test that Wayland stop calls pkill -INT wf-recorder." + (test-cleanup-setup) + (unwind-protect + (let ((fake-process (make-process :name "test-video" :command '("sleep" "1000"))) + (pkill-args-list nil)) + (setq cj/video-recording-ffmpeg-process fake-process) + (cl-letf (((symbol-function 'cj/recording--wayland-p) (lambda () t)) + ((symbol-function 'call-process) + (lambda (program &rest args) + (when (equal program "pkill") + (push args pkill-args-list)) + 0)) + ((symbol-function 'signal-process) (lambda (_pid _sig) 0)) + ((symbol-function 'cj/recording--wait-for-exit) + (lambda (_proc _timeout) t))) + (cj/video-recording-stop) + ;; Should call pkill at least once with -INT wf-recorder + (should pkill-args-list) + (should (cl-some (lambda (args) + (and (member "-INT" args) + (member "wf-recorder" args))) + pkill-args-list))) (ignore-errors (delete-process fake-process))) (test-cleanup-teardown))) @@ -77,13 +140,16 @@ (when (equal program "pkill") (setq pkill-called t)) 0)) - ((symbol-function 'interrupt-process) (lambda (_proc) nil))) + ((symbol-function 'signal-process) (lambda (_pid _sig) 0)) + ((symbol-function 'cj/recording--wait-for-exit) + (lambda (_proc _timeout) t))) (cj/video-recording-stop) - ;; Should NOT call pkill on X11 (should-not pkill-called)) (ignore-errors (delete-process fake-process))) (test-cleanup-teardown))) +;;; Unit Tests - Start cleans orphans + (ert-deftest test-video-recording-start-wayland-kills-orphans () "Test that starting video on Wayland kills orphan wf-recorder processes." (test-cleanup-setup) @@ -102,7 +168,6 @@ (lambda (_name _buffer _command) (make-process :name "fake-video" :command '("sleep" "1000"))))) (cj/ffmpeg-record-video video-recordings-dir) - ;; Should call pkill to clean up orphans before starting (should pkill-called) (should (member "-INT" pkill-args)) (should (member "wf-recorder" pkill-args)))) @@ -123,10 +188,95 @@ (lambda (_name _buffer _command) (make-process :name "fake-video" :command '("sleep" "1000"))))) (cj/ffmpeg-record-video video-recordings-dir) - ;; Should NOT call pkill on X11 (should-not pkill-called))) (test-cleanup-teardown))) +;;; Unit Tests - Wait for exit + +(ert-deftest test-video-recording-stop-waits-for-process-exit () + "Test that stop waits for the process to exit instead of blind sit-for." + (test-cleanup-setup) + (unwind-protect + (let ((fake-process (make-process :name "test-video" :command '("sleep" "1000"))) + (wait-called nil) + (wait-timeout nil)) + (setq cj/video-recording-ffmpeg-process fake-process) + (cl-letf (((symbol-function 'cj/recording--wayland-p) (lambda () nil)) + ((symbol-function 'signal-process) (lambda (_pid _sig) 0)) + ((symbol-function 'cj/recording--wait-for-exit) + (lambda (_proc timeout) + (setq wait-called t) + (setq wait-timeout timeout) + t))) + (cj/video-recording-stop) + (should wait-called) + ;; Video uses 5 second timeout for MKV finalization + (should (= wait-timeout 5))) + (ignore-errors (delete-process fake-process))) + (test-cleanup-teardown))) + +(ert-deftest test-video-recording-stop-timeout-shows-warning () + "Test that video stop shows warning when process doesn't exit in time." + (test-cleanup-setup) + (unwind-protect + (let ((fake-process (make-process :name "test-video" :command '("sleep" "1000"))) + (warning-shown nil)) + (setq cj/video-recording-ffmpeg-process fake-process) + (cl-letf (((symbol-function 'cj/recording--wayland-p) (lambda () nil)) + ((symbol-function 'signal-process) (lambda (_pid _sig) 0)) + ((symbol-function 'cj/recording--wait-for-exit) + (lambda (_proc _timeout) nil)) ; Simulate timeout + ((symbol-function 'message) + (lambda (fmt &rest args) + (let ((msg (apply #'format fmt args))) + (when (string-match-p "did not exit" msg) + (setq warning-shown t)))))) + (cj/video-recording-stop) + (should warning-shown)) + (ignore-errors (delete-process fake-process))) + (test-cleanup-teardown))) + +(ert-deftest test-audio-recording-stop-timeout-shows-warning () + "Test that audio stop shows warning when process doesn't exit in time." + (test-cleanup-setup) + (unwind-protect + (let ((fake-process (make-process :name "test-audio" :command '("sleep" "1000"))) + (warning-shown nil)) + (setq cj/audio-recording-ffmpeg-process fake-process) + (cl-letf (((symbol-function 'signal-process) (lambda (_pid _sig) 0)) + ((symbol-function 'cj/recording--wait-for-exit) + (lambda (_proc _timeout) nil)) ; Simulate timeout + ((symbol-function 'message) + (lambda (fmt &rest args) + (let ((msg (apply #'format fmt args))) + (when (string-match-p "did not exit" msg) + (setq warning-shown t)))))) + (cj/audio-recording-stop) + (should warning-shown)) + (ignore-errors (delete-process fake-process))) + (test-cleanup-teardown))) + +(ert-deftest test-audio-recording-stop-waits-for-process-exit () + "Test that audio stop waits for the process to exit." + (test-cleanup-setup) + (unwind-protect + (let ((fake-process (make-process :name "test-audio" :command '("sleep" "1000"))) + (wait-called nil) + (wait-timeout nil)) + (setq cj/audio-recording-ffmpeg-process fake-process) + (cl-letf (((symbol-function 'signal-process) (lambda (_pid _sig) 0)) + ((symbol-function 'cj/recording--wait-for-exit) + (lambda (_proc timeout) + (setq wait-called t) + (setq wait-timeout timeout) + t))) + (cj/audio-recording-stop) + (should wait-called) + ;; Audio uses 3 second timeout for M4A finalization + (should (= wait-timeout 3))) + (ignore-errors (delete-process fake-process))) + (test-cleanup-teardown))) + ;;; Integration Tests - Verify actual process cleanup ;;; These tests require wf-recorder to be installed and a Wayland session @@ -150,21 +300,13 @@ This is an integration test that requires wf-recorder and Wayland." (test-cleanup-setup) (unwind-protect (let ((initial-count (test-cleanup--count-wf-recorder-processes))) - ;; Start recording (cj/ffmpeg-record-video video-recordings-dir) - (sit-for 1.0) ; Let it start - - ;; Should have at least one wf-recorder running + (sit-for 1.0) (should (> (test-cleanup--count-wf-recorder-processes) initial-count)) - - ;; Stop recording (cj/video-recording-stop) - (sit-for 1.0) ; Let cleanup complete - - ;; Should be back to initial count (no orphans) + (sit-for 1.0) (should (= (test-cleanup--count-wf-recorder-processes) initial-count))) (test-cleanup-teardown) - ;; Extra cleanup in case test failed mid-way (ignore-errors (call-process "pkill" nil nil nil "-INT" "wf-recorder")))) (ert-deftest test-integration-video-recording-start-cleans-orphans () @@ -179,28 +321,17 @@ This is an integration test that requires wf-recorder and Wayland." ;; Create an orphan wf-recorder (simulating a crash) (start-process "orphan-wf" nil "wf-recorder" "-c" "libx264" "-m" "matroska" "-f" "/dev/null") (sit-for 0.5) - (let ((orphan-pids (test-cleanup--get-wf-recorder-pids))) - (should orphan-pids) ; Verify orphan was created - - ;; Start recording - should clean up orphan + (should orphan-pids) (cj/ffmpeg-record-video video-recordings-dir) (sit-for 1.0) - - ;; The orphan PIDs should no longer exist (let ((current-pids (test-cleanup--get-wf-recorder-pids))) - ;; Old orphan should be gone (new recording process may exist) (dolist (orphan-pid orphan-pids) - ;; Check if old orphan is still in current pids - ;; It's OK if there's a NEW wf-recorder, just not the orphan (let ((still-running (member orphan-pid current-pids))) (should-not still-running)))) - - ;; Clean up the recording we started (cj/video-recording-stop) (sit-for 0.5))) (test-cleanup-teardown) - ;; Extra cleanup (ignore-errors (call-process "pkill" nil nil nil "-INT" "wf-recorder")))) (ert-deftest test-integration-video-recording-multiple-start-stop-cycles () @@ -212,17 +343,13 @@ This is an integration test that requires wf-recorder and Wayland." (test-cleanup-setup) (unwind-protect (let ((initial-count (test-cleanup--count-wf-recorder-processes))) - ;; Do 3 start/stop cycles (dotimes (_ 3) (cj/ffmpeg-record-video video-recordings-dir) (sit-for 0.5) (cj/video-recording-stop) (sit-for 0.5)) - - ;; Should be back to initial count - no accumulated orphans (should (= (test-cleanup--count-wf-recorder-processes) initial-count))) (test-cleanup-teardown) - ;; Extra cleanup (ignore-errors (call-process "pkill" nil nil nil "-INT" "wf-recorder")))) (provide 'test-video-audio-recording-process-cleanup) -- cgit v1.2.3