mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-10 18:59:56 +00:00
One word. server.py's TuningProviderRegistry.get_merged():
except Exception:
- logger.exception("tuning provider %r raised during get_merged()", provider_id)
+ log.exception("tuning provider %r raised during get_merged()", provider_id)
There is no `logger` in server.py — the module logger is `log`. So the handler written to
swallow-and-report a bad provider instead raised NameError from inside the except, and that
NameError propagated out of get_merged().
The effect was the exact OPPOSITE of what the handler is for: one misbehaving plugin took
the whole merged-tunings call down for every other provider, AND the traceback named the
wrong problem ("name 'logger' is not defined" rather than the provider that actually blew
up). Doubly silent: nothing was ever logged either, because the logging call was the thing
that crashed.
Found by pyflakes while carving server.py (R3b). It survived because NOTHING exercised the
failure path — no test ever had a provider raise. That is the whole reason this class of
bug is invisible: it lives only on error paths, so the suite is green and the feature is
broken exactly when it matters.
tests/test_tuning_provider_isolation.py is that path:
* a raising provider must not lose the HEALTHY providers' tunings, nor the defaults
* and the failure must actually be LOGGED — swallowing is only acceptable if it reports
Bite-tested: restoring `logger` fails both.
(The log assertion attaches caplog's handler to the feedBack logger directly. It sets
propagate=False, so pytest's root capture sees nothing from it — test_plugins.py has a
capture_logger() for this, but it is not importable here: pyproject pins pythonpath to
[".", "lib"], so `tests` is not a package. Three lines beat churning 21 call sites in an
unrelated file to convert that helper into a fixture.)
pytest 2410, pyflakes 0 undefined names in server.py, Codex 0.
Closes #899
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
70dbe45e27
commit
6b8f79dd9a
@@ -0,0 +1,72 @@
|
||||
"""A raising tuning provider must not take down get_merged() for everyone. (#899)
|
||||
|
||||
`TuningProviderRegistry.get_merged()` wraps each provider in a try/except precisely so one
|
||||
misbehaving plugin cannot break tunings for the rest. The handler called `logger.exception`
|
||||
— and there is no `logger` in server.py; the module logger is `log`. So the handler MEANT
|
||||
to swallow-and-report instead raised NameError, which propagated out of get_merged().
|
||||
|
||||
The net effect was the exact opposite of the handler's purpose: one bad provider took the
|
||||
whole merged-tunings call down, and the traceback named the wrong problem.
|
||||
|
||||
Nothing exercised the failure path, which is why it survived. This is that path.
|
||||
"""
|
||||
|
||||
import importlib
|
||||
import logging
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def registry(monkeypatch, tmp_path):
|
||||
monkeypatch.setenv("CONFIG_DIR", str(tmp_path))
|
||||
sys.modules.pop("server", None)
|
||||
mod = importlib.import_module("server")
|
||||
yield mod.TuningProviderRegistry()
|
||||
|
||||
|
||||
def test_a_raising_provider_does_not_break_the_others(registry, caplog):
|
||||
"""The whole point of the try/except. Before the fix this raised NameError."""
|
||||
def boom():
|
||||
raise RuntimeError("provider exploded")
|
||||
|
||||
def good():
|
||||
return {"guitar": {"My Tuning": [82.41, 110.0, 146.83, 196.0, 246.94, 329.63]}}
|
||||
|
||||
registry.register("bad-plugin", boom)
|
||||
registry.register("good-plugin", good)
|
||||
|
||||
merged = registry.get_merged() # must NOT raise
|
||||
|
||||
assert "My Tuning" in merged["guitar"], (
|
||||
"the healthy provider's tuning is missing — one raising provider took down the "
|
||||
"merged result for everyone"
|
||||
)
|
||||
# and the default tunings survive
|
||||
assert merged["guitar"], "default tunings were lost"
|
||||
|
||||
|
||||
def test_the_failure_is_actually_logged(registry, caplog):
|
||||
"""Swallowing is only acceptable if it is reported. A NameError in the handler meant
|
||||
nothing was ever logged — the failure was both fatal AND silent about its real cause."""
|
||||
def boom():
|
||||
raise RuntimeError("provider exploded")
|
||||
|
||||
registry.register("bad-plugin", boom)
|
||||
|
||||
# The feedBack logger sets propagate=False, so pytest's root-logger capture sees
|
||||
# NOTHING from it. Attach caplog's handler directly. (test_plugins.py has a
|
||||
# capture_logger() context manager for this, but it is not importable from here:
|
||||
# pyproject pins pythonpath to [".", "lib"], so `tests` is not a package.)
|
||||
lg = logging.getLogger("feedBack")
|
||||
lg.addHandler(caplog.handler)
|
||||
lg.setLevel(logging.ERROR)
|
||||
try:
|
||||
registry.get_merged()
|
||||
finally:
|
||||
lg.removeHandler(caplog.handler)
|
||||
|
||||
assert any("bad-plugin" in r.getMessage() for r in caplog.records), (
|
||||
"the raising provider was never named in the logs"
|
||||
)
|
||||
Reference in New Issue
Block a user