Conversation
Adds an asyncio-native client class alongside the existing sync MessageBusClient — fully backwards-compatible and optional. - ovos_bus_client/client/async_client.py AsyncMessageBusClient (websockets-based), AsyncMessageWaiter, AsyncMessageCollector — mirrors the sync API surface but all I/O methods are coroutines; on/once/remove stay synchronous. - Graceful ImportError guard: if 'websockets' is not installed the package still imports cleanly; connect() raises a descriptive error. - pyproject.toml: new [async] optional-dependency group (websockets>=10) - test/unittests/test_async_client.py: 20 new unit tests (all passing, total 127 tests green) - benchmarks/bench_async_vs_sync.py: emit throughput, wait latency, serialization baseline, and concurrent fan-out benchmark Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- docs/async_client.md: full AsyncMessageBusClient reference
— install, connection lifecycle, emit, listen, wait_for_response,
collect_responses, fan-out pattern, API table, benchmark results table
- docs/index.md: add async_client.md to contents table; update package
layout tree to show async_client.py
- docs/client.md: add "Async alternative" section pointing to async_client.md
- README.md: replace legacy MycroftBusClient section with side-by-side
sync and async usage examples including fan-out pattern
Benchmark numbers (n=3000, in-process mocks, Python 3.11):
emit mean: sync 0.022ms async 0.025ms (~1.1×)
wait_for_message: sync 0.020ms async 0.006ms (~3.4× faster)
fan-out ×200: ~31 000 msg/s via asyncio.gather
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ping! I've got your results right here. 🛎️I've aggregated the results of the automated checks for this PR below. 🔨 Build TestsThe build bots are giving this a thumbs up. 👍
❌ 3.10: Install OK, tests failed Beep boop, I'm just a script 🤖 |
📝 WalkthroughWalkthroughReplaces several local GitHub Actions workflows with reusable shared workflows; adds a full AsyncMessageBusClient (with waiter and collector), tests, docs, benchmarks, and migrates packaging to pyproject.toml (removing setup.py). Minor utilities, version metadata, and gitignore updates included. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant AsyncClient as AsyncMessageBusClient
participant WS as WebSocket
participant Handler as EventHandler
participant Waiter as AsyncMessageWaiter
App->>AsyncClient: connect(retry=True)
AsyncClient->>WS: websocket.connect(url)
WS-->>AsyncClient: connected
AsyncClient->>AsyncClient: start _recv_loop()
rect rgba(100,200,100,0.5)
App->>AsyncClient: emit(message)
AsyncClient->>AsyncClient: attach session/context
AsyncClient->>WS: send(serialized message)
end
rect rgba(100,100,200,0.5)
WS-->>AsyncClient: raw message
AsyncClient->>AsyncClient: _on_message(raw)
AsyncClient->>Handler: dispatch event
Handler-->>Handler: callback executes
end
rect rgba(200,150,100,0.5)
App->>Waiter: wait(timeout)
Waiter->>AsyncClient: register listener
WS-->>AsyncClient: matching message arrives
AsyncClient->>Waiter: deliver message
Waiter-->>App: return Message
end
App->>AsyncClient: close()
AsyncClient->>WS: close connection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
CI was failing with 'de' != 'de-DE' on all Python versions because the fallback path in ovos_utils.standardize_lang_tag (when langcodes is not installed) strips the territory code when macro=True (default). Passing macro=False always preserves the territory code in the fallback. Affected: get_message_lang() in ovos_bus_client/util/__init__.py Also increases unit test coverage from 23% to 28% (171 tests, was 127): - message.py 37% -> 66%: reply, forward, response, publish, CollectionMessage, GUIMessage, encrypt_as_dict/decrypt_from_dict - async_client.py 48% -> 61%: connect, close, _recv_loop, collector internals - client.py 16% -> 35%: on_collect, GUIWebsocketClient.on_open/on_message - util/__init__.py 7% -> 10%: get_message_lang session path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ovos_bus_client/__init__.py (1)
34-42:⚠️ Potential issue | 🟡 Minor
__all__unconditionally includesAsyncMessageBusClienteven when it'sNone.When
websocketsis not installed,AsyncMessageBusClientisNone. Users doingfrom ovos_bus_client import AsyncMessageBusClientwill silently receiveNoneand only fail at instantiation time with an opaqueTypeError.Consider either:
- Conditionally include it in
__all__only when available, or- Document this behavior clearly so users know to check for
None.🛡️ Option 1: Conditional export
__all__ = [ MessageBusClient, - AsyncMessageBusClient, GUIWebsocketClient, GUIMessage, Message, send, client_from_config ] +if AsyncMessageBusClient is not None: + __all__.append(AsyncMessageBusClient)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_bus_client/__init__.py` around lines 34 - 42, The __all__ list currently unconditionally exposes AsyncMessageBusClient even when it's None, causing imports to silently return None; to fix, build __all__ dynamically in ovos_bus_client/__init__.py by creating the base export list (MessageBusClient, GUIWebsocketClient, GUIMessage, Message, send, client_from_config) and only append "AsyncMessageBusClient" to that list when the AsyncMessageBusClient symbol is truthy (not None); ensure you reference the exact symbol name AsyncMessageBusClient and update __all__ accordingly so imports like "from ovos_bus_client import AsyncMessageBusClient" fail immediately with ImportError rather than returning None.
🧹 Nitpick comments (6)
docs/session.md (1)
128-131: Add language specifier to fenced code block.The code block showing the session sync protocol is missing a language specifier. Even for plain-text protocol diagrams, adding a language identifier (e.g.,
textorplaintext) improves markdown consistency.📝 Suggested fix
-``` +```text client → ovos.session.sync core → ovos.session.update_default {session_data: {...}}</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/session.mdaround lines 128 - 131, Add a language specifier to the
fenced code block that shows the session sync protocol (the block containing
"client → ovos.session.sync" and "core → ovos.session.update_default
{session_data: {...}}"); change the openingto include a language tag such astext or ```plaintext so the block becomes a labeled plaintext code fence
for consistent Markdown rendering.</details> </blockquote></details> <details> <summary>.github/workflows/coverage.yml (1)</summary><blockquote> `11-18`: **Unused `PYTHON` environment variable is inconsistent with actual version.** The `env.PYTHON` is set to `'3.9'` but is never referenced in the workflow, while `setup-python` uses `3.12`. Consider removing the unused env variable or using it in the `python-version` field for consistency. <details> <summary>🧹 Suggested cleanup</summary> ```diff env: - PYTHON: '3.9' + PYTHON: '3.12' steps: - uses: actions/checkout@v4 - name: Setup Python uses: actions/setup-python@v5 with: - python-version: 3.12 + python-version: ${{ env.PYTHON }} ``` Or simply remove the unused `env` block entirely. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.github/workflows/coverage.yml around lines 11 - 18, The workflow sets env.PYTHON = '3.9' but Actions uses actions/setup-python@v5 with python-version: 3.12; remove the unused env.PYTHON or make them consistent by using env.PYTHON in the setup call (replace the literal 3.12 with ${ { env.PYTHON } }), or delete the entire env block; update the python-version in the setup-python step or remove the env definition so env.PYTHON is not left unused. ``` </details> </blockquote></details> <details> <summary>pyproject.toml (1)</summary><blockquote> `14-19`: **Inconsistent spacing in version specifier.** The `pyee` version constraint has inconsistent spacing compared to other dependencies. Minor formatting nit. <details> <summary>🧹 Suggested fix</summary> ```diff dependencies = [ "ovos-config>=0.0.12,<3.0.0", "ovos-utils>=0.8.5a1,<1.0.0", "websocket-client>=0.54.0", - "pyee>= 8.1.0, < 13.0.0", + "pyee>=8.1.0,<13.0.0", ] ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 14 - 19, The pyee dependency spec in the dependencies list is inconsistently spaced ("pyee>= 8.1.0, < 13.0.0"); update the pyee entry in the dependencies array to match the other entries by removing the extra spaces around the operators and comma (use the same format as "ovos-config>=0.0.12,<3.0.0"), so the pyee spec reads without spaces around ">=" and ",". ``` </details> </blockquote></details> <details> <summary>ovos_bus_client/__init__.py (1)</summary><blockquote> `15-18`: **Consider exposing `AsyncMessageWaiter` and `AsyncMessageCollector` for API consistency.** The `client` submodule exports all three async classes (`AsyncMessageBusClient`, `AsyncMessageWaiter`, `AsyncMessageCollector`), but only `AsyncMessageBusClient` is re-exported here. Users who want to use the waiter/collector directly must import from the deeper path. If intentional (keeping helper classes as implementation details), this is fine. Otherwise, consider adding them for a symmetric API surface. <details> <summary>♻️ Optional: export async helpers at top level</summary> ```diff try: - from ovos_bus_client.client.async_client import AsyncMessageBusClient + from ovos_bus_client.client.async_client import AsyncMessageBusClient, AsyncMessageWaiter, AsyncMessageCollector except ImportError: AsyncMessageBusClient = None # websockets not installed + AsyncMessageWaiter = None + AsyncMessageCollector = None ``` And update `__all__`: ```diff __all__ = [ MessageBusClient, AsyncMessageBusClient, + AsyncMessageWaiter, + AsyncMessageCollector, GUIWebsocketClient, ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@ovos_bus_client/__init__.py` around lines 15 - 18, Add symmetric top-level exports for the async helper classes so users can import them from the package root: in the same try/except block that imports AsyncMessageBusClient, also attempt to import AsyncMessageWaiter and AsyncMessageCollector from ovos_bus_client.client.async_client (falling back to None on ImportError), and then add "AsyncMessageBusClient", "AsyncMessageWaiter", and "AsyncMessageCollector" to the module's __all__ list so the API surface is consistent. ``` </details> </blockquote></details> <details> <summary>docs/index.md (2)</summary><blockquote> `43-61`: **Add language identifier to fenced code block.** The package layout diagram lacks a language specifier, which triggers markdownlint MD040. Using `text` or `plaintext` silences the warning and improves rendering in some Markdown processors. <details> <summary>📝 Suggested fix</summary> ````diff -``` +```text ovos_bus_client/ ├── message.py # Message, CollectionMessage, GUIMessage, dig_for_message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.md` around lines 43 - 61, Update the fenced code block in docs/index.md that renders the package layout diagram by adding a language identifier (e.g., change the opening ``` to ```text or ```plaintext) so markdownlint MD040 is satisfied; locate the block containing the tree (the lines starting with "ovos_bus_client/" and the subsequent indented file list) and replace the fence accordingly so the diagram is treated as plain text.
4-4: Minor: "API interfaces" is redundant.Since "API" stands for "Application Programming Interface," saying "API interfaces" is tautological. Consider simplifying to "APIs" or "interfaces."
📝 Suggested fix
-`ovos-bus-client` provides the WebSocket client, `Message` objects, `Session` management, and high-level API interfaces for communicating with the OVOS MessageBus. +`ovos-bus-client` provides the WebSocket client, `Message` objects, `Session` management, and high-level APIs for communicating with the OVOS MessageBus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.md` at line 4, Update the phrasing in the docs line for ovos-bus-client to remove the redundancy "API interfaces"—replace "API interfaces" with either "APIs" or "interfaces" (e.g., change the phrase in the sentence containing `ovos-bus-client` and `"Message"` to read "...high-level APIs for communicating with the OVOS MessageBus" or "...high-level interfaces for communicating with the OVOS MessageBus").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/bench_async_vs_sync.py`:
- Around line 125-148: The benchmark reuses a single Message instance so
context["session"] is injected only on the first iteration and subsequent
timings are invalid; update bench_sync_emit and bench_async_emit to create a
fresh Message for each iteration by moving the Message(...) construction inside
the synchronous do() and asynchronous do() inner functions (used by _timeit and
_atimeit) so each call to bus.emit receives a newly-created message and measures
the full emit cost.
- Around line 111-118: The _stats function currently assumes times is non-empty
and has multiple samples; modify _stats(times: List[float], label: str) to
handle empty and single-sample lists: if times is empty, print the label with
placeholders like "min=NA mean=NA median=NA max=NA stdev=NA"; if there is
exactly one sample, compute and print min/mean/median/max from that single value
but set stdev to 0.0 (or "NA" per project convention) instead of calling
statistics.stdev; ensure you still convert to milliseconds (ms = [t * 1000 for t
in times]) and reference the function name _stats to locate the change.
- Around line 212-225: The benchmark reuses the same Message objects across
rounds so session context injected on the first emit makes later rounds cheaper;
inside bench_async_concurrent_emit (referencing messages and run_all) recreate
the batch per iteration by moving the messages list (and/or the run_all local
that references it) into the for _ in range(n) loop so each timed round
constructs fresh Message(...) instances before t0 and then awaits the concurrent
emit.
In `@docs/async_client.md`:
- Line 232: Update the documentation reference for the AsyncMessageBusClient to
point to the correct source file: change any occurrence of
"client.py:AsyncMessageBusClient" to "async_client.py:AsyncMessageBusClient" in
the docs/async_client.md entry for
ovos_bus_client.client.async_client.AsyncMessageBusClient and verify there are
no other stale references to client.py for this class elsewhere in the file.
In `@docs/configuration.md`:
- Around line 114-120: The example for `/etc/mycroft/bus.conf` shows port values
as strings ("8181", "8811") which conflicts with earlier integer examples;
verify the code that parses bus config (the logic that reads the "core" and
"gui" entries and their "port" fields) to confirm it expects integers, then
update the docs so port values are consistent: replace the quoted strings with
integers (e.g., 8181, 8811) in this snippet and any other examples (or
conversely change earlier examples to quoted strings if the parser requires
strings), ensuring the "core" and "gui" entries and their "port" fields match
the parser's required type.
In `@FAQ.md`:
- Around line 30-31: The FAQ references a non-existent QUICK_FACTS.md; either
add that file with the Python requirement or inline the requirement into FAQ.md.
Fix by either creating QUICK_FACTS.md containing the Python version line
"Python: >=3.9" (and any other quick facts) and keep the FAQ reference, or
replace the reference in the "What Python versions are supported?" section of
FAQ.md with the explicit text "Currently supported Python versions: >=3.9" and
remove the QUICK_FACTS.md link; update any other README/FAQ references to match.
In `@ovos_bus_client/client/async_client.py`:
- Around line 116-162: The collector currently ignores per-handler advertised
timeouts stored by _register_handler (handlers dict) and always waits a single
fixed interval (max_timeout - min_timeout); change collect() to honor each
handler's timeout by, after the initial await asyncio.sleep(self.min_timeout),
computing remaining_time for each outstanding handler as max(0, handler_timeout
- self.min_timeout), then enter a loop that waits on self._all_collected.wait()
with a timeout equal to the smallest remaining_time (but never exceeding the
overall remaining ceiling max_timeout - time_waited), break when all responses
are received or when the overall max timeout is hit, update time_waited and
recompute remaining times each iteration (and handle new responses by checking
self.responses), finally teardown and return list(self.responses.values()) so
handlers that extended their timeout are not dropped prematurely; reference
_register_handler, collect, handlers, responses, _all_collected, min_timeout,
max_timeout.
- Around line 61-82: The wait() flow leaks callbacks when message_type has
multiple entries because only timeout branch removes handlers; ensure all
registered handlers are unregistered once any matching message arrives: after
the successful await in wait() (i.e. when asyncio.wait_for returns without
TimeoutError) iterate over self.msg_type and call self.bus.remove(mt,
self._handler) for each mt, catching ValueError/KeyError as currently done, or
alternatively perform that cleanup inside _handler right after setting
self.received_msg and self._event.set(); reference symbols: wait, _handler,
self.msg_type, self.bus.remove, self._handler.
- Around line 251-263: The recv loop currently wraps the entire async for in a
try/except so a single failure in _on_message(raw) stops the loop; change the
control flow in _recv_loop so that the async for self._ws yields inside an inner
try/except that catches and logs per-message exceptions (from await
self._on_message(raw)) using LOG.exception but continues the loop, while leaving
the outer try/except/ finally to handle real connection-level errors and only
call self._connected.clear() and self.emitter.emit("close") when the loop truly
ends; keep the existing exception types (ConnectionClosedOK,
ConnectionClosedError) handling in the outer scope and ensure other unexpected
exceptions still get LOG.exception.
---
Outside diff comments:
In `@ovos_bus_client/__init__.py`:
- Around line 34-42: The __all__ list currently unconditionally exposes
AsyncMessageBusClient even when it's None, causing imports to silently return
None; to fix, build __all__ dynamically in ovos_bus_client/__init__.py by
creating the base export list (MessageBusClient, GUIWebsocketClient, GUIMessage,
Message, send, client_from_config) and only append "AsyncMessageBusClient" to
that list when the AsyncMessageBusClient symbol is truthy (not None); ensure you
reference the exact symbol name AsyncMessageBusClient and update __all__
accordingly so imports like "from ovos_bus_client import AsyncMessageBusClient"
fail immediately with ImportError rather than returning None.
---
Nitpick comments:
In @.github/workflows/coverage.yml:
- Around line 11-18: The workflow sets env.PYTHON = '3.9' but Actions uses
actions/setup-python@v5 with python-version: 3.12; remove the unused env.PYTHON
or make them consistent by using env.PYTHON in the setup call (replace the
literal 3.12 with ${ { env.PYTHON } }), or delete the entire env block; update
the python-version in the setup-python step or remove the env definition so
env.PYTHON is not left unused.
In `@docs/index.md`:
- Around line 43-61: Update the fenced code block in docs/index.md that renders
the package layout diagram by adding a language identifier (e.g., change the
opening ``` to ```text or ```plaintext) so markdownlint MD040 is satisfied;
locate the block containing the tree (the lines starting with "ovos_bus_client/"
and the subsequent indented file list) and replace the fence accordingly so the
diagram is treated as plain text.
- Line 4: Update the phrasing in the docs line for ovos-bus-client to remove the
redundancy "API interfaces"—replace "API interfaces" with either "APIs" or
"interfaces" (e.g., change the phrase in the sentence containing
`ovos-bus-client` and `"Message"` to read "...high-level APIs for communicating
with the OVOS MessageBus" or "...high-level interfaces for communicating with
the OVOS MessageBus").
In `@docs/session.md`:
- Around line 128-131: Add a language specifier to the fenced code block that
shows the session sync protocol (the block containing "client →
ovos.session.sync" and "core → ovos.session.update_default {session_data:
{...}}"); change the opening ``` to include a language tag such as ```text or
```plaintext so the block becomes a labeled plaintext code fence for consistent
Markdown rendering.
In `@ovos_bus_client/__init__.py`:
- Around line 15-18: Add symmetric top-level exports for the async helper
classes so users can import them from the package root: in the same try/except
block that imports AsyncMessageBusClient, also attempt to import
AsyncMessageWaiter and AsyncMessageCollector from
ovos_bus_client.client.async_client (falling back to None on ImportError), and
then add "AsyncMessageBusClient", "AsyncMessageWaiter", and
"AsyncMessageCollector" to the module's __all__ list so the API surface is
consistent.
In `@pyproject.toml`:
- Around line 14-19: The pyee dependency spec in the dependencies list is
inconsistently spaced ("pyee>= 8.1.0, < 13.0.0"); update the pyee entry in the
dependencies array to match the other entries by removing the extra spaces
around the operators and comma (use the same format as
"ovos-config>=0.0.12,<3.0.0"), so the pyee spec reads without spaces around ">="
and ",".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ab83882-248a-4d81-bc1b-40f27cfdd204
📒 Files selected for processing (26)
.github/workflows/build_tests.yml.github/workflows/coverage.yml.github/workflows/downstream.yml.github/workflows/install_tests.yml.github/workflows/publish_stable.yml.github/workflows/release_workflow.yml.github/workflows/unit_tests.yml.gitignoreFAQ.mdMANIFEST.inREADME.mdbenchmarks/bench_async_vs_sync.pydocs/apis.mddocs/async_client.mddocs/client.mddocs/configuration.mddocs/index.mddocs/message.mddocs/session.mdovos_bus_client/__init__.pyovos_bus_client/client/__init__.pyovos_bus_client/client/async_client.pyovos_bus_client/version.pypyproject.tomlsetup.pytest/unittests/test_async_client.py
💤 Files with no reviewable changes (4)
- .github/workflows/install_tests.yml
- MANIFEST.in
- .github/workflows/unit_tests.yml
- setup.py
| def _stats(times: List[float], label: str): | ||
| ms = [t * 1000 for t in times] | ||
| print(f" {label}: " | ||
| f"min={min(ms):.3f}ms " | ||
| f"mean={statistics.mean(ms):.3f}ms " | ||
| f"median={statistics.median(ms):.3f}ms " | ||
| f"max={max(ms):.3f}ms " | ||
| f"stdev={statistics.stdev(ms):.3f}ms") |
There was a problem hiding this comment.
Handle empty and single-sample runs in _stats().
_stats() always calls min(), max(), and statistics.stdev(). That crashes for --n 0, and --n 1 still raises StatisticsError on stdev().
📏 Proposed fix
def _stats(times: List[float], label: str):
+ if not times:
+ raise ValueError("--n must be >= 1")
ms = [t * 1000 for t in times]
+ stdev = statistics.stdev(ms) if len(ms) > 1 else 0.0
print(f" {label}: "
f"min={min(ms):.3f}ms "
f"mean={statistics.mean(ms):.3f}ms "
f"median={statistics.median(ms):.3f}ms "
f"max={max(ms):.3f}ms "
- f"stdev={statistics.stdev(ms):.3f}ms")
+ f"stdev={stdev:.3f}ms")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/bench_async_vs_sync.py` around lines 111 - 118, The _stats
function currently assumes times is non-empty and has multiple samples; modify
_stats(times: List[float], label: str) to handle empty and single-sample lists:
if times is empty, print the label with placeholders like "min=NA mean=NA
median=NA max=NA stdev=NA"; if there is exactly one sample, compute and print
min/mean/median/max from that single value but set stdev to 0.0 (or "NA" per
project convention) instead of calling statistics.stdev; ensure you still
convert to milliseconds (ms = [t * 1000 for t in times]) and reference the
function name _stats to locate the change.
| def bench_sync_emit(n: int): | ||
| from ovos_bus_client.message import Message | ||
| bus = make_sync_bus() | ||
| msg = Message("benchmark.emit", {"payload": "x" * 128}) | ||
|
|
||
| def do(): | ||
| bus.emit(msg) | ||
|
|
||
| times = _timeit(do, n) | ||
| _stats(times, f"sync emit ×{n}") | ||
| return statistics.mean(times) | ||
|
|
||
|
|
||
| async def bench_async_emit(n: int): | ||
| from ovos_bus_client.message import Message | ||
| bus = make_async_bus() | ||
| msg = Message("benchmark.emit", {"payload": "x" * 128}) | ||
|
|
||
| async def do(): | ||
| await bus.emit(msg) | ||
|
|
||
| times = await _atimeit(do, n) | ||
| _stats(times, f"async emit ×{n}") | ||
| return statistics.mean(times) |
There was a problem hiding this comment.
Benchmark fresh messages instead of a reused, mutated one.
Both sync and async emit() inject context["session"] only when it is missing. Reusing the same msg across all iterations means only the first sample measures that work, so the emit numbers understate the cost of caller-created outbound messages.
📊 Proposed fix
def bench_sync_emit(n: int):
from ovos_bus_client.message import Message
bus = make_sync_bus()
- msg = Message("benchmark.emit", {"payload": "x" * 128})
+ payload = {"payload": "x" * 128}
def do():
- bus.emit(msg)
+ bus.emit(Message("benchmark.emit", payload.copy()))
@@
async def bench_async_emit(n: int):
from ovos_bus_client.message import Message
bus = make_async_bus()
- msg = Message("benchmark.emit", {"payload": "x" * 128})
+ payload = {"payload": "x" * 128}
async def do():
- await bus.emit(msg)
+ await bus.emit(Message("benchmark.emit", payload.copy()))🧰 Tools
🪛 Ruff (0.15.5)
[warning] 134-134: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
[warning] 147-147: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/bench_async_vs_sync.py` around lines 125 - 148, The benchmark
reuses a single Message instance so context["session"] is injected only on the
first iteration and subsequent timings are invalid; update bench_sync_emit and
bench_async_emit to create a fresh Message for each iteration by moving the
Message(...) construction inside the synchronous do() and asynchronous do()
inner functions (used by _timeit and _atimeit) so each call to bus.emit receives
a newly-created message and measures the full emit cost.
| async def bench_async_concurrent_emit(n: int, concurrency: int = 100): | ||
| """Emit `concurrency` messages concurrently, measure wall-clock time.""" | ||
| from ovos_bus_client.message import Message | ||
| bus = make_async_bus() | ||
| messages = [Message(f"bench.concurrent.{i}", {"i": i}) for i in range(concurrency)] | ||
|
|
||
| async def run_all(): | ||
| await asyncio.gather(*[bus.emit(m) for m in messages]) | ||
|
|
||
| times = [] | ||
| for _ in range(n): | ||
| t0 = time.perf_counter() | ||
| await run_all() | ||
| times.append(time.perf_counter() - t0) |
There was a problem hiding this comment.
Rebuild the concurrent message batch for each timed round.
The first round mutates every Message by injecting session context, so later rounds benchmark a cheaper code path than round 1. If this is meant to represent fresh fan-out traffic, create the batch inside the timed loop.
📦 Proposed fix
async def bench_async_concurrent_emit(n: int, concurrency: int = 100):
"""Emit `concurrency` messages concurrently, measure wall-clock time."""
from ovos_bus_client.message import Message
bus = make_async_bus()
- messages = [Message(f"bench.concurrent.{i}", {"i": i}) for i in range(concurrency)]
async def run_all():
- await asyncio.gather(*[bus.emit(m) for m in messages])
+ messages = [
+ Message(f"bench.concurrent.{i}", {"i": i})
+ for i in range(concurrency)
+ ]
+ await asyncio.gather(*(bus.emit(m) for m in messages))📝 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.
| async def bench_async_concurrent_emit(n: int, concurrency: int = 100): | |
| """Emit `concurrency` messages concurrently, measure wall-clock time.""" | |
| from ovos_bus_client.message import Message | |
| bus = make_async_bus() | |
| messages = [Message(f"bench.concurrent.{i}", {"i": i}) for i in range(concurrency)] | |
| async def run_all(): | |
| await asyncio.gather(*[bus.emit(m) for m in messages]) | |
| times = [] | |
| for _ in range(n): | |
| t0 = time.perf_counter() | |
| await run_all() | |
| times.append(time.perf_counter() - t0) | |
| async def bench_async_concurrent_emit(n: int, concurrency: int = 100): | |
| """Emit `concurrency` messages concurrently, measure wall-clock time.""" | |
| from ovos_bus_client.message import Message | |
| bus = make_async_bus() | |
| async def run_all(): | |
| messages = [ | |
| Message(f"bench.concurrent.{i}", {"i": i}) | |
| for i in range(concurrency) | |
| ] | |
| await asyncio.gather(*(bus.emit(m) for m in messages)) | |
| times = [] | |
| for _ in range(n): | |
| t0 = time.perf_counter() | |
| await run_all() | |
| times.append(time.perf_counter() - t0) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/bench_async_vs_sync.py` around lines 212 - 225, The benchmark
reuses the same Message objects across rounds so session context injected on the
first emit makes later rounds cheaper; inside bench_async_concurrent_emit
(referencing messages and run_all) recreate the batch per iteration by moving
the messages list (and/or the run_all local that references it) into the for _
in range(n) loop so each timed round constructs fresh Message(...) instances
before t0 and then awaits the concurrent emit.
|
|
||
| ### `AsyncMessageBusClient` | ||
|
|
||
| **`ovos_bus_client.client.async_client.AsyncMessageBusClient`** — `client.py:AsyncMessageBusClient` |
There was a problem hiding this comment.
Typo: incorrect file reference.
The reference says client.py:AsyncMessageBusClient but the class is defined in async_client.py.
📝 Suggested fix
-**`ovos_bus_client.client.async_client.AsyncMessageBusClient`** — `client.py:AsyncMessageBusClient`
+**`ovos_bus_client.client.async_client.AsyncMessageBusClient`**📝 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.
| **`ovos_bus_client.client.async_client.AsyncMessageBusClient`** — `client.py:AsyncMessageBusClient` | |
| **`ovos_bus_client.client.async_client.AsyncMessageBusClient`** |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/async_client.md` at line 232, Update the documentation reference for the
AsyncMessageBusClient to point to the correct source file: change any occurrence
of "client.py:AsyncMessageBusClient" to "async_client.py:AsyncMessageBusClient"
in the docs/async_client.md entry for
ovos_bus_client.client.async_client.AsyncMessageBusClient and verify there are
no other stale references to client.py for this class elsewhere in the file.
| `/etc/mycroft/bus.conf` format: | ||
| ```json | ||
| { | ||
| "core": {"route": "/core", "port": "8181"}, | ||
| "gui": {"route": "/gui", "port": "8811"} | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Inconsistent port type in example.
The /etc/mycroft/bus.conf example uses string values for ports ("8181", "8811"), while earlier examples use integers (8181, 18181). This inconsistency could confuse users. Verify which format the code expects and make examples consistent.
📝 Suggested fix (if integers are expected)
{
- "core": {"route": "/core", "port": "8181"},
- "gui": {"route": "/gui", "port": "8811"}
+ "core": {"route": "/core", "port": 8181},
+ "gui": {"route": "/gui", "port": 8811}
}📝 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.
| `/etc/mycroft/bus.conf` format: | |
| ```json | |
| { | |
| "core": {"route": "/core", "port": "8181"}, | |
| "gui": {"route": "/gui", "port": "8811"} | |
| } | |
| ``` | |
| `/etc/mycroft/bus.conf` format: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/configuration.md` around lines 114 - 120, The example for
`/etc/mycroft/bus.conf` shows port values as strings ("8181", "8811") which
conflicts with earlier integer examples; verify the code that parses bus config
(the logic that reads the "core" and "gui" entries and their "port" fields) to
confirm it expects integers, then update the docs so port values are consistent:
replace the quoted strings with integers (e.g., 8181, 8811) in this snippet and
any other examples (or conversely change earlier examples to quoted strings if
the parser requires strings), ensuring the "core" and "gui" entries and their
"port" fields match the parser's required type.
| ## What Python versions are supported? | ||
| See `QUICK_FACTS.md` — currently `>=3.9`. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if QUICK_FACTS.md exists
fd -t f "QUICK_FACTS.md" .Repository: OpenVoiceOS/ovos-bus-client
Length of output: 53
Remove reference to non-existent QUICK_FACTS.md or create the file.
The referenced file QUICK_FACTS.md does not exist in the repository. Either create this file with the Python version information or move the version requirement directly into the FAQ.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@FAQ.md` around lines 30 - 31, The FAQ references a non-existent
QUICK_FACTS.md; either add that file with the Python requirement or inline the
requirement into FAQ.md. Fix by either creating QUICK_FACTS.md containing the
Python version line "Python: >=3.9" (and any other quick facts) and keep the FAQ
reference, or replace the reference in the "What Python versions are supported?"
section of FAQ.md with the explicit text "Currently supported Python versions:
>=3.9" and remove the QUICK_FACTS.md link; update any other README/FAQ
references to match.
| def _handler(self, message: Message): | ||
| self.received_msg = message | ||
| self._event.set() | ||
|
|
||
| async def wait(self, timeout: float = 3.0) -> Optional[Message]: | ||
| """Await the expected message. | ||
|
|
||
| Arguments: | ||
| timeout: seconds before giving up | ||
|
|
||
| Returns: | ||
| The received Message or None on timeout. | ||
| """ | ||
| try: | ||
| await asyncio.wait_for(self._event.wait(), timeout) | ||
| except asyncio.TimeoutError: | ||
| for mt in self.msg_type: | ||
| try: | ||
| self.bus.remove(mt, self._handler) | ||
| except (ValueError, KeyError): | ||
| pass | ||
| return self.received_msg |
There was a problem hiding this comment.
Unregister the other once() handlers after the first match.
When message_type contains more than one event, only the listener for the winning event is auto-removed. The rest stay subscribed because cleanup only happens on timeout, so successful multi-type waits leak stale callbacks.
🧹 Proposed fix
def _handler(self, message: Message):
self.received_msg = message
+ for mt in self.msg_type:
+ if mt != message.msg_type:
+ self.bus.remove(mt, self._handler)
self._event.set()📝 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.
| def _handler(self, message: Message): | |
| self.received_msg = message | |
| self._event.set() | |
| async def wait(self, timeout: float = 3.0) -> Optional[Message]: | |
| """Await the expected message. | |
| Arguments: | |
| timeout: seconds before giving up | |
| Returns: | |
| The received Message or None on timeout. | |
| """ | |
| try: | |
| await asyncio.wait_for(self._event.wait(), timeout) | |
| except asyncio.TimeoutError: | |
| for mt in self.msg_type: | |
| try: | |
| self.bus.remove(mt, self._handler) | |
| except (ValueError, KeyError): | |
| pass | |
| return self.received_msg | |
| def _handler(self, message: Message): | |
| self.received_msg = message | |
| for mt in self.msg_type: | |
| if mt != message.msg_type: | |
| self.bus.remove(mt, self._handler) | |
| self._event.set() | |
| async def wait(self, timeout: float = 3.0) -> Optional[Message]: | |
| """Await the expected message. | |
| Arguments: | |
| timeout: seconds before giving up | |
| Returns: | |
| The received Message or None on timeout. | |
| """ | |
| try: | |
| await asyncio.wait_for(self._event.wait(), timeout) | |
| except asyncio.TimeoutError: | |
| for mt in self.msg_type: | |
| try: | |
| self.bus.remove(mt, self._handler) | |
| except (ValueError, KeyError): | |
| pass | |
| return self.received_msg |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovos_bus_client/client/async_client.py` around lines 61 - 82, The wait() flow
leaks callbacks when message_type has multiple entries because only timeout
branch removes handlers; ensure all registered handlers are unregistered once
any matching message arrives: after the successful await in wait() (i.e. when
asyncio.wait_for returns without TimeoutError) iterate over self.msg_type and
call self.bus.remove(mt, self._handler) for each mt, catching
ValueError/KeyError as currently done, or alternatively perform that cleanup
inside _handler right after setting self.received_msg and self._event.set();
reference symbols: wait, _handler, self.msg_type, self.bus.remove,
self._handler.
| def _register_handler(self, msg: Message): | ||
| handler_id = msg.data["handler"] | ||
| timeout = msg.data["timeout"] | ||
| if (msg.data["query"] == self.collect_id and | ||
| handler_id not in self.handlers): | ||
| self.handlers[handler_id] = timeout | ||
|
|
||
| def _receive_response(self, msg: Message): | ||
| if msg.data["query"] == self.collect_id: | ||
| self.responses[msg.data["handler"]] = msg | ||
| self.handlers[msg.data["handler"]] = 0 | ||
| self._queue.put_nowait(msg) | ||
| if (len(self.responses) == len(self.handlers) or | ||
| self.direct_return_func(msg)): | ||
| self._queue.put_nowait(None) | ||
| self._all_collected.set() | ||
|
|
||
| def _setup(self): | ||
| base = self.message.msg_type | ||
| self.bus.on(base + ".handling", self._register_handler) | ||
| self.bus.on(base + ".response", self._receive_response) | ||
|
|
||
| def _teardown(self): | ||
| base = self.message.msg_type | ||
| self.bus.remove(base + ".handling", self._register_handler) | ||
| self.bus.remove(base + ".response", self._receive_response) | ||
|
|
||
| async def collect(self) -> List[Message]: | ||
| """Emit the query and wait for all registered handlers to respond.""" | ||
| self._setup() | ||
| await self.bus.emit(self.message) | ||
| await asyncio.sleep(self.min_timeout) | ||
|
|
||
| if not self.handlers: | ||
| self._teardown() | ||
| return [] | ||
|
|
||
| # Wait for all handlers up to max_timeout | ||
| time_waited = self.min_timeout | ||
| deadline = self.max_timeout - self.min_timeout | ||
| try: | ||
| await asyncio.wait_for(self._all_collected.wait(), deadline) | ||
| except asyncio.TimeoutError: | ||
| pass | ||
|
|
||
| self._teardown() | ||
| return list(self.responses.values()) |
There was a problem hiding this comment.
Honor .handling timeout extensions during collection.
_register_handler() stores each handler's advertised timeout, but collect() never consults it and always waits the fixed max_timeout - min_timeout window. That makes .handling / CollectionMessage.extend(timeout) ineffective in the async collector, so a slow handler can still be dropped while it is within its declared budget.
💡 Fix direction
Drive the post-min_timeout wait from the outstanding handler timeouts, optionally clamped by max_timeout if that is meant to stay the hard ceiling, instead of a single fixed wait_for() call.
🧰 Tools
🪛 Ruff (0.15.5)
[error] 154-154: Local variable time_waited is assigned to but never used
Remove assignment to unused variable time_waited
(F841)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovos_bus_client/client/async_client.py` around lines 116 - 162, The collector
currently ignores per-handler advertised timeouts stored by _register_handler
(handlers dict) and always waits a single fixed interval (max_timeout -
min_timeout); change collect() to honor each handler's timeout by, after the
initial await asyncio.sleep(self.min_timeout), computing remaining_time for each
outstanding handler as max(0, handler_timeout - self.min_timeout), then enter a
loop that waits on self._all_collected.wait() with a timeout equal to the
smallest remaining_time (but never exceeding the overall remaining ceiling
max_timeout - time_waited), break when all responses are received or when the
overall max timeout is hit, update time_waited and recompute remaining times
each iteration (and handle new responses by checking self.responses), finally
teardown and return list(self.responses.values()) so handlers that extended
their timeout are not dropped prematurely; reference _register_handler, collect,
handlers, responses, _all_collected, min_timeout, max_timeout.
| async def _recv_loop(self): | ||
| """Receive loop — runs until the connection closes.""" | ||
| try: | ||
| async for raw in self._ws: | ||
| await self._on_message(raw) | ||
| except (ConnectionClosedOK, ConnectionClosedError): | ||
| pass | ||
| except Exception as e: | ||
| LOG.exception("AsyncMessageBusClient recv loop error: %s", e) | ||
| finally: | ||
| self._connected.clear() | ||
| self.emitter.emit("close") | ||
|
|
There was a problem hiding this comment.
Keep the receive loop alive when one message fails.
_on_message(raw) is inside the outer loop guard, so one deserialize or dispatch failure exits _recv_loop(), clears _connected, and stops all future receives. Catch and log per-message failures inside the async for body so a single bad frame does not tear down the client.
🛡️ Proposed fix
async def _recv_loop(self):
"""Receive loop — runs until the connection closes."""
try:
async for raw in self._ws:
- await self._on_message(raw)
+ try:
+ await self._on_message(raw)
+ except Exception:
+ LOG.exception("Failed to process inbound bus message")
except (ConnectionClosedOK, ConnectionClosedError):
pass🧰 Tools
🪛 Ruff (0.15.5)
[warning] 258-258: 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 `@ovos_bus_client/client/async_client.py` around lines 251 - 263, The recv loop
currently wraps the entire async for in a try/except so a single failure in
_on_message(raw) stops the loop; change the control flow in _recv_loop so that
the async for self._ws yields inside an inner try/except that catches and logs
per-message exceptions (from await self._on_message(raw)) using LOG.exception
but continues the loop, while leaving the outer try/except/ finally to handle
real connection-level errors and only call self._connected.clear() and
self.emitter.emit("close") when the loop truly ends; keep the existing exception
types (ConnectionClosedOK, ConnectionClosedError) handling in the outer scope
and ensure other unexpected exceptions still get LOG.exception.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovos_bus_client/util/__init__.py`:
- Around line 46-50: The session branch of get_message_lang returns sess.lang
directly which may be macro-normalized; update that branch to pass sess.lang
through standardize_lang_tag(..., macro=False) before returning so it matches
other paths; specifically, in the block that calls SessionManager.get(message)
and returns sess.lang, wrap the returned value with
standardize_lang_tag(sess.lang, macro=False) (use the existing
standardize_lang_tag function and SessionManager.get to locate the code).
In `@test/unittests/test_async_client.py`:
- Around line 260-264: The test test_connect_sets_connected_and_starts_recv is
using _make_bus() which pre-sets bus._connected and bus._ws, so replace that
with creating a fresh AsyncMessageBusClient instance and patching
websockets.connect (same pattern as the adjacent test) before calling
AsyncMessageBusClient.connect(); after connect() assert bus._connected.is_set()
and that bus._ws is not None and that the receive task was started (e.g., the
background receive/start_receive task created by connect()). Ensure you
reference AsyncMessageBusClient.connect, websockets.connect, _make_bus,
bus._connected and bus._ws when making the change.
- Around line 297-302: The cleanup currently swallows all exceptions from
awaiting bus._listen_task (in test_unittests/test_async_client.py), hiding real
failures; change the exception handling so only asyncio.CancelledError is
suppressed and any other exception raised when awaiting bus._listen_task is
allowed to propagate (i.e., remove the broad "except Exception: pass" and only
catch asyncio.CancelledError), ensuring regressions in the receive loop started
by connect() fail the test.
- Around line 87-93: The test currently relies on asyncio.wait_for timing rather
than emit()'s conversion logic; make the test deterministic by patching the
bus._wait_connected coroutine to immediately raise an asyncio.TimeoutError (or
TimeoutError) and then assert that bus.emit(msg) raises ValueError.
Specifically, in test_emit_connection_timeout_raises replace clearing
bus._connected with a stubbed/monkeypatched async function assigned to
bus._wait_connected that raises asyncio.TimeoutError, then use
self.assertRaises(ValueError, ...) (or await inside the context) to ensure
emit() converts the timeout to ValueError.
In `@test/unittests/test_client.py`:
- Around line 69-97: In test_on_collect replace the flaky time.sleep-based wait
with an explicit synchronization primitive: create a threading.Event (or Queue)
before registering the handler, have the handler set that event (or put an item
on the queue) when it appends to collected, then after emitting the message wait
on that event with a sensible timeout and assert it was set before performing
the same assertions (this removes reliance on ExecutorEventEmitter scheduling
and replaces the time.sleep call in test_on_collect and the analogous sleep in
the other test at lines ~200-219).
- Around line 98-112: In test_on_collect_remove, after calling
bus.remove("ev.type", handler) emit the event to verify the wrapper was detached
(e.g., call bus.client.emit("ev.type", {}) or bus.emit if available) and then
assert the handler was not invoked (assert calls == [] or assertEmpty), so the
test ensures both wrapped_funcs entry removal and actual listener deregistration
(refer to test_on_collect_remove, MessageBusClient, on_collect, remove,
wrapped_funcs).
In `@test/unittests/test_message.py`:
- Around line 237-253: Wrap the calls to encrypt_as_dict and decrypt_from_dict
in both test methods
(TestEncryptDecrypt.test_encrypt_as_dict_produces_expected_keys and
test_decrypt_from_dict_roundtrip) in a try/except ImportError block and call
self.skipTest("crypto backend not available") in the except to gracefully skip
when the optional crypto dependency isn't installed; keep the rest of the
assertions unchanged so they run only when encrypt_as_dict/decrypt_from_dict are
importable and usable.
In `@test/unittests/test_util.py`:
- Around line 90-102: The test test_lang_from_session_id_in_context currently
only asserts non-None and can miss exercising the session_id branch; modify it
to stub SessionManager.get to return the created Session (sess) when called,
then call get_message_lang(msg) and assert that the returned language equals
sess.lang (e.g., "nl-NL"); ensure you restore/undo the stub after the test.
Locate the test function test_lang_from_session_id_in_context and update it so
SessionManager.get is mocked/stubbed to return the Session instance you created,
and replace the broad assertIsNotNone(result) with an explicit
assertEqual(result, sess.lang).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4ff4b10-5a4d-40cb-8e2a-c751da1babc7
📒 Files selected for processing (5)
ovos_bus_client/util/__init__.pytest/unittests/test_async_client.pytest/unittests/test_client.pytest/unittests/test_message.pytest/unittests/test_util.py
| if "session_id" in message.context or "session" in message.context: | ||
| sess = SessionManager.get(message) | ||
| return sess.lang | ||
|
|
||
| return standardize_lang_tag(get_default_lang()) | ||
| return standardize_lang_tag(get_default_lang(), macro=False) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the exact file and lines mentioned
cat -n ovos_bus_client/util/__init__.py | sed -n '40,55p'Repository: OpenVoiceOS/ovos-bus-client
Length of output: 698
🏁 Script executed:
# Now let's find the Session.lang initialization around line 316
cat -n ovos_bus_client/session.py | sed -n '310,325p'Repository: OpenVoiceOS/ovos-bus-client
Length of output: 1264
🏁 Script executed:
# Let's see the full get_message_lang function
rg -A 10 -B 5 "def get_message_lang" ovos_bus_client/util/__init__.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 567
🏁 Script executed:
# Look for the failing test
rg -A 20 "test_lang_from_session_in_context" .Repository: OpenVoiceOS/ovos-bus-client
Length of output: 1703
🏁 Script executed:
# Find the standardize_lang_tag function definition
rg -B 5 -A 15 "def standardize_lang_tag" .Repository: OpenVoiceOS/ovos-bus-client
Length of output: 53
🏁 Script executed:
# Check if the function is from ovos_utils or defined locally
rg "from.*standardize_lang_tag|import.*standardize_lang_tag" ovos_bus_client/Repository: OpenVoiceOS/ovos-bus-client
Length of output: 228
🏁 Script executed:
# Let's also check what macro parameter does - look for docstring or usage
rg -B 2 -A 5 "macro=" . | head -100Repository: OpenVoiceOS/ovos-bus-client
Length of output: 1084
🏁 Script executed:
# Check if there are any comments or documentation about macro behavior
rg -B 3 -A 3 "macro" ovos_bus_client/ | head -50Repository: OpenVoiceOS/ovos-bus-client
Length of output: 1000
🏁 Script executed:
# Look at the test file more closely to understand what behavior is expected
cat -n ./test/unittests/test_util.py | sed -n '1,100p'Repository: OpenVoiceOS/ovos-bus-client
Length of output: 4727
🏁 Script executed:
# Check if there are any other tests that might clarify the macro behavior
rg "test_lang" ./test/unittests/test_util.py -A 10Repository: OpenVoiceOS/ovos-bus-client
Length of output: 1614
🌐 Web query:
ovos_utils standardize_lang_tag macro parameter behavior
💡 Result:
In ovos_utils.lang.standardize_lang_tag(lang_code: str, macro=True), the macro argument is passed through to langcodes.standardize_tag(tag, macro=...) when langcodes is installed. With macro=True, langcodes may replace certain specific language codes with their macrolanguage (e.g., arb → ar, cmn → zh) as part of standardization. [1][2]
If langcodes is unavailable or standardize_tag raises, ovos_utils falls back to:
macro=True: return only the primary language subtag, lowercased ("pt-BR"→"pt"). [3]macro=False: if there’s a-, format aslanguage-lower+region-UPPER("pt-br"→"pt-BR"); otherwise lowercase the whole tag. [3]
Sources: langcodes standardize_tag docs [1][2]; ovos_utils.lang.__init__.py implementation [3].
🏁 Script executed:
# Also let's check if there are any recent test results or CI logs that show the test failure
git log --oneline --all | grep -i "lang\|session" | head -20Repository: OpenVoiceOS/ovos-bus-client
Length of output: 956
🏁 Script executed:
# Check git diff to see what changed recently in these areas
git log -p --max-count=5 -- ovos_bus_client/util/__init__.py | head -150Repository: OpenVoiceOS/ovos-bus-client
Length of output: 53
Standardize the session lang output to match other code paths.
get_message_lang() returns region-preserved language tags ("pt-PT", "de-DE") from data and context paths via standardize_lang_tag(..., macro=False), and from the default path the same way. However, the session path returns sess.lang directly, which is initialized with standardize_lang_tag() using the default macro=True parameter in ovos_bus_client/session.py:316. This reduces regional variants to their base language code (e.g., "pt-BR" → "pt"), making the function return inconsistent formats depending on which branch executes.
Apply standardize_lang_tag(sess.lang, macro=False) before returning on line 48 to ensure all code paths produce the same language tag format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovos_bus_client/util/__init__.py` around lines 46 - 50, The session branch of
get_message_lang returns sess.lang directly which may be macro-normalized;
update that branch to pass sess.lang through standardize_lang_tag(...,
macro=False) before returning so it matches other paths; specifically, in the
block that calls SessionManager.get(message) and returns sess.lang, wrap the
returned value with standardize_lang_tag(sess.lang, macro=False) (use the
existing standardize_lang_tag function and SessionManager.get to locate the
code).
| async def test_emit_connection_timeout_raises(self): | ||
| bus = _make_bus() | ||
| bus._connected.clear() | ||
| msg = Message("test.timeout") | ||
| # asyncio.wait_for fires before the internal 10s wait, so we get TimeoutError | ||
| with self.assertRaises((ValueError, TimeoutError, asyncio.TimeoutError)): | ||
| await asyncio.wait_for(bus.emit(msg), timeout=0.05) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the AsyncMessageBusClient implementation
fd -e py | head -20Repository: OpenVoiceOS/ovos-bus-client
Length of output: 675
🏁 Script executed:
# Look for the async client implementation
rg "class AsyncMessageBusClient" -A 5Repository: OpenVoiceOS/ovos-bus-client
Length of output: 578
🏁 Script executed:
# Check the test file to understand the full context
cat test/unittests/test_async_client.py | head -100Repository: OpenVoiceOS/ovos-bus-client
Length of output: 3852
🏁 Script executed:
# Search for the emit() method implementation
rg "async def emit" -A 10 --type pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 870
🏁 Script executed:
# Search for _wait_connected implementation
rg "def _wait_connected|async def _wait_connected" -A 10 --type pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 872
🏁 Script executed:
# Get the full emit() method implementation
rg "async def emit" -A 30 ovos_bus_client/client/async_client.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 1155
🏁 Script executed:
# Also check if there's any error handling or ValueError raises in emit
rg "raise ValueError|raise.*Error" ovos_bus_client/client/async_client.py | head -20Repository: OpenVoiceOS/ovos-bus-client
Length of output: 166
🏁 Script executed:
# Get more context around emit to see the full method
cat ovos_bus_client/client/async_client.py | sed -n '/async def emit/,/^ async def/p' | head -50Repository: OpenVoiceOS/ovos-bus-client
Length of output: 1061
This timeout test isn't asserting emit()'s contract.
The outer asyncio.wait_for(..., 0.05) fires before the internal 10-second wait in emit(), raising asyncio.TimeoutError that satisfies the test without proving emit() properly converts timeouts to ValueError. Patch _wait_connected() to raise immediately and assert ValueError to verify the actual conversion logic.
More deterministic version
async def test_emit_connection_timeout_raises(self):
bus = _make_bus()
bus._connected.clear()
msg = Message("test.timeout")
- # asyncio.wait_for fires before the internal 10s wait, so we get TimeoutError
- with self.assertRaises((ValueError, TimeoutError, asyncio.TimeoutError)):
- await asyncio.wait_for(bus.emit(msg), timeout=0.05)
+ with patch.object(bus, "_wait_connected",
+ AsyncMock(side_effect=asyncio.TimeoutError)):
+ with self.assertRaises(ValueError):
+ await bus.emit(msg)📝 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.
| async def test_emit_connection_timeout_raises(self): | |
| bus = _make_bus() | |
| bus._connected.clear() | |
| msg = Message("test.timeout") | |
| # asyncio.wait_for fires before the internal 10s wait, so we get TimeoutError | |
| with self.assertRaises((ValueError, TimeoutError, asyncio.TimeoutError)): | |
| await asyncio.wait_for(bus.emit(msg), timeout=0.05) | |
| async def test_emit_connection_timeout_raises(self): | |
| bus = _make_bus() | |
| bus._connected.clear() | |
| msg = Message("test.timeout") | |
| with patch.object(bus, "_wait_connected", | |
| AsyncMock(side_effect=asyncio.TimeoutError)): | |
| with self.assertRaises(ValueError): | |
| await bus.emit(msg) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_async_client.py` around lines 87 - 93, The test currently
relies on asyncio.wait_for timing rather than emit()'s conversion logic; make
the test deterministic by patching the bus._wait_connected coroutine to
immediately raise an asyncio.TimeoutError (or TimeoutError) and then assert that
bus.emit(msg) raises ValueError. Specifically, in
test_emit_connection_timeout_raises replace clearing bus._connected with a
stubbed/monkeypatched async function assigned to bus._wait_connected that raises
asyncio.TimeoutError, then use self.assertRaises(ValueError, ...) (or await
inside the context) to ensure emit() converts the timeout to ValueError.
| async def test_connect_sets_connected_and_starts_recv(self): | ||
| bus = _make_bus() | ||
| # Already "connected" via _make_bus; verify state is correct | ||
| self.assertTrue(bus._connected.is_set()) | ||
| self.assertIsNotNone(bus._ws) |
There was a problem hiding this comment.
test_connect_sets_connected_and_starts_recv never calls connect().
_make_bus() pre-sets _connected and _ws, so this will pass even if AsyncMessageBusClient.connect() is broken. Build a fresh client and patch websockets.connect here like the next test does.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_async_client.py` around lines 260 - 264, The test
test_connect_sets_connected_and_starts_recv is using _make_bus() which pre-sets
bus._connected and bus._ws, so replace that with creating a fresh
AsyncMessageBusClient instance and patching websockets.connect (same pattern as
the adjacent test) before calling AsyncMessageBusClient.connect(); after
connect() assert bus._connected.is_set() and that bus._ws is not None and that
the receive task was started (e.g., the background receive/start_receive task
created by connect()). Ensure you reference AsyncMessageBusClient.connect,
websockets.connect, _make_bus, bus._connected and bus._ws when making the
change.
| if bus._listen_task: | ||
| bus._listen_task.cancel() | ||
| try: | ||
| await bus._listen_task | ||
| except (asyncio.CancelledError, Exception): | ||
| pass |
There was a problem hiding this comment.
Don’t swallow _listen_task failures during cleanup.
Catching Exception and pass here hides receive-loop regressions, so this test can still pass after connect() starts a broken task. Only suppress asyncio.CancelledError; let anything else fail the test.
Safer cleanup
- try:
- await bus._listen_task
- except (asyncio.CancelledError, Exception):
- pass
+ try:
+ await bus._listen_task
+ except asyncio.CancelledError:
+ pass📝 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.
| if bus._listen_task: | |
| bus._listen_task.cancel() | |
| try: | |
| await bus._listen_task | |
| except (asyncio.CancelledError, Exception): | |
| pass | |
| if bus._listen_task: | |
| bus._listen_task.cancel() | |
| try: | |
| await bus._listen_task | |
| except asyncio.CancelledError: | |
| pass |
🧰 Tools
🪛 Ruff (0.15.5)
[error] 301-302: try-except-pass detected, consider logging the exception
(S110)
[warning] 301-301: 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_async_client.py` around lines 297 - 302, The cleanup
currently swallows all exceptions from awaiting bus._listen_task (in
test_unittests/test_async_client.py), hiding real failures; change the exception
handling so only asyncio.CancelledError is suppressed and any other exception
raised when awaiting bus._listen_task is allowed to propagate (i.e., remove the
broad "except Exception: pass" and only catch asyncio.CancelledError), ensuring
regressions in the receive loop started by connect() fail the test.
| def test_on_collect(self): | ||
| # TODO | ||
| pass | ||
| with patch("ovos_bus_client.client.client.load_message_bus_config") as mock_cfg: | ||
| mock_cfg.return_value = MagicMock(host="localhost", port=8181, | ||
| route="/core", ssl=False) | ||
| bus = MessageBusClient() | ||
| bus.client = MagicMock() | ||
| bus.connected_event.set() | ||
| bus.started_running = True | ||
|
|
||
| collected = [] | ||
|
|
||
| def handler(cm): | ||
| collected.append(cm) | ||
|
|
||
| bus.on_collect("test.query", handler, timeout=3) | ||
| # Simulate incoming query message with __collect_id__ in context | ||
| from ovos_bus_client.message import CollectionMessage | ||
| query_msg = Message("test.query", {}, context={"__collect_id__": "cid123"}) | ||
| bus.emitter.emit("test.query", query_msg) | ||
|
|
||
| import time | ||
| time.sleep(0.1) | ||
| self.assertEqual(len(collected), 1) | ||
| cm = collected[0] | ||
| self.assertIsInstance(cm, CollectionMessage) | ||
| self.assertEqual(cm.query_id, "cid123") | ||
| # Verify acknowledgement was sent via bus.client.send | ||
| bus.client.send.assert_called() | ||
|
|
There was a problem hiding this comment.
Replace the fixed sleeps with an explicit signal.
These assertions depend on time.sleep(0.1) being long enough for ExecutorEventEmitter callbacks to run, which will get flaky under CI load. Wait on a threading.Event or queue entry set by the handler instead of sleeping.
Also applies to: 200-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_client.py` around lines 69 - 97, In test_on_collect
replace the flaky time.sleep-based wait with an explicit synchronization
primitive: create a threading.Event (or Queue) before registering the handler,
have the handler set that event (or put an item on the queue) when it appends to
collected, then after emitting the message wait on that event with a sensible
timeout and assert it was set before performing the same assertions (this
removes reliance on ExecutorEventEmitter scheduling and replaces the time.sleep
call in test_on_collect and the analogous sleep in the other test at lines
~200-219).
| def test_on_collect_remove(self): | ||
| with patch("ovos_bus_client.client.client.load_message_bus_config") as mock_cfg: | ||
| mock_cfg.return_value = MagicMock(host="localhost", port=8181, | ||
| route="/core", ssl=False) | ||
| bus = MessageBusClient() | ||
| bus.client = MagicMock() | ||
| bus.connected_event.set() | ||
| bus.started_running = True | ||
|
|
||
| calls = [] | ||
| handler = lambda cm: calls.append(cm) | ||
| bus.on_collect("ev.type", handler, timeout=1) | ||
| self.assertIn(handler, bus.wrapped_funcs) | ||
| bus.remove("ev.type", handler) | ||
| self.assertNotIn(handler, bus.wrapped_funcs) |
There was a problem hiding this comment.
Verify remove() detaches the wrapper from the emitter too.
This only proves that wrapped_funcs was popped. If _remove_normal() fails, the listener can still be registered and this test will stay green. Emit ev.type after remove() and assert the callback is not invoked.
🧰 Tools
🪛 Ruff (0.15.5)
[error] 108-108: Do not assign a lambda expression, use a def
Rewrite handler as a def
(E731)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_client.py` around lines 98 - 112, In
test_on_collect_remove, after calling bus.remove("ev.type", handler) emit the
event to verify the wrapper was detached (e.g., call bus.client.emit("ev.type",
{}) or bus.emit if available) and then assert the handler was not invoked
(assert calls == [] or assertEmpty), so the test ensures both wrapped_funcs
entry removal and actual listener deregistration (refer to
test_on_collect_remove, MessageBusClient, on_collect, remove, wrapped_funcs).
| class TestEncryptDecrypt(unittest.TestCase): | ||
| def test_encrypt_as_dict_produces_expected_keys(self): | ||
| from ovos_bus_client.message import encrypt_as_dict, decrypt_from_dict | ||
| key = "supersecretkey16" | ||
| plaintext = '{"type": "test", "data": {}, "context": {}}' | ||
| result = encrypt_as_dict(key, plaintext) | ||
| self.assertIn("ciphertext", result) | ||
| self.assertIn("tag", result) | ||
| self.assertIn("nonce", result) | ||
|
|
||
| def test_decrypt_from_dict_roundtrip(self): | ||
| from ovos_bus_client.message import encrypt_as_dict, decrypt_from_dict | ||
| key = "supersecretkey16" | ||
| plaintext = '{"type": "test", "data": {}, "context": {}}' | ||
| enc = encrypt_as_dict(key, plaintext) | ||
| dec = decrypt_from_dict(key, enc) | ||
| self.assertEqual(dec, plaintext) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 101
🏁 Script executed:
git ls-files ovos_bus_client/message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 97
🏁 Script executed:
rg -n "encrypt_as_dict|decrypt_from_dict" --type pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 1287
🏁 Script executed:
sed -n '241,270p' ovos_bus_client/message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 1206
🏁 Script executed:
sed -n '248,280p' ovos_bus_client/message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 1245
🏁 Script executed:
sed -n '237,260p' test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 1149
🏁 Script executed:
sed -n '1,50p' ovos_bus_client/message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 2010
🏁 Script executed:
rg -n "^def encrypt|^def decrypt" ovos_bus_client/message.py -A 5Repository: OpenVoiceOS/ovos-bus-client
Length of output: 663
🏁 Script executed:
rg -n "ovos_utils" ovos_bus_client/message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 171
🏁 Script executed:
git ls-files setup.py setup.cfg pyproject.toml requirements.txtRepository: OpenVoiceOS/ovos-bus-client
Length of output: 102
🏁 Script executed:
cat requirements.txtRepository: OpenVoiceOS/ovos-bus-client
Length of output: 170
🏁 Script executed:
cat pyproject.tomlRepository: OpenVoiceOS/ovos-bus-client
Length of output: 1399
🏁 Script executed:
rg -n "skipTest|ImportError|unittest.skip" test/unittests/test_message.py -B 2 -A 2Repository: OpenVoiceOS/ovos-bus-client
Length of output: 53
🏁 Script executed:
rg -n "try:.*encrypt\|except.*Import" test/unittests/test_message.py -A 3Repository: OpenVoiceOS/ovos-bus-client
Length of output: 53
🏁 Script executed:
sed -n '100,115p' test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 790
🏁 Script executed:
rg -n "except.*ImportError" test/unittests/test_message.py -B 3 -A 3Repository: OpenVoiceOS/ovos-bus-client
Length of output: 53
🏁 Script executed:
rg -rn "except.*ImportError" --type py test/Repository: OpenVoiceOS/ovos-bus-client
Length of output: 189
🏁 Script executed:
git log --oneline --all test/unittests/test_message.py | head -20Repository: OpenVoiceOS/ovos-bus-client
Length of output: 432
🏁 Script executed:
sed -n '237,260p' test/unittests/test_message.py | cat -nRepository: OpenVoiceOS/ovos-bus-client
Length of output: 1317
🏁 Script executed:
rg -n "except.*ModuleNotFoundError" test/unittests/test_client.py -B 3 -A 3 | head -20Repository: OpenVoiceOS/ovos-bus-client
Length of output: 278
🏁 Script executed:
python3 -c "from ovos_utils.security import encrypt, decrypt; print('Crypto available')" 2>&1 || echo "Crypto not available"Repository: OpenVoiceOS/ovos-bus-client
Length of output: 215
🏁 Script executed:
python3 << 'EOF'
try:
from ovos_utils.security import encrypt, decrypt
print("encrypt and decrypt imported successfully")
except ImportError as e:
print(f"ImportError: {e}")
except Exception as e:
print(f"Other error: {type(e).__name__}: {e}")
EOFRepository: OpenVoiceOS/ovos-bus-client
Length of output: 112
🏁 Script executed:
git ls-files .githubRepository: OpenVoiceOS/ovos-bus-client
Length of output: 309
🏁 Script executed:
find . -name "*.yml" -o -name "*.yaml" | grep -E "(\.github|\.gitea|\.gitlab)" | head -10Repository: OpenVoiceOS/ovos-bus-client
Length of output: 323
🏁 Script executed:
cat .github/workflows/build_tests.ymlRepository: OpenVoiceOS/ovos-bus-client
Length of output: 346
🏁 Script executed:
sed -n '100,130p' test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 1270
🏁 Script executed:
rg -n "class.*Test.*Encrypt\|def test.*encrypt\|def test.*decrypt" test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 53
🏁 Script executed:
rg -n "def test_encrypt\|def test_decrypt" test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 53
🏁 Script executed:
wc -l test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 105
🏁 Script executed:
tail -50 test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 2146
🏁 Script executed:
sed -n '237,253p' test/unittests/test_message.pyRepository: OpenVoiceOS/ovos-bus-client
Length of output: 877
🏁 Script executed:
rg -n "class TestEncryptDecrypt" test/unittests/test_message.py -A 20Repository: OpenVoiceOS/ovos-bus-client
Length of output: 1064
Guard the encryption tests against unavailable crypto backend.
The tests call encrypt_as_dict and decrypt_from_dict without handling the case where the crypto backend is not installed. When the optional crypto dependency is unavailable, these tests will fail hard rather than skip gracefully. Wrap the encryption calls in try/except to skip tests on ImportError, consistent with the pattern used elsewhere in the test suite.
Suggested fix
class TestEncryptDecrypt(unittest.TestCase):
def test_encrypt_as_dict_produces_expected_keys(self):
from ovos_bus_client.message import encrypt_as_dict, decrypt_from_dict
key = "supersecretkey16"
plaintext = '{"type": "test", "data": {}, "context": {}}'
- result = encrypt_as_dict(key, plaintext)
+ try:
+ result = encrypt_as_dict(key, plaintext)
+ except ImportError as e:
+ self.skipTest(str(e))
self.assertIn("ciphertext", result)
self.assertIn("tag", result)
self.assertIn("nonce", result)
def test_decrypt_from_dict_roundtrip(self):
from ovos_bus_client.message import encrypt_as_dict, decrypt_from_dict
key = "supersecretkey16"
plaintext = '{"type": "test", "data": {}, "context": {}}'
- enc = encrypt_as_dict(key, plaintext)
- dec = decrypt_from_dict(key, enc)
+ try:
+ enc = encrypt_as_dict(key, plaintext)
+ dec = decrypt_from_dict(key, enc)
+ except ImportError as e:
+ self.skipTest(str(e))
self.assertEqual(dec, plaintext)🧰 Tools
🪛 GitHub Actions: Run Build Tests
[error] 251-251: Encrypt/Decrypt from dict roundtrip failed due to ImportError in encryption backend.
[error] 242-242: Encrypt_as_dict produced keys but encryption backend unavailable (ImportError); test cannot complete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_message.py` around lines 237 - 253, Wrap the calls to
encrypt_as_dict and decrypt_from_dict in both test methods
(TestEncryptDecrypt.test_encrypt_as_dict_produces_expected_keys and
test_decrypt_from_dict_roundtrip) in a try/except ImportError block and call
self.skipTest("crypto backend not available") in the except to gracefully skip
when the optional crypto dependency isn't installed; keep the rest of the
assertions unchanged so they run only when encrypt_as_dict/decrypt_from_dict are
importable and usable.
| def test_lang_from_session_id_in_context(self): | ||
| sess = Session("sid-lang-test") | ||
| sess.lang = "nl-NL" | ||
| SessionManager.sessions[sess.session_id] = sess | ||
| # session_id alone triggers session path but SessionManager.get | ||
| # falls back to default session when only session_id key is present | ||
| # (no full session data). Verify the path is taken without asserting | ||
| # the exact lang value (depends on default session state). | ||
| msg = Message("test", data={}, | ||
| context={"session_id": sess.session_id}) | ||
| result = get_message_lang(msg) | ||
| self.assertIsNotNone(result) | ||
|
|
There was a problem hiding this comment.
Don’t let the session_id path pass with a no-op assertion.
This test stays green even if get_message_lang() ignores the registered session and falls back to unrelated state. Since the goal is to cover the session_id branch, stub SessionManager.get() here and assert the returned language explicitly.
Suggested test tightening
def test_lang_from_session_id_in_context(self):
sess = Session("sid-lang-test")
sess.lang = "nl-NL"
- SessionManager.sessions[sess.session_id] = sess
- # session_id alone triggers session path but SessionManager.get
- # falls back to default session when only session_id key is present
- # (no full session data). Verify the path is taken without asserting
- # the exact lang value (depends on default session state).
msg = Message("test", data={},
context={"session_id": sess.session_id})
- result = get_message_lang(msg)
- self.assertIsNotNone(result)
+ with patch("ovos_bus_client.util.SessionManager.get", return_value=sess):
+ result = get_message_lang(msg)
+ self.assertEqual(result, "nl-NL")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_util.py` around lines 90 - 102, The test
test_lang_from_session_id_in_context currently only asserts non-None and can
miss exercising the session_id branch; modify it to stub SessionManager.get to
return the created Session (sess) when called, then call get_message_lang(msg)
and assert that the returned language equals sess.lang (e.g., "nl-NL"); ensure
you restore/undo the stub after the test. Locate the test function
test_lang_from_session_id_in_context and update it so SessionManager.get is
mocked/stubbed to return the Session instance you created, and replace the broad
assertIsNotNone(result) with an explicit assertEqual(result, sess.lang).
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation