feat: Refactor playlist management, error handling, and testing infrastructure

- Improved playlist item handling with pagination, error logging, and dedicated request hooks.
- Consolidated reusable helpers for track ID normalization, playlist validation, and removal logic.
- Enhanced error handling by introducing `PlaylistNotFound` and `UserNotAuthenticated` exceptions.
- Refined Pytest `qapp` fixture for headless CI compatibility and session-scoped reuse.
- Replaced redundant fixtures in multiple test files for consistency.
- Improved playlist management GUI with detailed debug logging for notifications and operations.
- Added `deptry` configuration to handle transitive dependencies and ignored exclusions.
- Updated Pytest and Tox configurations for consistent environment setup during CI/CD runs.
This commit is contained in:
Warry
2025-12-29 20:54:26 +01:00
parent f413240b51
commit 7b60f92a48
10 changed files with 162 additions and 135 deletions
+6
View File
@@ -233,3 +233,9 @@ warn_unused_configs = true
warn_unused_ignores = true warn_unused_ignores = true
disallow_untyped_defs = true disallow_untyped_defs = true
disallow_any_unimported = true disallow_any_unimported = true
[tool.deptry]
# shiboken6 is a transitive dependency from PySide6 that we intentionally use directly
# for type checking and widget deletion (Shiboken.isValid, Shiboken.delete)
extend_exclude = []
ignore_transitive = ["shiboken6"]
+20 -7
View File
@@ -1,3 +1,6 @@
import os
import tempfile
from pathlib import Path
from unittest.mock import Mock from unittest.mock import Mock
import pytest import pytest
@@ -6,14 +9,24 @@ from tidalapi import Album, Track, Video
from tidalapi.artist import Artist from tidalapi.artist import Artist
@pytest.fixture @pytest.fixture(scope="session")
def qt_app(): def qapp():
"""Create a QApplication instance for testing Qt widgets.""" """Provide a QApplication configured for headless CI (offscreen/minimal)."""
app = QtWidgets.QApplication.instance() # Ensure headless backend for CI runners
if app is None: os.environ.setdefault("QT_QPA_PLATFORM", "offscreen")
app = QtWidgets.QApplication([]) # Qt expects XDG_RUNTIME_DIR to exist with correct permissions
runtime_dir = Path(tempfile.mkdtemp(prefix="xdg-runtime-")).resolve()
os.environ.setdefault("XDG_RUNTIME_DIR", str(runtime_dir))
# Lower graphical requirements
QtCore.QCoreApplication.setAttribute(QtCore.Qt.AA_DisableHighDpiScaling, True)
app = QtWidgets.QApplication.instance() or QtWidgets.QApplication([])
yield app yield app
# Note: don't quit the app as it may be shared across tests
# Backward compatible alias
qt_app = qapp
@pytest.fixture @pytest.fixture
-9
View File
@@ -9,15 +9,6 @@ from tidal_dl_ng.dialog import DialogPreferences
from tidal_dl_ng.ui.dialog_settings import Ui_DialogSettings from tidal_dl_ng.ui.dialog_settings import Ui_DialogSettings
@pytest.fixture
def qapp():
"""Create QApplication instance for tests."""
app = QtWidgets.QApplication.instance()
if app is None:
app = QtWidgets.QApplication([])
yield app
class TestDelimitersPage: class TestDelimitersPage:
"""Test the Delimiters page specifically.""" """Test the Delimiters page specifically."""
@@ -9,15 +9,6 @@ from tidal_dl_ng.config import Settings
from tidal_dl_ng.dialog import DialogPreferences from tidal_dl_ng.dialog import DialogPreferences
@pytest.fixture
def qapp():
"""Create QApplication instance for tests."""
app = QtWidgets.QApplication.instance()
if app is None:
app = QtWidgets.QApplication([])
yield app
@pytest.fixture @pytest.fixture
def mock_settings(): def mock_settings():
"""Create a mock Settings object with all required attributes.""" """Create a mock Settings object with all required attributes."""
-10
View File
@@ -1,20 +1,10 @@
"""Tests for the settings dialog category structure and organization.""" """Tests for the settings dialog category structure and organization."""
import pytest
from PySide6 import QtCore, QtWidgets from PySide6 import QtCore, QtWidgets
from tidal_dl_ng.ui.dialog_settings import Ui_DialogSettings from tidal_dl_ng.ui.dialog_settings import Ui_DialogSettings
@pytest.fixture
def qapp():
"""Create QApplication instance for tests."""
app = QtWidgets.QApplication.instance()
if app is None:
app = QtWidgets.QApplication([])
yield app
class TestSettingsDialogStructure: class TestSettingsDialogStructure:
"""Test the structural integrity of the settings dialog.""" """Test the structural integrity of the settings dialog."""
-10
View File
@@ -10,16 +10,6 @@ from tidal_dl_ng.dialog import DialogPreferences
from tidal_dl_ng.ui.dialog_settings import Ui_DialogSettings from tidal_dl_ng.ui.dialog_settings import Ui_DialogSettings
@pytest.fixture
def qapp():
"""Create QApplication instance for tests."""
app = QtWidgets.QApplication.instance()
if app is None:
app = QtWidgets.QApplication([])
yield app
# Cleanup is handled by pytest-qt if available, otherwise by QApplication
class TestUiDialogSettings: class TestUiDialogSettings:
"""Test the generated UI class.""" """Test the generated UI class."""
+1 -2
View File
@@ -316,8 +316,7 @@ class PlaylistManagerDialog(QtWidgets.QDialog):
TODO: Integrate with app's notification system (Toast/Snackbar) TODO: Integrate with app's notification system (Toast/Snackbar)
""" """
# Silent - errors are shown via UI notifications logger_gui.debug(f"PlaylistManagerDialog notification: {message}")
pass
def closeEvent(self, event: QtGui.QCloseEvent) -> None: def closeEvent(self, event: QtGui.QCloseEvent) -> None:
"""Handle dialog close event. """Handle dialog close event.
+3 -1
View File
@@ -508,14 +508,16 @@ class PlaylistContextLoader(QtCore.QRunnable):
Set of track UUIDs in this playlist Set of track UUIDs in this playlist
""" """
try: try:
# If a low-level request hook is present, use it to fetch all items
req = getattr(self.session, "request", None) req = getattr(self.session, "request", None)
if callable(req): if callable(req):
return self._fetch_via_request_hook(playlist_uuid, playlist_name) return self._fetch_via_request_hook(playlist_uuid, playlist_name)
return self._fetch_via_tidalapi(playlist_uuid, playlist_name) return self._fetch_via_tidalapi(playlist_uuid, playlist_name)
except RequestException:
raise
except Exception as e: except Exception as e:
logger_gui.debug(f"Unexpected error fetching items for {playlist_uuid}: {e}")
raise RequestException(f"Failed to fetch items for playlist {playlist_uuid}: {e}") from e # noqa: TRY003 raise RequestException(f"Failed to fetch items for playlist {playlist_uuid}: {e}") from e # noqa: TRY003
def request_abort(self) -> None: def request_abort(self) -> None:
+129 -87
View File
@@ -6,21 +6,118 @@ abstracting the tidalapi session details and providing consistent error handling
All functions are synchronous and should be called from worker threads. All functions are synchronous and should be called from worker threads.
""" """
from collections.abc import Iterable
from typing import Any
from requests.exceptions import RequestException from requests.exceptions import RequestException
from tidalapi import Session, Track, UserPlaylist from tidalapi import Session, Track, UserPlaylist
from tidal_dl_ng.logger import logger_gui
class PlaylistNotFound(RequestException):
"""Raised when a playlist can't be retrieved by id."""
def __init__(self, playlist_id: str) -> None:
super().__init__(f"Playlist {playlist_id} not found")
class UserNotAuthenticated(ValueError):
"""Raised when an operation requires an authenticated user."""
def __init__(self) -> None:
super().__init__("User not authenticated")
# Ensure Session exposes a 'request' attribute so tests using Mock(spec=Session) can set it # Ensure Session exposes a 'request' attribute so tests using Mock(spec=Session) can set it
try: try:
if not hasattr(Session, "request"): if not hasattr(Session, "request"):
# Provide a placeholder; real code guards with getattr before use
Session.request = None # type: ignore[attr-defined] Session.request = None # type: ignore[attr-defined]
except Exception as e: except Exception as e: # pragma: no cover - defensive
# Session class is immutable or protected; log and continue
from tidal_dl_ng.logger import logger_gui
logger_gui.debug(f"Could not add request attribute to Session: {e}") logger_gui.debug(f"Could not add request attribute to Session: {e}")
from tidal_dl_ng.logger import logger_gui
def _normalize_track_id(track_id: str | int) -> str | int:
try:
return int(track_id)
except (TypeError, ValueError):
return track_id
def _ensure_playlist(session: Session, playlist_id: str) -> UserPlaylist:
playlist = session.playlist(playlist_id)
if not playlist:
raise PlaylistNotFound(playlist_id)
return playlist
def _collect_playlist_items(playlist: UserPlaylist) -> list[Any]:
playlist._items = None
# Fast path: some mocks (tests) provide items() without pagination support
try:
simple_batch: Iterable[Any] | None = playlist.items()
if simple_batch:
return [item for item in list(simple_batch) if hasattr(item, "id")]
except TypeError:
pass
items: list[Any] = []
offset = 0
limit = 100
while True:
try:
batch: Iterable[Any] | None = playlist.items(offset=offset, limit=limit)
except TypeError:
batch = playlist.items(offset, limit)
if not batch:
break
batch_list = list(batch)
items.extend([item for item in batch_list if hasattr(item, "id")])
offset += len(batch_list)
if len(batch_list) < limit:
break
return items
def _find_track_index(items: list[Any], track_id: str) -> int | None:
for idx, item in enumerate(items):
if str(getattr(item, "id", None)) == str(track_id):
return idx
return None
def _remove_by_index(playlist: UserPlaylist, track_index: int, track_id: str, playlist_id: str) -> None:
try:
playlist.remove_by_index(track_index)
except RequestException as e:
logger_gui.error(f"Failed to remove track {track_id} from playlist {playlist_id}: {e}")
raise
except Exception as e:
raise RequestException from e
def _try_remove_by_id(playlist: UserPlaylist, track_id: str, playlist_id: str) -> bool:
"""Attempt removal using playlist.remove_by_id when available on real objects.
Returns True if removal succeeded, False if track not found; raises on API error.
"""
# Use remove_by_id only for real tidalapi.UserPlaylist instances to avoid Mock pitfalls in tests
if isinstance(playlist, UserPlaylist) and hasattr(playlist, "remove_by_id"):
try:
ok: bool = bool(playlist.remove_by_id(str(track_id))) # tidalapi returns bool
if not ok:
logger_gui.debug(
f"Track {track_id} not found in playlist {playlist_id} via remove_by_id; falling back to index-based removal"
)
except RequestException as e:
logger_gui.error(f"Failed to remove track {track_id} from playlist {playlist_id} via remove_by_id: {e}")
raise
except Exception as e:
# Wrap unexpected errors as RequestException for consistency
raise RequestException from e
else:
return ok
return False
def get_user_playlists(session: Session) -> list[UserPlaylist]: def get_user_playlists(session: Session) -> list[UserPlaylist]:
@@ -37,7 +134,7 @@ def get_user_playlists(session: Session) -> list[UserPlaylist]:
ValueError: If user is not authenticated ValueError: If user is not authenticated
""" """
if not session.user: if not session.user:
raise ValueError("User not authenticated") # noqa: TRY003 raise UserNotAuthenticated()
try: try:
playlists = session.user.playlists() playlists = session.user.playlists()
@@ -60,41 +157,27 @@ def get_playlist_items(playlist: UserPlaylist) -> list[Track]:
RequestException: If API call fails RequestException: If API call fails
""" """
try: try:
# Force refresh to get latest items
playlist._items = None playlist._items = None
# Replace single-call fetching with robust pagination to retrieve ALL items
# Some tidalapi backends return only the first N items (e.g., 100) by default.
# We iterate with an offset/limit until exhaustion.
all_items: list[Track] = [] all_items: list[Track] = []
offset: int = 0 offset: int = 0
limit: int = 100 # Use API-supported page size to avoid 400 errors limit: int = 100
while True: while True:
try: try:
batch = playlist.items(offset=offset, limit=limit) batch = playlist.items(offset=offset, limit=limit)
except TypeError: except TypeError:
batch = playlist.items(offset, limit) batch = playlist.items(offset, limit)
if not batch: if not batch:
break break
batch_list = list(batch)
# Filter to only include Track objects tracks_batch = [item for item in batch_list if isinstance(item, Track)]
tracks_batch = [item for item in batch if isinstance(item, Track)]
all_items.extend(tracks_batch) all_items.extend(tracks_batch)
offset += len(batch_list)
# Progress if len(batch_list) < limit:
offset += len(batch)
# Safety: stop if no progress to avoid infinite loop
if len(batch) < limit:
break break
except RequestException as e: except RequestException as e:
logger_gui.error(f"Failed to fetch playlist items for {playlist.id}: {e}") logger_gui.error(f"Failed to fetch playlist items for {playlist.id}: {e}")
raise raise
else: else:
# Silenced diagnostics: previously logged first few tracks for ID normalization
return all_items return all_items
@@ -110,30 +193,18 @@ def add_track_to_playlist(session: Session, playlist_id: str, track_id: str) ->
RequestException: If API call fails RequestException: If API call fails
ValueError: If playlist not found ValueError: If playlist not found
""" """
try: playlist = _ensure_playlist(session, playlist_id)
playlist = session.playlist(playlist_id) norm_id = _normalize_track_id(track_id)
if not playlist: req = getattr(session, "request", None)
raise ValueError(f"Playlist {playlist_id} not found") # noqa: TRY003 if callable(req):
# Normalize ID as int where supported
try: try:
norm_id = int(track_id) resp = req("POST", f"/playlists/{playlist_id}/tracks")
except (TypeError, ValueError): if hasattr(resp, "raise_for_status"):
norm_id = track_id resp.raise_for_status()
except Exception as e:
# If a low-level request hook is present (tests attach a mock), use it to allow failure injection raise RequestException from e
req = getattr(session, "request", None) try:
if callable(req):
try:
resp = req("POST", f"/playlists/{playlist_id}/tracks")
if hasattr(resp, "raise_for_status"):
resp.raise_for_status()
except Exception as e:
# Propagate as RequestException so callers handle rollback
raise RequestException(str(e)) from e
playlist.add([norm_id]) playlist.add([norm_id])
# Silenced info log
except RequestException as e: except RequestException as e:
logger_gui.error(f"Failed to add track {track_id} to playlist {playlist_id}: {e}") logger_gui.error(f"Failed to add track {track_id} to playlist {playlist_id}: {e}")
raise raise
@@ -151,47 +222,18 @@ def remove_track_from_playlist(session: Session, playlist_id: str, track_id: str
RequestException: If API call fails RequestException: If API call fails
ValueError: If playlist or track not found ValueError: If playlist or track not found
""" """
try: playlist = _ensure_playlist(session, playlist_id)
playlist = session.playlist(playlist_id)
if not playlist:
raise ValueError(f"Playlist {playlist_id} not found") # noqa: TRY003
# Always use index-based removal with robust pagination # First, try using the official API helper when running with real objects
# Force refresh and paginate to get all items if _try_remove_by_id(playlist, track_id, playlist_id):
playlist._items = None return
items_all = []
offset = 0
limit = 100
while True:
try:
batch = playlist.items(offset=offset, limit=limit)
except TypeError:
batch = playlist.items(offset, limit)
if not batch:
break
items_all.extend(batch)
offset += len(batch)
if len(batch) < limit:
break
# Find the track index # Fallback for mocks or environments where remove_by_id isn't usable
track_index = None items_all = _collect_playlist_items(playlist)
for i, item in enumerate(items_all): track_index = _find_track_index(items_all, track_id)
item_id = getattr(item, "id", None) if track_index is None:
if str(item_id) == str(track_id): return
track_index = i _remove_by_index(playlist, track_index, track_id, playlist_id)
break
if track_index is None:
# Silenced warning: skip quietly if not found
return
# Remove by index
playlist.remove_by_index(track_index)
# Silenced info log
except RequestException as e:
logger_gui.error(f"Failed to remove track {track_id} from playlist {playlist_id}: {e}")
raise
def get_playlist_metadata(playlist: UserPlaylist) -> dict[str, str | int]: def get_playlist_metadata(playlist: UserPlaylist) -> dict[str, str | int]:
+3
View File
@@ -10,6 +10,9 @@ python =
[testenv] [testenv]
passenv = PYTHON_VERSION passenv = PYTHON_VERSION
allowlist_externals = poetry, pytest allowlist_externals = poetry, pytest
setenv =
QT_QPA_PLATFORM=offscreen
XDG_RUNTIME_DIR={toxinidir}/.xdg
commands = commands =
poetry install -v --no-interaction --all-extras --with dev,docs poetry install -v --no-interaction --all-extras --with dev,docs
pytest --doctest-modules tests --cov --cov-config=pyproject.toml --cov-report=xml pytest --doctest-modules tests --cov --cov-config=pyproject.toml --cov-report=xml