From 8baecad379e9e21a669375a9e5696a51d05c595f Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Mon, 20 Apr 2026 21:20:38 -0400 Subject: [PATCH] fix(tests): use errors.Is() for error comparison and improve resource cleanup in analytics tests Replace direct error equality check with errors.Is() in media_scanner_hash_test. In analytics_test.go, improve defer patterns by capturing resp.Body as a named parameter to avoid stale references, and add require.NoError() checks on all json.NewDecoder().Decode() calls that were previously silently ignoring decode errors. --- cmd/server/tests/analytics_test.go | 122 +++++++++++++------ internal/services/media_scanner_hash_test.go | 3 +- 2 files changed, 89 insertions(+), 36 deletions(-) diff --git a/cmd/server/tests/analytics_test.go b/cmd/server/tests/analytics_test.go index fed457e..bbce565 100644 --- a/cmd/server/tests/analytics_test.go +++ b/cmd/server/tests/analytics_test.go @@ -4,6 +4,7 @@ import ( "bookhoard/internal/handlers" "bytes" "encoding/json" + "io" "net/http" "testing" "time" @@ -21,7 +22,9 @@ func TestAnalyticsReadingStats(t *testing.T) { req, _ := http.NewRequest("GET", setup.Server.URL+"/api/analytics/reading-stats", nil) resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) }) @@ -32,12 +35,15 @@ func TestAnalyticsReadingStats(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) var result handlers.ReadingStatsResponse - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) assert.GreaterOrEqual(t, result.TotalBooksRead, 0) assert.GreaterOrEqual(t, result.TotalPagesRead, 0) @@ -53,7 +59,9 @@ func TestAnalyticsReadingStats(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) }) @@ -64,7 +72,9 @@ func TestAnalyticsReadingStats(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) }) @@ -75,7 +85,9 @@ func TestAnalyticsReadingStats(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) }) @@ -86,12 +98,15 @@ func TestAnalyticsReadingStats(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) // Should return zero values for empty history assert.Equal(t, 0.0, result["total_books_read"]) @@ -108,7 +123,9 @@ func TestAnalyticsDeviceUsage(t *testing.T) { req, _ := http.NewRequest("GET", setup.Server.URL+"/api/analytics/device-usage", nil) resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) }) @@ -119,19 +136,19 @@ func TestAnalyticsDeviceUsage(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() assert.Equal(t, http.StatusOK, resp.StatusCode) var result handlers.DeviceUsageResponse - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) assert.NotNil(t, result.Devices) assert.Equal(t, 0, len(result.Devices)) }) t.Run("GetDeviceUsage_WithAuth_WithDevices", func(t *testing.T) { - // First create a device + // First, create a device deviceReq := map[string]interface{}{ "device_name": "Test Kobo", "device_type": "kobo", @@ -144,7 +161,9 @@ func TestAnalyticsDeviceUsage(t *testing.T) { resp, err := client.Do(deviceReqHTTP) require.NoError(t, err) - resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) // Now get device usage req, _ := http.NewRequest("GET", setup.Server.URL+"/api/analytics/device-usage", nil) @@ -152,12 +171,15 @@ func TestAnalyticsDeviceUsage(t *testing.T) { resp, err = client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) devices, ok := result["devices"].([]interface{}) assert.True(t, ok) @@ -171,12 +193,15 @@ func TestAnalyticsDeviceUsage(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) assert.Contains(t, result, "devices") @@ -203,7 +228,9 @@ func TestAnalyticsPopularBooks(t *testing.T) { req, _ := http.NewRequest("GET", setup.Server.URL+"/api/analytics/popular-books", nil) resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) }) @@ -214,12 +241,15 @@ func TestAnalyticsPopularBooks(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) var result handlers.PopularBooksResponse - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) assert.NotNil(t, result.Books) // Default limit is 10, but may be fewer if no reading history @@ -232,12 +262,15 @@ func TestAnalyticsPopularBooks(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) books := result["books"].([]interface{}) assert.True(t, len(books) <= 5) @@ -249,20 +282,23 @@ func TestAnalyticsPopularBooks(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) // Should default to 10 on invalid limit assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) books := result["books"].([]interface{}) assert.True(t, len(books) <= 10) }) t.Run("GetPopularBooks_ResponseStructure", func(t *testing.T) { - // First create a book and some reading history + // First, create a book and some reading history bookID := createTestMediaItemID(t, setup) // Create reading history for the book @@ -280,7 +316,9 @@ func TestAnalyticsPopularBooks(t *testing.T) { resp, err := client.Do(historyHTTP) require.NoError(t, err) - resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) // Now get popular books req, _ := http.NewRequest("GET", setup.Server.URL+"/api/analytics/popular-books", nil) @@ -288,12 +326,15 @@ func TestAnalyticsPopularBooks(t *testing.T) { resp, err = client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) books := result["books"].([]interface{}) @@ -315,12 +356,15 @@ func TestAnalyticsPopularBooks(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) books := result["books"].([]interface{}) // Should return empty array if no reading history @@ -342,13 +386,16 @@ func TestAnalyticsEdgeCases(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) // Should succeed but return empty stats assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) assert.Equal(t, 0.0, result["total_books_read"]) }) @@ -359,13 +406,16 @@ func TestAnalyticsEdgeCases(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) // Should handle limit=0 gracefully assert.Equal(t, http.StatusOK, resp.StatusCode) var result map[string]interface{} - json.NewDecoder(resp.Body).Decode(&result) + err = json.NewDecoder(resp.Body).Decode(&result) + require.NoError(t, err) books := result["books"].([]interface{}) assert.Equal(t, 0, len(books)) @@ -377,7 +427,9 @@ func TestAnalyticsEdgeCases(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - defer resp.Body.Close() + defer func(Body io.ReadCloser) { + _ = Body.Close() + }(resp.Body) // Should handle large limit assert.Equal(t, http.StatusOK, resp.StatusCode) diff --git a/internal/services/media_scanner_hash_test.go b/internal/services/media_scanner_hash_test.go index 16f0176..d50ef64 100644 --- a/internal/services/media_scanner_hash_test.go +++ b/internal/services/media_scanner_hash_test.go @@ -4,6 +4,7 @@ import ( "bufio" "crypto/sha256" "encoding/hex" + "errors" "fmt" "io" "os" @@ -73,7 +74,7 @@ func TestCalculateFileSHA256LargeFile(t *testing.T) { buf := make([]byte, 4096) for { n, err := file.Read(buf) - if err != nil && err != bufio.ErrBufferFull { + if err != nil && !errors.Is(err, bufio.ErrBufferFull) { if err == io.EOF { break }