diff --git a/core/extdata/extdata_helper_test.go b/core/extdata/extdata_helper_test.go new file mode 100644 index 000000000..9c5538d7a --- /dev/null +++ b/core/extdata/extdata_helper_test.go @@ -0,0 +1,245 @@ +package extdata + +import ( + "context" + "errors" + + "github.com/navidrome/navidrome/core/agents" + "github.com/navidrome/navidrome/model" + "github.com/stretchr/testify/mock" +) + +// --- Shared Mock Implementations --- + +// mockArtistRepo mocks model.ArtistRepository +type mockArtistRepo struct { + mock.Mock + model.ArtistRepository +} + +func newMockArtistRepo() *mockArtistRepo { + return &mockArtistRepo{} +} + +// SetData sets up basic Get expectations. +func (m *mockArtistRepo) SetData(artists model.Artists) { + for _, a := range artists { + artistCopy := a + m.On("Get", artistCopy.ID).Return(&artistCopy, nil) + } +} + +// Get implements model.ArtistRepository. +func (m *mockArtistRepo) Get(id string) (*model.Artist, error) { + args := m.Called(id) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*model.Artist), args.Error(1) +} + +// GetAll implements model.ArtistRepository. +func (m *mockArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, error) { + argsSlice := make([]interface{}, len(options)) + for i, v := range options { + argsSlice[i] = v + } + args := m.Called(argsSlice...) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(model.Artists), args.Error(1) +} + +// SetError is a helper to set up a generic error for GetAll. +func (m *mockArtistRepo) SetError(hasError bool) { + if hasError { + m.On("GetAll", mock.Anything).Return(nil, errors.New("mock repo error")) + } +} + +// FindByName is a helper to set up a GetAll expectation for finding by name. +func (m *mockArtistRepo) FindByName(name string, artist model.Artist) { + m.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { + return opt.Filters != nil + })).Return(model.Artists{artist}, nil).Once() +} + +// mockMediaFileRepo mocks model.MediaFileRepository +type mockMediaFileRepo struct { + mock.Mock + model.MediaFileRepository +} + +func newMockMediaFileRepo() *mockMediaFileRepo { + return &mockMediaFileRepo{} +} + +// SetData sets up basic Get expectations. +func (m *mockMediaFileRepo) SetData(mediaFiles model.MediaFiles) { + for _, mf := range mediaFiles { + mfCopy := mf + m.On("Get", mfCopy.ID).Return(&mfCopy, nil) + } +} + +// Get implements model.MediaFileRepository. +func (m *mockMediaFileRepo) Get(id string) (*model.MediaFile, error) { + args := m.Called(id) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*model.MediaFile), args.Error(1) +} + +// GetAll implements model.MediaFileRepository. +func (m *mockMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) { + argsSlice := make([]interface{}, len(options)) + for i, v := range options { + argsSlice[i] = v + } + args := m.Called(argsSlice...) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(model.MediaFiles), args.Error(1) +} + +// SetError is a helper to set up a generic error for GetAll. +func (m *mockMediaFileRepo) SetError(hasError bool) { + if hasError { + m.On("GetAll", mock.Anything).Return(nil, errors.New("mock repo error")) + } +} + +// FindByMBID is a helper to set up a GetAll expectation for finding by MBID. +func (m *mockMediaFileRepo) FindByMBID(mbid string, mediaFile model.MediaFile) { + m.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { + return opt.Filters != nil + })).Return(model.MediaFiles{mediaFile}, nil).Once() +} + +// FindByArtistAndTitle is a helper to set up a GetAll expectation for finding by artist/title. +func (m *mockMediaFileRepo) FindByArtistAndTitle(artistID string, title string, mediaFile model.MediaFile) { + m.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { + return opt.Filters != nil + })).Return(model.MediaFiles{mediaFile}, nil).Once() +} + +// mockSimilarArtistAgent mocks agents implementing ArtistTopSongsRetriever and ArtistSimilarRetriever +type mockSimilarArtistAgent struct { + mock.Mock + agents.Interface // Embed to satisfy methods not explicitly mocked +} + +func (m *mockSimilarArtistAgent) AgentName() string { + return "mockSimilar" +} + +func (m *mockSimilarArtistAgent) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { + args := m.Called(ctx, id, artistName, mbid, count) + if args.Get(0) != nil { + return args.Get(0).([]agents.Song), args.Error(1) + } + return nil, args.Error(1) +} + +func (m *mockSimilarArtistAgent) GetSimilarArtists(ctx context.Context, id, name, mbid string, limit int) ([]agents.Artist, error) { + args := m.Called(ctx, id, name, mbid, limit) + if args.Get(0) != nil { + return args.Get(0).([]agents.Artist), args.Error(1) + } + return nil, args.Error(1) +} + +// mockCombinedAgents mocks the main Agents interface used by Provider +type mockCombinedAgents struct { + topSongsAgent agents.ArtistTopSongsRetriever + similarAgent agents.ArtistSimilarRetriever + agents.Interface // Embed to satisfy non-overridden methods +} + +func (m *mockCombinedAgents) AgentName() string { + return "mockCombined" +} + +func (m *mockCombinedAgents) GetSimilarArtists(ctx context.Context, id, name, mbid string, limit int) ([]agents.Artist, error) { + if m.similarAgent != nil { + return m.similarAgent.GetSimilarArtists(ctx, id, name, mbid, limit) + } + return nil, agents.ErrNotFound +} + +func (m *mockCombinedAgents) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { + if m.topSongsAgent != nil { + return m.topSongsAgent.GetArtistTopSongs(ctx, id, artistName, mbid, count) + } + return nil, agents.ErrNotFound +} + +// --- Stubs for other Agents interface methods --- + +func (m *mockCombinedAgents) GetAlbumInfo(ctx context.Context, name, artist, mbid string) (*agents.AlbumInfo, error) { + if m.topSongsAgent != nil { + if ar, ok := m.topSongsAgent.(agents.AlbumInfoRetriever); ok { + return ar.GetAlbumInfo(ctx, name, artist, mbid) + } + } + return nil, agents.ErrNotFound +} + +func (m *mockCombinedAgents) GetArtistMBID(ctx context.Context, id string, name string) (string, error) { + if m.topSongsAgent != nil { + if ar, ok := m.topSongsAgent.(agents.ArtistMBIDRetriever); ok { + return ar.GetArtistMBID(ctx, id, name) + } + } + if m.similarAgent != nil { + if ar, ok := m.similarAgent.(agents.ArtistMBIDRetriever); ok { + return ar.GetArtistMBID(ctx, id, name) + } + } + return "", agents.ErrNotFound +} + +func (m *mockCombinedAgents) GetArtistURL(ctx context.Context, id, name, mbid string) (string, error) { + if m.topSongsAgent != nil { + if ar, ok := m.topSongsAgent.(agents.ArtistURLRetriever); ok { + return ar.GetArtistURL(ctx, id, name, mbid) + } + } + if m.similarAgent != nil { + if ar, ok := m.similarAgent.(agents.ArtistURLRetriever); ok { + return ar.GetArtistURL(ctx, id, name, mbid) + } + } + return "", agents.ErrNotFound +} + +func (m *mockCombinedAgents) GetArtistBiography(ctx context.Context, id, name, mbid string) (string, error) { + if m.topSongsAgent != nil { + if ar, ok := m.topSongsAgent.(agents.ArtistBiographyRetriever); ok { + return ar.GetArtistBiography(ctx, id, name, mbid) + } + } + if m.similarAgent != nil { + if ar, ok := m.similarAgent.(agents.ArtistBiographyRetriever); ok { + return ar.GetArtistBiography(ctx, id, name, mbid) + } + } + return "", agents.ErrNotFound +} + +func (m *mockCombinedAgents) GetArtistImages(ctx context.Context, id, name, mbid string) ([]agents.ExternalImage, error) { + if m.topSongsAgent != nil { + if ar, ok := m.topSongsAgent.(agents.ArtistImageRetriever); ok { + return ar.GetArtistImages(ctx, id, name, mbid) + } + } + if m.similarAgent != nil { + if ar, ok := m.similarAgent.(agents.ArtistImageRetriever); ok { + return ar.GetArtistImages(ctx, id, name, mbid) + } + } + return nil, agents.ErrNotFound +} diff --git a/core/extdata/provider_similarsongs_test.go b/core/extdata/provider_similarsongs_test.go index 24e785856..4398e9382 100644 --- a/core/extdata/provider_similarsongs_test.go +++ b/core/extdata/provider_similarsongs_test.go @@ -20,100 +20,80 @@ var _ = Describe("Provider - SimilarSongs", func() { var mockTopAgent agents.ArtistTopSongsRetriever var mockSimilarAgent agents.ArtistSimilarRetriever var agentsCombined Agents - var artistRepo *mockSimArtistRepo - var mediaFileRepo *mockSimMediaFileRepo + var artistRepo *mockArtistRepo + var mediaFileRepo *mockMediaFileRepo var ctx context.Context var originalAgentsConfig string BeforeEach(func() { ctx = context.Background() - - // Store the original agents config to restore it later originalAgentsConfig = conf.Server.Agents - // Setup mocks - Initialize here, but set expectations within each test - artistRepo = newMockSimArtistRepo() - mediaFileRepo = newMockSimMediaFileRepo() + artistRepo = newMockArtistRepo() + mediaFileRepo = newMockMediaFileRepo() ds = &tests.MockDataStore{ MockedArtist: artistRepo, MockedMediaFile: mediaFileRepo, } - // Clear the agents map to prevent interference from previous tests agents.Map = nil - // Create a mock agent that implements both required interfaces - // Re-initialize mockAgent in each test if necessary, or ensure Calls are cleared mockAgent = &mockSimilarArtistAgent{} mockTopAgent = mockAgent mockSimilarAgent = mockAgent - // Create a mock for the combined Agents interface agentsCombined = &mockCombinedAgents{ topSongsAgent: mockTopAgent, similarAgent: mockSimilarAgent, } - // Create the provider instance with our mock Agents implementation provider = NewProvider(ds, agentsCombined) }) AfterEach(func() { - // Restore original config conf.Server.Agents = originalAgentsConfig }) Describe("SimilarSongs", func() { It("returns similar songs from main artist and similar artists", func() { - // --- Test-specific setup --- artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} similarArtist := model.Artist{ID: "artist-3", Name: "Similar Artist"} song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1"} song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1"} song3 := model.MediaFile{ID: "song-3", Title: "Song Three", ArtistID: "artist-3"} - // Configure the Get method (needed for GetEntityByID in getArtist) artistRepo.On("Get", "artist-1").Return(&artist1, nil).Maybe() - artistRepo.On("Get", "artist-3").Return(&similarArtist, nil).Maybe() // For similar artist lookup if needed + artistRepo.On("Get", "artist-3").Return(&similarArtist, nil).Maybe() - // Configure the GetAll mock for finding the main artist in getArtist artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { return opt.Max == 1 && opt.Filters != nil })).Return(model.Artists{artist1}, nil).Once() - // Setup similar artists response from agent similarAgentsResp := []agents.Artist{ {Name: "Similar Artist", MBID: "similar-mbid"}, } mockAgent.On("GetSimilarArtists", mock.Anything, "artist-1", "Artist One", "", 15). Return(similarAgentsResp, nil).Once() - // Setup for mapping similar artists in mapSimilarArtists artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { - // This will match the query for similar artists (no Max limit set by caller) return opt.Max == 0 && opt.Filters != nil })).Return(model.Artists{similarArtist}, nil).Once() - // Setup Top Songs responses - // Main artist songs mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-1", "Artist One", "", mock.Anything). Return([]agents.Song{ {Name: "Song One", MBID: "mbid-1"}, {Name: "Song Two", MBID: "mbid-2"}, }, nil).Once() - // Similar artist songs mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-3", "Similar Artist", "", mock.Anything). Return([]agents.Song{ {Name: "Song Three", MBID: "mbid-3"}, }, nil).Once() - // Setup mediafile repository to find songs by MBID (via GetAll) mediaFileRepo.FindByMBID("mbid-1", song1) mediaFileRepo.FindByMBID("mbid-2", song2) mediaFileRepo.FindByMBID("mbid-3", song3) - // --- End Test-specific setup --- songs, err := provider.SimilarSongs(ctx, "artist-1", 3) @@ -125,18 +105,13 @@ var _ = Describe("Provider - SimilarSongs", func() { }) It("returns nil when artist is not found", func() { - // --- Test-specific setup --- - // Use prefixed ID for GetEntityByID artistRepo.On("Get", "artist-unknown-artist").Return(nil, model.ErrNotFound) mediaFileRepo.On("Get", "artist-unknown-artist").Return(nil, model.ErrNotFound) - // Setup for getArtist fallback to GetAll artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { return opt.Max == 1 && opt.Filters != nil })).Return(model.Artists{}, nil).Maybe() - // --- End Test-specific setup --- - // Use prefixed ID songs, err := provider.SimilarSongs(ctx, "artist-unknown-artist", 5) Expect(err).To(Equal(model.ErrNotFound)) @@ -144,35 +119,27 @@ var _ = Describe("Provider - SimilarSongs", func() { }) It("returns songs from main artist when GetSimilarArtists returns error", func() { - // --- Test-specific setup --- artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1"} - // Configure artist repo Get method for getArtist artistRepo.On("Get", "artist-1").Return(&artist1, nil).Maybe() - // Configure GetAll fallback for getArtist artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { return opt.Max == 1 && opt.Filters != nil - })).Return(model.Artists{artist1}, nil).Maybe() // Maybe because Get should find it + })).Return(model.Artists{artist1}, nil).Maybe() - // Set the error for GetSimilarArtists mockAgent.On("GetSimilarArtists", mock.Anything, "artist-1", "Artist One", "", 15). Return(nil, errors.New("error getting similar artists")).Once() - // Expect call to mapSimilarArtists -> artistRepo.GetAll (returns empty) artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { - return opt.Max == 0 && opt.Filters != nil // Matcher for mapSimilarArtists + return opt.Max == 0 && opt.Filters != nil })).Return(model.Artists{}, nil).Once() - // Setup for main artist's top songs mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-1", "Artist One", "", mock.Anything). Return([]agents.Song{ {Name: "Song One", MBID: "mbid-1"}, }, nil).Once() - // Setup mediafile repo for finding the song mediaFileRepo.FindByMBID("mbid-1", song1) - // --- End Test-specific setup --- songs, err := provider.SimilarSongs(ctx, "artist-1", 5) @@ -182,29 +149,22 @@ var _ = Describe("Provider - SimilarSongs", func() { }) It("returns empty list when GetArtistTopSongs returns error", func() { - // --- Test-specific setup --- artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} - // Configure artist repo Get method for getArtist artistRepo.On("Get", "artist-1").Return(&artist1, nil).Maybe() - // Configure GetAll fallback for getArtist artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { return opt.Max == 1 && opt.Filters != nil - })).Return(model.Artists{artist1}, nil).Maybe() // Maybe because Get should find it + })).Return(model.Artists{artist1}, nil).Maybe() - // Expect GetSimilarArtists call (returns empty) mockAgent.On("GetSimilarArtists", mock.Anything, "artist-1", "Artist One", "", 15). Return([]agents.Artist{}, nil).Once() - // Expect call to mapSimilarArtists -> artistRepo.GetAll (returns empty) artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { - return opt.Max == 0 && opt.Filters != nil // Matcher for mapSimilarArtists + return opt.Max == 0 && opt.Filters != nil })).Return(model.Artists{}, nil).Once() - // Set error for GetArtistTopSongs (for the main artist) mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-1", "Artist One", "", mock.Anything). Return(nil, errors.New("error getting top songs")).Once() - // --- End Test-specific setup --- songs, err := provider.SimilarSongs(ctx, "artist-1", 5) @@ -213,38 +173,30 @@ var _ = Describe("Provider - SimilarSongs", func() { }) It("respects count parameter", func() { - // --- Test-specific setup --- artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1"} song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1"} - // Configure artist repo Get method for getArtist artistRepo.On("Get", "artist-1").Return(&artist1, nil).Maybe() - // Configure GetAll fallback for getArtist artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { return opt.Max == 1 && opt.Filters != nil - })).Return(model.Artists{artist1}, nil).Maybe() // Maybe because Get should find it + })).Return(model.Artists{artist1}, nil).Maybe() - // Expect GetSimilarArtists call (returns empty) mockAgent.On("GetSimilarArtists", mock.Anything, "artist-1", "Artist One", "", 15). Return([]agents.Artist{}, nil).Once() - // Expect call to mapSimilarArtists -> artistRepo.GetAll (returns empty) artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { - return opt.Max == 0 && opt.Filters != nil // Matcher for mapSimilarArtists + return opt.Max == 0 && opt.Filters != nil })).Return(model.Artists{}, nil).Once() - // Setup for main artist's top songs mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-1", "Artist One", "", mock.Anything). Return([]agents.Song{ {Name: "Song One", MBID: "mbid-1"}, {Name: "Song Two", MBID: "mbid-2"}, }, nil).Once() - // Setup mediafile repo for finding the songs mediaFileRepo.FindByMBID("mbid-1", song1) mediaFileRepo.FindByMBID("mbid-2", song2) - // --- End Test-specific setup --- songs, err := provider.SimilarSongs(ctx, "artist-1", 1) @@ -254,194 +206,3 @@ var _ = Describe("Provider - SimilarSongs", func() { }) }) }) - -// Combined implementation of both ArtistTopSongsRetriever and ArtistSimilarRetriever interfaces -type mockSimilarArtistAgent struct { - mock.Mock - agents.Interface -} - -func (m *mockSimilarArtistAgent) AgentName() string { - return "mock" -} - -func (m *mockSimilarArtistAgent) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { - args := m.Called(ctx, id, artistName, mbid, count) - if args.Get(0) != nil { - return args.Get(0).([]agents.Song), args.Error(1) - } - return nil, args.Error(1) -} - -func (m *mockSimilarArtistAgent) GetSimilarArtists(ctx context.Context, id, name, mbid string, limit int) ([]agents.Artist, error) { - args := m.Called(ctx, id, name, mbid, limit) - if args.Get(0) != nil { - return args.Get(0).([]agents.Artist), args.Error(1) - } - return nil, args.Error(1) -} - -// A simple implementation of the Agents interface that combines separate implementations -type mockCombinedAgents struct { - topSongsAgent agents.ArtistTopSongsRetriever - similarAgent agents.ArtistSimilarRetriever -} - -func (m *mockCombinedAgents) AgentName() string { - return "mockCombined" -} - -func (m *mockCombinedAgents) GetArtistMBID(ctx context.Context, id string, name string) (string, error) { - return "", nil -} - -func (m *mockCombinedAgents) GetArtistURL(ctx context.Context, id, name, mbid string) (string, error) { - return "", nil -} - -func (m *mockCombinedAgents) GetArtistBiography(ctx context.Context, id, name, mbid string) (string, error) { - return "", nil -} - -func (m *mockCombinedAgents) GetSimilarArtists(ctx context.Context, id, name, mbid string, limit int) ([]agents.Artist, error) { - if m.similarAgent != nil { - return m.similarAgent.GetSimilarArtists(ctx, id, name, mbid, limit) - } - return nil, agents.ErrNotFound -} - -func (m *mockCombinedAgents) GetArtistImages(ctx context.Context, id, name, mbid string) ([]agents.ExternalImage, error) { - return nil, agents.ErrNotFound -} - -func (m *mockCombinedAgents) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { - if m.topSongsAgent != nil { - return m.topSongsAgent.GetArtistTopSongs(ctx, id, artistName, mbid, count) - } - return nil, agents.ErrNotFound -} - -func (m *mockCombinedAgents) GetAlbumInfo(ctx context.Context, name, artist, mbid string) (*agents.AlbumInfo, error) { - return nil, agents.ErrNotFound -} - -// Mocked ArtistRepo for similar songs tests -type mockSimArtistRepo struct { - mock.Mock - model.ArtistRepository -} - -func newMockSimArtistRepo() *mockSimArtistRepo { - return &mockSimArtistRepo{} -} - -func (m *mockSimArtistRepo) SetData(artists model.Artists) { - // Store the data for Get queries - for _, a := range artists { - // Capture the loop variable by value - artistCopy := a - // Revert: Get does not take context - m.On("Get", artistCopy.ID).Return(&artistCopy, nil) - } -} - -// Revert: Get does not take context -func (m *mockSimArtistRepo) Get(id string) (*model.Artist, error) { - // Revert: Remove context from Called - args := m.Called(id) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).(*model.Artist), args.Error(1) -} - -func (m *mockSimArtistRepo) SetError(hasError bool) { - if hasError { - // Revert: Remove context from GetAll mock setup - m.On("GetAll", mock.Anything).Return(nil, errors.New("error")) - } -} - -func (m *mockSimArtistRepo) FindByName(name string, artist model.Artist) { - // Set up a mock for finding an artist by name with LIKE filter, using Anything matcher for flexibility - // Revert: Remove context from GetAll mock setup - m.On("GetAll", mock.Anything).Return(model.Artists{artist}, nil).Once() -} - -// Revert: Remove context from GetAll signature -func (m *mockSimArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, error) { - // Pass options correctly to Called - // Convert options slice to []interface{} for Called - argsSlice := make([]interface{}, len(options)) - for i, v := range options { - argsSlice[i] = v - } - args := m.Called(argsSlice...) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).(model.Artists), args.Error(1) -} - -// Mocked MediaFileRepo for similar songs tests -type mockSimMediaFileRepo struct { - mock.Mock - model.MediaFileRepository -} - -func newMockSimMediaFileRepo() *mockSimMediaFileRepo { - return &mockSimMediaFileRepo{} -} - -func (m *mockSimMediaFileRepo) SetData(mediaFiles model.MediaFiles) { - // Store the data for Get queries - for _, mf := range mediaFiles { - mfCopy := mf // Capture loop variable - // Revert: Get does not take context - m.On("Get", mfCopy.ID).Return(&mfCopy, nil) - } -} - -// Revert: Get does not take context -func (m *mockSimMediaFileRepo) Get(id string) (*model.MediaFile, error) { - // Revert: Remove context from Called - args := m.Called(id) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).(*model.MediaFile), args.Error(1) -} - -func (m *mockSimMediaFileRepo) SetError(hasError bool) { - if hasError { - // Revert: Remove context from GetAll mock setup - m.On("GetAll", mock.Anything).Return(nil, errors.New("error")) - } -} - -func (m *mockSimMediaFileRepo) FindByMBID(mbid string, mediaFile model.MediaFile) { - // Set up a mock for finding a media file by MBID using Anything matcher for flexibility - // Revert: Remove context from GetAll mock setup - m.On("GetAll", mock.Anything).Return(model.MediaFiles{mediaFile}, nil).Once() -} - -func (m *mockSimMediaFileRepo) FindByArtistAndTitle(artistID string, title string, mediaFile model.MediaFile) { - // Set up a mock for finding a media file by artist ID and title - // Revert: Remove context from GetAll mock setup - m.On("GetAll", mock.Anything).Return(model.MediaFiles{mediaFile}, nil).Once() -} - -// Revert: Remove context from GetAll signature -func (m *mockSimMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) { - // Pass options correctly to Called - // Convert options slice to []interface{} for Called - argsSlice := make([]interface{}, len(options)) - for i, v := range options { - argsSlice[i] = v - } - args := m.Called(argsSlice...) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).(model.MediaFiles), args.Error(1) -} diff --git a/core/extdata/provider_topsongs_test.go b/core/extdata/provider_topsongs_test.go index 4526129c1..1b09577e5 100644 --- a/core/extdata/provider_topsongs_test.go +++ b/core/extdata/provider_topsongs_test.go @@ -4,12 +4,12 @@ import ( "context" "errors" + "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/agents" _ "github.com/navidrome/navidrome/core/agents/lastfm" _ "github.com/navidrome/navidrome/core/agents/listenbrainz" _ "github.com/navidrome/navidrome/core/agents/spotify" - "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" @@ -17,23 +17,20 @@ import ( "github.com/stretchr/testify/mock" ) -var _ = Describe("Provider", func() { +var _ = Describe("Provider - TopSongs", func() { var ds model.DataStore var provider Provider - var mockAgent *mockArtistTopSongsAgent - var mockAgents *mockAllAgents var artistRepo *mockArtistRepo var mediaFileRepo *mockMediaFileRepo + var mockTopSongsAgent *mockArtistTopSongsAgent + var agentsCombined Agents var ctx context.Context var originalAgentsConfig string BeforeEach(func() { ctx = context.Background() - - // Store the original agents config to restore it later originalAgentsConfig = conf.Server.Agents - // Setup mocks artistRepo = newMockArtistRepo() mediaFileRepo = newMockMediaFileRepo() @@ -42,194 +39,110 @@ var _ = Describe("Provider", func() { MockedMediaFile: mediaFileRepo, } - // Clear the agents map to prevent interference from previous tests + mockTopSongsAgent = &mockArtistTopSongsAgent{} + + agentsCombined = &mockCombinedAgents{ + topSongsAgent: mockTopSongsAgent, + similarAgent: nil, + } + agents.Map = nil - - // Create a mock agent - mockAgent = &mockArtistTopSongsAgent{} - log.Debug(ctx, "Creating mock agent", "agent", mockAgent) - - // Create a mock for the Agents interface that Provider depends on - mockAgents = newMockAllAgents() - mockAgents.topSongsRetriever = mockAgent - - // Create the provider instance with our mock Agents implementation - provider = NewProvider(ds, mockAgents) + provider = NewProvider(ds, agentsCombined) }) AfterEach(func() { - // Restore original config conf.Server.Agents = originalAgentsConfig + agents.Map = nil }) - Describe("TopSongs with direct agent injection", func() { + Describe("TopSongs", func() { BeforeEach(func() { - // Set up test data artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} artist2 := model.Artist{ID: "artist-2", Name: "Artist Two"} - - song1 := model.MediaFile{ - ID: "song-1", - Title: "Song One", - Artist: "Artist One", - ArtistID: "artist-1", - AlbumArtistID: "artist-1", - MbzReleaseTrackID: "mbid-1", - Missing: false, - } - - song2 := model.MediaFile{ - ID: "song-2", - Title: "Song Two", - Artist: "Artist One", - ArtistID: "artist-1", - AlbumArtistID: "artist-1", - MbzReleaseTrackID: "mbid-2", - Missing: false, - } - - song3 := model.MediaFile{ - ID: "song-3", - Title: "Song Three", - Artist: "Artist Two", - ArtistID: "artist-2", - AlbumArtistID: "artist-2", - MbzReleaseTrackID: "mbid-3", - Missing: false, - } - - // Set up basic data for the repos artistRepo.SetData(model.Artists{artist1, artist2}) + + song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzReleaseTrackID: "mbid-1"} + song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzReleaseTrackID: "mbid-2"} + song3 := model.MediaFile{ID: "song-3", Title: "Song Three", ArtistID: "artist-2", MbzReleaseTrackID: "mbid-3"} mediaFileRepo.SetData(model.MediaFiles{song1, song2, song3}) + mockTopSongsAgent.SetTopSongs([]agents.Song{ + {Name: "Song One", MBID: "mbid-1"}, + {Name: "Song Two", MBID: "mbid-2"}, + }) }) - It("returns matching songs from the agent results", func() { - // Setup data needed for this specific test + It("returns top songs for a known artist", func() { artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} artistRepo.FindByName("Artist One", artist1) - song1 := model.MediaFile{ - ID: "song-1", - Title: "Song One", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-1", - Missing: false, - } - - song2 := model.MediaFile{ - ID: "song-2", - Title: "Song Two", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-2", - Missing: false, - } - + song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzReleaseTrackID: "mbid-1"} + song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzReleaseTrackID: "mbid-2"} mediaFileRepo.FindByMBID("mbid-1", song1) mediaFileRepo.FindByMBID("mbid-2", song2) - // Configure the mockAgent to return some top songs - mockAgent.topSongs = []agents.Song{ - {Name: "Song One", MBID: "mbid-1"}, - {Name: "Song Two", MBID: "mbid-2"}, - } - - songs, err := provider.TopSongs(ctx, "Artist One", 5) + songs, err := provider.TopSongs(ctx, "Artist One", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) Expect(songs[0].ID).To(Equal("song-1")) Expect(songs[1].ID).To(Equal("song-2")) - - // Verify the agent was called with the right parameters - Expect(mockAgent.lastArtistID).To(Equal("artist-1")) - Expect(mockAgent.lastArtistName).To(Equal("Artist One")) - Expect(mockAgent.lastCount).To(Equal(5)) }) - It("returns nil when artist is not found", func() { - // Clear any previous mock setup to avoid conflicts - artistRepo = newMockArtistRepo() - - // Setup for artist not found scenario - return empty list - artistRepo.On("GetAll", mock.Anything).Return(model.Artists{}, nil).Once() - - // We need to recreate the datastore with the new mocks - ds = &tests.MockDataStore{ - MockedArtist: artistRepo, - MockedMediaFile: mediaFileRepo, - } - - // Create a new provider with the updated datastore - provider = NewProvider(ds, mockAgents) + It("returns nil for an unknown artist", func() { + artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { + if opt.Max != 1 || opt.Filters == nil { + return false + } + _, ok := opt.Filters.(squirrel.Like) + return ok + })).Return(model.Artists{}, model.ErrNotFound).Once() songs, err := provider.TopSongs(ctx, "Unknown Artist", 5) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) Expect(songs).To(BeNil()) }) - It("returns empty list when no matching songs are found", func() { - // Set up artist data + It("returns nil when the agent returns an error", func() { artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} artistRepo.FindByName("Artist One", artist1) - // Configure the agent to return songs that don't match our repo - mockAgent.topSongs = []agents.Song{ - {Name: "Nonexistent Song", MBID: "unknown-mbid"}, - } + mockTopSongsAgent.SetError(errors.New("agent error")) - // Default to empty response for any queries - mediaFileRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{}, nil).Maybe() + song1 := model.MediaFile{ID: "song-1"} + song2 := model.MediaFile{ID: "song-2"} + mediaFileRepo.FindByMBID("mbid-1", song1) + mediaFileRepo.FindByMBID("mbid-2", song2) songs, err := provider.TopSongs(ctx, "Artist One", 5) Expect(err).ToNot(HaveOccurred()) - Expect(songs).To(HaveLen(0)) - }) - - It("returns nil when agent returns errors", func() { - // Set up artist data - artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} - artistRepo.SetData(model.Artists{artist1}) - artistRepo.FindByName("Artist One", artist1) - - // Set the error - testError := errors.New("some agent error") - mockAgent.err = testError - - songs, err := provider.TopSongs(ctx, "Artist One", 5) - - // Current behavior returns nil for both error and songs - Expect(err).To(BeNil()) Expect(songs).To(BeNil()) }) - It("respects count parameter", func() { - // Set up test data + It("returns nil when the agent returns ErrNotFound", func() { artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} - song1 := model.MediaFile{ - ID: "song-1", - Title: "Song One", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-1", - Missing: false, - } - - // Set up mocks - artistRepo.SetData(model.Artists{artist1}) artistRepo.FindByName("Artist One", artist1) + + mockTopSongsAgent.SetError(agents.ErrNotFound) + + song1 := model.MediaFile{ID: "song-1"} + song2 := model.MediaFile{ID: "song-2"} mediaFileRepo.FindByMBID("mbid-1", song1) + mediaFileRepo.FindByMBID("mbid-2", song2) - // Configure the mockAgent - mockAgent.topSongs = []agents.Song{ - {Name: "Song One", MBID: "mbid-1"}, - {Name: "Song Two", MBID: "mbid-2"}, - {Name: "Song Three", MBID: "mbid-3"}, - } + songs, err := provider.TopSongs(ctx, "Artist One", 5) + Expect(err).ToNot(HaveOccurred()) + Expect(songs).To(BeNil()) + }) - // Default to empty response for any queries - mediaFileRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{}, nil).Maybe() + It("returns fewer songs if count is less than available top songs", func() { + artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} + artistRepo.FindByName("Artist One", artist1) + + song1 := model.MediaFile{ID: "song-1"} + mediaFileRepo.FindByMBID("mbid-1", song1) songs, err := provider.TopSongs(ctx, "Artist One", 1) @@ -237,281 +150,99 @@ var _ = Describe("Provider", func() { Expect(songs).To(HaveLen(1)) Expect(songs[0].ID).To(Equal("song-1")) }) - }) - Describe("TopSongs with agent registration", func() { - BeforeEach(func() { - // Set our mock agent as the only agent - conf.Server.Agents = "mock" - - // Set up test data + It("returns fewer songs if fewer matching tracks are found", func() { artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} - - song1 := model.MediaFile{ - ID: "song-1", - Title: "Song One", - Artist: "Artist One", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-1", - Missing: false, - } - - song2 := model.MediaFile{ - ID: "song-2", - Title: "Song Two", - Artist: "Artist One", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-2", - Missing: false, - } - - // Set up basic data for the repos - artistRepo.SetData(model.Artists{artist1}) - mediaFileRepo.SetData(model.MediaFiles{song1, song2}) - - // Set up the specific mock responses needed for the TopSongs method artistRepo.FindByName("Artist One", artist1) - mediaFileRepo.FindByMBID("mbid-1", song1) - mediaFileRepo.FindByMBID("mbid-2", song2) - // Setup default behavior for empty searches - mediaFileRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{}, nil).Maybe() + song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzReleaseTrackID: "mbid-1"} - // Configure and register the agent - mockAgent.topSongs = []agents.Song{ - {Name: "Song One", MBID: "mbid-1"}, - {Name: "Song Two", MBID: "mbid-2"}, + matcherForMBID := func(expectedMBID string) func(opt model.QueryOptions) bool { + return func(opt model.QueryOptions) bool { + if opt.Filters == nil { + return false + } + andClause, ok := opt.Filters.(squirrel.And) + if !ok { + return false + } + for _, condition := range andClause { + if eqClause, ok := condition.(squirrel.Eq); ok { + if mbid, exists := eqClause["mbz_recording_id"]; exists { + return mbid == expectedMBID + } + } + } + return false + } } - // Register our mock agent - agents.Register("mock", func(model.DataStore) agents.Interface { return mockAgent }) + matcherForTitleArtistFallback := func(artistID, title string) func(opt model.QueryOptions) bool { + return func(opt model.QueryOptions) bool { + if opt.Filters == nil { + return false + } + andClause, ok := opt.Filters.(squirrel.And) + if !ok || len(andClause) < 3 { + return false + } + foundLike := false + for _, condition := range andClause { + if likeClause, ok := condition.(squirrel.Like); ok { + if _, exists := likeClause["order_title"]; exists { + foundLike = true + break + } + } + } + return foundLike + } + } - // Create the provider instance with registered agents - provider = NewProvider(ds, agents.GetAgents(ds)) - }) + mediaFileRepo.On("GetAll", mock.MatchedBy(matcherForMBID("mbid-1"))).Return(model.MediaFiles{song1}, nil).Once() + mediaFileRepo.On("GetAll", mock.MatchedBy(matcherForMBID("mbid-2"))).Return(model.MediaFiles{}, nil).Once() + mediaFileRepo.On("GetAll", mock.MatchedBy(matcherForTitleArtistFallback("artist-1", "Song Two"))).Return(model.MediaFiles{}, nil).Once() - It("returns matching songs from the registered agent", func() { - songs, err := provider.TopSongs(ctx, "Artist One", 5) + songs, err := provider.TopSongs(ctx, "Artist One", 2) Expect(err).ToNot(HaveOccurred()) - Expect(songs).To(HaveLen(2)) + Expect(songs).To(HaveLen(1)) Expect(songs[0].ID).To(Equal("song-1")) - Expect(songs[1].ID).To(Equal("song-2")) - }) - }) - - Describe("Error propagation from agents", func() { - BeforeEach(func() { - // Set up test data - artist1 := model.Artist{ID: "artist-1", Name: "Artist One"} - - // Set up basic data for the repos - artistRepo.SetData(model.Artists{artist1}) - artistRepo.FindByName("Artist One", artist1) - - // Setup default behavior for empty searches - mediaFileRepo.On("GetAll", mock.Anything).Return(model.MediaFiles{}, nil).Maybe() - - // Create a mock with a custom GetArtistTopSongs implementation that returns an error - testError := errors.New("direct agent error") - mockAgent.getArtistTopSongsFn = func(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { - return nil, testError - } - }) - - It("handles errors from the agent according to current behavior", func() { - songs, err := provider.TopSongs(ctx, "Artist One", 5) - - // Current behavior returns nil for both error and songs - Expect(err).To(BeNil()) - Expect(songs).To(BeNil()) }) }) }) -// MockAllAgents implements the Agents interface that Provider depends on -type mockAllAgents struct { - mock.Mock - topSongsRetriever agents.ArtistTopSongsRetriever -} - -func newMockAllAgents() *mockAllAgents { - return &mockAllAgents{} -} - -func (m *mockAllAgents) AgentName() string { - return "mockAllAgents" -} - -func (m *mockAllAgents) GetArtistMBID(ctx context.Context, id string, name string) (string, error) { - args := m.Called(ctx, id, name) - return args.String(0), args.Error(1) -} - -func (m *mockAllAgents) GetArtistURL(ctx context.Context, id, name, mbid string) (string, error) { - args := m.Called(ctx, id, name, mbid) - return args.String(0), args.Error(1) -} - -func (m *mockAllAgents) GetArtistBiography(ctx context.Context, id, name, mbid string) (string, error) { - args := m.Called(ctx, id, name, mbid) - return args.String(0), args.Error(1) -} - -func (m *mockAllAgents) GetSimilarArtists(ctx context.Context, id, name, mbid string, limit int) ([]agents.Artist, error) { - args := m.Called(ctx, id, name, mbid, limit) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).([]agents.Artist), args.Error(1) -} - -func (m *mockAllAgents) GetArtistImages(ctx context.Context, id, name, mbid string) ([]agents.ExternalImage, error) { - args := m.Called(ctx, id, name, mbid) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).([]agents.ExternalImage), args.Error(1) -} - -func (m *mockAllAgents) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { - // Delegate to the top songs retriever if it's set - if m.topSongsRetriever != nil { - return m.topSongsRetriever.GetArtistTopSongs(ctx, id, artistName, mbid, count) - } - - args := m.Called(ctx, id, artistName, mbid, count) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).([]agents.Song), args.Error(1) -} - -func (m *mockAllAgents) GetAlbumInfo(ctx context.Context, name, artist, mbid string) (*agents.AlbumInfo, error) { - args := m.Called(ctx, name, artist, mbid) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).(*agents.AlbumInfo), args.Error(1) -} - -// Make sure mockAllAgents implements the Agents interface -var _ Agents = (*mockAllAgents)(nil) - -// Mock agent implementation for testing +// Mock implementation for ArtistTopSongsRetriever +// This remains here as it's specific to TopSongs tests and simpler than mockSimilarArtistAgent type mockArtistTopSongsAgent struct { - agents.Interface - err error - topSongs []agents.Song - lastArtistID string - lastArtistName string - lastMBID string - lastCount int - getArtistTopSongsFn func(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) + mock.Mock + topSongs []agents.Song + err error } func (m *mockArtistTopSongsAgent) AgentName() string { - return "mock" + return "mockTopSongs" +} + +func (m *mockArtistTopSongsAgent) SetTopSongs(songs []agents.Song) { + m.topSongs = songs + m.err = nil +} + +func (m *mockArtistTopSongsAgent) SetError(err error) { + m.err = err + m.topSongs = nil } func (m *mockArtistTopSongsAgent) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { - m.lastCount = count - m.lastArtistID = id - m.lastArtistName = artistName - m.lastMBID = mbid - - log.Debug(ctx, "MockAgent.GetArtistTopSongs called", "id", id, "name", artistName, "mbid", mbid, "count", count) - - // Use the custom function if available - if m.getArtistTopSongsFn != nil { - return m.getArtistTopSongsFn(ctx, id, artistName, mbid, count) - } - if m.err != nil { - log.Debug(ctx, "MockAgent.GetArtistTopSongs returning error", "err", m.err) return nil, m.err } - log.Debug(ctx, "MockAgent.GetArtistTopSongs returning songs", "count", len(m.topSongs)) + if len(m.topSongs) > count { + return m.topSongs[:count], nil + } return m.topSongs, nil } -// Make sure the mock agent implements the necessary interface var _ agents.ArtistTopSongsRetriever = (*mockArtistTopSongsAgent)(nil) - -// Mocked ArtistRepo that uses testify's mock -type mockArtistRepo struct { - mock.Mock - model.ArtistRepository -} - -func newMockArtistRepo() *mockArtistRepo { - return &mockArtistRepo{} -} - -func (m *mockArtistRepo) SetData(artists model.Artists) { - // Store the data for Get queries - for _, a := range artists { - m.On("Get", a.ID).Return(&a, nil) - } -} - -func (m *mockArtistRepo) SetError(hasError bool) { - if hasError { - m.On("GetAll", mock.Anything).Return(nil, errors.New("error")) - } -} - -func (m *mockArtistRepo) FindByName(name string, artist model.Artist) { - // Set up a mock for finding an artist by name with LIKE filter, using Anything matcher for flexibility - m.On("GetAll", mock.Anything).Return(model.Artists{artist}, nil).Once() -} - -func (m *mockArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, error) { - args := m.Called(mock.Anything) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).(model.Artists), args.Error(1) -} - -// Mocked MediaFileRepo that uses testify's mock -type mockMediaFileRepo struct { - mock.Mock - model.MediaFileRepository -} - -func newMockMediaFileRepo() *mockMediaFileRepo { - return &mockMediaFileRepo{} -} - -func (m *mockMediaFileRepo) SetData(mediaFiles model.MediaFiles) { - // Store the data for Get queries - for _, mf := range mediaFiles { - m.On("Get", mf.ID).Return(&mf, nil) - } -} - -func (m *mockMediaFileRepo) SetError(hasError bool) { - if hasError { - m.On("GetAll", mock.Anything).Return(nil, errors.New("error")) - } -} - -func (m *mockMediaFileRepo) FindByMBID(mbid string, mediaFile model.MediaFile) { - // Set up a mock for finding a media file by MBID using Anything matcher for flexibility - m.On("GetAll", mock.Anything).Return(model.MediaFiles{mediaFile}, nil).Once() -} - -func (m *mockMediaFileRepo) FindByArtistAndTitle(artistID string, title string, mediaFile model.MediaFile) { - // Set up a mock for finding a media file by artist ID and title - m.On("GetAll", mock.Anything).Return(model.MediaFiles{mediaFile}, nil).Once() -} - -func (m *mockMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) { - args := m.Called(mock.Anything) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).(model.MediaFiles), args.Error(1) -}