all: Grand test refactor (fixes #8779, fixes #8799)

This fixes various test issues with Go 1.20.

- Most tests rewritten to use fakefs where possible
- Some tests that were already skipped, or dubious (invasive,
  unmaintainable, unclear what they even tested) have been removed
- Some actual code rewritten to better support testing in fakefs

Co-authored-by: Eric P <eric@kastelo.net>

lib/fs/debug_symlink_unix.go

@@ -1,33 +0,0 @@
-// Copyright (C) 2017 The Syncthing Authors.
-//
-// This Source Code Form is subject to the terms of the Mozilla Public
-// License, v. 2.0. If a copy of the MPL was not distributed with this file,
-// You can obtain one at https://mozilla.org/MPL/2.0/.
-
-//go:build !windows
-// +build !windows
-
-package fs
-
-import (
-	"os"
-	"path/filepath"
-)
-
-// DebugSymlinkForTestsOnly is not and should not be used in Syncthing code,
-// hence the cumbersome name to make it obvious if this ever leaks. Its
-// reason for existence is the Windows version, which allows creating
-// symlinks when non-elevated.
-func DebugSymlinkForTestsOnly(oldFs, newFs Filesystem, oldname, newname string) error {
-	if fs, ok := unwrapFilesystem(newFs, filesystemWrapperTypeCase); ok {
-		caseFs := fs.(*caseFilesystem)
-		if err := caseFs.checkCase(newname); err != nil {
-			return err
-		}
-		caseFs.dropCache()
-	}
-	if err := os.Symlink(filepath.Join(oldFs.URI(), oldname), filepath.Join(newFs.URI(), newname)); err != nil {
-		return err
-	}
-	return nil
-}

lib/fs/debug_symlink_windows.go

@@ -1,140 +0,0 @@
-// Copyright 2009 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package fs
-
-import (
-	"os"
-	"path/filepath"
-	"syscall"
-)
-
-// DebugSymlinkForTestsOnly is os.Symlink taken from the 1.9.2 stdlib,
-// hacked with the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag to
-// create symlinks when not elevated.
-//
-// This is not and should not be used in Syncthing code, hence the
-// cumbersome name to make it obvious if this ever leaks. Nonetheless it's
-// useful in tests.
-func DebugSymlinkForTestsOnly(oldFs, newFS Filesystem, oldname, newname string) error {
-	oldname = filepath.Join(oldFs.URI(), oldname)
-	newname = filepath.Join(newFS.URI(), newname)
-
-	// CreateSymbolicLink is not supported before Windows Vista
-	if syscall.LoadCreateSymbolicLink() != nil {
-		return &os.LinkError{"symlink", oldname, newname, syscall.EWINDOWS}
-	}
-
-	// '/' does not work in link's content
-	oldname = filepath.FromSlash(oldname)
-
-	// need the exact location of the oldname when it's relative to determine if it's a directory
-	destpath := oldname
-	if !filepath.IsAbs(oldname) {
-		destpath = filepath.Dir(newname) + `\` + oldname
-	}
-
-	fi, err := os.Lstat(destpath)
-	isdir := err == nil && fi.IsDir()
-
-	n, err := syscall.UTF16PtrFromString(fixLongPath(newname))
-	if err != nil {
-		return &os.LinkError{"symlink", oldname, newname, err}
-	}
-	o, err := syscall.UTF16PtrFromString(fixLongPath(oldname))
-	if err != nil {
-		return &os.LinkError{"symlink", oldname, newname, err}
-	}
-
-	var flags uint32
-	if isdir {
-		flags |= syscall.SYMBOLIC_LINK_FLAG_DIRECTORY
-	}
-	flags |= 0x02 // SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
-	err = syscall.CreateSymbolicLink(n, o, flags)
-	if err != nil {
-		return &os.LinkError{"symlink", oldname, newname, err}
-	}
-	return nil
-}
-
-// fixLongPath returns the extended-length (\\?\-prefixed) form of
-// path when needed, in order to avoid the default 260 character file
-// path limit imposed by Windows. If path is not easily converted to
-// the extended-length form (for example, if path is a relative path
-// or contains .. elements), or is short enough, fixLongPath returns
-// path unmodified.
-//
-// See https://docs.microsoft.com/windows/win32/fileio/naming-a-file#maximum-path-length-limitation
-func fixLongPath(path string) string {
-	// Do nothing (and don't allocate) if the path is "short".
-	// Empirically (at least on the Windows Server 2013 builder),
-	// the kernel is arbitrarily okay with < 248 bytes. That
-	// matches what the docs above say:
-	// "When using an API to create a directory, the specified
-	// path cannot be so long that you cannot append an 8.3 file
-	// name (that is, the directory name cannot exceed MAX_PATH
-	// minus 12)." Since MAX_PATH is 260, 260 - 12 = 248.
-	//
-	// The MS docs appear to say that a normal path that is 248 bytes long
-	// will work; empirically the path must be less than 248 bytes long.
-	if len(path) < 248 {
-		// Don't fix. (This is how Go 1.7 and earlier worked,
-		// not automatically generating the \\?\ form)
-		return path
-	}
-
-	// The extended form begins with \\?\, as in
-	// \\?\c:\windows\foo.txt or \\?\UNC\server\share\foo.txt.
-	// The extended form disables evaluation of . and .. path
-	// elements and disables the interpretation of / as equivalent
-	// to \. The conversion here rewrites / to \ and elides
-	// . elements as well as trailing or duplicate separators. For
-	// simplicity it avoids the conversion entirely for relative
-	// paths or paths containing .. elements. For now,
-	// \\server\share paths are not converted to
-	// \\?\UNC\server\share paths because the rules for doing so
-	// are less well-specified.
-	if len(path) >= 2 && path[:2] == `\\` {
-		// Don't canonicalize UNC paths.
-		return path
-	}
-	if !filepath.IsAbs(path) {
-		// Relative path
-		return path
-	}
-
-	const prefix = `\\?`
-
-	pathbuf := make([]byte, len(prefix)+len(path)+len(`\`))
-	copy(pathbuf, prefix)
-	n := len(path)
-	r, w := 0, len(prefix)
-	for r < n {
-		switch {
-		case os.IsPathSeparator(path[r]):
-			// empty block
-			r++
-		case path[r] == '.' && (r+1 == n || os.IsPathSeparator(path[r+1])):
-			// /./
-			r++
-		case r+1 < n && path[r] == '.' && path[r+1] == '.' && (r+2 == n || os.IsPathSeparator(path[r+2])):
-			// /../ is currently unhandled
-			return path
-		default:
-			pathbuf[w] = '\\'
-			w++
-			for ; r < n && !os.IsPathSeparator(path[r]); r++ {
-				pathbuf[w] = path[r]
-				w++
-			}
-		}
-	}
-	// A drive's root directory needs a trailing \
-	if w == len(`\\?\c:`) {
-		pathbuf[w] = '\\'
-		w++
-	}
-	return string(pathbuf[:w])
-}

lib/fs/fakefs.go

@@ -52,6 +52,8 @@ const randomBlockShift = 14 // 128k
 //     seed=n     to set the initial random seed (default 0)
 //     insens=b   "true" makes filesystem case-insensitive Windows- or OSX-style (default false)
 //     latency=d  to set the amount of time each "disk" operation takes, where d is time.ParseDuration format
+//     content=true to save actual file contents instead of generating pseudorandomly; n.b. memory usage
+//     nostfolder=true skip the creation of .stfolder
 //
 // - Two fakeFS:s pointing at the same root path see the same files.
 type fakeFS struct {
@@ -108,7 +110,7 @@ func newFakeFilesystem(rootURI string, _ ...Option) *fakeFS {
 		root: &fakeEntry{
 			name:      "/",
 			entryType: fakeEntryTypeDir,
-			mode:      0700,
+			mode:      0o700,
 			mtime:     time.Now(),
 			children:  make(map[string]*fakeEntry),
 		},
@@ -123,6 +125,7 @@ func newFakeFilesystem(rootURI string, _ ...Option) *fakeFS {
 
 	fs.insens = params.Get("insens") == "true"
 	fs.withContent = params.Get("content") == "true"
+	nostfolder := params.Get("nostfolder") == "true"
 
 	if sizeavg == 0 {
 		sizeavg = 1 << 20
@@ -139,7 +142,7 @@ func newFakeFilesystem(rootURI string, _ ...Option) *fakeFS {
 		for (files == 0 || createdFiles < files) && (maxsize == 0 || writtenData>>20 < int64(maxsize)) {
 			dir := filepath.Join(fmt.Sprintf("%02x", rng.Intn(255)), fmt.Sprintf("%02x", rng.Intn(255)))
 			file := fmt.Sprintf("%016x", rng.Int63())
-			fs.MkdirAll(dir, 0755)
+			fs.MkdirAll(dir, 0o755)
 
 			fd, _ := fs.Create(filepath.Join(dir, file))
 			createdFiles++
@@ -153,8 +156,10 @@ func newFakeFilesystem(rootURI string, _ ...Option) *fakeFS {
 		}
 	}
 
-	// Also create a default folder marker for good measure
-	fs.Mkdir(".stfolder", 0700)
+	if !nostfolder {
+		// Also create a default folder marker for good measure
+		fs.Mkdir(".stfolder", 0o700)
+	}
 
 	// We only set the latency after doing the operations required to create
 	// the filesystem initially.
@@ -187,7 +192,6 @@ type fakeEntry struct {
 }
 
 func (fs *fakeFS) entryForName(name string) *fakeEntry {
-	// bug: lookup doesn't work through symlinks.
 	if fs.insens {
 		name = UnicodeLowercaseNormalized(name)
 	}
@@ -200,7 +204,7 @@ func (fs *fakeFS) entryForName(name string) *fakeEntry {
 	name = strings.Trim(name, "/")
 	comps := strings.Split(name, "/")
 	entry := fs.root
-	for _, comp := range comps {
+	for i, comp := range comps {
 		if entry.entryType != fakeEntryTypeDir {
 			return nil
 		}
@@ -209,6 +213,12 @@ func (fs *fakeFS) entryForName(name string) *fakeEntry {
 		if !ok {
 			return nil
 		}
+		if i < len(comps)-1 && entry.entryType == fakeEntryTypeSymlink {
+			// only absolute link targets are supported, and we assume
+			// lookup is Lstat-kind so we only resolve symlinks when they
+			// are not the last path component.
+			return fs.entryForName(entry.dest)
+		}
 	}
 	return entry
 }
@@ -267,7 +277,7 @@ func (fs *fakeFS) create(name string) (*fakeEntry, error) {
 		}
 		entry.size = 0
 		entry.mtime = time.Now()
-		entry.mode = 0666
+		entry.mode = 0o666
 		entry.content = nil
 		if fs.withContent {
 			entry.content = make([]byte, 0)
@@ -283,7 +293,7 @@ func (fs *fakeFS) create(name string) (*fakeEntry, error) {
 	}
 	new := &fakeEntry{
 		name:  base,
-		mode:  0666,
+		mode:  0o666,
 		mtime: time.Now(),
 	}
 
@@ -305,9 +315,9 @@ func (fs *fakeFS) Create(name string) (File, error) {
 		return nil, err
 	}
 	if fs.insens {
-		return &fakeFile{fakeEntry: entry, presentedName: filepath.Base(name)}, nil
+		return &fakeFile{fakeEntry: entry, presentedName: filepath.Base(name), mut: &fs.mut}, nil
 	}
-	return &fakeFile{fakeEntry: entry}, nil
+	return &fakeFile{fakeEntry: entry, mut: &fs.mut}, nil
 }
 
 func (fs *fakeFS) CreateSymlink(target, name string) error {
@@ -441,9 +451,9 @@ func (fs *fakeFS) Open(name string) (File, error) {
 	}
 
 	if fs.insens {
-		return &fakeFile{fakeEntry: entry, presentedName: filepath.Base(name)}, nil
+		return &fakeFile{fakeEntry: entry, presentedName: filepath.Base(name), mut: &fs.mut}, nil
 	}
-	return &fakeFile{fakeEntry: entry}, nil
+	return &fakeFile{fakeEntry: entry, mut: &fs.mut}, nil
 }
 
 func (fs *fakeFS) OpenFile(name string, flags int, mode FileMode) (File, error) {
@@ -486,7 +496,7 @@ func (fs *fakeFS) OpenFile(name string, flags int, mode FileMode) (File, error)
 	}
 
 	entry.children[key] = newEntry
-	return &fakeFile{fakeEntry: newEntry}, nil
+	return &fakeFile{fakeEntry: newEntry, mut: &fs.mut}, nil
 }
 
 func (fs *fakeFS) ReadSymlink(name string) (string, error) {
@@ -630,9 +640,31 @@ func (*fakeFS) SetXattr(_ string, _ []protocol.Xattr, _ XattrFilter) error {
 	return nil
 }
 
-func (*fakeFS) Glob(_ string) ([]string, error) {
-	// gnnh we don't seem to actually require this in practice
-	return nil, errors.New("not implemented")
+// A basic glob-impelementation that should be able to handle
+// simple test cases.
+func (fs *fakeFS) Glob(pattern string) ([]string, error) {
+	dir := filepath.Dir(pattern)
+	file := filepath.Base(pattern)
+	if _, err := fs.Lstat(dir); err != nil {
+		return nil, errPathInvalid
+	}
+
+	var matches []string
+	names, err := fs.DirNames(dir)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, n := range names {
+		matched, err := filepath.Match(file, n)
+		if err != nil {
+			return nil, err
+		}
+		if matched {
+			matches = append(matches, filepath.Join(dir, n))
+		}
+	}
+	return matches, err
 }
 
 func (*fakeFS) Roots() ([]string, error) {
@@ -703,7 +735,7 @@ func (fs *fakeFS) reportMetricsPer(b *testing.B, divisor float64, unit string) {
 // opened for reading or writing, it's all good.
 type fakeFile struct {
 	*fakeEntry
-	mut           sync.Mutex
+	mut           *sync.Mutex
 	rng           io.Reader
 	seed          int64
 	offset        int64

lib/fs/filesystem.go

@@ -172,21 +172,23 @@ var (
 
 // Equivalents from os package.
 
-const ModePerm = FileMode(os.ModePerm)
-const ModeSetgid = FileMode(os.ModeSetgid)
-const ModeSetuid = FileMode(os.ModeSetuid)
-const ModeSticky = FileMode(os.ModeSticky)
-const ModeSymlink = FileMode(os.ModeSymlink)
-const ModeType = FileMode(os.ModeType)
-const PathSeparator = os.PathSeparator
-const OptAppend = os.O_APPEND
-const OptCreate = os.O_CREATE
-const OptExclusive = os.O_EXCL
-const OptReadOnly = os.O_RDONLY
-const OptReadWrite = os.O_RDWR
-const OptSync = os.O_SYNC
-const OptTruncate = os.O_TRUNC
-const OptWriteOnly = os.O_WRONLY
+const (
+	ModePerm      = FileMode(os.ModePerm)
+	ModeSetgid    = FileMode(os.ModeSetgid)
+	ModeSetuid    = FileMode(os.ModeSetuid)
+	ModeSticky    = FileMode(os.ModeSticky)
+	ModeSymlink   = FileMode(os.ModeSymlink)
+	ModeType      = FileMode(os.ModeType)
+	PathSeparator = os.PathSeparator
+	OptAppend     = os.O_APPEND
+	OptCreate     = os.O_CREATE
+	OptExclusive  = os.O_EXCL
+	OptReadOnly   = os.O_RDONLY
+	OptReadWrite  = os.O_RDWR
+	OptSync       = os.O_SYNC
+	OptTruncate   = os.O_TRUNC
+	OptWriteOnly  = os.O_WRONLY
+)
 
 // SkipDir is used as a return value from WalkFuncs to indicate that
 // the directory named in the call is to be skipped. It is not returned
@@ -354,3 +356,20 @@ func unwrapFilesystem(fs Filesystem, wrapperType filesystemWrapperType) (Filesys
 		}
 	}
 }
+
+// WriteFile writes data to the named file, creating it if necessary.
+// If the file does not exist, WriteFile creates it with permissions perm (before umask);
+// otherwise WriteFile truncates it before writing, without changing permissions.
+// Since Writefile requires multiple system calls to complete, a failure mid-operation
+// can leave the file in a partially written state.
+func WriteFile(fs Filesystem, name string, data []byte, perm FileMode) error {
+	f, err := fs.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm)
+	if err != nil {
+		return err
+	}
+	_, err = f.Write(data)
+	if err1 := f.Close(); err1 != nil && err == nil {
+		err = err1
+	}
+	return err
+}

lib/fs/mtimefs_test.go

@@ -17,17 +17,16 @@ import (
 )
 
 func TestMtimeFS(t *testing.T) {
-	os.RemoveAll("testdata")
-	defer os.RemoveAll("testdata")
-	os.Mkdir("testdata", 0755)
-	os.WriteFile("testdata/exists0", []byte("hello"), 0644)
-	os.WriteFile("testdata/exists1", []byte("hello"), 0644)
-	os.WriteFile("testdata/exists2", []byte("hello"), 0644)
+	td := t.TempDir()
+	os.Mkdir(filepath.Join(td, "testdata"), 0o755)
+	os.WriteFile(filepath.Join(td, "testdata", "exists0"), []byte("hello"), 0o644)
+	os.WriteFile(filepath.Join(td, "testdata", "exists1"), []byte("hello"), 0o644)
+	os.WriteFile(filepath.Join(td, "testdata", "exists2"), []byte("hello"), 0o644)
 
 	// a random time with nanosecond precision
 	testTime := time.Unix(1234567890, 123456789)
 
-	mtimefs := newMtimeFS(".", make(mapStore))
+	mtimefs := newMtimeFS(td, make(mapStore))
 
 	// Do one Chtimes call that will go through to the normal filesystem
 	mtimefs.chtimes = os.Chtimes
@@ -62,7 +61,7 @@ func TestMtimeFS(t *testing.T) {
 	// when looking directly on disk though.
 
 	for _, file := range []string{"testdata/exists1", "testdata/exists2"} {
-		if info, err := os.Lstat(file); err != nil {
+		if info, err := os.Lstat(filepath.Join(td, file)); err != nil {
 			t.Error("Lstat shouldn't fail:", err)
 		} else if info.ModTime().Equal(testTime) {
 			t.Errorf("Unexpected time match; %v == %v", info.ModTime(), testTime)
@@ -74,7 +73,7 @@ func TestMtimeFS(t *testing.T) {
 	// filesystems.
 
 	testTime = time.Now().Add(5 * time.Hour).Truncate(time.Minute)
-	os.Chtimes("testdata/exists0", testTime, testTime)
+	os.Chtimes(filepath.Join(td, "testdata/exists0"), testTime, testTime)
 	if info, err := mtimefs.Lstat("testdata/exists0"); err != nil {
 		t.Error("Lstat shouldn't fail:", err)
 	} else if !info.ModTime().Equal(testTime) {
@@ -89,7 +88,7 @@ func TestMtimeFSWalk(t *testing.T) {
 	underlying := mtimefs.Filesystem
 	mtimefs.chtimes = failChtimes
 
-	if err := os.WriteFile(filepath.Join(dir, "file"), []byte("hello"), 0644); err != nil {
+	if err := os.WriteFile(filepath.Join(dir, "file"), []byte("hello"), 0o644); err != nil {
 		t.Fatal(err)
 	}
 
@@ -139,7 +138,7 @@ func TestMtimeFSOpen(t *testing.T) {
 	underlying := mtimefs.Filesystem
 	mtimefs.chtimes = failChtimes
 
-	if err := os.WriteFile(filepath.Join(dir, "file"), []byte("hello"), 0644); err != nil {
+	if err := os.WriteFile(filepath.Join(dir, "file"), []byte("hello"), 0o644); err != nil {
 		t.Fatal(err)
 	}
 
@@ -190,10 +189,10 @@ func TestMtimeFSInsensitive(t *testing.T) {
 	}
 
 	theTest := func(t *testing.T, fs *mtimeFS, shouldSucceed bool) {
-		os.RemoveAll("testdata")
-		defer os.RemoveAll("testdata")
-		os.Mkdir("testdata", 0755)
-		os.WriteFile("testdata/FiLe", []byte("hello"), 0644)
+		fs.RemoveAll("testdata")
+		defer fs.RemoveAll("testdata")
+		fs.Mkdir("testdata", 0o755)
+		WriteFile(fs, "testdata/FiLe", []byte("hello"), 0o644)
 
 		// a random time with nanosecond precision
 		testTime := time.Unix(1234567890, 123456789)
@@ -216,12 +215,12 @@ func TestMtimeFSInsensitive(t *testing.T) {
 
 	// The test should fail with a case sensitive mtimefs
 	t.Run("with case sensitive mtimefs", func(t *testing.T) {
-		theTest(t, newMtimeFS(".", make(mapStore)), false)
+		theTest(t, newMtimeFS(t.TempDir(), make(mapStore)), false)
 	})
 
 	// And succeed with a case insensitive one.
 	t.Run("with case insensitive mtimefs", func(t *testing.T) {
-		theTest(t, newMtimeFS(".", make(mapStore), WithCaseInsensitivity(true)), true)
+		theTest(t, newMtimeFS(t.TempDir(), make(mapStore), WithCaseInsensitivity(true)), true)
 	})
 }