diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-08-24 18:30:00 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-08-24 18:30:00 +0200 |
| commit | 584730f9486e153c6c477293f03a95e78f5ab5f3 (patch) | |
| tree | 5b8c65926d3cd2a2a187a532e6f92ec35b8b615b /packages/meshbay-node/tests | |
| parent | c2f5e0ed7ff22e2e176686d0074b027712e9efa3 (diff) | |
| download | meshbay-584730f9486e153c6c477293f03a95e78f5ab5f3.tar.gz | |
fix(node): stop inventing a fake artist from the shared root's name
Music grouping was measured against a real ~5700-file library and came
back worse than a plain file listing. Root cause: the artist/album
ancestor walk always climbed exactly two levels (parent = album,
grandparent = artist) with no idea where the group's own shared root
was. Any file in a flat top-level folder — common here: bare
`Artist/track.mp3`, no album subfolder at all — had its "grandparent"
resolve to the root directory's own name, so the artist got replaced
by the share's name. Measured: 289 of 5664 tracks across 41 real,
unrelated artists (Ben Harper, Dire Straits, Jimi Hendrix, Janis
Joplin, ...) collapsed into one fake artist this way — the single
biggest bucket in the whole library, ahead of every real one.
- `_artist_album_from_ancestors` now takes the entry's own root
boundary (daemon.py resolves it via `RootSet.split`) and refuses to
read it as a name. A file sitting in a top-level folder — genuinely
ambiguous, artist or a standalone album/compilation — is handled by
`_split_top_level_folder`: split on "Artist - Album" when the
(cleaned) folder name has that shape, otherwise the whole name
becomes the artist alone, the more common real case here.
- `_clean_tag` treats known tagger placeholders ("No Artist", a French
tool's "Nouvel artiste (334)") as absent rather than a real value —
they were just as truthy as a real name and were locking out the
fallback that would have done better. "Various Artists" is kept, a
real compilation credit rather than a placeholder.
- A `title` tag that's the bare filename copied verbatim (track number
included — found live on a whole CD-single) is stripped through the
same prefix rule the filename parser already used
(`title_parse.strip_track_prefix`), since a tag normally wins over
the parsed title.
- Cover art: only 11% of a 400-file sample had embedded art (expected
for this era of rip), but 267 loose cover images sit beside the
tracks across the library (Windows Media Player's `Folder.jpg`/
`AlbumArt_{guid}_*.jpg`, manual `cover.jpg`) and were never looked
at. `_find_sibling_cover` checks the track's own folder before
giving up — measured coverage 11% -> 26% on the same library, zero
network calls.
`_enriched_attempted` is in-memory and resets on restart, so a node
restart is enough to re-run enrichment over an already-scanned library
with the fixed logic — no rescan flag, no cache to clear by hand.
22 tests in test_enrich_audio.py (11 new), including the exact
regression case end to end through the real pool. Full suite: 1129
passed, no regressions.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KBi7ALLGfwcjBXt57yNMcy
Diffstat (limited to 'packages/meshbay-node/tests')
| -rw-r--r-- | packages/meshbay-node/tests/test_enrich_audio.py | 154 |
1 files changed, 149 insertions, 5 deletions
diff --git a/packages/meshbay-node/tests/test_enrich_audio.py b/packages/meshbay-node/tests/test_enrich_audio.py index 62d0a0a..221f1c6 100644 --- a/packages/meshbay-node/tests/test_enrich_audio.py +++ b/packages/meshbay-node/tests/test_enrich_audio.py @@ -6,12 +6,15 @@ import subprocess from pathlib import Path import pytest - from meshbay_common.protocol import IndexEntry from meshbay_node.indexer.enrich_audio import ( - AudioEnricher, _artist_album_from_ancestors, _extract_cover, + AudioEnricher, + _artist_album_from_ancestors, + _clean_tag, + _extract_cover, + _split_top_level_folder, ) -from meshbay_node.indexer.title_parse import parse_track_filename +from meshbay_node.indexer.title_parse import parse_track_filename, strip_track_prefix from meshbay_node.media_cache import MediaCache _HAVE_FFMPEG = shutil.which("ffmpeg") and shutil.which("ffprobe") @@ -54,12 +57,100 @@ def test_artist_album_from_ancestors_reads_artist_album_track_layout(tmp_path): track = folder / "01 - A Track.mp3" track.touch() - artist, album = _artist_album_from_ancestors(track) + artist, album = _artist_album_from_ancestors(track, tmp_path) assert artist == "Some Artist" assert album == "Some Album" +def test_artist_album_from_ancestors_refuses_to_name_the_root_as_artist(tmp_path): + """ + The regression this whole revision exists for: a flat `Artist/track.mp3` + layout (no album subfolder) used to read the *root's own directory + name* as the artist, because the walk always climbed two levels with no + idea where the root was. Measured live: 289 tracks across 41 real, + unrelated artists collapsed into one fake "artist" this way — the + single biggest bucket in the whole library. + """ + folder = tmp_path / "Ben Harper" + folder.mkdir(parents=True) + track = folder / "Ashes.mp3" + track.touch() + + artist, album = _artist_album_from_ancestors(track, tmp_path) + + assert artist == "Ben Harper" + assert album is None, "no album folder exists — must not invent one, or swap artist/album" + + +def test_artist_album_from_ancestors_file_directly_in_root_has_no_context(tmp_path): + track = tmp_path / "loose_track.mp3" + track.touch() + + artist, album = _artist_album_from_ancestors(track, tmp_path) + + assert (artist, album) == (None, None) + + +def test_artist_album_from_ancestors_without_a_root_keeps_the_old_two_level_behaviour(tmp_path): + """No root resolved (should not happen in practice, but must degrade safely).""" + folder = tmp_path / "Some Artist" / "Some Album" + folder.mkdir(parents=True) + track = folder / "01 - A Track.mp3" + track.touch() + + artist, album = _artist_album_from_ancestors(track, None) + + assert artist == "Some Artist" + assert album == "Some Album" + + +def test_split_top_level_folder_splits_artist_dash_album(): + artist, album = _split_top_level_folder("GHOST DOG - Soundtrack") + assert artist == "GHOST DOG" + assert album == "Soundtrack" + + +def test_split_top_level_folder_with_no_separator_is_artist_only(): + artist, album = _split_top_level_folder("Ben Harper") + assert artist == "Ben Harper" + assert album is None + + +def test_split_top_level_folder_cleans_rip_tag_noise(): + artist, album = _split_top_level_folder( + "Sinsemilia - Premiere Recolte [MP3 320kbps Album]") + assert artist == "Sinsemilia" + assert album == "Premiere Recolte" + + +def test_clean_tag_filters_known_placeholders(): + assert _clean_tag("No Artist") is None + assert _clean_tag("unknown artist") is None + assert _clean_tag("Nouvel artiste (334)") is None + assert _clean_tag("Nouveau titre (12)") is None + assert _clean_tag("") is None + assert _clean_tag(None) is None + + +def test_clean_tag_keeps_various_artists_as_a_real_credit(): + """A real, meaningful compilation credit — not a placeholder to blank out.""" + assert _clean_tag("Various Artists") == "Various Artists" + + +def test_clean_tag_keeps_a_real_value(): + assert _clean_tag("Björk") == "Björk" + + +def test_strip_track_prefix_removes_a_leaked_filename_number(): + assert strip_track_prefix("01 - Venus As A Boy (Edited Lp Version)") == \ + "Venus As A Boy (Edited Lp Version)" + + +def test_strip_track_prefix_is_a_noop_on_a_clean_title(): + assert strip_track_prefix("Venus As A Boy") == "Venus As A Boy" + + # ── end-to-end against a real (tiny, synthetic) MP3 file ──────────────────── pytestmark_ffmpeg = pytest.mark.skipif(not _HAVE_FFMPEG, reason="ffmpeg/ffprobe not installed") @@ -130,7 +221,7 @@ async def test_enricher_falls_back_to_filename_and_folder_when_tags_absent(tmp_p async def on_done(file_id, fields): done.set_result((file_id, fields)) - enricher.spawn(entry, clip, on_done) + enricher.spawn(entry, clip, on_done, tmp_path) _, fields = await asyncio.wait_for(done, timeout=30) assert fields["display_title"] == "Filename Title" @@ -140,6 +231,59 @@ async def test_enricher_falls_back_to_filename_and_folder_when_tags_absent(tmp_p @pytestmark_ffmpeg +@pytest.mark.asyncio +async def test_enricher_falls_back_to_artist_only_for_a_flat_top_level_dir(tmp_path, media_cache): + """The real-world regression case, end to end through the whole pool.""" + folder = tmp_path / "Ben Harper" + folder.mkdir(parents=True) + clip = folder / "Ashes.mp3" + _make_clip(clip) # no metadata tags at all + entry = IndexEntry(id="fileid3", name=clip.name, + path=str(clip.relative_to(tmp_path)), + size=clip.stat().st_size, type="audio", added_at=0) + + enricher = AudioEnricher(media_cache) + done = asyncio.get_event_loop().create_future() + + async def on_done(file_id, fields): + done.set_result((file_id, fields)) + + enricher.spawn(entry, clip, on_done, tmp_path) + _, fields = await asyncio.wait_for(done, timeout=30) + + assert fields["artist"] == "Ben Harper" + assert fields["album"] is None + assert fields["artist"] != tmp_path.name, \ + "must never fall back to the shared root's own directory name" + + +@pytestmark_ffmpeg +@pytest.mark.asyncio +async def test_enricher_uses_a_sibling_cover_file_when_no_embedded_art(tmp_path, media_cache): + folder = tmp_path / "Some Artist" / "Some Album" + folder.mkdir(parents=True) + clip = folder / "01 - A Track.mp3" + _make_clip(clip) + (folder / "Folder.jpg").write_bytes(b"\xff\xd8\xff\xe0fake-jpeg-bytes") + entry = IndexEntry(id="fileid4", name=clip.name, + path=str(clip.relative_to(tmp_path)), + size=clip.stat().st_size, type="audio", added_at=0) + + enricher = AudioEnricher(media_cache) + done = asyncio.get_event_loop().create_future() + + async def on_done(file_id, fields): + done.set_result((file_id, fields)) + + enricher.spawn(entry, clip, on_done, tmp_path) + _, fields = await asyncio.wait_for(done, timeout=30) + + assert fields.get("thumb_hash"), "a Folder.jpg beside the track must be picked up as its cover" + stored = await media_cache.get_thumb(fields["thumb_hash"]) + assert stored == b"\xff\xd8\xff\xe0fake-jpeg-bytes" + + +@pytestmark_ffmpeg def test_extract_cover_returns_none_when_no_apic_frame(tmp_path): from mutagen import File as MutagenFile |