Conversation
- Migrated from setup.py to pyproject.toml with dynamic versioning. - Updated GitHub workflows to use gh-automations reusable workflows. - Added ServiceInstaller to PlaybackService for pip plugin installation support. - Added comprehensive unit test suite, reaching 98% coverage of ovos_audio. - Added extended E2E test suite for AudioService and PlaybackService. - Added mandatory documentation: QUICK_FACTS.md, FAQ.md, MAINTENANCE_REPORT.md, AUDIT.md, SUGGESTIONS.md, and docs/ index. - Added LICENSE file.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughRefactors CI to delegate to external workflows, pins action versions, adds pyproject-based packaging and exported version, removes setup.py, adds Apache-2.0 LICENSE, deletes some in-repo workflows, and introduces extensive documentation plus many new unit and end-to-end tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Hello! The automated checks have been performed. 👋I've aggregated the results of the automated checks for this PR below. 🔨 Build TestsEnsuring the foundation is solid for these changes. 🏛️ ✅ All versions pass
⚖️ License CheckChecking the paperwork! Everything seems in order. 📂 ✅ No license violations found (41 packages). License distribution: 10× MIT License, 6× Apache Software License, 6× MIT, 5× Apache-2.0, 2× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +6 more Full breakdown — 41 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 📋 Repo HealthI've checked the repo's reflexes (aka build speed). ⚡ ✅ All required files present. Latest Version: ✅ 🏷️ Release PreviewThe release preview report is now officially ready. 📁 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔗 Downstream DependentsBeep boop! Standard processing sub-routine complete. 🦾 ovos_audio==1.1.1 📊 CoverageThe coverage detectives have finished their sweep. 🕵️♀️ ✅ 98.3% total coverage Per-file coverage (9 files)
Full report: download the 🔍 LintThe data has been harvested! Check the findings below. 🌾 ❌ ruff: issues found — see job log 🔒 Security (pip-audit)The security scan is now complete. 🏁 ✅ No known vulnerabilities found (60 packages scanned). Keeping the code clean, one PR at a time ✨ |
ovos_utils.skill_installer was added by accident in the modernisation commit. It only exists in ovos-utils>=0.8.5a1 (not yet stable), causing ModuleNotFoundError at import time on all CI Python versions. Remove the import and all pip_installer usage from PlaybackService. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (12)
.github/workflows/release_workflow.yml (1)
12-12: Consider pinning external workflow to a specific commit or tag.Using
@masterfor external workflows can lead to unexpected breakages if the upstream workflow changes. Consider pinning to a specific SHA or tag for reproducibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release_workflow.yml at line 12, The workflow currently references an external workflow with the pinned ref "uses: TigreGotico/gh-automations/.github/workflows/publish-alpha.yml@master"; update that reference to a fixed, immutable ref (a commit SHA or a release tag) instead of `@master` to ensure reproducible runs—replace the "@master" suffix with a specific SHA (e.g., "@<commit-sha>") or a tagged release (e.g., "@vX.Y.Z") and commit the updated string so the external workflow cannot change unexpectedly.docs/index.md (1)
20-31: Consider adding language specifier to diagram code blocks.The architecture diagram and package layout blocks (lines 20 and 68) don't have language specifiers. While this is acceptable for plain text diagrams, adding
textorplaintextwould satisfy markdown linters.📝 Optional fix
-``` +```text MessageBus │🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.md` around lines 20 - 31, The markdown code fences for the architecture diagram and package layout in docs/index.md are missing a language specifier; update the opening triple-backtick lines for those two blocks to use a plain-text specifier (e.g., ```text or ```plaintext) so linters recognize them as code blocks, leaving the block contents unchanged—look for the diagram block that starts with the MessageBus ASCII art and the package layout block further down and add the language tag to their existing ``` fences..github/workflows/build_tests.yml (1)
11-14: Consider pinning external workflow to a stable ref.Using
@devfor the external workflow reference (OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev) means any change to that dev branch could unexpectedly break this workflow. Consider pinning to a release tag or commit SHA for more predictable CI behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_tests.yml around lines 11 - 14, The workflow currently references an external workflow via the line "uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev"; change this to pin to a stable release tag or specific commit SHA (e.g., replace "@dev" with a release tag like "@v1.2.0" or a commit SHA) to avoid unexpected breaks, and ensure any related inputs (e.g., "test_path") remain unchanged; update the "uses" value accordingly so CI uses the fixed ref.pyproject.toml (1)
26-26: Minor: Using>instead of>=for ovoscope.The version specifier
ovoscope>0.10.0excludes version 0.10.0 exactly. If 0.10.0 is acceptable, consider using>=0.10.1or>=0.10.0for clarity. This may be intentional if 0.10.0 has a known issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 26, The dependency line "ovoscope>0.10.0" excludes version 0.10.0; update the version specifier to use a >= operator (e.g., change "ovoscope>0.10.0" to "ovoscope>=0.10.0" or "ovoscope>=0.10.1" if 0.10.0 is known-bad) in pyproject.toml so the intended patch/minor versions are included; ensure you modify the exact string "ovoscope>0.10.0" to the chosen >= form.test/unittests/test_playback_play.py (2)
184-189: Prefix unusedmock_procwith underscore.The unpacked variable is not used in this test.
Proposed fix
def test_play_exception_calls_on_end(self): - t, mock_proc = self._setup_thread_for_play() + t, _mock_proc = self._setup_thread_for_play() t._processing_queue = True with patch("ovos_audio.playback.play_audio", side_effect=RuntimeError("crash")): t._play() t.on_end.assert_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_playback_play.py` around lines 184 - 189, In test_play_exception_calls_on_end, the unpacked variable mock_proc returned from _setup_thread_for_play is unused; change the unpack to use an underscored name (e.g. _, or _mock_proc) so the test doesn't complain about an unused variable—update the line that calls self._setup_thread_for_play() in test_play_exception_calls_on_end accordingly and keep the rest of the test (t, _ = self._setup_thread_for_play() or t, _mock_proc = ...) unchanged.
221-231: Remove unused variableoriginal_get.The variable is assigned but never used.
Proposed fix
- original_get = t.queue.get - def boom_then_stop(timeout=None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_playback_play.py` around lines 221 - 231, Remove the unused assignment original_get = t.queue.get; simply delete that line so only the replacement function boom_then_stop is assigned to t.queue.get. Ensure you keep the replacement function (boom_then_stop), the call_count increment, setting t._terminated, raising queue.Empty, and the final t.run() call unchanged.test/unittests/test_service_handlers.py (1)
269-282: Remove unused variablecfg.The variable
cfgis assigned but never used in this test.Proposed fix
def test_disable_fallback_skips_fallback_reload(self): svc = _make_svc() svc.disable_fallback = True svc._tts_hash = "same" - cfg = {"module": "dummy", "dummy": {}} - import json + import json svc._tts_hash = hash(json.dumps({}, sort_keys=True))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_service_handlers.py` around lines 269 - 282, Remove the unused local variable cfg in the test_disable_fallback_skips_fallback_reload function: locate the test function (test_disable_fallback_skips_fallback_reload) where cfg = {"module": "dummy", "dummy": {}} is assigned but never referenced, delete that assignment so only svc setup, _tts_hash setup and the patched calls to Configuration and TTSFactory remain, ensuring the rest of the test (svc._maybe_reload_tts() and mock_factory.create.assert_not_called()) is unchanged.test/unittests/test_transformers.py (1)
119-130: Consider prefixing unusedctxwith underscore.The unpacked
ctxvariable is unused. Prefix with underscore to indicate intentional discard.Proposed fix
- result, ctx = svc.transform("hello", context={}) + result, _ctx = svc.transform("hello", context={}) self.assertEqual(result, "modified hello") def test_transform_plugin_exception_does_not_propagate(self): def bad_transform(dialog, context=None): raise ValueError("oops") svc = self._make_with_plugin(bad_transform) # Should not raise, returns original dialog - result, ctx = svc.transform("hello", context={}) + result, _ctx = svc.transform("hello", context={}) # plugin raised, dialog unchanged self.assertEqual(result, "hello")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_transformers.py` around lines 119 - 130, The test unpacks an unused context variable as ctx in test_transform_plugin_exception_does_not_propagate; change the unpack to use a prefixed underscore (e.g., _ctx) to mark it as intentionally unused. Locate the test function test_transform_plugin_exception_does_not_propagate, the bad_transform helper and the call to svc.transform("hello", context={}) and update the assignment from "result, ctx = svc.transform(...)" to "result, _ctx = svc.transform(...)" (and similarly rename any other unused unpacked ctx in this test file).docs/transformers.md (1)
5-19: Add a language specifier to the fenced code block.The ASCII diagram code block lacks a language specifier. Use
textorplaintextto satisfy markdown linters.Proposed fix
-``` +```text speak event │ ▼🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/transformers.md` around lines 5 - 19, The fenced ASCII diagram in docs/transformers.md has no language specifier; update the opening triple-backtick for the block that contains the "speak event → DialogTransformersService → TTS plugin → TTSTransformersService → PlaybackThread" diagram to include a language tag such as text or plaintext (e.g., change ``` to ```text) so markdown linters recognize it as a plain-text block.test/unittests/test_playback_thread.py (1)
112-124: Remove or use theemitted_typesvariable.The variable
emitted_typesis computed but never used in assertions. Either add an assertion or remove the computation.Proposed fix - Option 1: Add meaningful assertion
def test_begin_audio_ocp_cork(self): bus = MagicMock() t = _make_thread(bus=bus) cfg = {"tts": {"ocp_cork": True}} msg = MagicMock() msg.forward.return_value = MagicMock() with patch("ovos_audio.playback.Configuration", return_value=cfg): t.begin_audio(message=msg) - emitted_types = [c[0][0].msg_type if hasattr(c[0][0], 'msg_type') else str(c[0][0]) - for c in bus.emit.call_args_list] - # bus.emit was called at least once - self.assertTrue(bus.emit.called) + # Verify cork message was emitted + emitted = [c[0][0] for c in bus.emit.call_args_list] + cork_emitted = any(hasattr(m, 'msg_type') and m.msg_type == "ovos.common_play.cork" + for m in emitted) + self.assertTrue(cork_emitted or bus.emit.called)Proposed fix - Option 2: Remove unused variable
def test_begin_audio_ocp_cork(self): bus = MagicMock() t = _make_thread(bus=bus) cfg = {"tts": {"ocp_cork": True}} msg = MagicMock() msg.forward.return_value = MagicMock() with patch("ovos_audio.playback.Configuration", return_value=cfg): t.begin_audio(message=msg) - emitted_types = [c[0][0].msg_type if hasattr(c[0][0], 'msg_type') else str(c[0][0]) - for c in bus.emit.call_args_list] # bus.emit was called at least once self.assertTrue(bus.emit.called)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_playback_thread.py` around lines 112 - 124, The test computes emitted_types but never uses it; either remove the unused emitted_types calculation or replace it with a concrete assertion about bus.emit results. Update test_begin_audio_ocp_cork to delete the emitted_types list comprehension (leaving the existing self.assertTrue(bus.emit.called) assertion), or instead assert a specific emitted event by inspecting bus.emit.call_args_list (e.g., check the first emitted object's msg_type) inside the same test and keep emitted_types if you add that assertion; modify references around _make_thread and t.begin_audio accordingly.test/unittests/test_audio_service_extended.py (1)
415-426: Consider extracting repeatedcapture_emitpattern into a reusable helper.This pattern appears three times (lines 415-426, 432-443, 452-462). While acceptable for test code, extracting it would reduce duplication.
♻️ Optional: Extract shared helper
def _capture_emit_wrapper(bus): """Create an emit wrapper that captures messages.""" captured = [] orig_emit = bus.emit def capture_emit(m): captured.append(m) try: orig_emit(m) except Exception: pass # Suppress errors in test capture bus.emit = capture_emit return captured🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_audio_service_extended.py` around lines 415 - 426, Extract the repeated capture_emit pattern into a reusable helper function (e.g., _capture_emit_wrapper) and use it wherever svc.bus.emit is wrapped; the helper should accept the bus (or svc.bus), save the original emit, replace it with a wrapper that appends messages to a captured list and calls the original emit inside a try/except suppressing exceptions, then return the captured list so tests can assert on it instead of duplicating the capture logic in multiple places (replace the three inline blocks around svc._list_backends calls with calls to this helper).test/unittests/test_playback_service.py (1)
21-50: Fragile but intentional: Source inspection for config defaults.The source-inspection approach (
inspect.getsource) is unusual and will break if the source format changes. However, the docstring indicates this is intentional to catch when "TODO-flagged defaults" are modified. Consider adding a comment explaining this is a "canary test" to alert maintainers when defaults change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_playback_service.py` around lines 21 - 50, This test class intentionally uses inspect.getsource in _read_defaults_from_source and string assertions in tests (TestPlaybackServiceConfigDefaults, _read_defaults_from_source, test_audio_enabled_default_is_true, test_disable_ocp_default_is_false) as a canary to detect changes to TODO-flagged defaults; update the file by adding a short clarifying comment at the top of the class (or above _read_defaults_from_source) stating that the fragile source-inspection is intentional and used to alert maintainers when default config literals change, so future readers understand this is not an accidental brittle test but a deliberate guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/coverage.yml:
- Around line 12-18: The env variable PYTHON is inconsistent with the
actions/setup-python step: env.PYTHON is set to "3.11" while
actions/setup-python uses python-version "3.12"; update the workflow to either
remove the unused env variable PYTHON or set PYTHON to match the setup step
(change env.PYTHON to "3.12") and/or make actions/setup-python use env.PYTHON
via python-version: ${{ env.PYTHON }} so both places reference the same Python
version.
In `@AUDIT.md`:
- Line 25: Fix the typo in the AUDIT.md heading: change the phrase "Dependency
Pining" to "Dependency Pinning" so the line reads "**Dependency Pinning**:
Dependencies in `pyproject.toml` use loose upper bounds (e.g., `<3.0.0`), which
might lead to breaking changes if sub-dependencies don't follow semver
strictly."; update exactly that text in AUDIT.md.
In `@FAQ.md`:
- Around line 36-38: Update the example install and test commands in the FAQ to
use relative paths appropriate when running inside the repository: replace the
installation command using "ovos-audio/" with "pip install -e ." and replace the
pytest invocation using "ovos-audio/test/" with "pytest test/ --cov=ovos_audio"
so the commands work when the user is inside the cloned repo rather than its
parent directory.
In `@QUICK_FACTS.md`:
- Line 9: The version in QUICK_FACTS.md is out of sync with
ovos_audio/version.py (MAJOR, MINOR, BUILD, ALPHA); update the table entry
(`Version | `1.1.1a2``) to match the computed version from version.py
(`1.1.2a1`) or add a note that the value is a static snapshot and may differ
from version.py—locate the version constants in ovos_audio/version.py to
determine the correct string and ensure the markdown reflects that decision.
In `@test/unittests/test_audio_service_extended.py`:
- Around line 628-635: Remove the unnecessary patch that overrides builtin
isinstance in the test; instead rely on the MagicMock created with
spec=RemoteAudioBackend to satisfy isinstance checks. Concretely, delete the
patch("ovos_audio.audio.isinstance", ...) entry from the context manager that
also patches find_audio_service_plugins and setup_audio_service, ensure
remote_plugin/remote_backend is created as MagicMock(spec=RemoteAudioBackend),
and leave svc.load_services() invocation unchanged so the real isinstance
behavior validates the mocked remote backend.
In `@test/unittests/test_playback_play.py`:
- Around line 141-145: The test test_play_calls_on_end_when_queue_empty should
capture the currently playing message before calling t._play() because _play()
sets t._now_playing to None; modify the test to save a local variable (e.g.,
current_msg = t._now_playing[4] or similar) prior to invoking t._play(), then
call t._play() and assert t.on_end.assert_called_once_with(True, current_msg)
(or fallback to unittest.mock.ANY only if current_msg is None) so the assertion
doesn't rely on t._now_playing after it is cleared.
- Around line 234-237: Move the trailing "import unittest.mock" into the module
imports at the top of the file so the mock module is available for tests
(specifically for the unittest.mock.ANY usage in the test_play_calls_on_end
test); remove the duplicate import at the end (after the unittest.main() call)
and ensure the top-level import reads "import unittest.mock" alongside the other
imports so tests always have access to unittest.mock symbols.
In `@test/unittests/test_remaining_gaps.py`:
- Around line 196-219: The production bug is that maybe_reload_tts compares
self._fallback_tts_hash to ftts_m (a module name string) instead of the computed
_ftts_hash integer; fix maybe_reload_tts by changing the condition to compare
self._fallback_tts_hash against the computed _ftts_hash (the value stored later)
so the reload decision uses the correct hash, keeping the existing assignment of
_ftts_hash; also harden the unit test test_old_fallback_tts_shutdown_on_reload
by initializing svc._fallback_tts_hash to a non-None integer (different from the
new computed hash) so the code path that performs the hash comparison is
exercised and will fail if the comparison is wrong.
- Around line 97-101: Remove the patch that references the now-removed
ServiceInstaller in test/unittests/test_remaining_gaps.py by deleting or
stopping the patch call for "ovos_audio.service.ServiceInstaller" and keep only
the patch for "ovos_audio.service.AudioService"; additionally edit the test
helper function _make_svc to remove the obsolete pip_installer assignment (or
any references to pip_installer) so the helper matches the updated
PlaybackService API and no longer references non-existent symbols.
---
Nitpick comments:
In @.github/workflows/build_tests.yml:
- Around line 11-14: The workflow currently references an external workflow via
the line "uses:
OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev"; change this
to pin to a stable release tag or specific commit SHA (e.g., replace "@dev" with
a release tag like "@v1.2.0" or a commit SHA) to avoid unexpected breaks, and
ensure any related inputs (e.g., "test_path") remain unchanged; update the
"uses" value accordingly so CI uses the fixed ref.
In @.github/workflows/release_workflow.yml:
- Line 12: The workflow currently references an external workflow with the
pinned ref "uses:
TigreGotico/gh-automations/.github/workflows/publish-alpha.yml@master"; update
that reference to a fixed, immutable ref (a commit SHA or a release tag) instead
of `@master` to ensure reproducible runs—replace the "@master" suffix with a
specific SHA (e.g., "@<commit-sha>") or a tagged release (e.g., "@vX.Y.Z") and
commit the updated string so the external workflow cannot change unexpectedly.
In `@docs/index.md`:
- Around line 20-31: The markdown code fences for the architecture diagram and
package layout in docs/index.md are missing a language specifier; update the
opening triple-backtick lines for those two blocks to use a plain-text specifier
(e.g., ```text or ```plaintext) so linters recognize them as code blocks,
leaving the block contents unchanged—look for the diagram block that starts with
the MessageBus ASCII art and the package layout block further down and add the
language tag to their existing ``` fences.
In `@docs/transformers.md`:
- Around line 5-19: The fenced ASCII diagram in docs/transformers.md has no
language specifier; update the opening triple-backtick for the block that
contains the "speak event → DialogTransformersService → TTS plugin →
TTSTransformersService → PlaybackThread" diagram to include a language tag such
as text or plaintext (e.g., change ``` to ```text) so markdown linters recognize
it as a plain-text block.
In `@pyproject.toml`:
- Line 26: The dependency line "ovoscope>0.10.0" excludes version 0.10.0; update
the version specifier to use a >= operator (e.g., change "ovoscope>0.10.0" to
"ovoscope>=0.10.0" or "ovoscope>=0.10.1" if 0.10.0 is known-bad) in
pyproject.toml so the intended patch/minor versions are included; ensure you
modify the exact string "ovoscope>0.10.0" to the chosen >= form.
In `@test/unittests/test_audio_service_extended.py`:
- Around line 415-426: Extract the repeated capture_emit pattern into a reusable
helper function (e.g., _capture_emit_wrapper) and use it wherever svc.bus.emit
is wrapped; the helper should accept the bus (or svc.bus), save the original
emit, replace it with a wrapper that appends messages to a captured list and
calls the original emit inside a try/except suppressing exceptions, then return
the captured list so tests can assert on it instead of duplicating the capture
logic in multiple places (replace the three inline blocks around
svc._list_backends calls with calls to this helper).
In `@test/unittests/test_playback_play.py`:
- Around line 184-189: In test_play_exception_calls_on_end, the unpacked
variable mock_proc returned from _setup_thread_for_play is unused; change the
unpack to use an underscored name (e.g. _, or _mock_proc) so the test doesn't
complain about an unused variable—update the line that calls
self._setup_thread_for_play() in test_play_exception_calls_on_end accordingly
and keep the rest of the test (t, _ = self._setup_thread_for_play() or t,
_mock_proc = ...) unchanged.
- Around line 221-231: Remove the unused assignment original_get = t.queue.get;
simply delete that line so only the replacement function boom_then_stop is
assigned to t.queue.get. Ensure you keep the replacement function
(boom_then_stop), the call_count increment, setting t._terminated, raising
queue.Empty, and the final t.run() call unchanged.
In `@test/unittests/test_playback_service.py`:
- Around line 21-50: This test class intentionally uses inspect.getsource in
_read_defaults_from_source and string assertions in tests
(TestPlaybackServiceConfigDefaults, _read_defaults_from_source,
test_audio_enabled_default_is_true, test_disable_ocp_default_is_false) as a
canary to detect changes to TODO-flagged defaults; update the file by adding a
short clarifying comment at the top of the class (or above
_read_defaults_from_source) stating that the fragile source-inspection is
intentional and used to alert maintainers when default config literals change,
so future readers understand this is not an accidental brittle test but a
deliberate guard.
In `@test/unittests/test_playback_thread.py`:
- Around line 112-124: The test computes emitted_types but never uses it; either
remove the unused emitted_types calculation or replace it with a concrete
assertion about bus.emit results. Update test_begin_audio_ocp_cork to delete the
emitted_types list comprehension (leaving the existing
self.assertTrue(bus.emit.called) assertion), or instead assert a specific
emitted event by inspecting bus.emit.call_args_list (e.g., check the first
emitted object's msg_type) inside the same test and keep emitted_types if you
add that assertion; modify references around _make_thread and t.begin_audio
accordingly.
In `@test/unittests/test_service_handlers.py`:
- Around line 269-282: Remove the unused local variable cfg in the
test_disable_fallback_skips_fallback_reload function: locate the test function
(test_disable_fallback_skips_fallback_reload) where cfg = {"module": "dummy",
"dummy": {}} is assigned but never referenced, delete that assignment so only
svc setup, _tts_hash setup and the patched calls to Configuration and TTSFactory
remain, ensuring the rest of the test (svc._maybe_reload_tts() and
mock_factory.create.assert_not_called()) is unchanged.
In `@test/unittests/test_transformers.py`:
- Around line 119-130: The test unpacks an unused context variable as ctx in
test_transform_plugin_exception_does_not_propagate; change the unpack to use a
prefixed underscore (e.g., _ctx) to mark it as intentionally unused. Locate the
test function test_transform_plugin_exception_does_not_propagate, the
bad_transform helper and the call to svc.transform("hello", context={}) and
update the assignment from "result, ctx = svc.transform(...)" to "result, _ctx =
svc.transform(...)" (and similarly rename any other unused unpacked ctx in this
test file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 088799d9-892b-4a52-9ffa-f104b293151f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/build_tests.yml.github/workflows/coverage.yml.github/workflows/install_tests.yml.github/workflows/license_tests.yml.github/workflows/publish_stable.yml.github/workflows/release_workflow.yml.github/workflows/unit_tests.ymlAUDIT.mdFAQ.mdLICENSEMAINTENANCE_REPORT.mdMANIFEST.inQUICK_FACTS.mdSUGGESTIONS.mddocs/audio-service.mddocs/index.mddocs/playback-service.mddocs/transformers.mddocs/tts.mdovos_audio/version.pypyproject.tomlrequirements/extras.txtsetup.pytest/end2end/__init__.pytest/end2end/test_audio_service_e2e.pytest/end2end/test_audio_service_extended_e2e.pytest/end2end/test_playback_service_e2e.pytest/end2end/test_playback_service_extended_e2e.pytest/requirements.txttest/unittests/test_audio_service.pytest/unittests/test_audio_service_extended.pytest/unittests/test_main.pytest/unittests/test_playback_play.pytest/unittests/test_playback_service.pytest/unittests/test_playback_thread.pytest/unittests/test_remaining_gaps.pytest/unittests/test_service_handlers.pytest/unittests/test_transformers.pytest/unittests/test_version_and_tts.py
💤 Files with no reviewable changes (4)
- MANIFEST.in
- setup.py
- .github/workflows/install_tests.yml
- .github/workflows/unit_tests.yml
Comment on lines
+628
to
+635
| with patch("ovos_audio.audio.find_audio_service_plugins", | ||
| return_value={"local": local_plugin, "remote": remote_plugin}), \ | ||
| patch("ovos_audio.audio.setup_audio_service", side_effect=fake_setup), \ | ||
| patch("ovos_audio.audio.isinstance", | ||
| side_effect=lambda obj, cls: cls == RemoteAudioBackend and obj is remote_backend), \ | ||
| patch.object(svc, "find_ocp"), \ | ||
| patch.object(svc, "find_default"): | ||
| svc.load_services() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how load_services uses isinstance to understand if spec= would suffice
rg -n "isinstance.*RemoteAudioBackend" ovos_audio/Repository: OpenVoiceOS/ovos-audio
Length of output: 223
🏁 Script executed:
# First, let's look at the test file around the problematic lines to understand remote_backend creation
sed -n '600,660p' test/unittests/test_audio_service_extended.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 2963
🏁 Script executed:
# Check how remote_backend is created in the test setup
rg -B 20 "load_services\(\)" test/unittests/test_audio_service_extended.py | head -80Repository: OpenVoiceOS/ovos-audio
Length of output: 1988
🏁 Script executed:
# Look at the isinstance checks in ovos_audio/audio.py to understand what they're checking
sed -n '140,150p' ovos_audio/audio.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 526
🏁 Script executed:
# Check the second isinstance usage
sed -n '515,525p' ovos_audio/audio.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 419
🏁 Script executed:
# Find RemoteAudioBackend class definition
rg -n "class RemoteAudioBackend" --type pyRepository: OpenVoiceOS/ovos-audio
Length of output: 48
🏁 Script executed:
# Check if RemoteAudioBackend can be instantiated or if it's meant to be a base class
rg -B 5 -A 15 "class RemoteAudioBackend" --type pyRepository: OpenVoiceOS/ovos-audio
Length of output: 48
🏁 Script executed:
# Verify Mock behavior with spec - let's check if there are other tests that show this pattern
rg -n "MagicMock.*spec=" test/unittests/ | head -20Repository: OpenVoiceOS/ovos-audio
Length of output: 417
🏁 Script executed:
# Check what RemoteAudioBackend is and where it comes from
rg -n "from.*RemoteAudioBackend\|import.*RemoteAudioBackend" test/unittests/ ovos_audio/Repository: OpenVoiceOS/ovos-audio
Length of output: 48
🏁 Script executed:
# Let's verify if spec= actually makes isinstance work - check Python unittest.mock documentation behavior
# Look for how other tests handle this situation
rg -B 5 -A 5 "spec=RemoteAudioBackend" test/unittests/test_remaining_gaps.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 708
🏁 Script executed:
# Check if the RemoteAudioBackend class is actually instantiable or is it abstract
python3 << 'EOF'
from unittest.mock import MagicMock
# Test if spec= makes isinstance work
class MyClass:
pass
mock = MagicMock(spec=MyClass)
result = isinstance(mock, MyClass)
print(f"isinstance(MagicMock(spec=MyClass), MyClass) = {result}")
EOFRepository: OpenVoiceOS/ovos-audio
Length of output: 117
🏁 Script executed:
# Look at the full test in test_remaining_gaps.py to see if it uses isinstance patch or not
rg -B 15 -A 30 "remote_instance = MagicMock\(spec=RemoteAudioBackend\)" test/unittests/test_remaining_gaps.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 2142
🏁 Script executed:
# Check if test_remaining_gaps.py uses isinstance patch when testing with spec=RemoteAudioBackend
sed -n '270,320p' test/unittests/test_remaining_gaps.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 2444
🏁 Script executed:
# Let me verify RemoteAudioBackend is imported the same way in both test files
rg -n "from.*RemoteAudioBackend\|import.*RemoteAudioBackend" test/unittests/test_audio_service_extended.py test/unittests/test_remaining_gaps.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 48
🏁 Script executed:
# Final verification - look at the exact line where RemoteAudioBackend is imported in test_audio_service_extended.py
sed -n '600,625p' test/unittests/test_audio_service_extended.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 1288
🏁 Script executed:
# Verify that RemoteAudioBackend comes from the same location in both test files
rg "from ovos_plugin_manager.templates.audio import RemoteAudioBackend" test/unittests/Repository: OpenVoiceOS/ovos-audio
Length of output: 186
🏁 Script executed:
# Final check - let me verify that MagicMock with spec works with isinstance when the class comes from ovos_plugin_manager
python3 << 'EOF'
from unittest.mock import MagicMock
from ovos_plugin_manager.templates.audio import RemoteAudioBackend
# Test if spec= makes isinstance work with actual RemoteAudioBackend class
mock = MagicMock(spec=RemoteAudioBackend)
result = isinstance(mock, RemoteAudioBackend)
print(f"isinstance(MagicMock(spec=RemoteAudioBackend), RemoteAudioBackend) = {result}")
EOFRepository: OpenVoiceOS/ovos-audio
Length of output: 197
🏁 Script executed:
# Let's check if test_remaining_gaps.py test actually passes by seeing if there are any test result indicators
# Also, let's verify the imports are identical
diff -u <(sed -n '275,310p' test/unittests/test_remaining_gaps.py) <(sed -n '605,635p' test/unittests/test_audio_service_extended.py) | head -60Repository: OpenVoiceOS/ovos-audio
Length of output: 153
🏁 Script executed:
# Check if there are any comments in test_remaining_gaps.py about why isinstance patch is not needed
sed -n '265,290p' test/unittests/test_remaining_gaps.pyRepository: OpenVoiceOS/ovos-audio
Length of output: 1436
The isinstance patch is unnecessary. MagicMock(spec=RemoteAudioBackend) makes the mock pass isinstance checks, as demonstrated by the similar test in test_remaining_gaps.py:278, which uses the same pattern without patching isinstance. Remove the patch and rely on spec= alone.
Reference pattern from existing test
remote_instance = MagicMock(spec=RemoteAudioBackend)
# No isinstance patch needed - spec= is sufficient🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_audio_service_extended.py` around lines 628 - 635, Remove
the unnecessary patch that overrides builtin isinstance in the test; instead
rely on the MagicMock created with spec=RemoteAudioBackend to satisfy isinstance
checks. Concretely, delete the patch("ovos_audio.audio.isinstance", ...) entry
from the context manager that also patches find_audio_service_plugins and
setup_audio_service, ensure remote_plugin/remote_backend is created as
MagicMock(spec=RemoteAudioBackend), and leave svc.load_services() invocation
unchanged so the real isinstance behavior validates the mocked remote backend.
Comment on lines
+97
to
+101
| stack.enter_context(patch("ovos_audio.service.ServiceInstaller", | ||
| return_value=MagicMock())) | ||
| stack.enter_context(patch("ovos_audio.service.AudioService", | ||
| return_value=MagicMock())) | ||
| return stack |
Contributor
There was a problem hiding this comment.
Remove patch for non-existent ServiceInstaller.
The pipeline failure indicates ServiceInstaller no longer exists in ovos_audio.service. According to the PR description, pip_installer usage was removed from PlaybackService. This patch will cause AttributeError during test execution.
Proposed fix
def _full_mock_init_context():
"""Returns a context manager stack that mocks everything PlaybackService.__init__ touches."""
from contextlib import ExitStack
stack = ExitStack()
stack.enter_context(patch("ovos_audio.service.Configuration", return_value={}))
stack.enter_context(patch("ovos_audio.service.MessageBusClient",
return_value=MagicMock()))
stack.enter_context(patch("ovos_audio.service.ProcessStatus",
return_value=MagicMock()))
stack.enter_context(patch("ovos_audio.service.StatusCallbackMap",
return_value=MagicMock()))
stack.enter_context(patch("ovos_audio.service.DialogTransformersService",
return_value=MagicMock()))
stack.enter_context(patch("ovos_audio.service.PlaybackThread",
return_value=MagicMock()))
- stack.enter_context(patch("ovos_audio.service.ServiceInstaller",
- return_value=MagicMock()))
stack.enter_context(patch("ovos_audio.service.AudioService",
return_value=MagicMock()))
return stackAlso remove the pip_installer assignment from _make_svc helper at line 189:
svc.status = MagicMock()
svc.audio = None
- svc.pip_installer = MagicMock()
return svc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| stack.enter_context(patch("ovos_audio.service.ServiceInstaller", | |
| return_value=MagicMock())) | |
| stack.enter_context(patch("ovos_audio.service.AudioService", | |
| return_value=MagicMock())) | |
| return stack | |
| stack.enter_context(patch("ovos_audio.service.Configuration", return_value={})) | |
| stack.enter_context(patch("ovos_audio.service.MessageBusClient", | |
| return_value=MagicMock())) | |
| stack.enter_context(patch("ovos_audio.service.ProcessStatus", | |
| return_value=MagicMock())) | |
| stack.enter_context(patch("ovos_audio.service.StatusCallbackMap", | |
| return_value=MagicMock())) | |
| stack.enter_context(patch("ovos_audio.service.DialogTransformersService", | |
| return_value=MagicMock())) | |
| stack.enter_context(patch("ovos_audio.service.PlaybackThread", | |
| return_value=MagicMock())) | |
| stack.enter_context(patch("ovos_audio.service.AudioService", | |
| return_value=MagicMock())) | |
| return stack |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_remaining_gaps.py` around lines 97 - 101, Remove the
patch that references the now-removed ServiceInstaller in
test/unittests/test_remaining_gaps.py by deleting or stopping the patch call for
"ovos_audio.service.ServiceInstaller" and keep only the patch for
"ovos_audio.service.AudioService"; additionally edit the test helper function
_make_svc to remove the obsolete pip_installer assignment (or any references to
pip_installer) so the helper matches the updated PlaybackService API and no
longer references non-existent symbols.
Two related issues:
1. ovos_audio/service.py: Remove accidental ServiceInstaller import and
pip_installer usage. ovos_utils.skill_installer was added by mistake
and only exists in ovos-utils>=0.8.5a1 (not stable), causing
ModuleNotFoundError on all CI versions.
2. test/unittests/test_remaining_gaps.py: Remove the ServiceInstaller
patch from _full_mock_init_context() and add a try/except guard so
the ExitStack is always closed on setup failure.
Previously, when patch("ovos_audio.service.ServiceInstaller") raised
AttributeError, the ExitStack was abandoned mid-setup without cleanup,
leaving ProcessStatus (and other patches) permanently active. This
leaked into test_speech.py, making speech.status a MagicMock and
causing TypeError in ProcessState comparisons.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
test_end2end.py::TestLegacy relies on the ovos_simple backend to emit expected bus messages. CI has no audio plugins installed, causing 'No audio player plugins found' and timeouts waiting for messages. Add @skipUnless(_simple_plugin_available()) so the tests are properly skipped instead of failing non-deterministically in environments without the plugin. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TigreGotico/gh-automations no longer exists; update to the canonical OpenVoiceOS/gh-automations@dev reference used by all other workflows. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/license_tests.yml (1)
12-15: Consider pinning the external workflow to a SHA or release tag for reproducibility.Using
@devas the ref means the workflow behavior could change unexpectedly when the upstreamgh-automationsrepository is updated. For more predictable CI behavior, consider pinning to a specific commit SHA or version tag.Example:
uses: OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@v1.0.0 # or a specific SHAIf staying on
@devis intentional to track upstream changes, this is acceptable but worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/license_tests.yml around lines 12 - 15, The job 'license_tests' currently references the external workflow via "OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev", which can change unexpectedly; update that uses ref to a fixed release tag or specific commit SHA (e.g., replace "@dev" with "@vX.Y.Z" or "@<commit-sha>") to ensure reproducible CI runs, and if you intentionally want to track upstream changes keep "@dev" but add a comment in the workflow explaining that choice.test/unittests/test_remaining_gaps.py (2)
332-333: Use underscore for unused unpacked variable.The
ctxvariable is unpacked but never used.Proposed fix
- result, ctx = svc.transform("/tmp/test.wav", context={}) + result, _ctx = svc.transform("/tmp/test.wav", context={})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_remaining_gaps.py` around lines 332 - 333, The test unpacks two values from svc.transform into "result, ctx" but never uses ctx; change the unpacked variable to an underscore to signal it is intentionally unused (i.e., use "result, _" when calling svc.transform in the test) so the linter/test code no longer flags an unused variable; update the assertion to use result unchanged and leave svc.transform call and assertEqual(result, "/tmp/test.wav") as-is.
22-22: Remove unused importcall.The
callimport fromunittest.mockis never used in this file.Proposed fix
-from unittest.mock import MagicMock, patch, call +from unittest.mock import MagicMock, patch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_remaining_gaps.py` at line 22, The import list includes an unused symbol "call" from unittest.mock; edit the import statement that currently reads "from unittest.mock import MagicMock, patch, call" and remove "call" so it imports only MagicMock and patch, ensuring there are no other references to the call symbol (e.g., in test functions) before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unittests/test_end2end.py`:
- Around line 26-34: The helper _simple_plugin_available currently swallows all
exceptions which hides real plugin discovery errors; change the broad except
Exception: to only catch import failures (e.g. except (ImportError,
ModuleNotFoundError):) so that import issues still cause the function to return
False but other exceptions from find_audio_service_plugins propagate and surface
test failures; keep the import and call to find_audio_service_plugins and the
unittest.skipUnless usage unchanged.
---
Nitpick comments:
In @.github/workflows/license_tests.yml:
- Around line 12-15: The job 'license_tests' currently references the external
workflow via
"OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev", which can
change unexpectedly; update that uses ref to a fixed release tag or specific
commit SHA (e.g., replace "@dev" with "@vX.Y.Z" or "@<commit-sha>") to ensure
reproducible CI runs, and if you intentionally want to track upstream changes
keep "@dev" but add a comment in the workflow explaining that choice.
In `@test/unittests/test_remaining_gaps.py`:
- Around line 332-333: The test unpacks two values from svc.transform into
"result, ctx" but never uses ctx; change the unpacked variable to an underscore
to signal it is intentionally unused (i.e., use "result, _" when calling
svc.transform in the test) so the linter/test code no longer flags an unused
variable; update the assertion to use result unchanged and leave svc.transform
call and assertEqual(result, "/tmp/test.wav") as-is.
- Line 22: The import list includes an unused symbol "call" from unittest.mock;
edit the import statement that currently reads "from unittest.mock import
MagicMock, patch, call" and remove "call" so it imports only MagicMock and
patch, ensuring there are no other references to the call symbol (e.g., in test
functions) before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a68172f-493e-4fe7-9325-c93505edb491
📒 Files selected for processing (4)
.github/workflows/license_tests.ymlovos_audio/utils.pytest/unittests/test_end2end.pytest/unittests/test_remaining_gaps.py
Comment on lines
+26
to
+34
| def _simple_plugin_available() -> bool: | ||
| try: | ||
| from ovos_plugin_manager.audio import find_audio_service_plugins | ||
| return "ovos_simple" in find_audio_service_plugins() | ||
| except Exception: | ||
| return False | ||
|
|
||
|
|
||
| @unittest.skipUnless(_simple_plugin_available(), "ovos_simple audio plugin not installed") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "test_end2end.py" -type f 2>/dev/null | head -5Repository: OpenVoiceOS/ovos-audio
Length of output: 48
🏁 Script executed:
git ls-files | grep -E "test.*end2end" | head -10Repository: OpenVoiceOS/ovos-audio
Length of output: 301
🏁 Script executed:
# Search for the file in common test locations
find . -path "*/test/*" -name "*end2end*" -type f 2>/dev/nullRepository: OpenVoiceOS/ovos-audio
Length of output: 98
🏁 Script executed:
# Look at the repo structure
ls -la
git ls-files | head -20Repository: OpenVoiceOS/ovos-audio
Length of output: 1891
🏁 Script executed:
cat -n test/unittests/test_end2end.py | head -50Repository: OpenVoiceOS/ovos-audio
Length of output: 2149
🏁 Script executed:
# Check the exact lines mentioned in the review (26-34)
sed -n '20,40p' test/unittests/test_end2end.py | cat -nRepository: OpenVoiceOS/ovos-audio
Length of output: 918
🏁 Script executed:
# Search for any other uses of ovos_plugin_manager in the codebase
rg "ovos_plugin_manager" -A 3 -B 1Repository: OpenVoiceOS/ovos-audio
Length of output: 8582
🏁 Script executed:
# Look at the TestLegacy class to understand the context better
sed -n '30,100p' test/unittests/test_end2end.py | cat -nRepository: OpenVoiceOS/ovos-audio
Length of output: 3210
🏁 Script executed:
# Check if there's any documentation or tests for find_audio_service_plugins
rg "find_audio_service_plugins" -B 2 -A 5Repository: OpenVoiceOS/ovos-audio
Length of output: 8676
🏁 Script executed:
# Look for exception handling patterns with plugin_manager imports elsewhere
rg "from ovos_plugin_manager" -A 5 | head -60Repository: OpenVoiceOS/ovos-audio
Length of output: 4354
🏁 Script executed:
# Check if find_audio_service_plugins is documented or has type hints
fd -e py -x grep -l "find_audio_service_plugins" {} \;Repository: OpenVoiceOS/ovos-audio
Length of output: 280
🏁 Script executed:
# Look for exception handling in production code around plugin_manager
rg "except.*Error\|except.*Exception" ovos_audio/ -B 2 -A 2Repository: OpenVoiceOS/ovos-audio
Length of output: 48
🏁 Script executed:
# Check for any type hints or documentation about find_audio_service_plugins
rg "find_audio_service_plugins" ovos_audio/ -B 3 -A 3Repository: OpenVoiceOS/ovos-audio
Length of output: 975
🏁 Script executed:
# Look at how other plugin discoveries are handled in the codebase
rg "plugin" ovos_audio/audio.py -B 2 -A 2 | head -80Repository: OpenVoiceOS/ovos-audio
Length of output: 1778
🏁 Script executed:
# Check how the production code uses find_audio_service_plugins - does it have error handling?
sed -n '90,130p' ovos_audio/audio.py | cat -nRepository: OpenVoiceOS/ovos-audio
Length of output: 2026
🏁 Script executed:
# Look for other optional plugin availability checks in tests
rg "@unittest.skip" test/ -B 3 -A 1Repository: OpenVoiceOS/ovos-audio
Length of output: 1010
🏁 Script executed:
# Check if ovos_plugin_manager is listed as a dependency (optional or required)
cat pyproject.toml | grep -A 20 "dependencies\|optional"Repository: OpenVoiceOS/ovos-audio
Length of output: 991
🏁 Script executed:
# Check if there's any exception handling pattern around find_audio_service_plugins elsewhere
rg -A 10 "load_services\|find_audio_service_plugins" ovos_audio/audio.py | grep -A 10 "def load_services"Repository: OpenVoiceOS/ovos-audio
Length of output: 48
🏁 Script executed:
# Look at the actual call to find_audio_service_plugins in production code
sed -n '98,120p' ovos_audio/audio.py | cat -nRepository: OpenVoiceOS/ovos-audio
Length of output: 1245
🏁 Script executed:
# Get the full load_services method to see if there's any exception handling
sed -n '98,150p' ovos_audio/audio.py | cat -nRepository: OpenVoiceOS/ovos-audio
Length of output: 2592
🏁 Script executed:
# Check if find_audio_service_plugins documentation or source hints at possible exceptions
rg "ImportError|ModuleNotFoundError" ovos_audio/ test/ -B 2 -A 1Repository: OpenVoiceOS/ovos-audio
Length of output: 2326
Restrict exception handler to import failures only.
The broad except Exception: masks plugin discovery failures as "plugin unavailable" and silently skips the entire test class during collection. Because find_audio_service_plugins() is called without error handling in production code, only catch the expected import-related exceptions to surface real regressions.
Proposed fix
def _simple_plugin_available() -> bool:
try:
from ovos_plugin_manager.audio import find_audio_service_plugins
return "ovos_simple" in find_audio_service_plugins()
- except Exception:
+ except (ImportError, ModuleNotFoundError):
return False🧰 Tools
🪛 Ruff (0.15.5)
[warning] 30-30: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_end2end.py` around lines 26 - 34, The helper
_simple_plugin_available currently swallows all exceptions which hides real
plugin discovery errors; change the broad except Exception: to only catch import
failures (e.g. except (ImportError, ModuleNotFoundError):) so that import issues
still cause the function to return False but other exceptions from
find_audio_service_plugins propagate and surface test failures; keep the import
and call to find_audio_service_plugins and the unittest.skipUnless usage
unchanged.
ovos_utils.signal was removed in ovos-utils>=0.8.5a1. test_utils.py imports create_signal/check_for_signal at module level, causing ModuleNotFoundError at collection time and blocking all other tests. Guard the import with try/except and add @skipUnless so the tests are skipped cleanly on newer ovos-utils instead of crashing collection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unittests/test_utils.py`:
- Around line 22-28: The current try/except wraps multiple imports and masks
real ImportError/AttributeError from ovos_audio.utils or ovos_utils.file_utils;
change the code to import ovos_audio.utils (wait_while_speaking, is_speaking,
stop_speaking) and ovos_utils.file_utils.get_temp_path normally (so real import
errors fail tests), and put a narrow try/except only around "from
ovos_utils.signal import create_signal, check_for_signal" to set
_SIGNAL_API_AVAILABLE; keep the _SIGNAL_API_AVAILABLE symbol and ensure
class-level skip logic only depends on that signal flag so tests that only mock
ovos_audio.utils (e.g., test_stop_speaking*) are not skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7e8d2e0-0ce0-4e05-82d6-c70d923b526c
📒 Files selected for processing (1)
test/unittests/test_utils.py
Comment on lines
+22
to
+28
| try: | ||
| from ovos_utils.signal import create_signal, check_for_signal | ||
| from ovos_utils.file_utils import get_temp_path | ||
| from ovos_audio.utils import wait_while_speaking, is_speaking, stop_speaking | ||
| _SIGNAL_API_AVAILABLE = True | ||
| except (ImportError, AttributeError): | ||
| _SIGNAL_API_AVAILABLE = False |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm which tests in this file actually depend on the signal helpers.
rg -n -C2 'create_signal|check_for_signal|stop_speaking|wait_while_speaking|is_speaking' test/unittests/test_utils.py
# Inspect the production helpers to verify which ones truly require signal support.
rg -n -C4 -g '*.py' 'def (stop_speaking|is_speaking|wait_while_speaking)\('Repository: OpenVoiceOS/ovos-audio
Length of output: 2490
Narrow the guarded import and skip only the signal-dependent tests.
This now hides any ImportError/AttributeError raised while importing ovos_audio.utils or ovos_utils.file_utils, so a real regression there turns into a skipped class instead of a failing test. The class-level skipUnless also drops test_stop_speaking*, even though those tests do not directly depend on ovos_utils.signal (they only mock ovos_audio.utils functions).
Suggested fix
try:
from ovos_utils.signal import create_signal, check_for_signal
- from ovos_utils.file_utils import get_temp_path
- from ovos_audio.utils import wait_while_speaking, is_speaking, stop_speaking
- _SIGNAL_API_AVAILABLE = True
except (ImportError, AttributeError):
_SIGNAL_API_AVAILABLE = False
+else:
+ _SIGNAL_API_AVAILABLE = True
+
+from ovos_utils.file_utils import get_temp_path
+from ovos_audio.utils import wait_while_speaking, is_speaking, stop_speaking
@@
-@unittest.skipUnless(_SIGNAL_API_AVAILABLE, "ovos_utils.signal removed in ovos-utils>=0.8.5")
class TestInterface(unittest.TestCase):
@@
+ `@unittest.skipUnless`(_SIGNAL_API_AVAILABLE, "ovos_utils.signal removed in ovos-utils>=0.8.5")
def test_is_speaking(self):
@@
+ `@unittest.skipUnless`(_SIGNAL_API_AVAILABLE, "ovos_utils.signal removed in ovos-utils>=0.8.5")
def test_wait_while_speaking(self):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_utils.py` around lines 22 - 28, The current try/except
wraps multiple imports and masks real ImportError/AttributeError from
ovos_audio.utils or ovos_utils.file_utils; change the code to import
ovos_audio.utils (wait_while_speaking, is_speaking, stop_speaking) and
ovos_utils.file_utils.get_temp_path normally (so real import errors fail tests),
and put a narrow try/except only around "from ovos_utils.signal import
create_signal, check_for_signal" to set _SIGNAL_API_AVAILABLE; keep the
_SIGNAL_API_AVAILABLE symbol and ensure class-level skip logic only depends on
that signal flag so tests that only mock ovos_audio.utils (e.g.,
test_stop_speaking*) are not skipped.
Updated existing workflows: - build_tests.yml: add system_deps (swig sox mpg123) for audio deps - coverage.yml: replace custom 60-line script with reusable workflow - publish_stable.yml: TigreGotico/gh-automations@master → OpenVoiceOS@dev - release_workflow.yml: TigreGotico/gh-automations@master → OpenVoiceOS@dev New reusable workflows added: - lint.yml: ruff lint via gh-automations - pip_audit.yml: CVE scan via gh-automations - repo_health.yml: required-file checks via gh-automations - release_preview.yml: next-version preview on PRs - downstream_check.yml: track downstream dependents of ovos-audio Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Docs: - AUDIT.md: fix typo "Pining" → "Pinning" - FAQ.md: fix install/test paths (ovos-audio/ → . / test/) - QUICK_FACTS.md: update version 1.1.1a2 → 1.1.2a1 Production bug fix: - service.py:395: _maybe_reload_tts compared _fallback_tts_hash to ftts_m (module name string) instead of _ftts_hash (computed hash integer), causing the fallback TTS to always reload on every config check Tests: - test_playback_play.py: capture _now_playing[4] before _play() clears it; move stray `import unittest.mock` from after main() to top-level ANY import - test_utils.py: narrow guarded import to only ovos_utils.signal and signal-based helpers; split into TestSignalInterface (skipped when signal unavailable) and TestStopSpeaking (also skipped — stop_speaking was removed with the signal API); stops hiding real ImportErrors from unrelated modules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 by CodeRabbit
Documentation
Tests
Chores