Files
LEDMatrix/test/test_config_validation_edge_cases.py
T
ChuckandClaude Opus 5.5 b11bcfa204 fix(plugins): store and plugin-manager bugs; tidy src/plugin_system (#635)
* fix(store): don't read a ZIP-installed plugin's remote from the LEDMatrix repo

update_plugin looked up remote.origin.url with `git -C <plugin> config
--local` for plugins that are not git checkouts. Under plugin-repos/ git
walks up to the enclosing LEDMatrix repository, so the lookup returned
LEDMatrix's own URL and a plugin missing from the registry was
"reinstalled" from the LEDMatrix repo. Only ask git when the plugin
directory has its own .git, the test _get_local_git_info already uses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(schema): report each missing required field once, by name

validate_config_against_schema ran its own required-fields loop after
Draft7Validator.iter_errors, which already yields one `required` error
per missing field, so every missing top-level field was listed twice.
The validator's copy also printed the schema's whole `required` list
("Missing required property '['api_key', 'city']'") instead of the field.
Drop the loop and take the field name from the error itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(store): stop mangling repository URLs that contain ".git"

install_from_url and fetch_registry_from_url cleaned URLs with
`rstrip('/').replace('.git', '')`, which removes ".git" anywhere:
https://github.com/user/my.github.io became .../myhub.io, so installing
or browsing that repository asked GitHub for one that does not exist.

Add src/plugin_system/repo_urls.py with one anchored normalize_repo_url(),
same_repo() for comparisons, github_owner_repo() and github_api_headers(),
and use them for the five copies of the owner/repo parsing and GitHub
headers in the store and for saved repositories. GitHub URLs are now
recognised by urlparse().hostname everywhere: _get_latest_commit_info
used a substring test, and _install_from_monorepo_api parsed any host.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(store): install a repository whose only branch is not main/master

_install_via_git returned None both when every clone failed and when the
last-resort clone of the repository's default branch succeeded.
_install_plugin_impl papered over it with `and not plugin_path.exists()`;
install_from_url did not, so a repository whose only branch is e.g.
`develop` was cloned, then treated as a failure, then "downloaded" from
main/master archives that do not exist.

After a default-branch clone, return the branch the clone checked out
(read from .git/HEAD), so None means failure and nothing else, and give
both callers the same `branch_used is None` fallback.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(plugins): judge the memory limit on each call's own growth

monitor_call stores `metrics.memory_mb = max(previous, growth)`, and
_check_limits compared that high-water mark with max_memory_mb. It never
decreases, so once one update() grew the process past the limit every
later call raised ResourceLimitExceeded and the circuit breaker kept
reopening. Pass the call's own RSS growth to _check_limits; keep the
high-water mark for reporting and document what it measures.

Remove ResourceMetrics.update_average_execution_time: nothing called it,
and it overwrote the running total with the average.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(plugins): reload_plugin re-reads the manifest from the discovered directory

reload_plugin read `plugins_dir / plugin_id / "manifest.json"`, ignoring
the discovery map and the plugin_dirs rules. For a plugin whose
directory name differs from its manifest id the path did not exist, the
re-read was skipped without a word, and the reload kept the stale
manifest. Resolve the directory with find_plugin_directory, as
load_plugin does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(plugins): drop the always-null last_display from plugin state info

PluginStateManager reported `last_display` from `_last_display`, which
nothing ever wrote, so it was null for every plugin. Recording it in
PluginExecutor.execute_display would not help: get_state_info's only
reader is the web process, whose PluginManager never calls display().
Remove the field, its dict and get_last_display() (no caller in core,
the web UI or the plugin monorepo).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(store): share the rollback and requirements helpers, drop dead code

- install_plugin and _reinstall_with_rollback set aside, discard and
  restore the old copy through _set_aside/_discard_backup/_restore_backup
  instead of two copies of the same blocks.
- The loader and the store run the same pre-pip checks through
  contained_plugin_dir() and requirements_to_install() in plugin_loader.
  They still invoke pip differently (sys.executable -m pip vs. the sudo
  wrapper). `except (BrokenPipeError, OSError)` + `isinstance(e, OSError)`
  becomes `except OSError` checking errno.EPIPE.
- load_module never returns None, so load_plugin's check is gone and the
  docstring says what it raises.
- Remove the always-true JSONSCHEMA_AVAILABLE, the inline re-imports of
  re and permission_utils, the fake status_result object nobody reads,
  hasattr(git_error, 'cmd'), a redundant "merge conflict" test and
  `import traceback` (exc_info=True does it).
- Correct comments: install_from_url names the directory for the
  caller's id when given (not always the manifest id), _get_local_git_info
  saves one git subprocess (not four), _enrich calls two helpers,
  search_plugins documents all its arguments, _find_plugin_path states
  its behaviour instead of a TODO, and history narration is gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(plugins): tidy base_plugin, correct plugin_manager/state comments

- base_plugin: drop the unused `import logging`; get_display_duration
  runs the instance value and the config value through one
  _positive_seconds() helper instead of two copies of the coercion; the
  'static'/'none'/fallback branches of get_vegas_display_mode, which all
  returned FIXED_SEGMENT, are one; fix the mis-indented validate_config
  example; say that get_supported_vegas_modes/get_vegas_segment_width
  are not consulted by core (kept, plugins override them).
- schema_manager: import expand_style_elements normally rather than
  swallowing an ImportError of a core module.
- plugin_manager: the plugins directory is the configured one
  (plugin-repos/ by default), not plugins/; get_config() returns the live
  dict, not a copy, so the interval cache comments say what it saves.
- state_manager: config_version and the file version are not used to
  detect corruption; say what they are.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* refactor(plugins): stop writing data/plugin_operations.json

PluginOperationQueue wrote its finished-operation history to
data/plugin_operations.json after every operation, and read it back only
into its own in-memory list, which only get_operation_history() exposes
-- and nothing calls that. The operation-history endpoint reads
OperationHistory (data/operation_history.json). No code in src/,
web_interface/, scripts/ or test/ reads the file.

Drop the history_file/lazy_load parameters and the load/save code; the
bounded in-memory history stays. web_interface/app.py and the
integration test stop passing the removed arguments. An existing
data/plugin_operations.json is left in place (data/* is gitignored).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* docs(changelog): plugin-system

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-09-24 17:32:02 -04:00

347 lines
11 KiB
Python

"""
Tests for configuration validation edge cases.
Tests scenarios that commonly cause user configuration errors:
- Invalid JSON in config files
- Missing required fields
- Type mismatches
- Nested object validation
- Array validation
"""
import pytest
from pathlib import Path
# Add project root to path
import sys
project_root = Path(__file__).parent.parent
if str(project_root) not in sys.path:
sys.path.insert(0, str(project_root))
from src.config_manager import ConfigManager
from src.exceptions import ConfigError
from src.plugin_system.schema_manager import SchemaManager
class TestInvalidJson:
"""Test handling of invalid JSON in config files."""
def test_invalid_json_syntax(self, tmp_path):
"""Config with invalid JSON syntax should raise ConfigError."""
config_file = tmp_path / "config.json"
config_file.write_text("{ invalid json }")
config_manager = ConfigManager(config_path=str(config_file))
with pytest.raises(ConfigError):
config_manager.load_config()
def test_truncated_json(self, tmp_path):
"""Config with truncated JSON should raise ConfigError."""
config_file = tmp_path / "config.json"
config_file.write_text('{"plugin": {"enabled": true') # Missing closing braces
config_manager = ConfigManager(config_path=str(config_file))
with pytest.raises(ConfigError):
config_manager.load_config()
def test_empty_config_file(self, tmp_path):
"""Empty config file should raise ConfigError."""
config_file = tmp_path / "config.json"
config_file.write_text("")
config_manager = ConfigManager(config_path=str(config_file))
with pytest.raises(ConfigError):
config_manager.load_config()
class TestTypeValidation:
"""Test type validation and coercion."""
def test_string_where_number_expected(self):
"""String value where number expected should be handled."""
schema_manager = SchemaManager()
schema = {
"type": "object",
"properties": {
"display_duration": {"type": "number", "default": 15}
}
}
config = {"display_duration": "invalid_string"}
# Validation should fail
is_valid, errors = schema_manager.validate_config_against_schema(
config, schema, "test-plugin"
)
assert not is_valid
assert len(errors) > 0
def test_number_where_string_expected(self):
"""Number value where string expected should be handled."""
schema_manager = SchemaManager()
schema = {
"type": "object",
"properties": {
"team_name": {"type": "string", "default": ""}
}
}
config = {"team_name": 12345}
is_valid, errors = schema_manager.validate_config_against_schema(
config, schema, "test-plugin"
)
assert not is_valid
assert len(errors) > 0
def test_null_value_for_required_field(self):
"""Null value for required field should be detected."""
schema_manager = SchemaManager()
# Schema that explicitly disallows null for api_key
schema = {
"type": "object",
"properties": {
"api_key": {"type": "string"} # string type doesn't allow null
},
"required": ["api_key"]
}
config = {"api_key": None}
is_valid, errors = schema_manager.validate_config_against_schema(
config, schema, "test-plugin"
)
# JSON Schema Draft 7: null is not a valid string type
assert not is_valid, "Null value should fail validation for string type"
assert errors, "Should have validation errors"
assert any("api_key" in str(e).lower() or "null" in str(e).lower() or "type" in str(e).lower() for e in errors), \
f"Error should mention api_key, null, or type issue: {errors}"
class TestNestedValidation:
"""Test validation of nested configuration objects."""
def test_nested_object_missing_required(self):
"""Missing required field in nested object should be detected."""
schema_manager = SchemaManager()
schema = {
"type": "object",
"properties": {
"nfl": {
"type": "object",
"properties": {
"enabled": {"type": "boolean", "default": True},
"api_key": {"type": "string"}
},
"required": ["api_key"]
}
}
}
config = {"nfl": {"enabled": True}} # Missing api_key
is_valid, errors = schema_manager.validate_config_against_schema(
config, schema, "test-plugin"
)
assert not is_valid
def test_deeply_nested_validation(self):
"""Validation should work for deeply nested objects."""
schema_manager = SchemaManager()
schema = {
"type": "object",
"properties": {
"level1": {
"type": "object",
"properties": {
"level2": {
"type": "object",
"properties": {
"value": {"type": "number", "minimum": 0}
}
}
}
}
}
}
config = {"level1": {"level2": {"value": -5}}} # Invalid: negative
is_valid, errors = schema_manager.validate_config_against_schema(
config, schema, "test-plugin"
)
assert not is_valid
class TestArrayValidation:
"""Test validation of array configurations."""
def test_array_min_items(self):
"""Array with fewer items than minItems should fail."""
schema_manager = SchemaManager()
schema = {
"type": "object",
"properties": {
"teams": {
"type": "array",
"items": {"type": "string"},
"minItems": 1
}
}
}
config = {"teams": []} # Empty array, minItems is 1
is_valid, errors = schema_manager.validate_config_against_schema(
config, schema, "test-plugin"
)
assert not is_valid
def test_array_max_items(self):
"""Array with more items than maxItems should fail."""
schema_manager = SchemaManager()
schema = {
"type": "object",
"properties": {
"teams": {
"type": "array",
"items": {"type": "string"},
"maxItems": 2
}
}
}
config = {"teams": ["A", "B", "C", "D"]} # 4 items, maxItems is 2
is_valid, errors = schema_manager.validate_config_against_schema(
config, schema, "test-plugin"
)
assert not is_valid
class TestCollisionDetection:
"""Test config key collision detection."""
def test_reserved_key_collision(self):
"""Plugin IDs that conflict with reserved keys should be detected."""
schema_manager = SchemaManager()
plugin_ids = ["display", "custom-plugin", "schedule"]
collisions = schema_manager.detect_config_key_collisions(plugin_ids)
# Should detect 'display' and 'schedule' as collisions
collision_types = [c["type"] for c in collisions]
collision_plugins = [c["plugin_id"] for c in collisions]
assert "reserved_key_collision" in collision_types
assert "display" in collision_plugins
assert "schedule" in collision_plugins
def test_case_collision(self):
"""Plugin IDs that differ only in case should be detected."""
schema_manager = SchemaManager()
plugin_ids = ["football-scoreboard", "Football-Scoreboard", "other-plugin"]
collisions = schema_manager.detect_config_key_collisions(plugin_ids)
case_collisions = [c for c in collisions if c["type"] == "case_collision"]
assert len(case_collisions) == 1
def test_no_collisions(self):
"""Unique plugin IDs should not trigger collisions."""
schema_manager = SchemaManager()
plugin_ids = ["football-scoreboard", "odds-ticker", "weather-display"]
collisions = schema_manager.detect_config_key_collisions(plugin_ids)
assert len(collisions) == 0
class TestDefaultMerging:
"""Test default value merging with user config."""
def test_defaults_applied_to_missing_fields(self):
"""Missing fields should get default values from schema."""
schema_manager = SchemaManager()
defaults = {
"enabled": True,
"display_duration": 15,
"nfl": {"enabled": True}
}
config = {"display_duration": 30} # Only override one field
merged = schema_manager.merge_with_defaults(config, defaults)
assert merged["enabled"] is True # From defaults
assert merged["display_duration"] == 30 # User override
assert merged["nfl"]["enabled"] is True # Nested default
def test_user_values_not_overwritten(self):
"""User-provided values should not be overwritten by defaults."""
schema_manager = SchemaManager()
defaults = {"enabled": True, "display_duration": 15}
config = {"enabled": False, "display_duration": 60}
merged = schema_manager.merge_with_defaults(config, defaults)
assert merged["enabled"] is False
assert merged["display_duration"] == 60
class TestMissingRequiredFields:
"""One message per missing field, naming that field.
A manual ``required`` loop used to run after Draft7Validator, which already
reports ``required``, so every missing top-level field was listed twice --
and the validator's copy printed the schema's whole ``required`` list as
if it were the field name.
"""
SCHEMA = {
"type": "object",
"properties": {
"api_key": {"type": "string"},
"city": {"type": "string"},
"units": {"type": "string"},
},
"required": ["api_key", "city", "units"],
}
def test_each_missing_field_is_reported_once_by_name(self):
ok, errors = SchemaManager().validate_config_against_schema(
{"units": "metric"}, self.SCHEMA, "test-plugin")
assert not ok
assert errors == [
"Field root: Missing required property 'api_key'",
"Field root: Missing required property 'city'",
]
def test_nested_missing_field_names_the_field_and_its_parent(self):
schema = {
"type": "object",
"properties": {"nfl": {
"type": "object",
"properties": {"api_key": {"type": "string"}},
"required": ["api_key"],
}},
}
ok, errors = SchemaManager().validate_config_against_schema(
{"nfl": {}}, schema, "test-plugin")
assert not ok
assert errors == ["Field 'nfl': Missing required property 'api_key'"]