From 303a5dab6ac749e7a0cbe588b8e5f22108fdcccb Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 5 May 2019 12:50:47 +0200 Subject: [PATCH 1/5] archiver: Clarify value in test struct Since I could not remember what the value for `Check` means this commit renames it to `SameFile`: when set to true, the test should make sure that `FileChanged` should return false (=file is unmodified). --- internal/archiver/archiver_test.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 201ee1a10..aff4f10f3 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -560,7 +560,7 @@ func TestFileChanged(t *testing.T) { Content []byte Modify func(t testing.TB, filename string) IgnoreInode bool - Check bool + SameFile bool }{ { Name: "same-content-new-file", @@ -622,7 +622,7 @@ func TestFileChanged(t *testing.T) { setTimestamp(t, filename, fi.ModTime(), fi.ModTime()) }, IgnoreInode: true, - Check: true, + SameFile: true, }, } @@ -648,10 +648,15 @@ func TestFileChanged(t *testing.T) { test.Modify(t, filename) fiAfter := lstat(t, filename) - if test.Check == fileChanged(fiAfter, node, test.IgnoreInode) { - if test.Check { + + if test.SameFile { + // file should be detected as unchanged + if fileChanged(fiAfter, node, test.IgnoreInode) { t.Fatalf("unmodified file detected as changed") - } else { + } + } else { + // file should be detected as changed + if !fileChanged(fiAfter, node, test.IgnoreInode) && !test.SameFile { t.Fatalf("modified file detected as unchanged") } } From 6e2fe7318989e4e4b9f567a20af4621c30ae5055 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 4 May 2019 10:34:28 +0200 Subject: [PATCH 2/5] archiver: Move tests back into the same file Move all Archiver tests back into `archiver_test.go` and add some tiny helpers to mock what `lstat` returns (for Windows and Unix separately). --- internal/archiver/archiver_nowin_test.go | 98 ---------------------- internal/archiver/archiver_test.go | 74 +++++++++++++--- internal/archiver/archiver_unix_test.go | 41 +++++++++ internal/archiver/archiver_windows_test.go | 28 +++++++ 4 files changed, 133 insertions(+), 108 deletions(-) delete mode 100644 internal/archiver/archiver_nowin_test.go create mode 100644 internal/archiver/archiver_unix_test.go create mode 100644 internal/archiver/archiver_windows_test.go diff --git a/internal/archiver/archiver_nowin_test.go b/internal/archiver/archiver_nowin_test.go deleted file mode 100644 index a4d6432dc..000000000 --- a/internal/archiver/archiver_nowin_test.go +++ /dev/null @@ -1,98 +0,0 @@ -// +build !windows - -package archiver - -import ( - "context" - "os" - "syscall" - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/restic/restic/internal/checker" - "github.com/restic/restic/internal/fs" - "github.com/restic/restic/internal/restic" -) - -func TestMetadataChanged(t *testing.T) { - files := TestDir{ - "testfile": TestFile{ - Content: "foo bar test file", - }, - } - - tempdir, repo, cleanup := prepareTempdirRepoSrc(t, files) - defer cleanup() - - back := fs.TestChdir(t, tempdir) - defer back() - - // get metadata - fi := lstat(t, "testfile") - want, err := restic.NodeFromFileInfo("testfile", fi) - if err != nil { - t.Fatal(err) - } - - fs := &StatFS{ - FS: fs.Local{}, - OverrideLstat: map[string]os.FileInfo{ - "testfile": fi, - }, - } - - snapshotID, node2 := snapshot(t, repo, fs, restic.ID{}, "testfile") - - // set some values so we can then compare the nodes - want.Content = node2.Content - want.Path = "" - want.ExtendedAttributes = nil - - // make sure that metadata was recorded successfully - if !cmp.Equal(want, node2) { - t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node2)) - } - - // modify the mode - stat, ok := fi.Sys().(*syscall.Stat_t) - if ok { - // change a few values - stat.Mode = 0400 - stat.Uid = 1234 - stat.Gid = 1235 - - // wrap the os.FileInfo so we can return a modified stat_t - fi = wrappedFileInfo{ - FileInfo: fi, - sys: stat, - mode: 0400, - } - fs.OverrideLstat["testfile"] = fi - } else { - // skip the test on this platform - t.Skipf("unable to modify os.FileInfo, Sys() returned %T", fi.Sys()) - } - - want, err = restic.NodeFromFileInfo("testfile", fi) - if err != nil { - t.Fatal(err) - } - - // make another snapshot - snapshotID, node3 := snapshot(t, repo, fs, snapshotID, "testfile") - - // set some values so we can then compare the nodes - want.Content = node2.Content - want.Path = "" - want.ExtendedAttributes = nil - - // make sure that metadata was recorded successfully - if !cmp.Equal(want, node3) { - t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node2)) - } - - // make sure the content matches - TestEnsureFileContent(context.Background(), t, repo, "testfile", node3, files["testfile"].(TestFile)) - - checker.TestCheckRepo(t, repo) -} diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index aff4f10f3..531978812 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -14,6 +14,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/restic/restic/internal/checker" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/fs" @@ -2012,16 +2013,69 @@ func (f fileStat) Stat() (os.FileInfo, error) { return f.fi, nil } -type wrappedFileInfo struct { - os.FileInfo - sys interface{} - mode os.FileMode -} +func TestMetadataChanged(t *testing.T) { + files := TestDir{ + "testfile": TestFile{ + Content: "foo bar test file", + }, + } -func (fi wrappedFileInfo) Sys() interface{} { - return fi.sys -} + tempdir, repo, cleanup := prepareTempdirRepoSrc(t, files) + defer cleanup() -func (fi wrappedFileInfo) Mode() os.FileMode { - return fi.mode + back := fs.TestChdir(t, tempdir) + defer back() + + // get metadata + fi := lstat(t, "testfile") + want, err := restic.NodeFromFileInfo("testfile", fi) + if err != nil { + t.Fatal(err) + } + + fs := &StatFS{ + FS: fs.Local{}, + OverrideLstat: map[string]os.FileInfo{ + "testfile": fi, + }, + } + + snapshotID, node2 := snapshot(t, repo, fs, restic.ID{}, "testfile") + + // set some values so we can then compare the nodes + want.Content = node2.Content + want.Path = "" + want.ExtendedAttributes = nil + + // make sure that metadata was recorded successfully + if !cmp.Equal(want, node2) { + t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node2)) + } + + // modify the mode by wrapping it in a new struct + fs.OverrideLstat["testfile"] = wrapFileInfo(t, fi, 0400, 51234, 51235) + + // set the override values in the 'want' node which + want.Mode = 0400 + // ignore UID and GID on Windows + if runtime.GOOS != "windows" { + want.UID = 51234 + want.GID = 51235 + } + // no user and group name + want.User = "" + want.Group = "" + + // make another snapshot + snapshotID, node3 := snapshot(t, repo, fs, snapshotID, "testfile") + + // make sure that metadata was recorded successfully + if !cmp.Equal(want, node3) { + t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node3)) + } + + // make sure the content matches + TestEnsureFileContent(context.Background(), t, repo, "testfile", node3, files["testfile"].(TestFile)) + + checker.TestCheckRepo(t, repo) } diff --git a/internal/archiver/archiver_unix_test.go b/internal/archiver/archiver_unix_test.go new file mode 100644 index 000000000..4af972577 --- /dev/null +++ b/internal/archiver/archiver_unix_test.go @@ -0,0 +1,41 @@ +// +build !windows + +package archiver + +import ( + "os" + "syscall" + "testing" +) + +type wrappedFileInfo struct { + os.FileInfo + sys interface{} + mode os.FileMode +} + +func (fi wrappedFileInfo) Sys() interface{} { + return fi.sys +} + +func (fi wrappedFileInfo) Mode() os.FileMode { + return fi.mode +} + +// wrapFileInfo returns a new os.FileInfo with the mode, owner, and group fields changed. +func wrapFileInfo(t testing.TB, fi os.FileInfo, mode os.FileMode, uid, gid uint) os.FileInfo { + // get the underlying stat_t and modify the values + stat := fi.Sys().(*syscall.Stat_t) + stat.Mode = uint32(mode) + stat.Uid = uint32(uid) + stat.Gid = uint32(gid) + + // wrap the os.FileInfo so we can return a modified stat_t + res := wrappedFileInfo{ + FileInfo: fi, + sys: stat, + mode: mode, + } + + return res +} diff --git a/internal/archiver/archiver_windows_test.go b/internal/archiver/archiver_windows_test.go new file mode 100644 index 000000000..6a0100f49 --- /dev/null +++ b/internal/archiver/archiver_windows_test.go @@ -0,0 +1,28 @@ +// +build windows + +package archiver + +import ( + "os" + "testing" +) + +type wrappedFileInfo struct { + os.FileInfo + mode os.FileMode +} + +func (fi wrappedFileInfo) Mode() os.FileMode { + return fi.mode +} + +// wrapFileInfo returns a new os.FileInfo with the mode, owner, and group fields changed. +func wrapFileInfo(t testing.TB, fi os.FileInfo, mode os.FileMode, uid, gid uint) os.FileInfo { + // wrap the os.FileInfo and return the modified mode, uid and gid are ignored on Windows + res := wrappedFileInfo{ + FileInfo: fi, + mode: mode, + } + + return res +} From 355db0bc2924a6c9a7a771defe673ad312e2afa1 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 5 May 2019 12:45:56 +0200 Subject: [PATCH 3/5] windows: Use LastWriteTime for ctime and mtime Windows does not have a concept of a `change time` in the sense as Unix has it: the field `CreationTime` of the `Win32FileAttributeData` struct is not updated when attributes or content is changed. So from now on we're using the `LastWriteTime` as the `change time` on Windows. --- internal/restic/node_windows.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/restic/node_windows.go b/internal/restic/node_windows.go index aa0dee4b2..28ebff6ae 100644 --- a/internal/restic/node_windows.go +++ b/internal/restic/node_windows.go @@ -76,5 +76,6 @@ func (s statWin) mtim() syscall.Timespec { } func (s statWin) ctim() syscall.Timespec { - return syscall.NsecToTimespec(s.CreationTime.Nanoseconds()) + // Windows does not have the concept of a "change time" in the sense Unix uses it, so we're using the LastWriteTime here. + return syscall.NsecToTimespec(s.LastWriteTime.Nanoseconds()) } From b016dc2ff01d41c56d28c8eee77305fa49b77fd7 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 5 May 2019 12:51:26 +0200 Subject: [PATCH 4/5] archiver/Windows: Skip test new-content-same-filestamp --- internal/archiver/archiver_test.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 531978812..ce6bb7fa7 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -557,11 +557,12 @@ func TestFileChanged(t *testing.T) { } var tests = []struct { - Name string - Content []byte - Modify func(t testing.TB, filename string) - IgnoreInode bool - SameFile bool + Name string + SkipForWindows bool + Content []byte + Modify func(t testing.TB, filename string) + IgnoreInode bool + SameFile bool }{ { Name: "same-content-new-file", @@ -580,6 +581,10 @@ func TestFileChanged(t *testing.T) { }, { Name: "new-content-same-timestamp", + // on Windows, there's no "create time" field users cannot modify, + // so we're unable to detect if a file has been modified when the + // timestamps are reset, so we skip this test for Windows + SkipForWindows: true, Modify: func(t testing.TB, filename string) { fi, err := os.Stat(filename) if err != nil { @@ -629,6 +634,10 @@ func TestFileChanged(t *testing.T) { for _, test := range tests { t.Run(test.Name, func(t *testing.T) { + if runtime.GOOS == "windows" && test.SkipForWindows { + t.Skip("don't run test on Windows") + } + tempdir, cleanup := restictest.TempDir(t) defer cleanup() From 920d458a4a8d6cf39b856ce3145177663ed7cf0e Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 5 May 2019 14:57:38 +0200 Subject: [PATCH 5/5] archiver: Use untyped constants for testing FileInfo --- internal/archiver/archiver_test.go | 12 ++++++++++-- internal/archiver/archiver_unix_test.go | 10 +++++----- internal/archiver/archiver_windows_test.go | 4 ++-- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index ce6bb7fa7..3335d3cbb 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -2022,6 +2022,14 @@ func (f fileStat) Stat() (os.FileInfo, error) { return f.fi, nil } +// used by wrapFileInfo, use untyped const in order to avoid having a version +// of wrapFileInfo for each OS +const ( + mockFileInfoMode = 0400 + mockFileInfoUID = 51234 + mockFileInfoGID = 51235 +) + func TestMetadataChanged(t *testing.T) { files := TestDir{ "testfile": TestFile{ @@ -2061,8 +2069,8 @@ func TestMetadataChanged(t *testing.T) { t.Fatalf("metadata does not match:\n%v", cmp.Diff(want, node2)) } - // modify the mode by wrapping it in a new struct - fs.OverrideLstat["testfile"] = wrapFileInfo(t, fi, 0400, 51234, 51235) + // modify the mode by wrapping it in a new struct, uses the consts defined above + fs.OverrideLstat["testfile"] = wrapFileInfo(t, fi) // set the override values in the 'want' node which want.Mode = 0400 diff --git a/internal/archiver/archiver_unix_test.go b/internal/archiver/archiver_unix_test.go index 4af972577..f7e827e7e 100644 --- a/internal/archiver/archiver_unix_test.go +++ b/internal/archiver/archiver_unix_test.go @@ -23,18 +23,18 @@ func (fi wrappedFileInfo) Mode() os.FileMode { } // wrapFileInfo returns a new os.FileInfo with the mode, owner, and group fields changed. -func wrapFileInfo(t testing.TB, fi os.FileInfo, mode os.FileMode, uid, gid uint) os.FileInfo { +func wrapFileInfo(t testing.TB, fi os.FileInfo) os.FileInfo { // get the underlying stat_t and modify the values stat := fi.Sys().(*syscall.Stat_t) - stat.Mode = uint32(mode) - stat.Uid = uint32(uid) - stat.Gid = uint32(gid) + stat.Mode = mockFileInfoMode + stat.Uid = mockFileInfoUID + stat.Gid = mockFileInfoGID // wrap the os.FileInfo so we can return a modified stat_t res := wrappedFileInfo{ FileInfo: fi, sys: stat, - mode: mode, + mode: mockFileInfoMode, } return res diff --git a/internal/archiver/archiver_windows_test.go b/internal/archiver/archiver_windows_test.go index 6a0100f49..9b3d77898 100644 --- a/internal/archiver/archiver_windows_test.go +++ b/internal/archiver/archiver_windows_test.go @@ -17,11 +17,11 @@ func (fi wrappedFileInfo) Mode() os.FileMode { } // wrapFileInfo returns a new os.FileInfo with the mode, owner, and group fields changed. -func wrapFileInfo(t testing.TB, fi os.FileInfo, mode os.FileMode, uid, gid uint) os.FileInfo { +func wrapFileInfo(t testing.TB, fi os.FileInfo) os.FileInfo { // wrap the os.FileInfo and return the modified mode, uid and gid are ignored on Windows res := wrappedFileInfo{ FileInfo: fi, - mode: mode, + mode: mockFileInfoMode, } return res