From 70337b0f26f1363cc62c81f4df4fa59dcd1e0fb6 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sat, 6 Apr 2024 16:21:57 -0400 Subject: [PATCH 01/11] Run tests for Windows --- .github/workflows/build-and-test.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 37ad9931..0291e555 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -60,6 +60,5 @@ jobs: - name: Build run: go build ./... - - name: Targeted Test - run: | - go test ./internal/api + - name: Test + run: go test ./... From c8731b987f35e81c3860732c86848cd34c0237ae Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sat, 6 Apr 2024 01:44:32 -0400 Subject: [PATCH 02/11] Fix flaky test --- pkg/restic/restic_test.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/restic/restic_test.go b/pkg/restic/restic_test.go index 67f764ec..aeb51cbd 100644 --- a/pkg/restic/restic_test.go +++ b/pkg/restic/restic_test.go @@ -5,8 +5,11 @@ import ( "context" "errors" "fmt" + "path/filepath" "reflect" + "runtime" "slices" + "strings" "testing" "github.com/garethgeorge/backrest/test/helpers" @@ -396,6 +399,11 @@ func TestResticRestore(t *testing.T) { restorePath := t.TempDir() testData := helpers.CreateTestData(t) + dirCount := strings.Count(testData, string(filepath.Separator)) + if runtime.GOOS == "windows" { + // On Windows, the volume name is also included as a dir in the path. + dirCount += 1 + } snapshot, err := r.Backup(context.Background(), []string{testData}, nil) if err != nil { @@ -411,8 +419,9 @@ func TestResticRestore(t *testing.T) { } // should be 100 files + parent directories. - if summary.TotalFiles != 103 { - t.Errorf("wanted 101 files to be restored, got: %d", summary.TotalFiles) + fileCount := 100 + dirCount + if summary.TotalFiles != int64(fileCount) { + t.Errorf("wanted %d files to be restored, got: %d", fileCount, summary.TotalFiles) } } From 48954517746838c8d25fd200be89916925b333cd Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sat, 6 Apr 2024 16:33:30 -0400 Subject: [PATCH 03/11] Skip Bash hook test on Windows --- internal/hook/hook_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/hook/hook_test.go b/internal/hook/hook_test.go index 1cef76da..944e220e 100644 --- a/internal/hook/hook_test.go +++ b/internal/hook/hook_test.go @@ -3,6 +3,7 @@ package hook import ( "bytes" "os/exec" + "runtime" "testing" v1 "github.com/garethgeorge/backrest/gen/go/v1" @@ -28,6 +29,10 @@ func TestHookCommandInDefaultShell(t *testing.T) { } func TestHookCommandInBashShell(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on windows") + } + hook := Hook(v1.Hook{ Conditions: []v1.Hook_Condition{v1.Hook_CONDITION_SNAPSHOT_START}, Action: &v1.Hook_ActionCommand{ From 28d346b38d28373a55845396dbb24d1d32acf3f4 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sat, 6 Apr 2024 16:33:58 -0400 Subject: [PATCH 04/11] Fix file handle leak in BoltDB tests --- internal/oplog/indexutil/indexutil_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/oplog/indexutil/indexutil_test.go b/internal/oplog/indexutil/indexutil_test.go index 6784c38c..e90958d5 100644 --- a/internal/oplog/indexutil/indexutil_test.go +++ b/internal/oplog/indexutil/indexutil_test.go @@ -13,6 +13,7 @@ func TestIndexing(t *testing.T) { if err != nil { t.Fatalf("error opening database: %s", err) } + defer db.Close() if err := db.Update(func(tx *bbolt.Tx) error { b, err := tx.CreateBucket([]byte("test")) @@ -51,6 +52,7 @@ func TestIndexJoin(t *testing.T) { if err != nil { t.Fatalf("error opening database: %s", err) } + defer db.Close() if err := db.Update(func(tx *bbolt.Tx) error { b, err := tx.CreateBucket([]byte("test")) From e8e0f70919bb898a743d9eedfa8a821edd7da7e6 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sat, 6 Apr 2024 16:34:08 -0400 Subject: [PATCH 05/11] Don't test for gzip compression on Windows --- webui/webui_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/webui/webui_test.go b/webui/webui_test.go index 1626e91d..c9229f23 100644 --- a/webui/webui_test.go +++ b/webui/webui_test.go @@ -3,6 +3,7 @@ package webui import ( "net/http" "net/http/httptest" + "runtime" "testing" ) @@ -33,7 +34,9 @@ func TestServeIndex(t *testing.T) { status, http.StatusOK) } - if rr.Header().Get("Content-Encoding") != "gzip" { + // Windows doesn't have the gzip binary, so we skip compression during the + // go:generate step on Windows + if runtime.GOOS != "windows" && rr.Header().Get("Content-Encoding") != "gzip" { t.Errorf("handler returned wrong content encoding: got %v want %v", rr.Header().Get("Content-Encoding"), "gzip") } From e61233b5300db87c14ff3dcd4052ef98acbc9286 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sat, 6 Apr 2024 17:37:54 -0400 Subject: [PATCH 06/11] Create unreadable file with go-acl on Windows --- go.mod | 1 + go.sum | 3 +++ test/helpers/testdata.go | 6 ++++++ 3 files changed, 10 insertions(+) diff --git a/go.mod b/go.mod index 09aa3bdb..96b48ccd 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ require ( github.com/golang-jwt/jwt/v5 v5.2.1 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/hashicorp/go-multierror v1.1.1 + github.com/hectane/go-acl v0.0.0-20230122075934-ca0b05cb1adb github.com/mattn/go-colorable v0.1.13 github.com/natefinch/atomic v1.0.1 go.etcd.io/bbolt v1.3.9 diff --git a/go.sum b/go.sum index e227dc57..1f8a2640 100644 --- a/go.sum +++ b/go.sum @@ -29,6 +29,8 @@ github.com/hashicorp/errwrap v1.1.0 h1:OxrOeh75EUXMY8TBjag2fzXGZ40LB6IKw45YeGUDY github.com/hashicorp/errwrap v1.1.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+lD48awMYo= github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM= +github.com/hectane/go-acl v0.0.0-20230122075934-ca0b05cb1adb h1:PGufWXXDq9yaev6xX1YQauaO1MV90e6Mpoq1I7Lz/VM= +github.com/hectane/go-acl v0.0.0-20230122075934-ca0b05cb1adb/go.mod h1:QiyDdbZLaJ/mZP4Zwc9g2QsfaEA4o7XvvgZegSci5/E= github.com/jarcoal/httpmock v1.3.0 h1:2RJ8GP0IIaWwcC9Fp2BmVi8Kog3v2Hn7VXM3fTd+nuc= github.com/jarcoal/httpmock v1.3.0/go.mod h1:3yb8rc4BI7TCBhFY8ng0gjuLKJNquuDNiPaZjnENuYg= github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= @@ -62,6 +64,7 @@ golang.org/x/net v0.22.0 h1:9sGLhx7iRIHEiX0oAJ3MRZMUCElJgy7Br1nO+AMN3Tc= golang.org/x/net v0.22.0/go.mod h1:JKghWKKOSdJwpW2GEx0Ja7fmaKnMsbu+MWVZTokSYmg= golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/sys v0.0.0-20190529164535-6a60838ec259/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.18.0 h1:DBdB3niSjOA/O0blCZBqDefyWNYveAYMNF1Wum0DYQ4= diff --git a/test/helpers/testdata.go b/test/helpers/testdata.go index 235d23cb..b44044c1 100644 --- a/test/helpers/testdata.go +++ b/test/helpers/testdata.go @@ -5,6 +5,8 @@ import ( "os" "path" "testing" + + acl "github.com/hectane/go-acl" ) func CreateTestData(t *testing.T) string { @@ -28,4 +30,8 @@ func CreateUnreadable(t *testing.T, path string) { if err != nil { t.Fatalf("failed to create unreadable file: %v", err) } + + if err := acl.Chmod(path, 0200); err != nil { + t.Fatalf("failed to set file ACL: %v", err) + } } From e5a8fdc3ea85ff0fc31dd31575bcc84e619cb511 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sat, 6 Apr 2024 17:54:02 -0400 Subject: [PATCH 07/11] Fix unreadable file test on Windows --- pkg/restic/restic_test.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/restic/restic_test.go b/pkg/restic/restic_test.go index aeb51cbd..57ddb05c 100644 --- a/pkg/restic/restic_test.go +++ b/pkg/restic/restic_test.go @@ -127,12 +127,13 @@ func TestResticPartialBackup(t *testing.T) { } testDataUnreadable := t.TempDir() - helpers.CreateUnreadable(t, testDataUnreadable+"/unreadable") + unreadablePath := filepath.Join(testDataUnreadable, "unreadable") + helpers.CreateUnreadable(t, unreadablePath) - var entries []*BackupProgressEntry + var entries []BackupProgressEntry summary, err := r.Backup(context.Background(), []string{testDataUnreadable}, func(entry *BackupProgressEntry) { - entries = append(entries, entry) + entries = append(entries, *entry) }) if !errors.Is(err, ErrPartialBackup) { t.Fatalf("wanted error to be partial backup, got: %v", err) @@ -145,10 +146,14 @@ func TestResticPartialBackup(t *testing.T) { t.Errorf("wanted 0 files, got: %d", summary.TotalFilesProcessed) } - if !slices.ContainsFunc(entries, func(e *BackupProgressEntry) bool { - return e.MessageType == "error" && e.Item == testDataUnreadable+"/unreadable" + if !slices.ContainsFunc(entries, func(e BackupProgressEntry) bool { + return e.MessageType == "error" && e.Item == unreadablePath }) { - t.Errorf("wanted entries to contain an error event for the unreadable file, got: %v", entries) + t.Errorf("wanted entries to contain an error event for the unreadable file (%s), but did not find it", unreadablePath) + t.Logf("entries:\n") + for _, entry := range entries { + t.Logf("%+v\n", entry) + } } } From 1d4f9c9b9e20b467f43b81c7fbf628cd4b8d4429 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sat, 6 Apr 2024 19:44:08 -0400 Subject: [PATCH 08/11] Fix path filter error on Windows ``` restic_test.go:261: failed to list directory: command "C:\\Program Files (x86)/restic/restic-0.16.4.exe ls --json 0bca3b482c620f384eae2ca000f7d5ce212171049e5563dd16bc4591345407d7 C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\TestLs1936002979\\002 --no-cache" failed: exit status 1 Process STDOUT: Fatal: All path filters must be absolute, starting with a forward slash '/' ``` --- pkg/restic/path.go | 8 ++++++++ pkg/restic/path_win.go | 37 +++++++++++++++++++++++++++++++++++++ pkg/restic/path_win_test.go | 36 ++++++++++++++++++++++++++++++++++++ pkg/restic/restic.go | 1 + 4 files changed, 82 insertions(+) create mode 100644 pkg/restic/path.go create mode 100644 pkg/restic/path_win.go create mode 100644 pkg/restic/path_win_test.go diff --git a/pkg/restic/path.go b/pkg/restic/path.go new file mode 100644 index 00000000..838e54cf --- /dev/null +++ b/pkg/restic/path.go @@ -0,0 +1,8 @@ +//go:build !windows +// +build !windows + +package restic + +func toPathFilter(path string) string { + return path +} diff --git a/pkg/restic/path_win.go b/pkg/restic/path_win.go new file mode 100644 index 00000000..26dac346 --- /dev/null +++ b/pkg/restic/path_win.go @@ -0,0 +1,37 @@ +//go:build windows +// +build windows + +package restic + +import ( + "path/filepath" + "strings" +) + +// toPathFilter converts a directory path to a path filter. Restic requires +// these to be absolute, starting with a forward slash. On Windows, we convert +// the drive letter to a single character and prepend a forward slash. +func toPathFilter(path string) string { + cleanedPath := filepath.Clean(path) + // If clean results in an empty string, ignoring the volume, it returns "." + if cleanedPath != path+"." { + path = cleanedPath + } + + before, after, found := strings.Cut(path, string(filepath.Separator)) + if !found { + // e.g. if path is "C:" + before = path + after = "" + } + + if len(before) == 2 && before[1] == ':' && before[0] >= 'A' && before[0] <= 'Z' { + path = filepath.Join(string(before[0]), after) + } + + if path[0] != filepath.Separator { + path = filepath.Join(string(filepath.Separator), path) + } + + return filepath.ToSlash(path) +} diff --git a/pkg/restic/path_win_test.go b/pkg/restic/path_win_test.go new file mode 100644 index 00000000..9c506120 --- /dev/null +++ b/pkg/restic/path_win_test.go @@ -0,0 +1,36 @@ +//go:build windows +// +build windows + +package restic + +import "testing" + +func TestToPathFilter(t *testing.T) { + t.Parallel() + + tcs := []struct { + path string + want string + }{ + {"C:\\Users\\user\\Documents", "/C/Users/user/Documents"}, + {"C:\\", "/C"}, + {"C:", "/C"}, + {"/Users/user/Documents", "/Users/user/Documents"}, + {"\\Users\\user\\Documents", "/Users/user/Documents"}, + + // network share - not sure if this is correct + {"\\\\network-share\\path\\to\\file", "//network-share/path/to/file"}, + + // invalid / not handled... + {"1:\\foobar", "/1:/foobar"}, + {"AA:\\foobar", "/AA:/foobar"}, + {"a/relative/directory", "/a/relative/directory"}, + } + + for _, tc := range tcs { + got := toPathFilter(tc.path) + if got != tc.want { + t.Errorf("toPathFilter(%q) == %q, want %q", tc.path, got, tc.want) + } + } +} diff --git a/pkg/restic/restic.go b/pkg/restic/restic.go index e2223b59..b847eec4 100644 --- a/pkg/restic/restic.go +++ b/pkg/restic/restic.go @@ -306,6 +306,7 @@ func (r *Repo) ListDirectory(ctx context.Context, snapshot string, path string, // an empty path can trigger very expensive operations (e.g. iterates all files in the snapshot) return nil, nil, errors.New("path must not be empty") } + path = toPathFilter(path) cmd := r.commandWithContext(ctx, []string{"ls", "--json", snapshot, path}, opts...) output := bytes.NewBuffer(nil) From bfbef114a1916e6bea55f3f8bc80114b00515014 Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sun, 7 Apr 2024 08:17:16 -0400 Subject: [PATCH 09/11] Move toPathFilter to tests as toRepoPath --- pkg/restic/path.go | 8 -------- pkg/restic/path_win.go | 37 ------------------------------------- pkg/restic/path_win_test.go | 36 ------------------------------------ pkg/restic/restic.go | 1 - pkg/restic/restic_test.go | 14 +++++++++++++- 5 files changed, 13 insertions(+), 83 deletions(-) delete mode 100644 pkg/restic/path.go delete mode 100644 pkg/restic/path_win.go delete mode 100644 pkg/restic/path_win_test.go diff --git a/pkg/restic/path.go b/pkg/restic/path.go deleted file mode 100644 index 838e54cf..00000000 --- a/pkg/restic/path.go +++ /dev/null @@ -1,8 +0,0 @@ -//go:build !windows -// +build !windows - -package restic - -func toPathFilter(path string) string { - return path -} diff --git a/pkg/restic/path_win.go b/pkg/restic/path_win.go deleted file mode 100644 index 26dac346..00000000 --- a/pkg/restic/path_win.go +++ /dev/null @@ -1,37 +0,0 @@ -//go:build windows -// +build windows - -package restic - -import ( - "path/filepath" - "strings" -) - -// toPathFilter converts a directory path to a path filter. Restic requires -// these to be absolute, starting with a forward slash. On Windows, we convert -// the drive letter to a single character and prepend a forward slash. -func toPathFilter(path string) string { - cleanedPath := filepath.Clean(path) - // If clean results in an empty string, ignoring the volume, it returns "." - if cleanedPath != path+"." { - path = cleanedPath - } - - before, after, found := strings.Cut(path, string(filepath.Separator)) - if !found { - // e.g. if path is "C:" - before = path - after = "" - } - - if len(before) == 2 && before[1] == ':' && before[0] >= 'A' && before[0] <= 'Z' { - path = filepath.Join(string(before[0]), after) - } - - if path[0] != filepath.Separator { - path = filepath.Join(string(filepath.Separator), path) - } - - return filepath.ToSlash(path) -} diff --git a/pkg/restic/path_win_test.go b/pkg/restic/path_win_test.go deleted file mode 100644 index 9c506120..00000000 --- a/pkg/restic/path_win_test.go +++ /dev/null @@ -1,36 +0,0 @@ -//go:build windows -// +build windows - -package restic - -import "testing" - -func TestToPathFilter(t *testing.T) { - t.Parallel() - - tcs := []struct { - path string - want string - }{ - {"C:\\Users\\user\\Documents", "/C/Users/user/Documents"}, - {"C:\\", "/C"}, - {"C:", "/C"}, - {"/Users/user/Documents", "/Users/user/Documents"}, - {"\\Users\\user\\Documents", "/Users/user/Documents"}, - - // network share - not sure if this is correct - {"\\\\network-share\\path\\to\\file", "//network-share/path/to/file"}, - - // invalid / not handled... - {"1:\\foobar", "/1:/foobar"}, - {"AA:\\foobar", "/AA:/foobar"}, - {"a/relative/directory", "/a/relative/directory"}, - } - - for _, tc := range tcs { - got := toPathFilter(tc.path) - if got != tc.want { - t.Errorf("toPathFilter(%q) == %q, want %q", tc.path, got, tc.want) - } - } -} diff --git a/pkg/restic/restic.go b/pkg/restic/restic.go index b847eec4..e2223b59 100644 --- a/pkg/restic/restic.go +++ b/pkg/restic/restic.go @@ -306,7 +306,6 @@ func (r *Repo) ListDirectory(ctx context.Context, snapshot string, path string, // an empty path can trigger very expensive operations (e.g. iterates all files in the snapshot) return nil, nil, errors.New("path must not be empty") } - path = toPathFilter(path) cmd := r.commandWithContext(ctx, []string{"ls", "--json", snapshot, path}, opts...) output := bytes.NewBuffer(nil) diff --git a/pkg/restic/restic_test.go b/pkg/restic/restic_test.go index 57ddb05c..991ed24f 100644 --- a/pkg/restic/restic_test.go +++ b/pkg/restic/restic_test.go @@ -255,7 +255,7 @@ func TestLs(t *testing.T) { t.Fatalf("failed to backup and create new snapshot: %v", err) } - _, entries, err := r.ListDirectory(context.Background(), snapshot.SnapshotId, testData) + _, entries, err := r.ListDirectory(context.Background(), snapshot.SnapshotId, toRepoPath(testData)) if err != nil { t.Fatalf("failed to list directory: %v", err) @@ -464,3 +464,15 @@ func TestResticStats(t *testing.T) { t.Errorf("wanted non-zero total blob count, got: %d", stats.TotalBlobCount) } } + +func toRepoPath(path string) string { + sepIdx := strings.Index(path, string(filepath.Separator)) + if sepIdx != 2 || path[1] != ':' { + return path + } + return filepath.ToSlash(filepath.Join( + string(filepath.Separator), // leading slash + string(path[0]), // drive volume + path[3:], // path + )) +} From 85674db7cc52779bba8ec17188a62bbc09fc1e3c Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sun, 7 Apr 2024 08:49:10 -0400 Subject: [PATCH 10/11] Skip flaky/failing task queue tests on Windows --- internal/orchestrator/scheduledtaskheap_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/orchestrator/scheduledtaskheap_test.go b/internal/orchestrator/scheduledtaskheap_test.go index 6a0fee69..93a540da 100644 --- a/internal/orchestrator/scheduledtaskheap_test.go +++ b/internal/orchestrator/scheduledtaskheap_test.go @@ -4,6 +4,7 @@ import ( "context" "math/rand" "reflect" + "runtime" "sort" "strconv" "testing" @@ -39,6 +40,10 @@ func (t *heapTestTask) OperationId() int64 { } func TestTaskQueueOrdering(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test is flaky on Windows") + } + h := taskQueue{} h.Push(scheduledTask{runAt: time.Now().Add(1 * time.Millisecond), task: &heapTestTask{name: "1"}}) @@ -125,6 +130,10 @@ func TestTasksOrderedByPriority(t *testing.T) { } func TestFuzzTaskQueue(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test does not pass on Windows") + } + h := taskQueue{} count := 100 From 906c6a7343f13b956fcc94a46d0bc67613c75ffc Mon Sep 17 00:00:00 2001 From: John Maguire Date: Sun, 7 Apr 2024 09:14:46 -0400 Subject: [PATCH 11/11] Limit toRepoPath to Windows --- pkg/restic/restic_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/restic/restic_test.go b/pkg/restic/restic_test.go index 991ed24f..c9f32a8a 100644 --- a/pkg/restic/restic_test.go +++ b/pkg/restic/restic_test.go @@ -466,6 +466,12 @@ func TestResticStats(t *testing.T) { } func toRepoPath(path string) string { + if runtime.GOOS != "windows" { + return path + } + + // On Windows, the temp directory path needs to be converted to a repo path + // for restic to interpret it correctly in restore/snapshot operations. sepIdx := strings.Index(path, string(filepath.Separator)) if sepIdx != 2 || path[1] != ':' { return path