From 8a4936dbc68fa9e619be2c0f14b3be3829f212a5 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 16 Jun 2025 12:58:20 -0400 Subject: [PATCH] test: enhance GetCoverArt tests with context cancellation handling Signed-off-by: Deluan --- server/subsonic/media_retrieval_test.go | 113 ++++++++++++++---------- 1 file changed, 67 insertions(+), 46 deletions(-) diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index c2c49552..d80f1f9c 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -14,7 +14,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/core/artwork" - "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/tests" @@ -25,7 +24,7 @@ import ( var _ = Describe("MediaRetrievalController", func() { var router *Router var ds model.DataStore - mockRepo := &mockedMediaFile{} + mockRepo := &mockedMediaFile{MockMediaFileRepo: tests.MockMediaFileRepo{}} var artwork *fakeArtwork var w *httptest.ResponseRecorder @@ -33,7 +32,7 @@ var _ = Describe("MediaRetrievalController", func() { ds = &tests.MockDataStore{ MockedMediaFile: mockRepo, } - artwork = &fakeArtwork{} + artwork = &fakeArtwork{data: "image data"} router = New(ds, artwork, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) w = httptest.NewRecorder() DeferCleanup(configtest.SetupConfig()) @@ -42,27 +41,19 @@ var _ = Describe("MediaRetrievalController", func() { Describe("GetCoverArt", func() { It("should return data for that id", func() { - artwork.data = "image data" - r := newGetRequest("id=34", "size=128") + r := newGetRequest("id=34", "size=128", "square=true") _, err := router.GetCoverArt(w, r) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) Expect(artwork.recvId).To(Equal("34")) Expect(artwork.recvSize).To(Equal(128)) - Expect(w.Body.String()).To(Equal(artwork.data)) - }) - - It("should return placeholder if id parameter is missing (mimicking Subsonic)", func() { - r := newGetRequest() - _, err := router.GetCoverArt(w, r) - - Expect(err).To(BeNil()) + Expect(artwork.recvSquare).To(BeTrue()) Expect(w.Body.String()).To(Equal(artwork.data)) }) It("should fail when the file is not found", func() { artwork.err = model.ErrNotFound - r := newGetRequest("id=34", "size=128") + r := newGetRequest("id=34", "size=128", "square=true") _, err := router.GetCoverArt(w, r) Expect(err).To(MatchError("Artwork not found")) @@ -75,6 +66,45 @@ var _ = Describe("MediaRetrievalController", func() { Expect(err).To(MatchError("weird error")) }) + + When("client disconnects (context is cancelled)", func() { + It("should not call the service if cancelled before the call", func() { + // Create a request + ctx, cancel := context.WithCancel(context.Background()) + r := newGetRequest("id=34", "size=128", "square=true") + r = r.WithContext(ctx) + cancel() // Cancel the context before the call + + // Call the GetCoverArt method + _, err := router.GetCoverArt(w, r) + + // Expect no error and no call to the artwork service + Expect(err).ToNot(HaveOccurred()) + Expect(artwork.recvId).To(Equal("")) + Expect(artwork.recvSize).To(Equal(0)) + Expect(artwork.recvSquare).To(BeFalse()) + Expect(w.Body.String()).To(BeEmpty()) + }) + + It("should not return data if cancelled during the call", func() { + // Create a request with a context that will be cancelled + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() // Ensure the context is cancelled after the test (best practices) + r := newGetRequest("id=34", "size=128", "square=true") + r = r.WithContext(ctx) + artwork.ctxCancelFunc = cancel // Set the cancel function to simulate cancellation in the service + + // Call the GetCoverArt method + _, err := router.GetCoverArt(w, r) + + // Expect no error and the service to have been called + Expect(err).ToNot(HaveOccurred()) + Expect(artwork.recvId).To(Equal("34")) + Expect(artwork.recvSize).To(Equal(128)) + Expect(artwork.recvSquare).To(BeTrue()) + Expect(w.Body.String()).To(BeEmpty()) + }) + }) }) Describe("GetLyrics", func() { @@ -111,10 +141,7 @@ var _ = Describe("MediaRetrievalController", func() { }, }) response, err := router.GetLyrics(r) - if err != nil { - log.Error("You're missing something.", err) - } - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) Expect(response.Lyrics.Artist).To(Equal("Rick Astley")) Expect(response.Lyrics.Title).To(Equal("Never Gonna Give You Up")) Expect(response.Lyrics.Value).To(Equal("We're no strangers to love\nYou know the rules and so do I\n")) @@ -123,10 +150,7 @@ var _ = Describe("MediaRetrievalController", func() { r := newGetRequest("artist=Dheeraj", "title=Rinkiya+Ke+Papa") mockRepo.SetData(model.MediaFiles{}) response, err := router.GetLyrics(r) - if err != nil { - log.Error("You're missing something.", err) - } - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) Expect(response.Lyrics.Artist).To(Equal("")) Expect(response.Lyrics.Title).To(Equal("")) Expect(response.Lyrics.Value).To(Equal("")) @@ -148,14 +172,14 @@ var _ = Describe("MediaRetrievalController", func() { }, }) response, err := router.GetLyrics(r) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) Expect(response.Lyrics.Artist).To(Equal("Rick Astley")) Expect(response.Lyrics.Title).To(Equal("Never Gonna Give You Up")) Expect(response.Lyrics.Value).To(Equal("We're no strangers to love\nYou know the rules and so do I\n")) }) }) - Describe("getLyricsBySongId", func() { + Describe("GetLyricsBySongId", func() { const syncedLyrics = "[00:18.80]We're no strangers to love\n[00:22.801]You know the rules and so do I" const unsyncedLyrics = "We're no strangers to love\nYou know the rules and so do I" const metadata = "[ar:Rick Astley]\n[ti:That one song]\n[offset:-100]" @@ -295,10 +319,12 @@ var _ = Describe("MediaRetrievalController", func() { type fakeArtwork struct { artwork.Artwork - data string - err error - recvId string - recvSize int + data string + err error + ctxCancelFunc func() + recvId string + recvSize int + recvSquare bool } func (c *fakeArtwork) GetOrPlaceholder(_ context.Context, id string, size int, square bool) (io.ReadCloser, time.Time, error) { @@ -307,25 +333,29 @@ func (c *fakeArtwork) GetOrPlaceholder(_ context.Context, id string, size int, s } c.recvId = id c.recvSize = size + c.recvSquare = square + if c.ctxCancelFunc != nil { + c.ctxCancelFunc() // Simulate context cancellation + return nil, time.Time{}, context.Canceled + } return io.NopCloser(bytes.NewReader([]byte(c.data))), time.Time{}, nil } type mockedMediaFile struct { - model.MediaFileRepository - data model.MediaFiles -} - -func (m *mockedMediaFile) SetData(mfs model.MediaFiles) { - m.data = mfs + tests.MockMediaFileRepo } func (m *mockedMediaFile) GetAll(opts ...model.QueryOptions) (model.MediaFiles, error) { + data, err := m.MockMediaFileRepo.GetAll(opts...) + if err != nil { + return nil, err + } if len(opts) == 0 || opts[0].Sort != "lyrics, updated_at" { - return m.data, nil + return data, nil } // Hardcoded support for lyrics sorting - result := slices.Clone(m.data) + result := slices.Clone(data) // Sort by presence of lyrics, then by updated_at. Respect the order specified in opts. slices.SortFunc(result, func(a, b model.MediaFile) int { diff := cmp.Or( @@ -339,12 +369,3 @@ func (m *mockedMediaFile) GetAll(opts ...model.QueryOptions) (model.MediaFiles, }) return result, nil } - -func (m *mockedMediaFile) Get(id string) (*model.MediaFile, error) { - for _, mf := range m.data { - if mf.ID == id { - return &mf, nil - } - } - return nil, model.ErrNotFound -}