storagedriver/s3: test refactor. fixed a bug in WalkFallback preventing ErrSkipDir from stopping gracefully

registry/storage/driver/walk.go

@@ -22,9 +22,14 @@ type WalkFn func(fileInfo FileInfo) error
 // to a directory, the directory will not be entered and Walk
 // will continue the traversal.  If fileInfo refers to a normal file, processing stops
 func WalkFallback(ctx context.Context, driver StorageDriver, from string, f WalkFn) error {
+	err, _ := doWalkFallback(ctx, driver, from, f)
+	return err
+}
+
+func doWalkFallback(ctx context.Context, driver StorageDriver, from string, f WalkFn) (error, bool) {
 	children, err := driver.List(ctx, from)
 	if err != nil {
-		return err
+		return err, false
 	}
 	sort.Stable(sort.StringSlice(children))
 	for _, child := range children {
@@ -40,22 +45,22 @@ func WalkFallback(ctx context.Context, driver StorageDriver, from string, f Walk
 				logrus.WithField("path", child).Infof("ignoring deleted path")
 				continue
 			default:
-				return err
+				return err, false
 			}
 		}
 		err = f(fileInfo)
 		if err == nil && fileInfo.IsDir() {
-			if err := WalkFallback(ctx, driver, child, f); err != nil {
+			if err, ok := doWalkFallback(ctx, driver, child, f); err != nil || !ok {
-				return err
+				return err, ok
 			}
 		} else if err == ErrSkipDir {
-			// Stop iteration if it's a file, otherwise noop if it's a directory
+			// noop for folders, will just skip
 			if !fileInfo.IsDir() {
-				return nil
+				return nil, false // no error but stop iteration
 			}
 		} else if err != nil {
-			return err
+			return err, false
 		}
 	}
-	return nil
+	return nil, true
 }

registry/storage/driver/walk_test.go

@@ -2,7 +2,6 @@ package driver
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 	"testing"
@@ -14,10 +13,10 @@ type changingFileSystem struct {
 	keptFiles map[string]bool
 }
 
