From 0d6393e712b216c9eeafc96d1c916bf16b64a374 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Tue, 31 Dec 2024 00:08:10 +0000 Subject: [PATCH 1/3] Fix track matching I had previously tested the `munkres` -> `lapjv` replacement extensively, so I was today surprised to find that nothing gets matched correctly when I tried importing some new tracks. On the other hand I now remember making a small adjustment in the logic to make autotagging tests pass which is when I introduced a bug: I did not realize that `lapjv` returns index '-1' for each unmatched item. This issue did not get caught by tests because this 'unmatched' item index '-1' anecdotally ended up pointing to the last (expected) item in the test making it pass. This commit adjusts the aforementioned test to catch this issue and fixes the logic to correctly identify unmatched tracks. --- beets/autotag/match.py | 16 +++++++++++----- test/test_autotag.py | 6 +++--- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/beets/autotag/match.py b/beets/autotag/match.py index db4a35b13..a7121fd34 100644 --- a/beets/autotag/match.py +++ b/beets/autotag/match.py @@ -127,15 +127,21 @@ def assign_items( objects. These "extra" objects occur when there is an unequal number of objects of the two types. """ + log.debug("Computing track assignment...") # Construct the cost matrix. costs = [[float(track_distance(i, t)) for t in tracks] for i in items] - # Find a minimum-cost bipartite matching. - log.debug("Computing track assignment...") - cost, _, assigned_idxs = lap.lapjv(np.array(costs), extend_cost=True) + # Assign items to tracks + _, _, assigned_item_idxs = lap.lapjv(np.array(costs), extend_cost=True) log.debug("...done.") - # Produce the output matching. - mapping = {items[i]: tracks[t] for (t, i) in enumerate(assigned_idxs)} + # Each item in `assigned_item_idxs` list corresponds to a track in the + # `tracks` list. Each value is either an index into the assigned item in + # `items` list, or -1 if that track has no match. + mapping = { + items[iidx]: t + for iidx, t in zip(assigned_item_idxs, tracks) + if iidx != -1 + } extra_items = list(set(items) - mapping.keys()) extra_items.sort(key=lambda i: (i.disc, i.track, i.title)) extra_tracks = list(set(tracks) - set(mapping.values())) diff --git a/test/test_autotag.py b/test/test_autotag.py index e131a6be1..d6f4606e1 100644 --- a/test/test_autotag.py +++ b/test/test_autotag.py @@ -551,7 +551,7 @@ class AssignmentTest(unittest.TestCase): def test_order_works_with_missing_tracks(self): items = [] items.append(self.item("one", 1)) - items.append(self.item("three", 3)) + items.append(self.item("two", 2)) trackinfo = [] trackinfo.append(TrackInfo(title="one")) trackinfo.append(TrackInfo(title="two")) @@ -560,8 +560,8 @@ class AssignmentTest(unittest.TestCase): items, trackinfo ) assert extra_items == [] - assert extra_tracks == [trackinfo[1]] - assert mapping == {items[0]: trackinfo[0], items[1]: trackinfo[2]} + assert extra_tracks == [trackinfo[2]] + assert mapping == {items[0]: trackinfo[0], items[1]: trackinfo[1]} def test_order_works_with_extra_tracks(self): items = [] From 084cf6490e630962d7b40e7e8bf7600c13e4ffa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Tue, 31 Dec 2024 08:12:03 +0000 Subject: [PATCH 2/3] matching: add additional test cases and refactor tests --- test/test_autotag.py | 119 +++++++++++++------------------------------ 1 file changed, 34 insertions(+), 85 deletions(-) diff --git a/test/test_autotag.py b/test/test_autotag.py index d6f4606e1..31afc6a63 100644 --- a/test/test_autotag.py +++ b/test/test_autotag.py @@ -498,85 +498,41 @@ class AlbumDistanceTest(BeetsTestCase): assert dist == 0 -class AssignmentTest(unittest.TestCase): - def item(self, title, track): - return Item( - title=title, - track=track, - mb_trackid="", - mb_albumid="", - mb_artistid="", - ) +class TestAssignment: + A = "one" + B = "two" + C = "three" - def test_reorder_when_track_numbers_incorrect(self): - items = [] - items.append(self.item("one", 1)) - items.append(self.item("three", 2)) - items.append(self.item("two", 3)) - trackinfo = [] - trackinfo.append(TrackInfo(title="one")) - trackinfo.append(TrackInfo(title="two")) - trackinfo.append(TrackInfo(title="three")) - mapping, extra_items, extra_tracks = match.assign_items( - items, trackinfo - ) - assert extra_items == [] - assert extra_tracks == [] - assert mapping == { - items[0]: trackinfo[0], - items[1]: trackinfo[2], - items[2]: trackinfo[1], - } + @pytest.mark.parametrize( + # 'expected' is a tuple of expected (mapping, extra_items, extra_tracks) + "item_titles, track_titles, expected", + [ + # items ordering gets corrected + ([A, C, B], [A, B, C], ({A: A, B: B, C: C}, [], [])), + # unmatched tracks are returned as 'extra_tracks' + # the first track is unmatched + ([B, C], [A, B, C], ({B: B, C: C}, [], [A])), + # the middle track is unmatched + ([A, C], [A, B, C], ({A: A, C: C}, [], [B])), + # the last track is unmatched + ([A, B], [A, B, C], ({A: A, B: B}, [], [C])), + # unmatched items are returned as 'extra_items' + ([A, C, B], [A, C], ({A: A, C: C}, [B], [])), + ], + ) + def test_assign_tracks(self, item_titles, track_titles, expected): + expected_mapping, expected_extra_items, expected_extra_tracks = expected - def test_order_works_with_invalid_track_numbers(self): - items = [] - items.append(self.item("one", 1)) - items.append(self.item("three", 1)) - items.append(self.item("two", 1)) - trackinfo = [] - trackinfo.append(TrackInfo(title="one")) - trackinfo.append(TrackInfo(title="two")) - trackinfo.append(TrackInfo(title="three")) - mapping, extra_items, extra_tracks = match.assign_items( - items, trackinfo - ) - assert extra_items == [] - assert extra_tracks == [] - assert mapping == { - items[0]: trackinfo[0], - items[1]: trackinfo[2], - items[2]: trackinfo[1], - } + items = [Item(title=title) for title in item_titles] + tracks = [TrackInfo(title=title) for title in track_titles] - def test_order_works_with_missing_tracks(self): - items = [] - items.append(self.item("one", 1)) - items.append(self.item("two", 2)) - trackinfo = [] - trackinfo.append(TrackInfo(title="one")) - trackinfo.append(TrackInfo(title="two")) - trackinfo.append(TrackInfo(title="three")) - mapping, extra_items, extra_tracks = match.assign_items( - items, trackinfo - ) - assert extra_items == [] - assert extra_tracks == [trackinfo[2]] - assert mapping == {items[0]: trackinfo[0], items[1]: trackinfo[1]} + mapping, extra_items, extra_tracks = match.assign_items(items, tracks) - def test_order_works_with_extra_tracks(self): - items = [] - items.append(self.item("one", 1)) - items.append(self.item("two", 2)) - items.append(self.item("three", 3)) - trackinfo = [] - trackinfo.append(TrackInfo(title="one")) - trackinfo.append(TrackInfo(title="three")) - mapping, extra_items, extra_tracks = match.assign_items( - items, trackinfo - ) - assert extra_items == [items[1]] - assert extra_tracks == [] - assert mapping == {items[0]: trackinfo[0], items[2]: trackinfo[1]} + assert ( + {i.title: t.title for i, t in mapping.items()}, + [i.title for i in extra_items], + [t.title for t in extra_tracks], + ) == (expected_mapping, expected_extra_items, expected_extra_tracks) def test_order_works_when_track_names_are_entirely_wrong(self): # A real-world test case contributed by a user. @@ -587,9 +543,6 @@ class AssignmentTest(unittest.TestCase): title=f"ben harper - Burn to Shine {i}", track=i, length=length, - mb_trackid="", - mb_albumid="", - mb_artistid="", ) items = [] @@ -623,13 +576,9 @@ class AssignmentTest(unittest.TestCase): trackinfo.append(info(11, "Beloved One", 243.733)) trackinfo.append(info(12, "In the Lord's Arms", 186.13300000000001)) - mapping, extra_items, extra_tracks = match.assign_items( - items, trackinfo - ) - assert extra_items == [] - assert extra_tracks == [] - for item, info in mapping.items(): - assert items.index(item) == trackinfo.index(info) + expected = dict(zip(items, trackinfo)), [], [] + + assert match.assign_items(items, trackinfo) == expected class ApplyTestUtil: From ef902ea14f3a67cd18264dbd560b400790b504fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0ar=C5=ABnas=20Nejus?= Date: Tue, 31 Dec 2024 08:13:13 +0000 Subject: [PATCH 3/3] item assignment: set track distance configuration in tests explicitly These tests depend on certain `track_length_grace` and `track_length_max` configuration which was set by other tests in this module. I discovered this issue when I tried to run `test_order_works_when_track_names_are_entirely_wrong` test only - I found that my local configuration was read and the test failed. --- beets/test/helper.py | 42 ++++++++++++++++++++++-------------------- test/test_autotag.py | 9 +++++++-- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/beets/test/helper.py b/beets/test/helper.py index 124063d76..4effa47f8 100644 --- a/beets/test/helper.py +++ b/beets/test/helper.py @@ -48,7 +48,7 @@ from mediafile import Image, MediaFile import beets import beets.plugins -from beets import autotag, config, importer, logging, util +from beets import autotag, importer, logging, util from beets.autotag.hooks import AlbumInfo, TrackInfo from beets.importer import ImportSession from beets.library import Album, Item, Library @@ -153,12 +153,27 @@ def check_reflink_support(path: str) -> bool: return reflink.supported_at(path) +class ConfigMixin: + @cached_property + def config(self) -> beets.IncludeLazyConfig: + """Base beets configuration for tests.""" + config = beets.config + config.sources = [] + config.read(user=False, defaults=True) + + config["plugins"] = [] + config["verbose"] = 1 + config["ui"]["color"] = False + config["threaded"] = False + return config + + NEEDS_REFLINK = unittest.skipUnless( check_reflink_support(gettempdir()), "no reflink support for libdir" ) -class TestHelper(_common.Assertions): +class TestHelper(_common.Assertions, ConfigMixin): """Helper mixin for high-level cli and plugin tests. This mixin provides methods to isolate beets' global state provide @@ -184,8 +199,6 @@ class TestHelper(_common.Assertions): - ``libdir`` Path to a subfolder of ``temp_dir``, containing the library's media files. Same as ``config['directory']``. - - ``config`` The global configuration used by beets. - - ``lib`` Library instance created with the settings from ``config``. @@ -202,15 +215,6 @@ class TestHelper(_common.Assertions): ) self.env_patcher.start() - self.config = beets.config - self.config.sources = [] - self.config.read(user=False, defaults=True) - - self.config["plugins"] = [] - self.config["verbose"] = 1 - self.config["ui"]["color"] = False - self.config["threaded"] = False - self.libdir = os.path.join(self.temp_dir, b"libdir") os.mkdir(syspath(self.libdir)) self.config["directory"] = os.fsdecode(self.libdir) @@ -229,8 +233,6 @@ class TestHelper(_common.Assertions): self.io.restore() self.lib._close() self.remove_temp_dir() - beets.config.clear() - beets.config._materialized = False # Library fixtures methods @@ -452,7 +454,7 @@ class ItemInDBTestCase(BeetsTestCase): self.i = _common.item(self.lib) -class PluginMixin: +class PluginMixin(ConfigMixin): plugin: ClassVar[str] preload_plugin: ClassVar[bool] = True @@ -473,7 +475,7 @@ class PluginMixin: """ # FIXME this should eventually be handled by a plugin manager plugins = (self.plugin,) if hasattr(self, "plugin") else plugins - beets.config["plugins"] = plugins + self.config["plugins"] = plugins beets.plugins.load_plugins(plugins) beets.plugins.find_plugins() @@ -494,7 +496,7 @@ class PluginMixin: # FIXME this should eventually be handled by a plugin manager for plugin_class in beets.plugins._instances: plugin_class.listeners = None - beets.config["plugins"] = [] + self.config["plugins"] = [] beets.plugins._classes = set() beets.plugins._instances = {} Item._types = getattr(Item, "_original_types", {}) @@ -504,7 +506,7 @@ class PluginMixin: @contextmanager def configure_plugin(self, config: Any): - beets.config[self.plugin].set(config) + self.config[self.plugin].set(config) self.load_plugins(self.plugin) yield @@ -624,7 +626,7 @@ class ImportHelper(TestHelper): def setup_importer( self, import_dir: bytes | None = None, **kwargs ) -> ImportSession: - config["import"].set_args({**self.default_import_config, **kwargs}) + self.config["import"].set_args({**self.default_import_config, **kwargs}) self.importer = self._get_import_session(import_dir or self.import_dir) return self.importer diff --git a/test/test_autotag.py b/test/test_autotag.py index 31afc6a63..7f8ed3d2e 100644 --- a/test/test_autotag.py +++ b/test/test_autotag.py @@ -23,7 +23,7 @@ from beets import autotag, config from beets.autotag import AlbumInfo, TrackInfo, correct_list_fields, match from beets.autotag.hooks import Distance, string_dist from beets.library import Item -from beets.test.helper import BeetsTestCase +from beets.test.helper import BeetsTestCase, ConfigMixin from beets.util import plurality @@ -498,11 +498,16 @@ class AlbumDistanceTest(BeetsTestCase): assert dist == 0 -class TestAssignment: +class TestAssignment(ConfigMixin): A = "one" B = "two" C = "three" + @pytest.fixture(autouse=True) + def _setup_config(self): + self.config["match"]["track_length_grace"] = 10 + self.config["match"]["track_length_max"] = 30 + @pytest.mark.parametrize( # 'expected' is a tuple of expected (mapping, extra_items, extra_tracks) "item_titles, track_titles, expected",