Repository navigation
fix(tts): make test_isolation_mode_in_process_vs_subprocess host-family-aware (#2696) - #2697
Open
rollroyces wants to merge 1 commit into
Open
rollroyces wants to merge 1 commit into
rollroyces wants to merge 1 commit into
Conversation
…ly-aware (debpalash#2696) The assertion was added in commit 8e2928c (Sep 9 2026) with the comment 'The pre-existing OmniVoice backend is in-process — sanity check.' A later change in tts_backend.py introduced _effective_backend_class, which intentionally routes the canonical omnivoice id to the killable OmniVoiceMPSSubprocessBackend on Apple Silicon hosts. The new behaviour is documented in list_backends() and the comment for the 'omnivoice-subprocess' lazy-registry entry; the test was simply forgotten. On every Linux/Windows CI runner (host_family != mps) the assertion still passes; on every Apple Silicon Mac it now reads 'isolated_mode = subprocess' and fails. Local contributors see a red CI on a single test that is not a real regression, which masks any genuine future failure of the same test. Make the assertion host-family-aware: 'in-process' on non-MPS hosts, 'subprocess' on MPS hosts. The behaviour is unchanged; only the test adapts to it. Detected by detect_host_caps() — the same signal the underlying production code uses. Fixes debpalash#2696.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to This change aligns the isolation-mode test with the host-dependent behavior described for the backend; no production behavior changes, so no material merge risk remains. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title uses Conventional Commit format with the required scope and includes issue reference #2696. It accurately describes the host-family-aware test fix. |
| Description check | |
The description includes the required summary, changes, type, testing, checklist, and release cadence sections. It explains the issue, scope, validation results, and confirms that runtime behavior is … |
| Linked Issues check | |
Issue #2696 requires a host-family-aware assertion for the existing omnivoice routing. The changed test uses detect_host_caps().family and expects subprocess for mps and in-process otherwise… |
| Out of Scope Changes check | |
The pull request changes only test_isolation_mode_in_process_vs_subprocess and its explanatory comment. The import and conditional assertion directly support issue #2696, and no unrelated production… |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. |
| Cross-Platform Default Parity | |
The pull request changes only tests/backend/services/test_tts_backend_registry.py. It does not change runtime defaults or default-mode behavior. The unchanged production code already routes `omnivoi… |
| I18n Completeness (21 Locales) | |
The pull request changes only tests/backend/services/test_tts_backend_registry.py. The diff contains no Electron UI code, no t('...') keys, and no user-facing UI strings that bypass i18n. The 21-l… |
| Local-First Guarantee | |
PASS — The PR changes only tests/backend/services/test_tts_backend_registry.py. It adds a local detect_host_caps() read; its implementation documents and performs no network call. The diff adds no… |
| Backward Compatibility | |
The pull request changes only tests/backend/services/test_tts_backend_registry.py. It adds a test-only detect_host_caps() import and changes the expected assertion for omnivoice; it does not mod… |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Contributor
|
[Low impact] The PR appears safe to merge.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tests/backend/services/test_tts_backend_registry.py::test_isolation_mode_in_process_vs_subprocessfails on every Apple Silicon Mac. The assertion was added before_effective_backend_class()started routing the canonicalomnivoiceid toOmniVoiceMPSSubprocessBackendon MPS hosts — a documented, intentional behaviour. The test was forgotten and the stale comment "The pre-existing OmniVoice backend is in-process — sanity check." stayed in.This PR makes the assertion host-family-aware:
in-processon non-MPS hosts,subprocesson MPS hosts. The behaviour is unchanged; only the test adapts to it.detect_host_caps()is the same signal the production code uses, so there is no new dependency.Fixes #2696.
Changes
tests/backend/services/test_tts_backend_registry.py(line ~276): replaced the unconditionalassert out["omnivoice"]["isolation_mode"] == "in-process"with a host-family-aware assertion. The pre-existingfake-subandhealthy-inprocassertions are unchanged. The replacement comment documents WHY the assertion is now branched, with a pointer to the production docstring intts_backend.py:list_backends().Why the test was wrong, not the code
tts_backend.py:_effective_backend_class()was added later than the test and intentionally routesomnivoicetoOmniVoiceMPSSubprocessBackendon Apple Silicon.list_backends(): "On MPS, the canonicalomnivoiceid already resolves to the killable OmniVoice sidecar."omnivoice-subprocesslazy-registry entry exists to keep the configurableOMNIVOICE_TTS_BACKEND=omnivoice-subprocessoverride working, not to expose a second runtime.subprocesson Apple Silicon,in-processeverywhere else — and the test must read the same signal the runtime reads.Type
Testing
uv run --frozen pytest 'tests/backend/services/test_tts_backend_registry.py::test_isolation_mode_in_process_vs_subprocess' -q→ 1 passed (this host is Apple Silicon)uv run --frozen pytest tests/backend/services/test_tts_backend_registry.py -q→ 15 passed (whole file, no regression)assert 'subprocess' == 'in-process'. Un-stashed, it passes again. The "fails without this change" criterion is satisfied.Checklist
mainfix(tts): …)rollroyceswith AI assistance; reviewed, accepted, and signed)The test now expects
omnivoiceisolation mode to be"subprocess"on MPS hosts and"in-process"on other hosts. This matches the existing MPS routing and corrects the stale assertion without changing runtime behavior. No specific merge risk is evident from the supplied changes.