MacOS: enable all tests to achieve parity with Windows tests#1676
MacOS: enable all tests to achieve parity with Windows tests#1676sandboxcoder wants to merge 1 commit intostagingfrom
Conversation
a8900f6 to
e076193
Compare
168cab5 to
962da50
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce macOS CI flakiness and improve cross-platform test parity by enabling previously skipped macOS tests, improving test/worker cleanup, and adding better CI diagnostics (logs, timeouts, retries).
Changes:
- Enable/adjust macOS execution for multiple OSN test suites (removing darwin skips, adding CI-aware skips + logging where GPU is required, improving cleanup with try/finally).
- Improve stability/diagnostics: longer signal timeouts + warning telemetry, lower default video FPS, more retryable timeout patterns, and explicit per-test log filename support.
- CI workflow improvements: macOS runner upgrade to macos-15,
fail-fast: false, setCIenv var, suppress noisy server stdout/stderr by default, and upload OBS logs as artifacts.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/osn-tests/util/obs_handler.ts | Adds log filename arg to init, expands retryable timeout detection, increases signal timeout w/ warning, lowers default FPS, adds listener removal helper. |
| tests/osn-tests/src/test_osn_simple_streaming.ts | Removes macOS skips to run streaming tests on darwin. |
| tests/osn-tests/src/test_osn_simple_replayBuffer.ts | Removes macOS skips to run replay buffer tests on darwin. |
| tests/osn-tests/src/test_osn_simple_recording.ts | Wraps resource usage in try/finally and removes macOS skips; adds proper callback removal on teardown. |
| tests/osn-tests/src/test_osn_input.ts | Removes macOS skip for adding video filters. |
| tests/osn-tests/src/test_osn_enhanced_broadcasting_simple_streaming.ts | Makes second video context CI-aware and adds CI skip logging for GPU-required tests; removes one delay-related test. |
| tests/osn-tests/src/test_osn_enhanced_broadcasting_advanced_streaming.ts | Makes second video context CI-aware and adds CI skip logging for GPU-required tests. |
| tests/osn-tests/src/test_osn_dual_output.ts | Removes macOS skips and adds try/finally cleanup patterns for dual output tests. |
| tests/osn-tests/src/test_osn_advanced_streaming.ts | Removes macOS skips for advanced streaming tests. |
| tests/osn-tests/src/test_osn_advanced_replayBuffer.ts | Adds try/finally cleanup and removes macOS skips. |
| tests/osn-tests/src/test_osn_advanced_recording.ts | Adds try/finally cleanup and removes macOS skips. |
| tests/osn-tests/src/test_nodeobs_service.ts | Removes macOS skips for NodeObs service integration tests. |
| tests/osn-tests/src/test_nodeobs_autoconfig.ts | Removes macOS skip (and a suite-level timeout line). |
| package.json | Increases CI test retries from 2 to 3. |
| obs-studio-server/source/osn-replay-buffer.cpp | Switches replay buffer save implementation to invoke output handler proc call instead of hotkey enumeration. |
| obs-studio-server/source/nodeobs_api.cpp | Adds optional log filename prefix support for server log file naming. |
| obs-studio-client/source/worker-signals.hpp | Prevents orphan worker from polling new sessions on InvalidReference; avoids negative sleep under overload. |
| obs-studio-client/source/nodeobs_service.cpp | Avoids negative sleep under overload. |
| obs-studio-client/source/nodeobs_autoconfig.cpp | Avoids negative sleep under overload. |
| obs-studio-client/source/nodeobs_api.cpp | Adds optional log filename argument passthrough to server init call. |
| obs-studio-client/source/controller.cpp | Attempts to kill prior obs64 instances, suppresses server stdout/stderr based on env var, adds waitpid after SIGKILL. |
| .github/workflows/main.yml | Upgrades macOS arm runner to macos-15, disables fail-fast for mac tests, sets CI env + suppress logs, uploads OBS logs artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
551f455 to
4abec8a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
17a3d7d to
451f187
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
08810b8 to
77a3c7f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
81e2a5e to
c9597ab
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
544547a to
176b13d
Compare
* enable recording, streaming, dual-output tests on MacOS * set CI env var for MacOS github runner for obs_handler.ts * fix warning we cant assign null: `let secondContext: osn.IVideo = null;` * do not invoke `AddVideoContext` on CI builds in enhanced broadcasting tests since these are disabled. * main.yml: upgrade to macos-15 for arm64 ARCH (same OS Intel ARCH is using). * main.yml: upload OBS logs to troubleshoot unit tests. main.yml: set fail-fast to false * turn off fail-fast so the tests can continue running which will fix issue where if macos-15-intel fails, macos-arm64 is cancelled before it can finish running. osn_handler: log time delta for long duration * add warning when signal received took longer then expectedDeadline obs_handler: assign explicit log filename for tests Fix orphaned worker thread: The WorkerSignals::worker keeps polling Query. When a test fails before calling destroy(), the server-side recording gets freed when OBS shuts down, but the orphaned worker thread continues. When the next test file creates a fresh OBS connection, Controller::GetInstance().GetConnection() now returns the new session's connection. The orphaned worker sends Query(oldUID) to the new server, which has no knowledge of that uid → "Recording reference is not valid." Remove source message listener when test ends * stops worker thread properly * test_osn_simple_recording: remove lamda to enable timeout Tests: simple_recording,test_osn_advanced_replayBuffer,test_osn_advanced_recording,dual_output - wrap with try/finally * update any test that starts a worker thread via create() to properly invoke destroy() within a finally block to guanrantee the worker thread is stopped even if the test fails. Prevents orphaned worker threads from invoking Query() every 33ms on the new IPC connection created by other tests. guarantees cleanup even when tests throw so they tests will clean up proactively, and any orphaned workers that slip through will stop themselves rather than polluting subsequent test runs. validate totalSleepMS does not have a negative value When the IPC call takes longer than sleepIntervalMS on an overloaded CI runner), sleepIntervalMS - dur.count() produces a negative result that wraps to a huge size_t value, causing the worker to sleep for an effectively infinite duration. After that, no more signals are ever polled — any test waiting for a signal hits the 30-second timeout. do not share stdout/stderr with parent process On macOS, the server is spawned with posix_spawnp with NULL for file_actions, which means it inherits the parent's stdout/stderr. On Windows this doesn't happen because CreateProcessW uses DETACHED_PROCESS | CREATE_NO_WINDOW, which cuts the server off from the parent's console entirely. obs_handler: add more retryable timeouts for flaky tests advanced-recording fix * Removed extraneous set_video_mix since audio encoders don't have a video mix which produced the "encoder 'track1' is not a video encoder" warning * Reduce defaultVideoContext from 1280x720@60fps to 1280x720@30fps to reduce CPU load on the slow x86_64 CI machine. osn-replay-buffer: invoke output handler to save * makes test more reliable * follows similar pattern used by OBS::ReplayBufferSave
176b13d to
d39796b
Compare
obs_handler.tsso it knows when its' running on CI github runner. I do not know why Windows CI machine does not need this step it somehow hasCIenv var defined somewhere. It would have been nice to reuse what Windows is doing?let secondContext: osn.IVideo = null;AddVideoContexton CI builds in enhanced broadcasting tests since these are disabled.macos-14which seemed more unstable.SUPPRESS_STREAMLABS_OBS_LOGS, I added to main.yml to allow the obs64 child process to attach to stdout/stderr like it used to- if one likes to see the verbose backend spew which can be helpful for troubleshooting CI-only issues...test_osn_dual_outputmost of all (testStart Dual Output with recording and scene itemsandStart Dual Output with advanced recording and audio scene items-record stop signal timeoutcase). For now, bumping retries to 3 helps but this will be revisited.main.yml: set fail-fast to false
osn_handler: log time delta for long duration
obs_handler: assign explicit log filename for tests
Fix orphaned worker thread:
The
WorkerSignals::workerkeeps pollingQuery. When a test fails before callingdestroy(), the server-side recording gets freed when OBS shuts down, but the orphaned worker thread continues. When the next test file creates a fresh OBS connection, Controller::GetInstance().GetConnection() now returns the new session's connection. The orphaned worker sends Query(oldUID) to the new server, which has no knowledge of that uid → "Recording reference is not valid." Note: from my research, it is best to use good try/catch/finally pattern on the caller side to invokedestroy()to ensure the Worker state variables are properly reset unless we add something more extensive on backend to defend against orphaned worker threads.Remove source message listener when test ends
removeSourceMessageListenerTests: simple_recording,test_osn_advanced_replayBuffer.ts,test_osn_advanced_recording,dual_output - wrap with try/catch/finally
guarantees cleanup even when tests throw so these tests will clean up proactively, and any orphaned workers that slip through will stop themselves rather than polluting subsequent test runs.
validate totalSleepMS does not have a negative value
When the IPC call takes longer than sleepIntervalMS on an overloaded CI runner,
sleepIntervalMS - dur.count()produces a negative result that wraps to a huge size_t value, causing the worker to sleep for an effectively infinite duration. After that, no more signals are ever polled — any test waiting for a signal hits the 30-second timeout.do not share stdout/stderr with parent process
On macOS, the server is spawned with
posix_spawnpwith NULL for file_actions, which means it inherits the parent's stdout/stderr. On Windows this doesn't happen because CreateProcessW uses DETACHED_PROCESS | CREATE_NO_WINDOW, which cuts the server off from the parent's console entirely. This merely helps declutter the test logs. OBS logs are now uploaded. AddedSUPPRESS_STREAMLABS_OBS_LOGSenvironment variable to switch this back to legacy if desired. Locally,SUPPRESS_STREAMLABS_OBS_LOGSis false to maintain legacy behavior so you can still see backend spew which can be helpful for debugging imo.obs_handler: add more retryable timeouts for flaky tests
obs_handler: Reduce defaultVideoContext from 1280x720@60fps to 1280x720@30fps to reduce CPU load on the slow x86_64 CI machine. I did not profile the exact benefit however....
osn-replay-buffer: invoke output handler to save
ReplayBufferSaveDescription
Motivation and Context
Enable all tests to run on MacOS
How Has This Been Tested?
Ran tons of builds.
Types of changes
Checklist: