mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 22:35:08 +00:00
* fix(logos): stop a failed download pinning a team to a grey box forever When a logo download fails, create_placeholder_logo writes a 64x64 grey PNG under the *real* logo's filename. Every later call then hits `if filepath.exists(): return True` and reports success, so the real logo is never attempted again. One transient failure -- no network at boot, ESPN blipping -- permanently costs that team its logo. This is not hypothetical. Five of the eleven cached AFL logos in my checkout were 384-byte stubs written in a single bad minute, and they had stayed that way ever since; the scoreboard rendered COLL, FRE, NMFC, PORT and SYD as grey text boxes on every card. Placeholders are now stamped with a `ledmatrix_placeholder` PNG text chunk carrying their creation time, and `is_placeholder_logo` recognises them. It also matches on the placeholder's exact geometry and background colour, so the stubs already sitting on users' disks are picked up too -- without that, this fix would only help teams whose logos break in future. Verified against the real stubs: all five detected, all six real logos untouched. `download_missing_logo` now treats an existing placeholder as the failed download it is and retries, rather than as a satisfied request. The retry is rate-limited to PLACEHOLDER_RETRY_SECONDS (6h) so this does not trade a permanent grey box for an ESPN request every frame; a failed retry rewrites the placeholder, restarting the clock. The age comes from the stamp rather than mtime, so a backup restore, an rsync, or a permissions script cannot silently reset it. `download_missing_logos_for_league` gets the same treatment -- a bulk pass is exactly where a previously failed logo should get another chance -- and `LogoHelper.load_logo_with_download` no longer accepts a stale placeholder as a cache hit. That import is lazy and guarded so the module still works against a core build predating the marker. `LogoHelper._create_placeholder_logo` needs no change: it returns an in-memory image and never writes it to disk, which is the behaviour this bug argues for. Tests cover marked and legacy-unmarked detection, the two false-positive cases (a real 500x500 logo, and a 64x64 image that is merely the same size), the retry, the rate limit, and that the age survives an mtime touch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(logos): address review — unify eligibility, invalidate cache, restart back-off Three findings from the review on #512, all confirmed against the code: 1. The three download sites each had their own idea of "already have it". download_missing_logos_for_league() retried *any* placeholder, ignoring the back-off entirely, while download_all_ncaa_football_logos() was never updated and still skipped placeholders forever. They now share one should_attempt_download(), which also covers force_download, so the sites cannot drift apart again. download_missing_logo() reads through the same helper. 2. LogoHelper.load_logo_with_download() answered from the in-memory cache before touching the disk, so after a stale placeholder was successfully replaced the *cached placeholder image* was still returned -- the real logo would not have appeared until the process restarted. The cache entry for that file (every size of it) is now dropped after a successful download. 3. A failed retry left the stale placeholder on disk with its old timestamp, so the next call saw it as stale again and retried immediately: a download attempt per call, which is precisely what the back-off exists to prevent. refresh_placeholder_timestamp() restamps it, and the helper calls that on the failure path. It refuses to touch anything that is not a placeholder. Tests cover both bulk loops in both directions (fresh placeholder skipped, stale one retried), the eligibility rule including force_download, the timestamp refresh, and the two LogoHelper paths -- including that a freshly-downloaded logo is actually what comes back rather than the cached placeholder. Two of the new bulk-loop tests initially passed for the wrong reason: the fetch_teams_data stub returned {}, which is falsy, so the loops bailed before reaching the eligibility check at all. Fixed to return a truthy payload. Re-verified end to end: with both halves in place, rendering the AFL scoreboard took FRE.png from a 362-byte stub to a 12,928-byte logo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
354 lines
15 KiB
Python
354 lines
15 KiB
Python
"""
|
|
Tests for src/logo_downloader.py
|
|
|
|
Focuses on the pure/static methods that don't require network calls:
|
|
normalize_abbreviation, get_logo_filename_variations, get_logo_directory,
|
|
ensure_logo_directory, and the download_missing_logo function path
|
|
(with HTTP mocked).
|
|
"""
|
|
|
|
import os
|
|
import time
|
|
|
|
import pytest
|
|
from pathlib import Path
|
|
from unittest.mock import patch, Mock, MagicMock
|
|
|
|
from PIL import Image
|
|
from PIL.PngImagePlugin import PngInfo
|
|
|
|
from src.logo_downloader import (
|
|
PLACEHOLDER_BG,
|
|
PLACEHOLDER_MARKER,
|
|
PLACEHOLDER_RETRY_SECONDS,
|
|
PLACEHOLDER_SIZE,
|
|
LogoDownloader,
|
|
download_missing_logo,
|
|
is_placeholder_logo,
|
|
placeholder_age_seconds,
|
|
refresh_placeholder_timestamp,
|
|
should_attempt_download,
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# normalize_abbreviation
|
|
# ---------------------------------------------------------------------------
|
|
|
|
class TestNormalizeAbbreviation:
|
|
def test_basic_lowercase(self):
|
|
result = LogoDownloader.normalize_abbreviation("lal")
|
|
assert result == "LAL"
|
|
|
|
def test_uppercases(self):
|
|
result = LogoDownloader.normalize_abbreviation("bos")
|
|
assert result == "BOS"
|
|
|
|
def test_ampersand_replaced(self):
|
|
result = LogoDownloader.normalize_abbreviation("TA&M")
|
|
assert "&" not in result
|
|
assert "AND" in result
|
|
|
|
def test_forward_slash_replaced(self):
|
|
result = LogoDownloader.normalize_abbreviation("A/B")
|
|
assert "/" not in result
|
|
|
|
def test_empty_returns_empty(self):
|
|
result = LogoDownloader.normalize_abbreviation("")
|
|
assert result == ""
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# get_logo_filename_variations
|
|
# ---------------------------------------------------------------------------
|
|
|
|
class TestGetLogoFilenameVariations:
|
|
def test_returns_list(self):
|
|
result = LogoDownloader.get_logo_filename_variations("LAL")
|
|
assert isinstance(result, list)
|
|
assert len(result) > 0
|
|
|
|
def test_includes_png(self):
|
|
result = LogoDownloader.get_logo_filename_variations("KC")
|
|
filenames = " ".join(result)
|
|
assert ".png" in filenames
|
|
|
|
def test_includes_original(self):
|
|
result = LogoDownloader.get_logo_filename_variations("LAL")
|
|
assert any("LAL" in f for f in result)
|
|
|
|
def test_ampersand_variation(self):
|
|
result = LogoDownloader.get_logo_filename_variations("TA&M")
|
|
# Should produce at least the normalized version
|
|
assert len(result) > 0
|
|
|
|
def test_empty_string_no_crash(self):
|
|
result = LogoDownloader.get_logo_filename_variations("")
|
|
assert isinstance(result, list)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# get_logo_directory
|
|
# ---------------------------------------------------------------------------
|
|
|
|
class TestGetLogoDirectory:
|
|
def test_known_sport_returns_string(self):
|
|
downloader = LogoDownloader()
|
|
result = downloader.get_logo_directory("nfl")
|
|
assert isinstance(result, str)
|
|
assert len(result) > 0
|
|
|
|
def test_known_sport_nba(self):
|
|
downloader = LogoDownloader()
|
|
result = downloader.get_logo_directory("nba")
|
|
assert "nba" in result.lower() or "sports" in result.lower()
|
|
|
|
def test_unknown_sport_returns_string(self):
|
|
downloader = LogoDownloader()
|
|
result = downloader.get_logo_directory("unknown_sport_xyz")
|
|
assert isinstance(result, str)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# ensure_logo_directory
|
|
# ---------------------------------------------------------------------------
|
|
|
|
class TestEnsureLogoDirectory:
|
|
def test_creates_writable_directory(self, tmp_path):
|
|
downloader = LogoDownloader()
|
|
test_dir = str(tmp_path / "logos" / "nfl")
|
|
result = downloader.ensure_logo_directory(test_dir)
|
|
assert result is True
|
|
assert Path(test_dir).is_dir()
|
|
|
|
def test_existing_writable_directory(self, tmp_path):
|
|
downloader = LogoDownloader()
|
|
test_dir = str(tmp_path)
|
|
result = downloader.ensure_logo_directory(test_dir)
|
|
assert result is True
|
|
|
|
def test_returns_false_when_write_test_fails(self, tmp_path):
|
|
"""Simulate a directory that exists but raises PermissionError on write."""
|
|
downloader = LogoDownloader()
|
|
test_dir = str(tmp_path / "logos")
|
|
|
|
import builtins
|
|
original_open = builtins.open
|
|
|
|
def mock_open(path, *args, **kwargs):
|
|
if ".write_test" in str(path):
|
|
raise PermissionError("no write access")
|
|
return original_open(path, *args, **kwargs)
|
|
|
|
with patch("builtins.open", side_effect=mock_open):
|
|
result = downloader.ensure_logo_directory(test_dir)
|
|
assert result is False
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Placeholder detection and retry
|
|
#
|
|
# A failed download used to be cached as a placeholder wearing the real logo's
|
|
# filename, and download_missing_logo returned early on "the file exists". One
|
|
# transient failure therefore pinned a team to a grey box permanently.
|
|
# ---------------------------------------------------------------------------
|
|
|
|
class TestPlaceholderLogos:
|
|
def _placeholder(self, tmp_path, abbrev="COLL"):
|
|
downloader = LogoDownloader()
|
|
assert downloader.create_placeholder_logo(abbrev, str(tmp_path)) is True
|
|
return tmp_path / f"{abbrev}.png"
|
|
|
|
def test_generated_placeholder_is_recognised(self, tmp_path):
|
|
assert is_placeholder_logo(self._placeholder(tmp_path)) is True
|
|
|
|
def test_real_logo_is_not_a_placeholder(self, tmp_path):
|
|
real = tmp_path / "REAL.png"
|
|
Image.new("RGBA", (500, 500), (12, 34, 56, 255)).save(real)
|
|
assert is_placeholder_logo(real) is False
|
|
|
|
def test_legacy_unmarked_placeholder_is_recognised(self, tmp_path):
|
|
"""Placeholders written before the marker existed must still be caught.
|
|
|
|
They are already sitting on users' disks; if they were not recognised
|
|
those teams would stay grey boxes forever even after this fix.
|
|
"""
|
|
legacy = tmp_path / "LEGACY.png"
|
|
Image.new("RGBA", PLACEHOLDER_SIZE, PLACEHOLDER_BG).save(legacy)
|
|
assert is_placeholder_logo(legacy) is True
|
|
|
|
def test_same_size_but_different_colour_is_not_a_placeholder(self, tmp_path):
|
|
real = tmp_path / "SMALL.png"
|
|
Image.new("RGBA", PLACEHOLDER_SIZE, (10, 200, 10, 255)).save(real)
|
|
assert is_placeholder_logo(real) is False
|
|
|
|
def test_missing_file_is_not_a_placeholder(self, tmp_path):
|
|
assert is_placeholder_logo(tmp_path / "nope.png") is False
|
|
|
|
def test_existing_real_logo_short_circuits_without_downloading(self, tmp_path):
|
|
real = tmp_path / "REAL.png"
|
|
Image.new("RGBA", (500, 500), (1, 2, 3, 255)).save(real)
|
|
with patch.object(LogoDownloader, "download_logo") as download:
|
|
assert download_missing_logo(
|
|
"afl", "1", "REAL", real, logo_url="http://example/x.png") is True
|
|
download.assert_not_called()
|
|
|
|
def _age_placeholder(self, path, seconds):
|
|
"""Rewrite a placeholder's marker so it reads as `seconds` old."""
|
|
metadata = PngInfo()
|
|
metadata.add_text(PLACEHOLDER_MARKER, str(time.time() - seconds))
|
|
with Image.open(path) as img:
|
|
img.copy().save(path, "PNG", pnginfo=metadata)
|
|
|
|
def test_stale_placeholder_triggers_a_retry(self, tmp_path):
|
|
path = self._placeholder(tmp_path)
|
|
self._age_placeholder(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
|
assert placeholder_age_seconds(path) > PLACEHOLDER_RETRY_SECONDS
|
|
|
|
with patch.object(LogoDownloader, "download_logo", return_value=True) as download:
|
|
assert download_missing_logo(
|
|
"afl", "1", "COLL", path,
|
|
logo_url="http://example/coll.png") is True
|
|
download.assert_called_once()
|
|
|
|
def test_placeholder_age_survives_an_mtime_touch(self, tmp_path):
|
|
"""The age comes from the stamp, not the filesystem.
|
|
|
|
Anything that rewrites file times -- a backup restore, an rsync, a
|
|
permissions fix script -- would otherwise reset the retry clock.
|
|
"""
|
|
path = self._placeholder(tmp_path)
|
|
self._age_placeholder(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
|
now = time.time()
|
|
os.utime(path, (now, now))
|
|
assert placeholder_age_seconds(path) > PLACEHOLDER_RETRY_SECONDS
|
|
|
|
def test_fresh_placeholder_does_not_retry(self, tmp_path):
|
|
"""Rate limiting: a placeholder written seconds ago must not re-download.
|
|
|
|
Without this the fix would trade a permanent grey box for an ESPN
|
|
request on every frame.
|
|
"""
|
|
path = self._placeholder(tmp_path)
|
|
with patch.object(LogoDownloader, "download_logo") as download:
|
|
assert download_missing_logo(
|
|
"afl", "1", "COLL", path,
|
|
logo_url="http://example/coll.png") is True
|
|
download.assert_not_called()
|
|
|
|
|
|
class TestDownloadEligibility:
|
|
"""One rule, shared by every download site.
|
|
|
|
The two bulk loops and the single-logo path each had their own idea of what
|
|
counted as "already have it", which is how one of them ended up retrying
|
|
fresh placeholders and the other skipping stale ones forever.
|
|
"""
|
|
|
|
def _placeholder(self, tmp_path, abbrev="COLL"):
|
|
assert LogoDownloader().create_placeholder_logo(abbrev, str(tmp_path))
|
|
return tmp_path / f"{abbrev}.png"
|
|
|
|
def _age(self, path, seconds):
|
|
metadata = PngInfo()
|
|
metadata.add_text(PLACEHOLDER_MARKER, str(time.time() - seconds))
|
|
with Image.open(path) as img:
|
|
img.copy().save(path, "PNG", pnginfo=metadata)
|
|
|
|
def test_missing_file_is_eligible(self, tmp_path):
|
|
assert should_attempt_download(tmp_path / "nope.png") is True
|
|
|
|
def test_real_logo_is_not_eligible(self, tmp_path):
|
|
real = tmp_path / "REAL.png"
|
|
Image.new("RGBA", (500, 500), (1, 2, 3, 255)).save(real)
|
|
assert should_attempt_download(real) is False
|
|
|
|
def test_force_download_beats_a_real_logo(self, tmp_path):
|
|
real = tmp_path / "REAL.png"
|
|
Image.new("RGBA", (500, 500), (1, 2, 3, 255)).save(real)
|
|
assert should_attempt_download(real, force_download=True) is True
|
|
|
|
def test_fresh_placeholder_is_not_eligible(self, tmp_path):
|
|
assert should_attempt_download(self._placeholder(tmp_path)) is False
|
|
|
|
def test_stale_placeholder_is_eligible(self, tmp_path):
|
|
path = self._placeholder(tmp_path)
|
|
self._age(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
|
assert should_attempt_download(path) is True
|
|
|
|
def test_league_bulk_loop_skips_a_fresh_placeholder(self, tmp_path):
|
|
"""A bulk pass honours the same back-off as everything else."""
|
|
self._placeholder(tmp_path, "AAA")
|
|
downloader = LogoDownloader()
|
|
teams = [{"abbreviation": "AAA", "display_name": "A", "logo_url": "http://x/a.png"}]
|
|
with patch.object(LogoDownloader, "get_logo_directory", return_value=str(tmp_path)):
|
|
with patch.object(LogoDownloader, "fetch_teams_data", return_value={"sports": [{}]}):
|
|
with patch.object(LogoDownloader, "extract_teams_from_data", return_value=teams):
|
|
with patch.object(LogoDownloader, "download_logo") as download:
|
|
downloader.download_missing_logos_for_league("nfl")
|
|
download.assert_not_called()
|
|
|
|
def test_league_bulk_loop_retries_a_stale_placeholder(self, tmp_path):
|
|
path = self._placeholder(tmp_path, "AAA")
|
|
self._age(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
|
downloader = LogoDownloader()
|
|
teams = [{"abbreviation": "AAA", "display_name": "A", "logo_url": "http://x/a.png"}]
|
|
with patch.object(LogoDownloader, "get_logo_directory", return_value=str(tmp_path)):
|
|
with patch.object(LogoDownloader, "fetch_teams_data", return_value={"sports": [{}]}):
|
|
with patch.object(LogoDownloader, "extract_teams_from_data", return_value=teams):
|
|
with patch.object(LogoDownloader, "download_logo", return_value=True) as download:
|
|
downloader.download_missing_logos_for_league("nfl")
|
|
download.assert_called_once()
|
|
|
|
def test_ncaa_bulk_loop_retries_a_stale_placeholder(self, tmp_path):
|
|
"""This loop skipped placeholders forever; it now shares the rule."""
|
|
path = self._placeholder(tmp_path, "AAA")
|
|
self._age(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
|
downloader = LogoDownloader()
|
|
teams = [{"abbreviation": "AAA", "display_name": "A",
|
|
"logo_url": "http://x/a.png", "category": "FBS",
|
|
"conference": "SEC"}]
|
|
with patch.object(LogoDownloader, "get_logo_directory", return_value=str(tmp_path)):
|
|
with patch.object(LogoDownloader, "fetch_teams_data", return_value={"sports": [{}]}):
|
|
with patch.object(LogoDownloader, "extract_teams_from_data", return_value=teams):
|
|
with patch.object(LogoDownloader, "download_logo", return_value=True) as download:
|
|
downloader.download_all_ncaa_football_logos()
|
|
download.assert_called_once()
|
|
|
|
def test_ncaa_bulk_loop_skips_a_fresh_placeholder(self, tmp_path):
|
|
self._placeholder(tmp_path, "AAA")
|
|
downloader = LogoDownloader()
|
|
teams = [{"abbreviation": "AAA", "display_name": "A",
|
|
"logo_url": "http://x/a.png", "category": "FBS",
|
|
"conference": "SEC"}]
|
|
with patch.object(LogoDownloader, "get_logo_directory", return_value=str(tmp_path)):
|
|
with patch.object(LogoDownloader, "fetch_teams_data", return_value={"sports": [{}]}):
|
|
with patch.object(LogoDownloader, "extract_teams_from_data", return_value=teams):
|
|
with patch.object(LogoDownloader, "download_logo") as download:
|
|
downloader.download_all_ncaa_football_logos()
|
|
download.assert_not_called()
|
|
|
|
|
|
class TestRefreshPlaceholderTimestamp:
|
|
def test_restarts_the_back_off(self, tmp_path):
|
|
assert LogoDownloader().create_placeholder_logo("COLL", str(tmp_path))
|
|
path = tmp_path / "COLL.png"
|
|
metadata = PngInfo()
|
|
metadata.add_text(PLACEHOLDER_MARKER, str(time.time() - (PLACEHOLDER_RETRY_SECONDS + 60)))
|
|
with Image.open(path) as img:
|
|
img.copy().save(path, "PNG", pnginfo=metadata)
|
|
assert should_attempt_download(path) is True
|
|
|
|
assert refresh_placeholder_timestamp(path) is True
|
|
assert should_attempt_download(path) is False
|
|
|
|
def test_refuses_to_touch_a_real_logo(self, tmp_path):
|
|
real = tmp_path / "REAL.png"
|
|
Image.new("RGBA", (500, 500), (1, 2, 3, 255)).save(real)
|
|
before = real.read_bytes()
|
|
assert refresh_placeholder_timestamp(real) is False
|
|
assert real.read_bytes() == before
|
|
|
|
def test_missing_file_is_not_an_error(self, tmp_path):
|
|
assert refresh_placeholder_timestamp(tmp_path / "nope.png") is False
|