Files
LEDMatrix/test/test_logo_downloader.py
T
ChuckandClaude Opus 5 92f9d06af9 fix(logos): stop a failed download pinning a team to a grey box forever (#512)
* 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>
2026-09-02 13:21:07 -04:00

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