Kacper PaczosandGitHub
b7bfdd2dd1
* fix(config): share one ConfigManager across the process main(), the tray, and the sound-effects check each constructed their own ConfigManager. Every instance loads the whole config into memory at construction time and save_config() writes that whole cache back, so any save from a stale instance silently reverts what another instance saved in the meantime — no race needed, the second save just wins with old values. The disk-read workarounds in main (_should_append_trailing_space) already existed to dodge exactly this. Add get_shared_config_manager() and route the three call sites through it. It keeps ConfigManager constructible for tests, and the tray/dialog pair already shared one instance, so this extends that arrangement to the rest of the process. Tests: a regression test runs the two-instance scenario through the accessor, and a source guard fails if application code constructs ConfigManager directly again. conftest drops the shared instance between tests, since it would otherwise be created under whichever mocks the first caller had active and leak them into later tests. The two single-instance tests now patch acquire_lock itself: they used patch.dict(sys.modules, ...), which main()'s "from . import" ignores once an earlier test has imported the real module — they only passed through an unrelated mock leaking into the tray's shortcut setup, which this change stops. * fix(config): lock the shared accessor and prove the revert it prevents Review follow-ups on #691: - guard the lazy create with a lock, so two threads racing the first call cannot each build a ConfigManager and reintroduce #689 - take both handles before the first save in the accessor test, and assert identity there — the old order let a fresh ConfigManager pass - add a characterization test: two direct ConfigManager instances still revert each other, which is the footgun the accessor exists to hide - patch get_shared_config_manager in test_main and test_audio_feedback instead of the class, so a leftover singleton cannot skip the mock - close the ExitStack in _boot_under_patches when main() raises; the caller only closes a stack it received, so a failure there leaked those patches into every later test * fix(audio): route the tone lookup through the shared ConfigManager The tone picker merged on main (#707) constructs its own ConfigManager, which is exactly the stale-cache overwrite this branch removes; the new source guard in test_shared_config_manager.py flags it.
99 lines
3.5 KiB
Python
99 lines
3.5 KiB
Python
"""Tests for the process-wide shared ConfigManager."""
|
|
|
|
import json
|
|
import os
|
|
|
|
import pytest
|
|
|
|
from vocalinux.ui import config_manager as cm
|
|
|
|
|
|
@pytest.fixture
|
|
def isolated_config(tmp_path, monkeypatch):
|
|
"""Point the config file at a temp dir and reset the shared instance."""
|
|
monkeypatch.setattr(cm, "CONFIG_DIR", str(tmp_path))
|
|
monkeypatch.setattr(cm, "CONFIG_FILE", str(tmp_path / "config.json"))
|
|
monkeypatch.setattr(cm, "_shared_instance", None)
|
|
yield tmp_path
|
|
cm._shared_instance = None
|
|
|
|
|
|
def _seed_config(engine, model_size):
|
|
"""Write a config file that both readers will load before any save."""
|
|
with open(cm.CONFIG_FILE, "w") as handle:
|
|
json.dump({"speech_recognition": {"engine": engine, "model_size": model_size}}, handle)
|
|
|
|
|
|
def _read_config():
|
|
with open(cm.CONFIG_FILE) as handle:
|
|
return json.load(handle)
|
|
|
|
|
|
def test_accessor_returns_one_instance(isolated_config):
|
|
assert cm.get_shared_config_manager() is cm.get_shared_config_manager()
|
|
|
|
|
|
def test_saves_through_the_accessor_do_not_revert_each_other(isolated_config):
|
|
"""The failure mode of separate instances, run through the accessor.
|
|
|
|
Both handles are taken before either save, the way two collaborators
|
|
(tray and audio feedback) grab one at startup. With two ConfigManagers
|
|
the second save would win with its stale cache and revert the first
|
|
one's engine; one shared instance keeps every earlier save in the file.
|
|
"""
|
|
_seed_config(engine="vosk", model_size="medium")
|
|
|
|
first = cm.get_shared_config_manager()
|
|
second = cm.get_shared_config_manager()
|
|
assert first is second
|
|
|
|
first.update_speech_recognition_settings({"engine": "whisper_cpp", "model_size": "small"})
|
|
first.save_config()
|
|
|
|
second.set("general", "autostart", True)
|
|
second.save_config()
|
|
|
|
on_disk = _read_config()
|
|
assert on_disk["speech_recognition"]["engine"] == "whisper_cpp"
|
|
assert on_disk["speech_recognition"]["model_size"] == "small"
|
|
assert on_disk["general"]["autostart"] is True
|
|
|
|
|
|
def test_two_instances_still_revert_each_other(isolated_config):
|
|
"""Characterization of the bug the accessor exists to prevent (#689).
|
|
|
|
Constructing ConfigManager directly is still possible, and it still
|
|
loses writes. This is what the production call sites used to do, and
|
|
what a future one would reintroduce.
|
|
"""
|
|
_seed_config(engine="vosk", model_size="medium")
|
|
|
|
first = cm.ConfigManager()
|
|
second = cm.ConfigManager()
|
|
|
|
first.update_speech_recognition_settings({"engine": "whisper_cpp", "model_size": "small"})
|
|
first.save_config()
|
|
|
|
second.set("general", "autostart", True)
|
|
second.save_config()
|
|
|
|
on_disk = _read_config()
|
|
assert on_disk["speech_recognition"]["engine"] == "vosk"
|
|
assert on_disk["speech_recognition"]["model_size"] == "medium"
|
|
|
|
|
|
def test_application_code_does_not_construct_its_own_instance():
|
|
"""Source guard: a direct ConfigManager() anywhere in src reintroduces the
|
|
stale-cache overwrite, so only the defining module may construct one."""
|
|
src_root = os.path.join(os.path.dirname(__file__), "..", "src", "vocalinux")
|
|
offenders = []
|
|
for root, _dirs, files in os.walk(src_root):
|
|
for name in files:
|
|
if not name.endswith(".py") or name == "config_manager.py":
|
|
continue
|
|
path = os.path.join(root, name)
|
|
with open(path, "r") as handle:
|
|
if "ConfigManager()" in handle.read():
|
|
offenders.append(os.path.relpath(path, src_root))
|
|
assert offenders == []
|