From 6919f0214a9d456315b25fb40a3e5a1697498740 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 5 Jan 2025 13:31:32 +0100 Subject: [PATCH 1/3] Code fix --- src/tui/app/machine/match_state.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index 7c2f202..a66867a 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -212,6 +212,10 @@ impl AppMachine { mh.set_artist_mb_ref(&matches.matching.id, tuple.mb_ref) } + fn filter_mb_ref(left: &AlbumMbRef, right: &AlbumMbRef) -> bool { + (left == right) && !matches!(left, AlbumMbRef::CannotHaveMbid) + } + fn select_album( inner: &mut AppInner, matches: &AlbumMatches, @@ -225,7 +229,7 @@ impl AppMachine { let iter = artist.albums.iter(); let filtered = iter .filter(|album| album.meta.id != matches.matching) - .filter(|album| album.meta.id.mb_ref == tuple.mb_ref); + .filter(|album| Self::filter_mb_ref(&album.meta.id.mb_ref, &tuple.mb_ref)); clashing.extend(filtered.map(|a| &a.meta.id).cloned()) } -- 2.47.1 From 6c8913131680b6c73e34b28a3a05de939abe1a27 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 5 Jan 2025 14:24:23 +0100 Subject: [PATCH 2/3] Add unit test --- src/tui/app/machine/match_state.rs | 74 ++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 14 deletions(-) diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index a66867a..566e4d7 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -682,15 +682,13 @@ mod tests { reduced[0].albums.retain(|a| a.meta.id.mb_ref != mb_ref); // We need a custom music_hoard to override the get_filtered expectations. - let mut music_hoard = MockIMusicHoard::new(); + let mut mh = MockIMusicHoard::new(); let mut seq = Sequence::new(); - music_hoard - .expect_get_filtered() + mh.expect_get_filtered() .times(2) .in_sequence(&mut seq) .return_const(collection.clone()); - music_hoard - .expect_get_filtered() + mh.expect_get_filtered() .times(1) .in_sequence(&mut seq) .return_const(reduced.clone()); @@ -701,31 +699,27 @@ mod tests { let artist = matches.artist.clone(); let mut seq = Sequence::new(); - music_hoard - .expect_get_collection() + mh.expect_get_collection() .times(1) .in_sequence(&mut seq) .return_const(collection.clone()); - music_hoard - .expect_remove_album() + mh.expect_remove_album() .times(1) .with(eq(artist), eq(removed.meta.id.clone())) .in_sequence(&mut seq) .return_once(|_, _| Ok(())); - music_hoard - .expect_merge_album_info() + mh.expect_merge_album_info() .times(1) .in_sequence(&mut seq) .return_once(|_, _, _| Ok(())); - music_hoard - .expect_set_album_mb_ref() + mh.expect_set_album_mb_ref() .times(1) .in_sequence(&mut seq) .return_once(|_, _, _| Ok(())); } } - let mut matches = AppMachine::match_state(inner(music_hoard), app_matches); + let mut matches = AppMachine::match_state(inner(mh), app_matches); // Make sure the last album is selected. let selection = &mut matches.inner.selection; @@ -747,6 +741,58 @@ mod tests { assert_eq!(album.meta, selected.meta); } + #[test] + fn set_album_info_with_clashing_cannot_have_mbid() { + let matches_info = album_match(); + + // The collection below should trigger MH to remove the album with the same MBID but not + // matching album_id. + + // (1) Same artist as matches_info. + let mut artist = Artist::new(ArtistId::new("Artist")); + + // (2) An album with the same album_id as the selected one. + artist.albums.push(Album::new(AlbumId::new("Album"))); + + // (3) An album with a different album_id than the selected one. + // (4) This album has an MBID that is identical to that of the selected match. + let removed = + Album::new(AlbumId::new("Album: Not the Same").with_mb_ref(AlbumMbRef::CannotHaveMbid)); + artist.albums.push(removed.clone()); + + // (5) An album after the one that will be removed. Selection must remain on it. + let selected = Album::new(AlbumId::new("Album: Z")); + artist.albums.push(selected.clone()); + + let (_tx, rx) = mpsc::channel(); + let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx)); + + let collection = vec![artist]; + let mut mh = music_hoard(collection.clone()); + + match matches_info { + EntityMatches::Artist(_) => panic!(), + EntityMatches::Album(_) => { + let mut seq = Sequence::new(); + mh.expect_get_collection() + .times(1) + .in_sequence(&mut seq) + .return_const(collection.clone()); + mh.expect_merge_album_info() + .times(1) + .in_sequence(&mut seq) + .return_once(|_, _, _| Ok(())); + mh.expect_set_album_mb_ref() + .times(1) + .in_sequence(&mut seq) + .return_once(|_, _, _| Ok(())); + } + } + + let matches = AppMachine::match_state(inner(mh), app_matches); + matches.select().unwrap_fetch(); + } + #[test] fn set_info_error() { let matches_info = artist_match(); -- 2.47.1 From 8b2d7fb07a39c57ba320ecf5156d9fb9dd3c7a6f Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 5 Jan 2025 14:42:27 +0100 Subject: [PATCH 3/3] Shorten code --- src/tui/app/machine/match_state.rs | 176 +++++++++++++---------------- 1 file changed, 79 insertions(+), 97 deletions(-) diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index 566e4d7..7abbef1 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -327,6 +327,7 @@ mod tests { Album, AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType, }, artist::{Artist, ArtistId, ArtistMeta}, + Collection, }; use crate::tui::{ @@ -608,6 +609,42 @@ mod tests { matches.select().unwrap_fetch(); } + fn set_album_info_expectations( + mh: &mut MockIMusicHoard, + collection: Collection, + matches: AlbumMatches, + mut album_id: AlbumId, + removed_id: Option, + ) { + let meta = album_meta(album_id.clone()); + let mb_ref = album_id.mb_ref.clone(); + album_id.clear_mb_ref(); + let artist = matches.artist.clone(); + + let mut seq = Sequence::new(); + mh.expect_get_collection() + .times(1) + .in_sequence(&mut seq) + .return_const(collection); + if let Some(removed_id) = removed_id { + mh.expect_remove_album() + .times(1) + .with(eq(artist.clone()), eq(removed_id)) + .in_sequence(&mut seq) + .return_once(|_, _| Ok(())); + } + mh.expect_merge_album_info() + .with(eq(artist.clone()), eq(album_id.clone()), eq(meta.info)) + .times(1) + .in_sequence(&mut seq) + .return_once(|_, _, _| Ok(())); + mh.expect_set_album_mb_ref() + .with(eq(artist.clone()), eq(album_id.clone()), eq(mb_ref)) + .times(1) + .in_sequence(&mut seq) + .return_once(|_, _, _| Ok(())); + } + #[test] fn set_album_info() { let matches_info = album_match(); @@ -616,45 +653,22 @@ mod tests { let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx)); let collection = vec![]; - let mut music_hoard = music_hoard(collection.clone()); + let mut mh = music_hoard(collection.clone()); match matches_info { EntityMatches::Artist(_) => panic!(), EntityMatches::Album(matches) => { - let mut album_id = album_id(); - let meta = album_meta(album_id.clone()); - let mb_ref = album_id.mb_ref.clone(); - album_id.clear_mb_ref(); - let artist = matches.artist.clone(); - - let mut seq = Sequence::new(); - music_hoard - .expect_get_collection() - .times(1) - .in_sequence(&mut seq) - .return_const(collection); - music_hoard - .expect_merge_album_info() - .with(eq(artist.clone()), eq(album_id.clone()), eq(meta.info)) - .times(1) - .in_sequence(&mut seq) - .return_once(|_, _, _| Ok(())); - music_hoard - .expect_set_album_mb_ref() - .with(eq(artist.clone()), eq(album_id.clone()), eq(mb_ref)) - .times(1) - .in_sequence(&mut seq) - .return_once(|_, _, _| Ok(())); + set_album_info_expectations(&mut mh, collection, matches, album_id(), None) } } - let matches = AppMachine::match_state(inner(music_hoard), app_matches); + let matches = AppMachine::match_state(inner(mh), app_matches); matches.select().unwrap_fetch(); } - #[test] - fn set_album_info_with_clashing_mbid() { - let matches_info = album_match(); - + fn albums_with_clashing_mbid( + clashing_id: AlbumId, + last_id: AlbumId, + ) -> (Collection, Collection) { // The collection below should trigger MH to remove the album with the same MBID but not // matching album_id. @@ -666,20 +680,31 @@ mod tests { // (3) An album with a different album_id than the selected one. // (4) This album has an MBID that is identical to that of the selected match. - let mb_ref = AlbumMbRef::Some(mbid().into()); - let removed = Album::new(AlbumId::new("Album: Not the Same").with_mb_ref(mb_ref.clone())); - artist.albums.push(removed.clone()); + let mb_ref = clashing_id.mb_ref.clone(); + artist.albums.push(Album::new(clashing_id)); // (5) An album after the one that will be removed. Selection must remain on it. - let selected = Album::new(AlbumId::new("Album: Z")); - artist.albums.push(selected.clone()); + artist.albums.push(Album::new(last_id)); + + let collection = vec![artist]; + + let mut reduced = collection.clone(); + reduced[0].albums.retain(|a| a.meta.id.mb_ref != mb_ref); + + (collection, reduced) + } + + #[test] + fn set_album_info_with_clashing_mbid() { + let matches_info = album_match(); let (_tx, rx) = mpsc::channel(); let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx)); - let collection = vec![artist]; - let mut reduced = collection.clone(); - reduced[0].albums.retain(|a| a.meta.id.mb_ref != mb_ref); + let mb_ref = AlbumMbRef::Some(mbid().into()); + let clashing = AlbumId::new("Album: Not the Same").with_mb_ref(mb_ref.clone()); + let selected = AlbumId::new("Album: Z"); + let (collection, reduced) = albums_with_clashing_mbid(clashing.clone(), selected.clone()); // We need a custom music_hoard to override the get_filtered expectations. let mut mh = MockIMusicHoard::new(); @@ -695,28 +720,13 @@ mod tests { match matches_info { EntityMatches::Artist(_) => panic!(), - EntityMatches::Album(matches) => { - let artist = matches.artist.clone(); - - let mut seq = Sequence::new(); - mh.expect_get_collection() - .times(1) - .in_sequence(&mut seq) - .return_const(collection.clone()); - mh.expect_remove_album() - .times(1) - .with(eq(artist), eq(removed.meta.id.clone())) - .in_sequence(&mut seq) - .return_once(|_, _| Ok(())); - mh.expect_merge_album_info() - .times(1) - .in_sequence(&mut seq) - .return_once(|_, _, _| Ok(())); - mh.expect_set_album_mb_ref() - .times(1) - .in_sequence(&mut seq) - .return_once(|_, _, _| Ok(())); - } + EntityMatches::Album(matches) => set_album_info_expectations( + &mut mh, + collection.clone(), + matches, + album_id(), + Some(clashing.clone()), + ), } let mut matches = AppMachine::match_state(inner(mh), app_matches); @@ -730,7 +740,7 @@ mod tests { // Check that the correct album is selected. let state = selection.state_album(&collection).unwrap(); let album = &collection[0].albums[state.index]; - assert_eq!(album.meta, selected.meta); + assert_eq!(album.meta.id, selected); let mut fetch = matches.select().unwrap_fetch(); @@ -738,54 +748,26 @@ mod tests { let selection = &mut fetch.inner.selection; let state = selection.state_album(&reduced).unwrap(); let album = &reduced[0].albums[state.index]; - assert_eq!(album.meta, selected.meta); + assert_eq!(album.meta.id, selected); } #[test] fn set_album_info_with_clashing_cannot_have_mbid() { let matches_info = album_match(); - // The collection below should trigger MH to remove the album with the same MBID but not - // matching album_id. - - // (1) Same artist as matches_info. - let mut artist = Artist::new(ArtistId::new("Artist")); - - // (2) An album with the same album_id as the selected one. - artist.albums.push(Album::new(AlbumId::new("Album"))); - - // (3) An album with a different album_id than the selected one. - // (4) This album has an MBID that is identical to that of the selected match. - let removed = - Album::new(AlbumId::new("Album: Not the Same").with_mb_ref(AlbumMbRef::CannotHaveMbid)); - artist.albums.push(removed.clone()); - - // (5) An album after the one that will be removed. Selection must remain on it. - let selected = Album::new(AlbumId::new("Album: Z")); - artist.albums.push(selected.clone()); - let (_tx, rx) = mpsc::channel(); let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx)); - let collection = vec![artist]; - let mut mh = music_hoard(collection.clone()); + let mb_ref = AlbumMbRef::CannotHaveMbid; + let clashing = AlbumId::new("Album: Not the Same").with_mb_ref(mb_ref); + let selected = AlbumId::new("Album: Z"); + let (collection, _) = albums_with_clashing_mbid(clashing.clone(), selected.clone()); + let mut mh = music_hoard(collection.clone()); match matches_info { EntityMatches::Artist(_) => panic!(), - EntityMatches::Album(_) => { - let mut seq = Sequence::new(); - mh.expect_get_collection() - .times(1) - .in_sequence(&mut seq) - .return_const(collection.clone()); - mh.expect_merge_album_info() - .times(1) - .in_sequence(&mut seq) - .return_once(|_, _, _| Ok(())); - mh.expect_set_album_mb_ref() - .times(1) - .in_sequence(&mut seq) - .return_once(|_, _, _| Ok(())); + EntityMatches::Album(matches) => { + set_album_info_expectations(&mut mh, collection, matches, album_id(), None) } } -- 2.47.1