diff --git a/tests/test_download_plugin_conformance.py b/tests/test_download_plugin_conformance.py new file mode 100644 index 00000000..2cb341bb --- /dev/null +++ b/tests/test_download_plugin_conformance.py @@ -0,0 +1,163 @@ +"""Pin the structural conformance of every download source plugin +class to ``DownloadSourcePlugin``. + +Each registered source class MUST: +- Implement every protocol method by name. +- Mark async methods as `async def` so the orchestrator can `await` + them uniformly. + +When someone adds a new source (e.g. Usenet) and forgets one of +these methods, this test fails at the contract — long before the +first real download attempt would have raised AttributeError in +production. When someone CHANGES the contract (adds a method to +the protocol), this test forces every existing source to be +updated. + +Catches the smell that motivated the refactor in the first place: +8 sources independently grew the same shape because every +consumer site needed the same calls, but nothing enforced parity. + +NOTE on test design: these tests check CLASSES, not instances. +Instantiating real client classes (TidalDownloadClient, etc.) at +fixture setup pollutes module-level state in tidalapi / spotipy +imports and breaks downstream tests that rely on a clean import +graph. Class-level checks are equally strict for structural +conformance — the protocol only constrains the method surface, not +runtime instance behavior. +""" + +from __future__ import annotations + +import inspect + +import pytest + + +REQUIRED_SYNC_METHODS = {'is_configured'} + +REQUIRED_ASYNC_METHODS = { + 'check_connection', + 'search', + 'download', + 'get_all_downloads', + 'get_download_status', + 'cancel_download', + 'clear_all_completed_downloads', +} + + +def _import_plugin_classes(): + """Import every download source class lazily inside the test + rather than at module load — avoids dragging tidalapi / + spotipy / yt-dlp imports into every other test module's + collection phase.""" + from core.soulseek_client import SoulseekClient + from core.youtube_client import YouTubeClient + from core.tidal_download_client import TidalDownloadClient + from core.qobuz_client import QobuzClient + from core.hifi_client import HiFiClient + from core.deezer_download_client import DeezerDownloadClient + from core.lidarr_download_client import LidarrDownloadClient + from core.soundcloud_client import SoundcloudClient + + return { + 'soulseek': SoulseekClient, + 'youtube': YouTubeClient, + 'tidal': TidalDownloadClient, + 'qobuz': QobuzClient, + 'hifi': HiFiClient, + 'deezer': DeezerDownloadClient, + 'lidarr': LidarrDownloadClient, + 'soundcloud': SoundcloudClient, + } + + +def test_default_registry_registers_all_eight_sources(): + """Smoke check that the foundation registry knows about every + source the orchestrator historically dispatched to. If someone + drops a registration here, every other test in this module would + silently miss the missing source.""" + from core.download_plugins.registry import build_default_registry + + registry = build_default_registry() + expected = { + 'soulseek', 'youtube', 'tidal', 'qobuz', + 'hifi', 'deezer', 'lidarr', 'soundcloud', + } + assert set(registry.names()) == expected + + +def test_deezer_dl_alias_is_registered_against_deezer_spec(): + """Legacy ``deezer_dl`` source-name string used in config + per- + source dispatch must keep resolving — frontend, settings, + download_orchestrator's username dispatch all depend on it.""" + from core.download_plugins.registry import build_default_registry + + registry = build_default_registry() + spec = registry.get_spec('deezer_dl') + assert spec is not None + assert spec.name == 'deezer' + assert 'deezer_dl' in spec.aliases + + +@pytest.mark.parametrize('plugin_name', [ + 'soulseek', 'youtube', 'tidal', 'qobuz', + 'hifi', 'deezer', 'lidarr', 'soundcloud', +]) +def test_plugin_class_has_all_required_methods(plugin_name): + """Every registered plugin class exposes every protocol method + by name. Diagnostic-friendly: tells you WHICH method is missing + when a new source is added without all the required methods.""" + classes = _import_plugin_classes() + cls = classes[plugin_name] + + missing = [] + for method_name in REQUIRED_SYNC_METHODS | REQUIRED_ASYNC_METHODS: + if not hasattr(cls, method_name): + missing.append(method_name) + assert not missing, ( + f"{plugin_name} ({cls.__name__}) missing methods: {missing}" + ) + + +@pytest.mark.parametrize('plugin_name', [ + 'soulseek', 'youtube', 'tidal', 'qobuz', + 'hifi', 'deezer', 'lidarr', 'soundcloud', +]) +def test_plugin_class_async_methods_are_coroutines(plugin_name): + """Methods declared async in the protocol must be async on every + plugin class. A sync `download()` would silently skip the + orchestrator's `await` and return a coroutine object instead of + a download_id — the kind of bug that only surfaces at runtime + against a live user.""" + classes = _import_plugin_classes() + cls = classes[plugin_name] + + not_async = [] + for method_name in REQUIRED_ASYNC_METHODS: + method = getattr(cls, method_name, None) + if method is None: + continue + if not inspect.iscoroutinefunction(method): + not_async.append(method_name) + assert not not_async, ( + f"{plugin_name} ({cls.__name__}) declared these methods as " + f"sync but the protocol requires async: {not_async}" + ) + + +def test_orchestrator_uses_registry_for_dispatch(): + """The orchestrator must hold a registry reference and the + backward-compat ``self.`` attributes must point at the + SAME instances the registry returned. Anything that reaches in + for ``orchestrator.soulseek`` and any future code that uses + ``orchestrator.registry.get('soulseek')`` should be looking at + the same object.""" + from core.download_orchestrator import DownloadOrchestrator + + orchestrator = DownloadOrchestrator() + assert hasattr(orchestrator, 'registry') + assert orchestrator.soulseek is orchestrator.registry.get('soulseek') + assert orchestrator.youtube is orchestrator.registry.get('youtube') + assert orchestrator.deezer_dl is orchestrator.registry.get('deezer') + assert orchestrator.lidarr is orchestrator.registry.get('lidarr') diff --git a/webui/static/helper.js b/webui/static/helper.js index 411196c9..8d890fc0 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3436,6 +3436,7 @@ const WHATS_NEW = { { title: 'Internal: Migrate Album-Info Builders to Typed Path', desc: 'internal — steps 2+3 of the typed metadata migration in one pr. two album-info builders now route through `Album.from__dict()` when the caller passes a known source: `_build_album_info` (used by every album-tracks lookup) and the embedded album section of `_build_single_import_context_payload` (used by single-track import context resolution). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — so a converter bug can\'t break album resolution or import context. caller-provided album_id / album_name / artist_name fallbacks apply on the typed path the same way they did on legacy. zero behavior change for existing callers since they don\'t pass a source yet — opt-in only. 22 new tests pin the typed path, the legacy fallback, and parametrized coverage across registered providers.' }, { title: 'Internal: Migrate Discography + Quality Scanner to Typed Path', desc: 'internal — next round of the typed metadata migration. three more album-shape consumers now route through `Album.from__dict()` when the caller passes a known source: `_build_discography_release_dict` (artist discography release cards), `_build_artist_detail_release_card` (artist detail page release cards), and `_normalize_track_album` (quality scanner result normalization). legacy duck-typed extraction stays as the fallback when source is empty/unknown, raw input isn\'t a dict, or the typed converter raises — same safety contract as the prior migration steps. 20 new tests pin the typed path + legacy fallback + parametrized coverage across registered providers.' }, { title: 'Fix: Maintenance Findings Badge Showed Inflated Count With Empty Findings Tab', desc: 'discord report (husoyo): duplicate detector and cover art filler badges showed "364 findings" / "31 findings" after a scan, but clicking into the findings tab showed nothing. cause: `_create_finding` silently dedup-skipped re-discovered issues (when an equivalent row already existed with status pending/resolved/dismissed) but the caller incremented `result.findings_created` regardless of whether a row was actually inserted. so on a re-scan that found the same problems as a prior scan, the badge snapshot recorded 364 even though zero NEW pending rows hit the db. fix: `_create_finding` now returns a bool (True on insert, False on dedup-skip / db error). all 16 repair jobs updated to only increment `findings_created` on True. new `findings_skipped_dedup` counter added to job results and surfaced in the scan log: "Done: 2791 scanned, 0 fixed, 0 findings (363 already existed), 0 errors" — so re-scans show a real count, and you can see at a glance how many findings were carried over from prior scans. also fixed a missing `job_id` kwarg in the album tag consistency job that was silently breaking finding creation for that scan. companion ux improvement: findings tab now auto-switches its status filter from "pending" to "all status" when 0 pending rows exist but resolved/dismissed/auto-fixed rows do — with a small notice so you can see what carried over instead of staring at an "all clear" empty state.', page: 'library' }, + { title: 'Internal: Download Source Plugin Contract', desc: 'internal — first step of a multi-step refactor on the multi-source download dispatcher. the orchestrator historically had 8 download sources (soulseek/youtube/tidal/qobuz/hifi/deezer/lidarr/soundcloud) hardcoded into 6+ different dispatch sites — `if username == "youtube" elif username == "tidal" elif ...` chains in `__init__`, search, download, get_all_downloads, cancel_download, etc. adding usenet (planned) would have meant 700+ lines of copy-paste across the same files. new `core/download_plugins/` package defines `DownloadSourcePlugin` (Protocol) — the canonical contract every source must satisfy: `is_configured`, `check_connection`, `search`, `download`, `get_all_downloads`, `get_download_status`, `cancel_download`, `clear_all_completed_downloads`. plus `DownloadPluginRegistry` — single source of truth for which sources exist, with name/alias resolution (legacy `deezer_dl` alias preserved). orchestrator now dispatches through the registry instead of hardcoded `[self.soulseek, self.youtube, ...]` lists; backward-compat `self.` attributes still work so external callers reaching for source-specific internals (e.g. `orchestrator.soulseek._make_request`) keep working unchanged. zero behavior change for users — pure additive foundation that lets future PRs extract shared logic (background thread workers, search query normalization, post-processing context) into the contract instead of copy-pasted across all 8 sources. 19 new tests pin every plugin class\'s structural conformance to the contract — drift in any source will fail at the test boundary instead of at runtime against a live download.' }, { title: 'Discogs Collection in "Your Albums"', desc: 'discord request: pull your discogs collection into the your albums section on discover, similar to spotify liked albums. set your discogs personal access token on settings → connections (already there from prior work) and add discogs as one of the configured sources via the gear button on your albums. background fetcher pulls your full collection (all folders, all pages — capped at 5000 releases), normalizes artist names (strips discogs `(N)` disambiguation suffix), dedupes against any spotify/tidal/deezer-saved versions of the same album. clicking a discogs-only album opens with discogs context — full release detail (year, format, label, country, tracklist) from the /releases endpoint. clicking an album that exists in both your spotify saved AND discogs collection prefers spotify (download flow is more direct). discogs is physical-media-first so many releases won\'t have streaming equivalents — those still show in the grid but the modal flow may need to fall back to a name search to find a downloadable digital version.', page: 'discover' }, { title: 'Drop Redundant "Your Spotify Library" Section on Discover', desc: 'discover page used to show two near-identical sections: "Your Albums" (cross-source aggregator across spotify/deezer/etc) AND "Your Spotify Library" (spotify-only). same UI, same grid, same filter / sort / download-missing controls — the spotify-only one was a strict subset of what your albums already covers. removed it. spotify saved albums still surface via the your albums section with spotify as one of its configured sources (gear button → configure sources). backend collection / storage is unchanged — the watchlist scanner still populates the spotify_library_albums cache for your albums to read.', page: 'discover' }, { title: 'Library Disk Usage on Stats Page', desc: 'discord request (samuel [KC]): show how much disk space the library takes. new card on stats → system statistics shows total bytes + per-format breakdown (FLAC vs MP3 vs M4A bars). data comes from `tracks.file_size` populated during deep scan from whatever the media server already returns (plex MediaPart.size, jellyfin MediaSources[].Size, navidrome song.size, soulsync standalone os.path.getsize) — zero filesystem walk overhead. existing libraries see "Run a Deep Scan to populate" until the next deep scan fills in sizes; partial coverage shown as "X tracks measured (+Y pending)". migration is additive (NULL on legacy rows) so upgrading users have nothing to do.', page: 'stats' },