-
Notifications
You must be signed in to change notification settings - Fork 0
PR-03: DBus IPC (Service, Client, Signals) #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,96 @@ | ||||||
| """DBus client for testing and interacting with Shortcut Sage daemon.""" | ||||||
|
|
||||||
| import json | ||||||
| import logging | ||||||
| from collections.abc import Callable | ||||||
| from typing import Any | ||||||
|
|
||||||
| logger = logging.getLogger(__name__) | ||||||
|
|
||||||
| # Try to import DBus, but allow fallback if not available | ||||||
| try: | ||||||
| import dbus | ||||||
|
|
||||||
| DBUS_AVAILABLE = True | ||||||
| except ImportError: | ||||||
| DBUS_AVAILABLE = False | ||||||
| logger.warning("DBus not available, client will not work") | ||||||
|
|
||||||
|
|
||||||
| class DBusClient: | ||||||
| """Client for interacting with Shortcut Sage DBus daemon.""" | ||||||
|
|
||||||
| BUS_NAME = "org.shortcutsage.Daemon" | ||||||
| OBJECT_PATH = "/org/shortcutsage/Daemon" | ||||||
| INTERFACE = "org.shortcutsage.Daemon" | ||||||
|
|
||||||
| def __init__(self) -> None: | ||||||
| """Initialize the DBus client.""" | ||||||
| if not DBUS_AVAILABLE: | ||||||
| raise ImportError("DBus not available") | ||||||
|
|
||||||
| self.bus = dbus.SessionBus() | ||||||
| self.proxy = self.bus.get_object(self.BUS_NAME, self.OBJECT_PATH) | ||||||
| self.interface = dbus.Interface(self.proxy, dbus_interface=self.INTERFACE) | ||||||
|
|
||||||
| def send_event(self, event_json: str | dict[str, Any]) -> None: | ||||||
| """Send an event to the daemon. | ||||||
|
|
||||||
| Args: | ||||||
| event_json: Event as JSON string or dict. If dict, will be serialized. | ||||||
|
|
||||||
| Raises: | ||||||
| dbus.DBusException: If the daemon is not running or the call fails. | ||||||
| """ | ||||||
| if isinstance(event_json, dict): | ||||||
| event_json = json.dumps(event_json) | ||||||
|
|
||||||
| self.interface.SendEvent(event_json) | ||||||
| logger.debug(f"Sent event: {event_json}") | ||||||
|
||||||
| logger.debug(f"Sent event: {event_json}") | |
| logger.debug("Sent event: %s", event_json) |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above - the f-string in the debug log will be evaluated even when debug logging is disabled. Use lazy logging: logger.debug("Ping result: %s", result).
| logger.debug(f"Ping result: {result}") | |
| logger.debug("Ping result: %s", result) |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue - use lazy logging: logger.debug("Received suggestions: %s", suggestions_json).
| logger.debug(f"Received suggestions: {suggestions_json}") | |
| logger.debug("Received suggestions: %s", suggestions_json) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,252 @@ | ||||||||||||||||||||||||||||||||||
| """Integration tests for DBus IPC.""" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||||||||
| from datetime import datetime | ||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import pytest | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| from sage.dbus_client import DBUS_AVAILABLE | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Skip all tests if DBus is not available | ||||||||||||||||||||||||||||||||||
| pytestmark = pytest.mark.skipif(not DBUS_AVAILABLE, reason="DBus not available") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if DBUS_AVAILABLE: | ||||||||||||||||||||||||||||||||||
| import dbus | ||||||||||||||||||||||||||||||||||
| from dbus.mainloop.glib import DBusGMainLoop | ||||||||||||||||||||||||||||||||||
| from gi.repository import GLib # type: ignore[import-not-found] | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| from sage.dbus_client import DBusClient | ||||||||||||||||||||||||||||||||||
| from sage.dbus_daemon import Daemon | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @pytest.fixture | ||||||||||||||||||||||||||||||||||
| def temp_config_dir(tmp_path: Path) -> Path: | ||||||||||||||||||||||||||||||||||
| """Create a temporary config directory with test configuration.""" | ||||||||||||||||||||||||||||||||||
| config_dir = tmp_path / "config" | ||||||||||||||||||||||||||||||||||
| config_dir.mkdir() | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Create minimal shortcuts.yaml | ||||||||||||||||||||||||||||||||||
| shortcuts_yaml = config_dir / "shortcuts.yaml" | ||||||||||||||||||||||||||||||||||
| shortcuts_yaml.write_text( | ||||||||||||||||||||||||||||||||||
| """version: "1.0" | ||||||||||||||||||||||||||||||||||
| shortcuts: | ||||||||||||||||||||||||||||||||||
| - key: "Meta+Tab" | ||||||||||||||||||||||||||||||||||
| action: "overview" | ||||||||||||||||||||||||||||||||||
| description: "Show overview" | ||||||||||||||||||||||||||||||||||
| category: "desktop" | ||||||||||||||||||||||||||||||||||
| - key: "Meta+Left" | ||||||||||||||||||||||||||||||||||
| action: "tile_left" | ||||||||||||||||||||||||||||||||||
| description: "Tile window left" | ||||||||||||||||||||||||||||||||||
| category: "window" | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Create minimal rules.yaml | ||||||||||||||||||||||||||||||||||
| rules_yaml = config_dir / "rules.yaml" | ||||||||||||||||||||||||||||||||||
| rules_yaml.write_text( | ||||||||||||||||||||||||||||||||||
| """version: "1.0" | ||||||||||||||||||||||||||||||||||
| rules: | ||||||||||||||||||||||||||||||||||
| - name: "after_show_desktop" | ||||||||||||||||||||||||||||||||||
| context: | ||||||||||||||||||||||||||||||||||
| type: "event_sequence" | ||||||||||||||||||||||||||||||||||
| pattern: ["show_desktop"] | ||||||||||||||||||||||||||||||||||
| window: 3 | ||||||||||||||||||||||||||||||||||
| suggest: | ||||||||||||||||||||||||||||||||||
| - action: "overview" | ||||||||||||||||||||||||||||||||||
| priority: 80 | ||||||||||||||||||||||||||||||||||
| cooldown: 300 | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return config_dir | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| @pytest.fixture | ||||||||||||||||||||||||||||||||||
| def daemon_process(temp_config_dir: Path) -> Daemon: | ||||||||||||||||||||||||||||||||||
| """Start a daemon process for testing.""" | ||||||||||||||||||||||||||||||||||
| # Initialize DBus main loop | ||||||||||||||||||||||||||||||||||
| DBusGMainLoop(set_as_default=True) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Create daemon with test configuration | ||||||||||||||||||||||||||||||||||
| log_dir = temp_config_dir.parent / "logs" | ||||||||||||||||||||||||||||||||||
| daemon = Daemon(str(temp_config_dir), enable_dbus=True, log_dir=log_dir) | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Start the daemon in a background thread | ||||||||||||||||||||||||||||||||||
| daemon.start() | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+68
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎.
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Give daemon time to initialize | ||||||||||||||||||||||||||||||||||
| time.sleep(0.5) | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+81
to
+82
|
||||||||||||||||||||||||||||||||||
| # Give daemon time to initialize | |
| time.sleep(0.5) | |
| # Wait for daemon to be ready by polling DBus | |
| timeout = 5.0 # seconds | |
| interval = 0.05 # seconds | |
| start_time = time.time() | |
| while True: | |
| try: | |
| client = DBusClient() | |
| if client.ping() == "pong": | |
| break | |
| except Exception: | |
| pass | |
| if time.time() - start_time > timeout: | |
| raise RuntimeError("Timed out waiting for daemon to be ready") | |
| time.sleep(interval) |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous test, the hard-coded time.sleep(0.1) wait times make the tests potentially flaky. Consider using polling with assertions or implementing a proper synchronization mechanism to verify that events have been processed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions signal asserted although daemon never emits it
test_suggestions_signal subscribes to Suggestions and asserts a non‑empty received_suggestions, but Daemon.emit_suggestions() currently only returns the JSON string and does not emit any DBus signal when DBus is enabled. Even if a bus service is started, the callback will never fire and this assertion will always fail; external clients also cannot receive suggestions. The daemon needs to emit the Suggestions signal (e.g. by calling the DBus signal defined in dbus_daemon.py) before this test or client code can work.
Useful? React with 👍 / 👎.
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expects the daemon to emit suggestions via DBus signal, but there's no mechanism to actually trigger the signal emission. Looking at the Daemon code, emit_suggestions() returns the JSON string but doesn't actually emit a DBus signal when DBus is enabled. The Daemon class defines emit_suggestions() but doesn't call the DBus signal method. This test will likely fail because no signal will be received.
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception type check uses dbus.exceptions.DBusException which may not exist depending on the dbus-python version. The dbus library typically uses dbus.DBusException directly. The dbus.exceptions.DBusException reference appears to be redundant and may cause issues if that submodule doesn't exist. Consider using only dbus.DBusException in the exception tuple.
| with pytest.raises((dbus.DBusException, dbus.exceptions.DBusException)): # type: ignore[attr-defined] | |
| with pytest.raises(dbus.DBusException): |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DBusClient constructor attempts to get a DBus object that may not exist yet, which will raise an exception. However, the test test_dbus_error_handling expects the exception to be raised when calling client.ping(), not during DBusClient() construction. This means the test logic is incorrect - the exception will be raised on line 251 (during client creation) not line 252 (during ping). Consider moving the DBusClient instantiation inside the with pytest.raises() block.
| client.ping() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring states the method raises
dbus.DBusException, but based on the error handling in other parts of the codebase and common dbus-python behavior, it could also raisedbus.exceptions.DBusExceptionor other DBus-related exceptions. Consider being more general in the documentation, e.g., "Raises: DBus-related exceptions if the daemon is not running or the call fails."