-func (cfs *changingFileSystem) List(ctx context.Context, path string) ([]string, error) {
+func (cfs *changingFileSystem) List(_ context.Context, _ string) ([]string, error) {
 	return cfs.fileset, nil
 }
-func (cfs *changingFileSystem) Stat(ctx context.Context, path string) (FileInfo, error) {
+func (cfs *changingFileSystem) Stat(_ context.Context, path string) (FileInfo, error) {
 	kept, ok := cfs.keptFiles[path]
 	if ok && kept {
 		return &FileInfoInternal{
@@ -31,6 +30,7 @@ func (cfs *changingFileSystem) Stat(ctx context.Context, path string) (FileInfo,
 
 type fileSystem struct {
 	StorageDriver
+	// maps folder to list results
 	fileset map[string][]string
 }
 
@@ -81,120 +81,98 @@ func TestWalkFallback(t *testing.T) {
 			"/folder2": {"/folder2/file1"},
 		},
 	}
-	expected := []string{
+	noopFn := func(fileInfo FileInfo) error { return nil }
+
+	tcs := []struct {
+		name     string
+		fn       WalkFn
+		from     string
+		expected []string
+		err      bool
+	}{
+		{
+			name: "walk all",
+			fn:   noopFn,
+			expected: []string{
 				"/file1",
 				"/folder1",
 				"/folder1/file1",
 				"/folder2",
 				"/folder2/file1",
-	}
-
-	var walked []string
-	err := WalkFallback(context.Background(), d, "/", func(fileInfo FileInfo) error {
-		if fileInfo.IsDir() != d.isDir(fileInfo.Path()) {
-			t.Fatalf("fileInfo isDir not matching file system: expected %t actual %t", d.isDir(fileInfo.Path()), fileInfo.IsDir())
-		}
-		walked = append(walked, fileInfo.Path())
-		return nil
-	})
-	if err != nil {
-		t.Fatalf(err.Error())
-	}
-	compareWalked(t, expected, walked)
-}
-
-// Walk is expected to skip directory on ErrSkipDir
-func TestWalkFallbackSkipDirOnDir(t *testing.T) {
-	d := &fileSystem{
-		fileset: map[string][]string{
-			"/":        {"/file1", "/folder1", "/folder2"},
-			"/folder1": {"/folder1/file1"},
-			"/folder2": {"/folder2/file1"},
 			},
+		},
+		{
+			name: "skip directory",
+			fn: func(fileInfo FileInfo) error {
+				if fileInfo.Path() == "/folder1" {
+					return ErrSkipDir
 				}
-	skipDir := "/folder1"
+				if strings.Contains(fileInfo.Path(), "/folder1") {
-	expected := []string{
+					t.Fatalf("skipped dir %s and should not walk %s", "/folder1", fileInfo.Path())
+				}
+				return nil
+			},
+			expected: []string{
 				"/file1",
 				"/folder1", // return ErrSkipDir, skip anything under /folder1
 				// skip /folder1/file1
 				"/folder2",
 				"/folder2/file1",
-	}
-
-	var walked []string
-	err := WalkFallback(context.Background(), d, "/", func(fileInfo FileInfo) error {
-		walked = append(walked, fileInfo.Path())
-		if fileInfo.Path() == skipDir {
-			return ErrSkipDir
-		}
-		if strings.Contains(fileInfo.Path(), skipDir) {
-			t.Fatalf("skipped dir %s and should not walk %s", skipDir, fileInfo.Path())
-		}
-		return nil
-	})
-	if err != nil {
-		t.Fatalf("expected Walk to not error %v", err)
-	}
-	compareWalked(t, expected, walked)
-}
-
-func TestWalkFallbackSkipDirOnFile(t *testing.T) {
-	d := &fileSystem{
-		fileset: map[string][]string{
-			"/": {"/file1", "/file2", "/file3"},
 			},
-	}
+		},
-	skipFile := "/file2"
+		{
-	expected := []string{
+			name: "stop early",
-		"/file1",
+			fn: func(fileInfo FileInfo) error {
-		"/file2", // return ErrSkipDir, stop early
+				if fileInfo.Path() == "/folder1/file1" {
-	}
-
-	var walked []string
-	err := WalkFallback(context.Background(), d, "/", func(fileInfo FileInfo) error {
-		walked = append(walked, fileInfo.Path())
-		if fileInfo.Path() == skipFile {
 					return ErrSkipDir
 				}
 				return nil
-	})
+			},
-	if err != nil {
+			expected: []string{
-		t.Fatalf("expected Walk to not error %v", err)
+				"/file1",
-	}
+				"/folder1",
-	compareWalked(t, expected, walked)
+				"/folder1/file1",
-}
+				// stop early
-
+			},
-// Walk is expected to skip directory on ErrSkipDir
+		},
-func TestWalkFallbackErr(t *testing.T) {
+		{
-	d := &fileSystem{
+			name: "from folder",
-		fileset: map[string][]string{
+			fn:   noopFn,
-			"/": {"/file1", "/file2", "/file3"},
+			expected: []string{
+				"/folder1/file1",
+			},
+			from: "/folder1",
 		},
 	}
-	errFile := "/file2"
-	expected := []string{
-		"/file1",
-		"/file2", // return ErrSkipDir, stop early
-	}
-	expectedErr := errors.New("foo")
 
+	for _, tc := range tcs {
 		var walked []string
-	err := WalkFallback(context.Background(), d, "/", func(fileInfo FileInfo) error {
+		if tc.from == "" {
+			tc.from = "/"
+		}
+		t.Run(tc.name, func(t *testing.T) {
+			err := WalkFallback(context.Background(), d, tc.from, func(fileInfo FileInfo) error {
 				walked = append(walked, fileInfo.Path())
-		if fileInfo.Path() == errFile {
+				if fileInfo.IsDir() != d.isDir(fileInfo.Path()) {
-			return expectedErr
+					t.Fatalf("fileInfo isDir not matching file system: expected %t actual %t", d.isDir(fileInfo.Path()), fileInfo.IsDir())
 				}
-		return nil
+				return tc.fn(fileInfo)
 			})
-	if err != expectedErr {
+			if tc.err && err == nil {
-		t.Fatalf("unexpected err %v", err)
+				t.Fatalf("expected err")
 			}
-	compareWalked(t, expected, walked)
+			if !tc.err && err != nil {
+				t.Fatalf(err.Error())
+			}
+			compareWalked(t, tc.expected, walked)
+		})
+	}
+
 }
 
 func compareWalked(t *testing.T, expected, walked []string) {
 	if len(walked) != len(expected) {
-		t.Fatalf("Mismatch number of fileInfo walked %d expected %d", len(walked), len(expected))
+		t.Fatalf("Mismatch number of fileInfo walked %d expected %d; walked %s; expected %s;", len(walked), len(expected), walked, expected)
 	}
 	for i := range walked {
 		if walked[i] != expected[i] {