chore: increase/stabilize go test coverage (#3411)

* chore: increase/stabilize coverage for the local storage driver

Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>

* chore: add/stabilize coverage for soring ImageSummary objects

Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>

* chore: stabilize coverage in sync tests

Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>

---------

Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>

go.mod

@@ -22,7 +22,6 @@ require (
 	github.com/chartmuseum/auth v0.5.0
 	github.com/cloudevents/sdk-go/protocol/nats/v2 v2.16.2
 	github.com/cloudevents/sdk-go/v2 v2.16.2
-	github.com/containers/image/v5 v5.36.2
 	github.com/dchest/siphash v1.2.3
 	github.com/didip/tollbooth/v7 v7.0.2
 	github.com/distribution/distribution/v3 v3.0.0
@@ -213,7 +212,6 @@ require (
 	github.com/containerd/stargz-snapshotter/estargz v0.16.3 // indirect
 	github.com/containerd/ttrpc v1.2.7 // indirect
 	github.com/containerd/typeurl/v2 v2.2.3 // indirect
-	github.com/containers/storage v1.59.1 // indirect
 	github.com/coreos/go-oidc/v3 v3.14.1 // indirect
 	github.com/cpuguy83/go-md2man/v2 v2.0.7 // indirect
 	github.com/cyberphone/json-canonicalization v0.0.0-20241213102144-19d51d7fe467 // indirect

go.sum

@@ -1022,10 +1022,6 @@ github.com/containerd/ttrpc v1.2.7 h1:qIrroQvuOL9HQ1X6KHe2ohc7p+HP/0VE6XPU7elJRq
 github.com/containerd/ttrpc v1.2.7/go.mod h1:YCXHsb32f+Sq5/72xHubdiJRQY9inL4a4ZQrAbN1q9o=
 github.com/containerd/typeurl/v2 v2.2.3 h1:yNA/94zxWdvYACdYO8zofhrTVuQY73fFU1y++dYSw40=
 github.com/containerd/typeurl/v2 v2.2.3/go.mod h1:95ljDnPfD3bAbDJRugOiShd/DlAAsxGtUBhJxIn7SCk=
-github.com/containers/image/v5 v5.36.2 h1:GcxYQyAHRF/pLqR4p4RpvKllnNL8mOBn0eZnqJbfTwk=
-github.com/containers/image/v5 v5.36.2/go.mod h1:b4GMKH2z/5t6/09utbse2ZiLK/c72GuGLFdp7K69eA4=
-github.com/containers/storage v1.59.1 h1:11Zu68MXsEQGBBd+GadPrHPpWeqjKS8hJDGiAHgIqDs=
-github.com/containers/storage v1.59.1/go.mod h1:KoAYHnAjP3/cTsRS+mmWZGkufSY2GACiKQ4V3ZLQnR0=
 github.com/coreos/go-oidc/v3 v3.14.1 h1:9ePWwfdwC4QKRlCXsJGou56adA/owXczOzwKdOumLqk=
 github.com/coreos/go-oidc/v3 v3.14.1/go.mod h1:HaZ3szPaZ0e4r6ebqvsLWlk2Tn+aejfmrfah6hnSYEU=
 github.com/coreos/go-systemd/v22 v22.5.0 h1:RrqgGjYQKalulkV8NGVIfkXQf6YYmOyiJKk8iXXhfZs=

pkg/extensions/search/resolver_test.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"sort"
 	"testing"
 	"time"
 
@@ -2667,6 +2668,135 @@ func TestExpandedRepoInfoErrors(t *testing.T) {
 	})
 }
 
+func TestImageSummarySort(t *testing.T) {
+	Convey("Test sorting ImageSummary", t, func() {
+		Convey("Swap elements at valid indices", func() {
+			// Create test data with different timestamps
+			time1 := time.Date(2023, 1, 1, 12, 0, 0, 0, time.UTC)
+			time2 := time.Date(2023, 1, 2, 12, 0, 0, 0, time.UTC)
+			time3 := time.Date(2023, 1, 3, 12, 0, 0, 0, time.UTC)
+
+			image1 := &gql_generated.ImageSummary{
+				Tag:         ref("tag1"),
+				LastUpdated: &time1,
+			}
+			image2 := &gql_generated.ImageSummary{
+				Tag:         ref("tag2"),
+				LastUpdated: &time2,
+			}
+			image3 := &gql_generated.ImageSummary{
+				Tag:         ref("tag3"),
+				LastUpdated: &time3,
+			}
+
+			// Create timeSlice with test data
+			timeSliceData := timeSlice{image1, image2, image3}
+
+			// Verify initial order
+			So(*timeSliceData[0].Tag, ShouldEqual, "tag1")
+			So(*timeSliceData[1].Tag, ShouldEqual, "tag2")
+			So(*timeSliceData[2].Tag, ShouldEqual, "tag3")
+
+			// Swap elements at indices 0 and 2
+			timeSliceData.Swap(0, 2)
+
+			// Verify elements are swapped
+			So(*timeSliceData[0].Tag, ShouldEqual, "tag3")
+			So(*timeSliceData[1].Tag, ShouldEqual, "tag2")
+			So(*timeSliceData[2].Tag, ShouldEqual, "tag1")
+
+			// Swap elements at indices 1 and 2
+			timeSliceData.Swap(1, 2)
+
+			// Verify elements are swapped again
+			So(*timeSliceData[0].Tag, ShouldEqual, "tag3")
+			So(*timeSliceData[1].Tag, ShouldEqual, "tag1")
+			So(*timeSliceData[2].Tag, ShouldEqual, "tag2")
+		})
+
+		Convey("Swap elements at same index (no-op)", func() {
+			time1 := time.Date(2023, 1, 1, 12, 0, 0, 0, time.UTC)
+			time2 := time.Date(2023, 1, 2, 12, 0, 0, 0, time.UTC)
+
+			image1 := &gql_generated.ImageSummary{
+				Tag:         ref("tag1"),
+				LastUpdated: &time1,
+			}
+			image2 := &gql_generated.ImageSummary{
+				Tag:         ref("tag2"),
+				LastUpdated: &time2,
+			}
+
+			timeSliceData := timeSlice{image1, image2}
+
+			// Swap element with itself
+			timeSliceData.Swap(0, 0)
+
+			// Verify no change
+			So(*timeSliceData[0].Tag, ShouldEqual, "tag1")
+			So(*timeSliceData[1].Tag, ShouldEqual, "tag2")
+		})
+
+		Convey("Swap with single element slice", func() {
+			time1 := time.Date(2023, 1, 1, 12, 0, 0, 0, time.UTC)
+
+			image1 := &gql_generated.ImageSummary{
+				Tag:         ref("tag1"),
+				LastUpdated: &time1,
+			}
+
+			timeSliceData := timeSlice{image1}
+
+			// Swap single element with itself
+			timeSliceData.Swap(0, 0)
+
+			// Verify no change
+			So(*timeSliceData[0].Tag, ShouldEqual, "tag1")
+		})
+
+		Convey("Swap with empty slice", func() {
+			timeSliceData := timeSlice{}
+
+			// Verify slice is empty
+			// Note: Calling Swap on empty slice would panic, which is expected behavior
+			// The Swap method doesn't check bounds, so it's the caller's responsibility
+			// to ensure valid indices. This is consistent with Go's standard library behavior.
+			So(len(timeSliceData), ShouldEqual, 0)
+		})
+
+		Convey("Integration test with sort.Sort", func() {
+			// Create test data with unsorted timestamps
+			time1 := time.Date(2023, 1, 1, 12, 0, 0, 0, time.UTC)
+			time2 := time.Date(2023, 1, 3, 12, 0, 0, 0, time.UTC) // Latest
+			time3 := time.Date(2023, 1, 2, 12, 0, 0, 0, time.UTC)
+
+			image1 := &gql_generated.ImageSummary{
+				Tag:         ref("tag1"),
+				LastUpdated: &time1,
+			}
+			image2 := &gql_generated.ImageSummary{
+				Tag:         ref("tag2"),
+				LastUpdated: &time2,
+			}
+			image3 := &gql_generated.ImageSummary{
+				Tag:         ref("tag3"),
+				LastUpdated: &time3,
+			}
+
+			// Create timeSlice with unsorted data
+			timeSliceData := timeSlice{image1, image2, image3}
+
+			// Sort using sort.Sort (which will call Swap internally)
+			sort.Sort(timeSliceData)
+
+			// Verify sorted order (newest first due to Less implementation)
+			So(*timeSliceData[0].Tag, ShouldEqual, "tag2") // Latest time
+			So(*timeSliceData[1].Tag, ShouldEqual, "tag3") // Middle time
+			So(*timeSliceData[2].Tag, ShouldEqual, "tag1") // Oldest time
+		})
+	})
+}
+
 func TestUtils(t *testing.T) {
 	Convey("utils", t, func() {
 		Convey("", func() {

pkg/extensions/sync/sync_internal_test.go

@@ -7,11 +7,13 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"os"
 	"testing"
 
 	godigest "github.com/opencontainers/go-digest"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/regclient/regclient/types/ref"
 	. "github.com/smartystreets/goconvey/convey"
 
 	zerr "zotregistry.dev/zot/errors"
@@ -41,6 +43,119 @@ func TestService(t *testing.T) {
 		err = service.SyncRepo(context.Background(), "repo")
 		So(err, ShouldNotBeNil)
 	})
+
+	Convey("test context cancellation in SyncRepo without mock", t, func() {
+		conf := syncconf.RegistryConfig{
+			URLs: []string{"http://localhost"},
+		}
+
+		service, err := New(conf, "", nil, os.TempDir(), storage.StoreController{}, mocks.MetaDBMock{}, log.Logger{})
+		So(err, ShouldBeNil)
+
+		// Create a context that's already cancelled
+		ctx, cancel := context.WithCancel(context.Background())
+		cancel() // Cancel immediately
+
+		err = service.SyncRepo(ctx, "repo")
+		So(err, ShouldNotBeNil)
+		// This will fail at getTags before reaching the cancellation check
+	})
+
+	Convey("test context cancellation in SyncRepo with mock", t, func() {
+		conf := syncconf.RegistryConfig{
+			URLs: []string{"http://localhost"},
+		}
+
+		service, err := New(conf, "", nil, os.TempDir(), storage.StoreController{}, mocks.MetaDBMock{}, log.Logger{})
+		So(err, ShouldBeNil)
+
+		// Create a mock remote that returns tags so we can reach the loop
+		mockRemote := &mocks.SyncRemoteMock{
+			GetTagsFn: func(ctx context.Context, repo string) ([]string, error) {
+				return []string{"tag1", "tag2", "tag3"}, nil
+			},
+		}
+		service.remote = mockRemote
+
+		// Create a context that's already cancelled
+		ctx, cancel := context.WithCancel(context.Background())
+		cancel() // Cancel immediately
+
+		err = service.SyncRepo(ctx, "repo")
+		So(err, ShouldNotBeNil)
+		So(errors.Is(err, context.Canceled), ShouldBeTrue)
+	})
+
+	Convey("test SyncReferrers ReferrerList error", t, func() {
+		conf := syncconf.RegistryConfig{
+			URLs: []string{"http://localhost"},
+		}
+
+		service, err := New(conf, "", nil, os.TempDir(), storage.StoreController{}, mocks.MetaDBMock{}, log.Logger{})
+		So(err, ShouldBeNil)
+
+		// Create a minimal mock remote that only returns tags
+		mockRemote := &mocks.SyncRemoteMock{
+			GetTagsFn: func(ctx context.Context, repo string) ([]string, error) {
+				return []string{"tag1"}, nil
+			},
+		}
+		service.remote = mockRemote
+
+		// Set rc to nil to force a panic at ReferrerList call
+		service.rc = nil
+
+		// Use defer to catch the panic - this confirms we reached the ReferrerList call
+		var panicOccurred bool
+		defer func() {
+			if r := recover(); r != nil {
+				panicOccurred = true
+				t.Logf("SyncReferrers panic (expected): %v", r)
+			}
+		}()
+
+		ctx := context.Background()
+		err = service.SyncReferrers(ctx, "repo", "tag1", []string{"signature"})
+
+		// We expect a panic when rc is nil, which confirms we reached the ReferrerList call
+		So(panicOccurred, ShouldBeTrue)
+	})
+
+	Convey("test syncImage ReferrerList error with OnlySigned", t, func() {
+		onlySigned := true
+		conf := syncconf.RegistryConfig{
+			URLs:       []string{"http://invalid-registry-that-does-not-exist:9999"},
+			OnlySigned: &onlySigned,
+		}
+
+		service, err := New(conf, "", nil, os.TempDir(), storage.StoreController{}, mocks.MetaDBMock{}, log.Logger{})
+		So(err, ShouldBeNil)
+
+		// Create a mock remote that returns necessary data
+		mockRemote := &mocks.SyncRemoteMock{
+			GetImageReferenceFn: func(repo string, tag string) (ref.Ref, error) {
+				return ref.New("invalid-registry-that-does-not-exist:9999/" + repo + ":" + tag)
+			},
+			GetDigestFn: func(ctx context.Context, repo, tag string) (godigest.Digest, error) {
+				return godigest.Digest("sha256:abc123"), nil
+			},
+		}
+		service.remote = mockRemote
+
+		// Create a mock destination
+		mockDest := &mocks.SyncDestinationMock{
+			GetImageReferenceFn: func(repo string, tag string) (ref.Ref, error) {
+				return ref.New("local/" + repo + ":" + tag)
+			},
+		}
+		service.destination = mockDest
+
+		ctx := context.Background()
+		err = service.syncImage(ctx, "localrepo", "remoterepo", "tag1", []string{}, true)
+
+		// We expect an error when ReferrerList fails (network/connection error in this case)
+		So(err, ShouldNotBeNil)
+	})
 }
 
 func TestDestinationRegistry(t *testing.T) {

pkg/storage/local/driver.go

@@ -359,8 +359,15 @@ func (fi fileInfo) IsDir() bool {
 	return fi.FileInfo.IsDir()
 }
 
+// fileInternal defines the interface for file operations (internal).
+type fileInternal interface {
+	io.WriteCloser
+	Sync() error
+	Name() string
+}
+
 type fileWriter struct {
-	file      *os.File
+	file      fileInternal
 	size      int64
 	bw        *bufio.Writer
 	closed    bool
@@ -369,7 +376,7 @@ type fileWriter struct {
 	commit    bool
 }
 
-func newFileWriter(file *os.File, size int64, commit bool) *fileWriter {
+func newFileWriter(file fileInternal, size int64, commit bool) *fileWriter {
 	return &fileWriter{
 		file:   file,
 		size:   size,
@@ -378,6 +385,11 @@ func newFileWriter(file *os.File, size int64, commit bool) *fileWriter {
 	}
 }
 
+// NewFileWriter creates a new fileWriter for testing purposes.
+func NewFileWriter(file fileInternal, size int64, commit bool) *fileWriter {
+	return newFileWriter(file, size, commit)
+}
+
 func (fw *fileWriter) Write(buf []byte) (int, error) {
 	//nolint: gocritic
 	if fw.closed {

pkg/storage/local/driver_internal_test.go

@@ -0,0 +1,65 @@
+package local
+
+import (
+	"errors"
+	"testing"
+
+	storagedriver "github.com/distribution/distribution/v3/registry/storage/driver"
+	. "github.com/smartystreets/goconvey/convey"
+)
+
+func TestFormatErr(t *testing.T) {
+	Convey("Test formatErr error handling", t, func() {
+		driver := New(true)
+
+		Convey("Test formatErr with nil error", func() {
+			err := driver.formatErr(nil)
+			So(err, ShouldBeNil)
+		})
+
+		Convey("Test formatErr with PathNotFoundError", func() {
+			pathErr := storagedriver.PathNotFoundError{Path: "/test"}
+			formatted := driver.formatErr(pathErr)
+			So(formatted, ShouldNotBeNil)
+
+			// Check if it's still a PathNotFoundError with driver name set
+			var pathNotFoundErr storagedriver.PathNotFoundError
+			So(errors.As(formatted, &pathNotFoundErr), ShouldBeTrue)
+			So(pathNotFoundErr.DriverName, ShouldEqual, "local")
+		})
+
+		Convey("Test formatErr with InvalidPathError", func() {
+			invalidErr := storagedriver.InvalidPathError{Path: "/test"}
+			formatted := driver.formatErr(invalidErr)
+			So(formatted, ShouldNotBeNil)
+
+			// Check if it's still an InvalidPathError with driver name set
+			var invalidPathErr storagedriver.InvalidPathError
+			So(errors.As(formatted, &invalidPathErr), ShouldBeTrue)
+			So(invalidPathErr.DriverName, ShouldEqual, "local")
+		})
+
+		Convey("Test formatErr with InvalidOffsetError", func() {
+			offsetErr := storagedriver.InvalidOffsetError{Path: "/test", Offset: 100}
+			formatted := driver.formatErr(offsetErr)
+			So(formatted, ShouldNotBeNil)
+
+			// Check if it's still an InvalidOffsetError with driver name set
+			var invalidOffsetErr storagedriver.InvalidOffsetError
+			So(errors.As(formatted, &invalidOffsetErr), ShouldBeTrue)
+			So(invalidOffsetErr.DriverName, ShouldEqual, "local")
+		})
+
+		Convey("Test formatErr with generic error", func() {
+			genericErr := errors.New("generic error")
+			formatted := driver.formatErr(genericErr)
+			So(formatted, ShouldNotBeNil)
+
+			// Check if it's wrapped in a storagedriver.Error
+			var storageErr storagedriver.Error
+			So(errors.As(formatted, &storageErr), ShouldBeTrue)
+			So(storageErr.DriverName, ShouldEqual, "local")
+			So(storageErr.Detail, ShouldEqual, genericErr)
+		})
+	})
+}

pkg/test/mocks/sync_remote_mock.go

@@ -3,54 +3,30 @@ package mocks
 import (
 	"context"
 
-	"github.com/containers/image/v5/types"
 	"github.com/opencontainers/go-digest"
+	"github.com/regclient/regclient/types/ref"
 )
 
-type SyncRemote struct {
-	// Get temporary ImageReference, is used by functions in containers/image package
-	GetImageReferenceFn func(repo string, tag string) (types.ImageReference, error)
-
-	// Get local oci layout context, is used by functions in containers/image package
-	GetContextFn func() *types.SystemContext
-
-	// Get a list of repos (catalog)
-	GetRepositoriesFn func(ctx context.Context) ([]string, error)
-
-	// Get a list of tags given a repo
-	GetRepoTagsFn func(repo string) ([]string, error)
-
-	GetDockerRemoteRepoFn func(repo string) string
-
-	// Get manifest content, mediaType, digest given an ImageReference
-	GetManifestContentFn func(imageReference types.ImageReference) ([]byte, string, digest.Digest, error)
+type SyncRemoteMock struct {
+	// Methods required by sync Remote interface.
+	GetHostNameFn       func() string
+	GetRepositoriesFn   func(ctx context.Context) ([]string, error)
+	GetTagsFn           func(ctx context.Context, repo string) ([]string, error)
+	GetOCIDigestFn      func(ctx context.Context, repo, tag string) (digest.Digest, digest.Digest, bool, error)
+	GetDigestFn         func(ctx context.Context, repo, tag string) (digest.Digest, error)
+	GetImageReferenceFn func(repo string, tag string) (ref.Ref, error)
 }
 
-func (remote SyncRemote) GetDockerRemoteRepo(repo string) string {
-	if remote.GetDockerRemoteRepoFn != nil {
-		return remote.GetDockerRemoteRepoFn(repo)
+// Methods required by sync Remote interface.
+func (remote SyncRemoteMock) GetHostName() string {
+	if remote.GetHostNameFn != nil {
+		return remote.GetHostNameFn()
 	}
 
-	return ""
+	return "mock-host"
 }
 
-func (remote SyncRemote) GetImageReference(repo string, tag string) (types.ImageReference, error) {
-	if remote.GetImageReferenceFn != nil {
-		return remote.GetImageReferenceFn(repo, tag)
-	}
-
-	return nil, nil //nolint:nilnil
-}
-
-func (remote SyncRemote) GetContext() *types.SystemContext {
-	if remote.GetContextFn != nil {
-		return remote.GetContextFn()
-	}
-
-	return nil
-}
-
-func (remote SyncRemote) GetRepositories(ctx context.Context) ([]string, error) {
+func (remote SyncRemoteMock) GetRepositories(ctx context.Context) ([]string, error) {
 	if remote.GetRepositoriesFn != nil {
 		return remote.GetRepositoriesFn(ctx)
 	}
@@ -58,23 +34,77 @@ func (remote SyncRemote) GetRepositories(ctx context.Context) ([]string, error)
 	return []string{}, nil
 }
 
-func (remote SyncRemote) GetRepoTags(repo string) ([]string, error) {
-	if remote.GetRepoTagsFn != nil {
-		return remote.GetRepoTagsFn(repo)
+func (remote SyncRemoteMock) GetTags(ctx context.Context, repo string) ([]string, error) {
+	if remote.GetTagsFn != nil {
+		return remote.GetTagsFn(ctx, repo)
 	}
 
 	return []string{}, nil
 }
 
-func (remote SyncRemote) GetManifestContent(imageReference types.ImageReference) (
-	[]byte, string, digest.Digest, error,
+func (remote SyncRemoteMock) GetOCIDigest(ctx context.Context, repo, tag string) (
+	digest.Digest, digest.Digest, bool, error,
 ) {
-	if remote.GetManifestContentFn != nil {
-		return remote.GetManifestContentFn(imageReference)
+	if remote.GetOCIDigestFn != nil {
+		return remote.GetOCIDigestFn(ctx, repo, tag)
 	}
 
-	return nil, "", "", nil
+	return digest.Digest("sha256:abc123"), digest.Digest("sha256:def456"), false, nil
 }
 
-func (remote SyncRemote) SetUpstreamAuthConfig(username, password string) {
+func (remote SyncRemoteMock) GetDigest(ctx context.Context, repo, tag string) (digest.Digest, error) {
+	if remote.GetDigestFn != nil {
+		return remote.GetDigestFn(ctx, repo, tag)
+	}
+
+	return digest.Digest("sha256:abc123"), nil
+}
+
+func (remote SyncRemoteMock) GetImageReference(repo string, tag string) (ref.Ref, error) {
+	if remote.GetImageReferenceFn != nil {
+		return remote.GetImageReferenceFn(repo, tag)
+	}
+
+	return ref.New("mock-registry/" + repo + ":" + tag)
+}
+
+type SyncDestinationMock struct {
+	// Methods required by sync Destination interface.
+	GetImageReferenceFn func(repo string, tag string) (ref.Ref, error)
+	CanSkipImageFn      func(repo string, tag string, digest digest.Digest) (bool, error)
+	CommitAllFn         func(repo string, imageReference ref.Ref) error
+	CleanupImageFn      func(imageReference ref.Ref, repo string) error
+}
+
+// Methods required by sync Destination interface.
+func (dest SyncDestinationMock) GetImageReference(repo string, tag string) (ref.Ref, error) {
+	if dest.GetImageReferenceFn != nil {
+		return dest.GetImageReferenceFn(repo, tag)
+	}
+
+	return ref.New("mock-local/" + repo + ":" + tag)
+}
+
+func (dest SyncDestinationMock) CanSkipImage(repo string, tag string, digest digest.Digest) (bool, error) {
+	if dest.CanSkipImageFn != nil {
+		return dest.CanSkipImageFn(repo, tag, digest)
+	}
+
+	return false, nil
+}
+
+func (dest SyncDestinationMock) CommitAll(repo string, imageReference ref.Ref) error {
+	if dest.CommitAllFn != nil {
+		return dest.CommitAllFn(repo, imageReference)
+	}
+
+	return nil
+}
+
+func (dest SyncDestinationMock) CleanupImage(imageReference ref.Ref, repo string) error {
+	if dest.CleanupImageFn != nil {
+		return dest.CleanupImageFn(imageReference, repo)
+	}
+
+	return nil
 }