elie/CBZOptimizer
1
0
Fork 0
mirror of https://github.com/Belphemur/CBZOptimizer.git synced 2026-07-21 19:05:39 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix: address copilot and coderabbit review feedback

Browse Source 
- Use named returns for IsAlreadyConverted and WriteChapterToCBZ so
  errs.Capture defers propagate close errors to the caller.
- Add context.Context to IsAlreadyConverted and ExtractChapter for
  cancellation/timeout support; thread it from Optimize.
- Filter non-image files (__MACOSX, Thumbs.db, .DS_Store, etc.) and
  only extract supported image extensions in ExtractChapter.
- Validate TempDir is non-empty in ConvertChapter before creating output dir.
- Separate fatal conversion errors from PageIgnoredError in the WebP
  converter result aggregation so real failures aren't masked.
- Fix error message "failed to load chapter" → "failed to extract chapter".
- Use %w instead of %v in fmt.Errorf for proper error wrapping.
- Fix LoadChapter doc comment to match its actual behavior.
- Extract shared parseConvertedComment/parseConvertedCommentTime helpers
  to avoid duplicated zip-comment parsing logic.
- Convert tests to use testify assertions (assert/require) per guidelines.

Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot]
2026-07-04 22:39:19 +00:00
committed by GitHub
co-authored by Belphemur
parent 5ae05e5f0c
commit 5e5cac4883
6 changed files with 216 additions and 256 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/cbz/cbz_loader_test.go
+78 -199
View File
@@ -2,14 +2,16 @@ package cbz
import (
"archive/zip"
"context"
"fmt"
"os"
"path/filepath"
"strings"
"testing"
"time"
"github.com/belphemur/CBZOptimizer/v2/internal/manga"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestLoadChapter(t *testing.T) {
@@ -48,29 +50,18 @@ func TestLoadChapter(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
chapter, err := LoadChapter(tc.filePath)
if err != nil {
t.Fatalf("Failed to load chapter: %v", err)
}
require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }()
actualPages := len(chapter.Pages)
if actualPages != tc.expectedPages {
t.Errorf("Expected %d pages, but got %d", tc.expectedPages, actualPages)
}
assert.Equal(t, tc.expectedPages, len(chapter.Pages))
if !strings.Contains(chapter.ComicInfoXml, tc.expectedSeries) {
t.Errorf("ComicInfoXml does not contain the expected series: %s", tc.expectedSeries)
}
assert.Contains(t, chapter.ComicInfoXml, tc.expectedSeries)
if chapter.IsConverted != tc.expectedConversion {
t.Errorf("Expected chapter to be converted: %t, but got %t", tc.expectedConversion, chapter.IsConverted)
}
assert.Equal(t, tc.expectedConversion, chapter.IsConverted)
// Verify pages are on disk
for _, page := range chapter.Pages {
if page.FilePath == "" {
t.Errorf("Page %d has no file path", page.Index)
}
assert.NotEmpty(t, page.FilePath, "Page %d has no file path", page.Index)
}
})
}
@@ -101,40 +92,28 @@ func TestIsAlreadyConverted(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
result, err := IsAlreadyConverted(tc.filePath)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
if result != tc.expected {
t.Errorf("Expected %t, got %t", tc.expected, result)
}
result, err := IsAlreadyConverted(context.Background(), tc.filePath)
require.NoError(t, err)
assert.Equal(t, tc.expected, result)
})
}
}
func TestIsAlreadyConverted_NonexistentFile(t *testing.T) {
_, err := IsAlreadyConverted("/nonexistent/file.cbz")
if err == nil {
t.Error("Expected error for nonexistent file")
}
_, err := IsAlreadyConverted(context.Background(), "/nonexistent/file.cbz")
require.Error(t, err)
}
func TestIsAlreadyConverted_InvalidExtension(t *testing.T) {
// Create a temp file with unsupported extension
tmpFile, err := os.CreateTemp("", "test-*.txt")
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
defer func() { _ = os.Remove(tmpFile.Name()) }()
_ = tmpFile.Close()
result, err := IsAlreadyConverted(tmpFile.Name())
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
if result {
t.Error("Expected false for unsupported extension")
}
result, err := IsAlreadyConverted(context.Background(), tmpFile.Name())
require.NoError(t, err)
assert.False(t, result, "Expected false for unsupported extension")
}
func TestIsAlreadyConverted_CBZWithConvertedComment(t *testing.T) {
@@ -143,27 +122,19 @@ func TestIsAlreadyConverted_CBZWithConvertedComment(t *testing.T) {
cbzPath := filepath.Join(tmpDir, "test.cbz")
f, err := os.Create(cbzPath)
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
w := zip.NewWriter(f)
_ = w.SetComment(time.Now().Format(time.RFC3339))
// Add a dummy file
fw, err := w.Create("page.webp")
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
_, _ = fw.Write([]byte("dummy"))
_ = w.Close()
_ = f.Close()
result, err := IsAlreadyConverted(cbzPath)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
if !result {
t.Error("Expected CBZ with date comment to be detected as converted")
}
result, err := IsAlreadyConverted(context.Background(), cbzPath)
require.NoError(t, err)
assert.True(t, result, "Expected CBZ with date comment to be detected as converted")
}
func TestIsAlreadyConverted_CBZWithNonDateComment(t *testing.T) {
@@ -171,120 +142,82 @@ func TestIsAlreadyConverted_CBZWithNonDateComment(t *testing.T) {
cbzPath := filepath.Join(tmpDir, "test.cbz")
f, err := os.Create(cbzPath)
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
w := zip.NewWriter(f)
_ = w.SetComment("this is not a date")
fw, err := w.Create("page.jpg")
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
_, _ = fw.Write([]byte("dummy"))
_ = w.Close()
_ = f.Close()
result, err := IsAlreadyConverted(cbzPath)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
if result {
t.Error("Expected CBZ with non-date comment to NOT be detected as converted")
}
result, err := IsAlreadyConverted(context.Background(), cbzPath)
require.NoError(t, err)
assert.False(t, result, "Expected CBZ with non-date comment to NOT be detected as converted")
}
func TestExtractChapter_NonexistentFile(t *testing.T) {
_, err := ExtractChapter("/nonexistent/file.cbz")
if err == nil {
t.Error("Expected error for nonexistent file")
}
_, err := ExtractChapter(context.Background(), "/nonexistent/file.cbz")
require.Error(t, err)
}
func TestExtractChapter_PageExtensions(t *testing.T) {
chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz")
if err != nil {
t.Fatalf("Failed to extract chapter: %v", err)
}
chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz")
require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }()
// All pages should have valid image extensions
validExts := map[string]bool{".jpg": true, ".jpeg": true, ".png": true, ".webp": true, ".gif": true}
for _, page := range chapter.Pages {
if !validExts[page.Extension] {
t.Errorf("Page %d has unexpected extension: %s", page.Index, page.Extension)
}
assert.True(t, validExts[page.Extension], "Page %d has unexpected extension: %s", page.Index, page.Extension)
}
}
func TestExtractChapter_PagesHaveSequentialIndices(t *testing.T) {
chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz")
if err != nil {
t.Fatalf("Failed to extract chapter: %v", err)
}
chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz")
require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }()
for i, page := range chapter.Pages {
if page.Index != uint16(i) {
t.Errorf("Expected page index %d, got %d", i, page.Index)
}
assert.Equal(t, uint16(i), page.Index)
}
}
func TestExtractChapter_Cleanup(t *testing.T) {
chapter, err := ExtractChapter("../../testdata/Chapter 128.cbz")
if err != nil {
t.Fatalf("Failed to extract chapter: %v", err)
}
chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz")
require.NoError(t, err)
tempDir := chapter.TempDir
// Verify temp dir exists
if _, err := os.Stat(tempDir); os.IsNotExist(err) {
t.Fatal("TempDir does not exist after extraction")
}
assert.DirExists(t, tempDir)
// Cleanup should remove temp dir
err = chapter.Cleanup()
if err != nil {
t.Fatalf("Cleanup failed: %v", err)
}
require.NoError(t, err)
if _, err := os.Stat(tempDir); !os.IsNotExist(err) {
t.Error("TempDir should not exist after cleanup")
}
assert.NoDirExists(t, tempDir)
}
func TestExtractChapter_CBR(t *testing.T) {
chapter, err := ExtractChapter("../../testdata/Chapter 1.cbr")
if err != nil {
t.Fatalf("Failed to extract CBR chapter: %v", err)
}
chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 1.cbr")
require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }()
if len(chapter.Pages) != 16 {
t.Errorf("Expected 16 pages, got %d", len(chapter.Pages))
}
assert.Len(t, chapter.Pages, 16)
// All page files should exist on disk
for _, page := range chapter.Pages {
if _, err := os.Stat(page.FilePath); os.IsNotExist(err) {
t.Errorf("Page file does not exist on disk: %s", page.FilePath)
assert.FileExists(t, page.FilePath)
}
}
}
func TestExtractChapter_ConvertedStatus(t *testing.T) {
chapter, err := ExtractChapter("../../testdata/Chapter 10_converted.cbz")
if err != nil {
t.Fatalf("Failed to extract chapter: %v", err)
}
chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 10_converted.cbz")
require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }()
if !chapter.IsConverted {
t.Error("Expected converted chapter to have IsConverted = true")
}
if chapter.ConvertedTime.IsZero() {
t.Error("Expected non-zero ConvertedTime for converted chapter")
}
assert.True(t, chapter.IsConverted, "Expected converted chapter to have IsConverted = true")
assert.False(t, chapter.ConvertedTime.IsZero(), "Expected non-zero ConvertedTime for converted chapter")
}
func TestWriteChapterToCBZ_EmptyChapter(t *testing.T) {
@@ -298,15 +231,11 @@ func TestWriteChapterToCBZ_EmptyChapter(t *testing.T) {
}
err := WriteChapterToCBZ(chapter, outputPath)
if err != nil {
t.Fatalf("Should not error on empty chapter: %v", err)
}
require.NoError(t, err)
// Verify the file is a valid zip
r, err := zip.OpenReader(outputPath)
if err != nil {
t.Fatalf("Failed to open output CBZ: %v", err)
}
require.NoError(t, err)
_ = r.Close()
}
@@ -331,15 +260,11 @@ func TestWriteChapterToCBZ_WithComicInfo(t *testing.T) {
chapter.SetConverted()
err := WriteChapterToCBZ(chapter, outputPath)
if err != nil {
t.Fatalf("Failed to write chapter: %v", err)
}
require.NoError(t, err)
// Verify ComicInfo.xml is present
r, err := zip.OpenReader(outputPath)
if err != nil {
t.Fatalf("Failed to open output CBZ: %v", err)
}
require.NoError(t, err)
defer func() { _ = r.Close() }()
foundComicInfo := false
@@ -348,14 +273,10 @@ func TestWriteChapterToCBZ_WithComicInfo(t *testing.T) {
foundComicInfo = true
}
}
if !foundComicInfo {
t.Error("ComicInfo.xml not found in output CBZ")
}
assert.True(t, foundComicInfo, "ComicInfo.xml not found in output CBZ")
// Verify zip comment has converted timestamp
if r.Comment == "" {
t.Error("Expected zip comment with conversion timestamp")
}
assert.NotEmpty(t, r.Comment, "Expected zip comment with conversion timestamp")
}
func TestWriteChapterToCBZ_InvalidPagePath(t *testing.T) {
@@ -371,9 +292,7 @@ func TestWriteChapterToCBZ_InvalidPagePath(t *testing.T) {
}
err := WriteChapterToCBZ(chapter, outputPath)
if err == nil {
t.Error("Expected error when page file does not exist")
}
require.Error(t, err)
}
func TestIsAlreadyConverted_CBZWithConvertedTxt(t *testing.T) {
@@ -382,32 +301,22 @@ func TestIsAlreadyConverted_CBZWithConvertedTxt(t *testing.T) {
cbzPath := filepath.Join(tmpDir, "test.cbz")
f, err := os.Create(cbzPath)
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
w := zip.NewWriter(f)
// Add converted.txt with a date
fw, err := w.Create("converted.txt")
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
_, _ = fw.Write([]byte(time.Now().Format(time.RFC3339)))
// Add a dummy page
fw, err = w.Create("page.webp")
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
_, _ = fw.Write([]byte("dummy"))
_ = w.Close()
_ = f.Close()
result, err := IsAlreadyConverted(cbzPath)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
if !result {
t.Error("Expected CBZ with converted.txt to be detected as converted")
}
result, err := IsAlreadyConverted(context.Background(), cbzPath)
require.NoError(t, err)
assert.True(t, result, "Expected CBZ with converted.txt to be detected as converted")
}
func TestIsAlreadyConverted_CBZWithInvalidConvertedTxt(t *testing.T) {
@@ -416,25 +325,17 @@ func TestIsAlreadyConverted_CBZWithInvalidConvertedTxt(t *testing.T) {
cbzPath := filepath.Join(tmpDir, "test.cbz")
f, err := os.Create(cbzPath)
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
w := zip.NewWriter(f)
fw, err := w.Create("converted.txt")
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
_, _ = fw.Write([]byte("not a valid date"))
_ = w.Close()
_ = f.Close()
result, err := IsAlreadyConverted(cbzPath)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
if result {
t.Error("Expected CBZ with invalid date in converted.txt to NOT be detected as converted")
}
result, err := IsAlreadyConverted(context.Background(), cbzPath)
require.NoError(t, err)
assert.False(t, result, "Expected CBZ with invalid date in converted.txt to NOT be detected as converted")
}
func TestExtractChapter_WithConvertedTxt(t *testing.T) {
@@ -443,36 +344,24 @@ func TestExtractChapter_WithConvertedTxt(t *testing.T) {
cbzPath := filepath.Join(tmpDir, "test.cbz")
f, err := os.Create(cbzPath)
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
w := zip.NewWriter(f)
fw, err := w.Create("converted.txt")
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
convertedTime := time.Date(2024, 6, 15, 12, 0, 0, 0, time.UTC)
_, _ = fw.Write([]byte(convertedTime.Format(time.RFC3339)))
fw, err = w.Create("page001.jpg")
if err != nil {
t.Fatal(err)
}
require.NoError(t, err)
_, _ = fw.Write([]byte("fake image"))
_ = w.Close()
_ = f.Close()
chapter, err := ExtractChapter(cbzPath)
if err != nil {
t.Fatalf("Failed to extract: %v", err)
}
chapter, err := ExtractChapter(context.Background(), cbzPath)
require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }()
if !chapter.IsConverted {
t.Error("Expected chapter with converted.txt to be marked as converted")
}
if chapter.ConvertedTime.IsZero() {
t.Error("Expected non-zero ConvertedTime")
}
assert.True(t, chapter.IsConverted, "Expected chapter with converted.txt to be marked as converted")
assert.False(t, chapter.ConvertedTime.IsZero(), "Expected non-zero ConvertedTime")
}
func TestWriteChapterToCBZ_MultiplePages(t *testing.T) {
@@ -501,27 +390,19 @@ func TestWriteChapterToCBZ_MultiplePages(t *testing.T) {
chapter.SetConverted()
err := WriteChapterToCBZ(chapter, outputPath)
if err != nil {
t.Fatalf("Failed to write: %v", err)
}
require.NoError(t, err)
// Verify archive contents
r, err := zip.OpenReader(outputPath)
if err != nil {
t.Fatalf("Failed to open: %v", err)
}
require.NoError(t, err)
defer func() { _ = r.Close() }()
// Should have 5 pages = 5 files (conversion status stored in zip comment, not as file)
expectedFiles := 5
if len(r.File) != expectedFiles {
t.Errorf("Expected %d files in archive, got %d", expectedFiles, len(r.File))
}
assert.Len(t, r.File, expectedFiles)
// Verify zip comment is set
if r.Comment == "" {
t.Error("Expected zip comment for converted chapter")
}
assert.NotEmpty(t, r.Comment, "Expected zip comment for converted chapter")
}
func TestWriteChapterToCBZ_InvalidOutputPath(t *testing.T) {
@@ -541,7 +422,5 @@ func TestWriteChapterToCBZ_InvalidOutputPath(t *testing.T) {
}
err := WriteChapterToCBZ(chapter, "/nonexistent/dir/output.cbz")
if err == nil {
t.Error("Expected error for invalid output path")
}
require.Error(t, err)
}