diff options
author | Jakob Borg <jakob@kastelo.net> | 2023-05-03 10:25:36 +0200 |
---|---|---|
committer | Jakob Borg <jakob@kastelo.net> | 2023-05-09 10:01:57 +0000 |
commit | 1103a27337ba97ba17e7ec77bd002fb4061c5d24 (patch) | |
tree | bd2b1d47023e90b9cc64bc784928d2de336dbd11 | |
parent | ddce692f72af19606b4a714fb6981457368a5727 (diff) |
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>
54 files changed, 1127 insertions, 1588 deletions
diff --git a/.github/workflows/build-syncthing.yaml b/.github/workflows/build-syncthing.yaml index 966e23cd5c..7b8cc91bb7 100644 --- a/.github/workflows/build-syncthing.yaml +++ b/.github/workflows/build-syncthing.yaml @@ -6,7 +6,7 @@ on: env: # The go version to use for builds. - GO_VERSION: "1.19.6" + GO_VERSION: "^1.20.3" # Optimize compatibility on the slow archictures. GO386: softfloat @@ -42,7 +42,7 @@ jobs: runner: ["windows-latest", "ubuntu-latest", "macos-latest"] # The oldest version in this list should match what we have in our go.mod. # Variables don't seem to be supported here, or we could have done something nice. - go: ["1.19"] # Skip Go 1.20 for now, https://github.com/syncthing/syncthing/issues/8799 + go: ["1.19", "1.20"] runs-on: ${{ matrix.runner }} steps: - name: Set git to use LF diff --git a/lib/api/api.go b/lib/api/api.go index 6f9451dcc5..5e41badb6a 100644 --- a/lib/api/api.go +++ b/lib/api/api.go @@ -775,9 +775,9 @@ func (s *service) getDBBrowse(w http.ResponseWriter, r *http.Request) { } func (s *service) getDBCompletion(w http.ResponseWriter, r *http.Request) { - var qs = r.URL.Query() - var folder = qs.Get("folder") // empty means all folders - var deviceStr = qs.Get("device") // empty means local device ID + qs := r.URL.Query() + folder := qs.Get("folder") // empty means all folders + deviceStr := qs.Get("device") // empty means local device ID // We will check completion status for either the local device, or a // specific given device ID. @@ -814,14 +814,14 @@ func (s *service) getDBStatus(w http.ResponseWriter, r *http.Request) { } func (s *service) postDBOverride(_ http.ResponseWriter, r *http.Request) { - var qs = r.URL.Query() - var folder = qs.Get("folder") + qs := r.URL.Query() + folder := qs.Get("folder") go s.model.Override(folder) } func (s *service) postDBRevert(_ http.ResponseWriter, r *http.Request) { - var qs = r.URL.Query() - var folder = qs.Get("folder") + qs := r.URL.Query() + folder := qs.Get("folder") go s.model.Revert(folder) } @@ -1015,7 +1015,7 @@ func (s *service) postSystemRestart(w http.ResponseWriter, _ *http.Request) { } func (s *service) postSystemReset(w http.ResponseWriter, r *http.Request) { - var qs = r.URL.Query() + qs := r.URL.Query() folder := qs.Get("folder") if len(folder) > 0 { @@ -1210,7 +1210,6 @@ func (s *service) getSupportBundle(w http.ResponseWriter, r *http.Request) { l.Warnln("Support bundle: failed to serialize usage-reporting.json.txt", err) } else { files = append(files, fileEntry{name: "usage-reporting.json.txt", data: usageReportingData}) - } } @@ -1243,7 +1242,7 @@ func (s *service) getSupportBundle(w http.ResponseWriter, r *http.Request) { zipFilePath := filepath.Join(locations.GetBaseDir(locations.ConfigBaseDir), zipFileName) // Write buffer zip to local zip file (back up) - if err := os.WriteFile(zipFilePath, zipFilesBuffer.Bytes(), 0600); err != nil { + if err := os.WriteFile(zipFilePath, zipFilesBuffer.Bytes(), 0o600); err != nil { l.Warnln("Support bundle: support bundle zip could not be created:", err) } @@ -1299,7 +1298,6 @@ func (s *service) getReport(w http.ResponseWriter, r *http.Request) { } else { sendJSON(w, r) } - } func (*service) getRandomString(w http.ResponseWriter, r *http.Request) { @@ -1497,8 +1495,8 @@ func (s *service) postSystemUpgrade(w http.ResponseWriter, _ *http.Request) { func (s *service) makeDevicePauseHandler(paused bool) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - var qs = r.URL.Query() - var deviceStr = qs.Get("device") + qs := r.URL.Query() + deviceStr := qs.Get("device") var msg string var status int @@ -1573,8 +1571,8 @@ func (*service) getHealth(w http.ResponseWriter, _ *http.Request) { } func (*service) getQR(w http.ResponseWriter, r *http.Request) { - var qs = r.URL.Query() - var text = qs.Get("text") + qs := r.URL.Query() + text := qs.Get("text") code, err := qr.Encode(text, qr.M) if err != nil { http.Error(w, "Invalid", http.StatusInternalServerError) @@ -1655,7 +1653,6 @@ func (s *service) getFolderErrors(w http.ResponseWriter, r *http.Request) { page, perpage := getPagingParams(qs) errors, err := s.model.FolderErrors(folder) - if err != nil { http.Error(w, err.Error(), http.StatusNotFound) return @@ -1687,7 +1684,21 @@ func (*service) getSystemBrowse(w http.ResponseWriter, r *http.Request) { var fsType fs.FilesystemType fsType.UnmarshalText([]byte(qs.Get("filesystem"))) - sendJSON(w, browseFiles(current, fsType)) + sendJSON(w, browse(fsType, current)) +} + +func browse(fsType fs.FilesystemType, current string) []string { + if current == "" { + return browseRoots(fsType) + } + + parent, base := parentAndBase(current) + ffs := fs.NewFilesystem(fsType, parent) + files := browseFiles(ffs, base) + for i := range files { + files[i] = filepath.Join(parent, files[i]) + } + return files } const ( @@ -1708,14 +1719,18 @@ func checkPrefixMatch(s, prefix string) int { return noMatch } -func browseFiles(current string, fsType fs.FilesystemType) []string { - if current == "" { - filesystem := fs.NewFilesystem(fsType, "") - if roots, err := filesystem.Roots(); err == nil { - return roots - } - return nil +func browseRoots(fsType fs.FilesystemType) []string { + filesystem := fs.NewFilesystem(fsType, "") + if roots, err := filesystem.Roots(); err == nil { + return roots } + + return nil +} + +// parentAndBase returns the parent directory and the remaining base of the +// path. The base may be empty if the path ends with a path separator. +func parentAndBase(current string) (string, string) { search, _ := fs.ExpandTilde(current) pathSeparator := string(fs.PathSeparator) @@ -1731,24 +1746,27 @@ func browseFiles(current string, fsType fs.FilesystemType) []string { searchFile = filepath.Base(search) } - fs := fs.NewFilesystem(fsType, searchDir) + return searchDir, searchFile +} - subdirectories, _ := fs.DirNames(".") +func browseFiles(ffs fs.Filesystem, search string) []string { + subdirectories, _ := ffs.DirNames(".") + pathSeparator := string(fs.PathSeparator) exactMatches := make([]string, 0, len(subdirectories)) caseInsMatches := make([]string, 0, len(subdirectories)) for _, subdirectory := range subdirectories { - info, err := fs.Stat(subdirectory) + info, err := ffs.Stat(subdirectory) if err != nil || !info.IsDir() { continue } - switch checkPrefixMatch(subdirectory, searchFile) { + switch checkPrefixMatch(subdirectory, search) { case matchExact: - exactMatches = append(exactMatches, filepath.Join(searchDir, subdirectory)+pathSeparator) + exactMatches = append(exactMatches, subdirectory+pathSeparator) case matchCaseIns: - caseInsMatches = append(caseInsMatches, filepath.Join(searchDir, subdirectory)+pathSeparator) + caseInsMatches = append(caseInsMatches, subdirectory+pathSeparator) } } diff --git a/lib/api/api_test.go b/lib/api/api_test.go index 7736b9b8c3..3cb01d7892 100644 --- a/lib/api/api_test.go +++ b/lib/api/api_test.go @@ -39,6 +39,7 @@ import ( "github.com/syncthing/syncthing/lib/model" modelmocks "github.com/syncthing/syncthing/lib/model/mocks" "github.com/syncthing/syncthing/lib/protocol" + "github.com/syncthing/syncthing/lib/rand" "github.com/syncthing/syncthing/lib/svcutil" "github.com/syncthing/syncthing/lib/sync" "github.com/syncthing/syncthing/lib/tlsutil" @@ -1168,45 +1169,39 @@ func TestBrowse(t *testing.T) { pathSep := string(os.PathSeparator) - tmpDir := t.TempDir() + ffs := fs.NewFilesystem(fs.FilesystemTypeFake, rand.String(32)+"?nostfolder=true") - if err := os.Mkdir(filepath.Join(tmpDir, "dir"), 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(tmpDir, "file"), []byte("hello"), 0644); err != nil { - t.Fatal(err) - } - if err := os.Mkdir(filepath.Join(tmpDir, "MiXEDCase"), 0755); err != nil { - t.Fatal(err) - } + _ = ffs.Mkdir("dir", 0o755) + _ = fs.WriteFile(ffs, "file", []byte("hello"), 0o644) + _ = ffs.Mkdir("MiXEDCase", 0o755) // We expect completion to return the full path to the completed // directory, with an ending slash. - dirPath := filepath.Join(tmpDir, "dir") + pathSep - mixedCaseDirPath := filepath.Join(tmpDir, "MiXEDCase") + pathSep + dirPath := "dir" + pathSep + mixedCaseDirPath := "MiXEDCase" + pathSep cases := []struct { current string returns []string }{ // The directory without slash is completed to one with slash. - {tmpDir, []string{tmpDir + pathSep}}, + {"dir", []string{"dir" + pathSep}}, // With slash it's completed to its contents. // Dirs are given pathSeps. // Files are not returned. - {tmpDir + pathSep, []string{mixedCaseDirPath, dirPath}}, + {"", []string{mixedCaseDirPath, dirPath}}, // Globbing is automatic based on prefix. - {tmpDir + pathSep + "d", []string{dirPath}}, - {tmpDir + pathSep + "di", []string{dirPath}}, - {tmpDir + pathSep + "dir", []string{dirPath}}, - {tmpDir + pathSep + "f", nil}, - {tmpDir + pathSep + "q", nil}, + {"d", []string{dirPath}}, + {"di", []string{dirPath}}, + {"dir", []string{dirPath}}, + {"f", nil}, + {"q", nil}, // Globbing is case-insensitive - {tmpDir + pathSep + "mixed", []string{mixedCaseDirPath}}, + {"mixed", []string{mixedCaseDirPath}}, } for _, tc := range cases { - ret := browseFiles(tc.current, fs.FilesystemTypeBasic) + ret := browseFiles(ffs, tc.current) if !util.EqualStrings(ret, tc.returns) { t.Errorf("browseFiles(%q) => %q, expected %q", tc.current, ret, tc.returns) } diff --git a/lib/config/config_test.go b/lib/config/config_test.go index a0446e6445..0b667e44d7 100644 --- a/lib/config/config_test.go +++ b/lib/config/config_test.go @@ -27,15 +27,21 @@ import ( "github.com/syncthing/syncthing/lib/events" "github.com/syncthing/syncthing/lib/fs" "github.com/syncthing/syncthing/lib/protocol" + "github.com/syncthing/syncthing/lib/rand" ) var device1, device2, device3, device4 protocol.DeviceID +var testFs fs.Filesystem + func init() { device1, _ = protocol.DeviceIDFromString("AIR6LPZ7K4PTTUXQSMUUCPQ5YWOEDFIIQJUG7772YQXXR5YD6AWQ") device2, _ = protocol.DeviceIDFromString("GYRZZQB-IRNPV4Z-T7TC52W-EQYJ3TT-FDQW6MW-DFLMU42-SSSU6EM-FBK2VAY") device3, _ = protocol.DeviceIDFromString("LGFPDIT-7SKNNJL-VJZA4FC-7QNCRKA-CE753K7-2BW5QDK-2FOZ7FR-FEP57QJ") device4, _ = protocol.DeviceIDFromString("P56IOI7-MZJNU2Y-IQGDREY-DM2MGTI-MGL3BXN-PQ6W5BM-TBBZ4TJ-XZWICQ2") + + testFs = fs.NewFilesystem(fs.FilesystemTypeFake, rand.String(32)+"?content=true") + loadTestFiles() } func TestDefaultValues(t *testing.T) { @@ -144,19 +150,19 @@ func TestDefaultValues(t *testing.T) { func TestDeviceConfig(t *testing.T) { for i := OldestHandledVersion; i <= CurrentVersion; i++ { - cfgFile := fmt.Sprintf("testdata/v%d.xml", i) - if _, err := os.Stat(cfgFile); os.IsNotExist(err) { + cfgFile := fmt.Sprintf("v%d.xml", i) + if _, err := testFs.Stat(cfgFile); os.IsNotExist(err) { continue } - os.RemoveAll(filepath.Join("testdata", DefaultMarkerName)) - wr, wrCancel, err := copyAndLoad(cfgFile, device1) + testFs.RemoveAll(DefaultMarkerName) + wr, wrCancel, err := copyAndLoad(testFs, cfgFile, device1) defer wrCancel() if err != nil { t.Fatal(err) } - _, err = os.Stat(filepath.Join("testdata", DefaultMarkerName)) + _, err = testFs.Stat(DefaultMarkerName) if i < 6 && err != nil { t.Fatal(err) } else if i >= 6 && err == nil { @@ -217,7 +223,7 @@ func TestDeviceConfig(t *testing.T) { t.Errorf("%d: Incorrect version %d != %d", i, cfg.Version, CurrentVersion) } if diff, equal := messagediff.PrettyDiff(expectedFolders, cfg.Folders); !equal { - t.Errorf("%d: Incorrect Folders. Diff:\n%s", i, diff) + t.Errorf("%d: Incorrect Folders. Diff:\n%s\n%v", i, diff, cfg) } if diff, equal := messagediff.PrettyDiff(expectedDevices, cfg.Devices); !equal { t.Errorf("%d: Incorrect Devices. Diff:\n%s", i, diff) @@ -229,10 +235,10 @@ func TestDeviceConfig(t *testing.T) { } func TestNoListenAddresses(t *testing.T) { - cfg, cfgCancel, err := copyAndLoad("testdata/nolistenaddress.xml", device1) + cfg, cfgCancel, err := copyAndLoad(testFs, "nolistenaddress.xml", device1) defer cfgCancel() if err != nil { - t.Error(err) + t.Fatal(err) } expected := []string{""} @@ -292,7 +298,7 @@ func TestOverriddenValues(t *testing.T) { expectedPath := "/media/syncthing" os.Unsetenv("STNOUPGRADE") - cfg, cfgCancel, err := copyAndLoad("testdata/overridenvalues.xml", device1) + cfg, cfgCancel, err := copyAndLoad(testFs, "overridenvalues.xml", device1) defer cfgCancel() if err != nil { t.Error(err) @@ -338,7 +344,7 @@ func TestDeviceAddressesDynamic(t *testing.T) { }, } - cfg, cfgCancel, err := copyAndLoad("testdata/deviceaddressesdynamic.xml", device4) + cfg, cfgCancel, err := copyAndLoad(testFs, "deviceaddressesdynamic.xml", device4) defer cfgCancel() if err != nil { t.Error(err) @@ -384,7 +390,7 @@ func TestDeviceCompression(t *testing.T) { }, } - cfg, cfgCancel, err := copyAndLoad("testdata/devicecompression.xml", device4) + cfg, cfgCancel, err := copyAndLoad(testFs, "devicecompression.xml", device4) defer cfgCancel() if err != nil { t.Error(err) @@ -427,7 +433,7 @@ func TestDeviceAddressesStatic(t *testing.T) { }, } - cfg, cfgCancel, err := copyAndLoad("testdata/deviceaddressesstatic.xml", device4) + cfg, cfgCancel, err := copyAndLoad(testFs, "deviceaddressesstatic.xml", device4) defer cfgCancel() if err != nil { t.Error(err) @@ -440,7 +446,7 @@ func TestDeviceAddressesStatic(t *testing.T) { } func TestVersioningConfig(t *testing.T) { - cfg, cfgCancel, err := copyAndLoad("testdata/versioningconfig.xml", device4) + cfg, cfgCancel, err := copyAndLoad(testFs, "versioningconfig.xml", device4) defer cfgCancel() if err != nil { t.Error(err) @@ -468,7 +474,7 @@ func TestIssue1262(t *testing.T) { t.Skipf("path gets converted to absolute as part of the filesystem initialization on linux") } - cfg, cfgCancel, err := copyAndLoad("testdata/issue-1262.xml", device4) + cfg, cfgCancel, err := copyAndLoad(testFs, "issue-1262.xml", device4) defer cfgCancel() if err != nil { t.Fatal(err) @@ -483,7 +489,7 @@ func TestIssue1262(t *testing.T) { } func TestIssue1750(t *testing.T) { - cfg, cfgCancel, err := copyAndLoad("testdata/issue-1750.xml", device4) + cfg, cfgCancel, err := copyAndLoad(testFs, "issue-1750.xml", device4) defer cfgCancel() if err != nil { t.Fatal(err) @@ -521,20 +527,15 @@ func TestFolderPath(t *testing.T) { } func TestFolderCheckPath(t *testing.T) { - n := t.TempDir() - testFs := fs.NewFilesystem(fs.FilesystemTypeBasic, n) - - err := os.MkdirAll(filepath.Join(n, "dir", ".stfolder"), os.FileMode(0o777)) - if err != nil { - t.Fatal(err) - } + tmpFs := fs.NewFilesystem(fs.FilesystemTypeFake, rand.String(16)+"?nostfolder=true") + _ = tmpFs.MkdirAll(filepath.Join("dir", ".stfolder"), 0o777) testcases := []struct { path string err error }{ { - path: "", + path: ".", err: ErrMarkerMissing, }, { @@ -547,35 +548,20 @@ func TestFolderCheckPath(t *testing.T) { }, } - err = fs.DebugSymlinkForTestsOnly(testFs, testFs, "dir", "link") - if err == nil { - t.Log("running with symlink check") - testcases = append(testcases, struct { - path string - err error - }{ - path: "link", - err: nil, - }) - } else if !build.IsWindows { - t.Log("running without symlink check") - t.Fatal(err) - } - for _, testcase := range testcases { cfg := FolderConfiguration{ - Path: filepath.Join(n, testcase.path), - MarkerName: DefaultMarkerName, + FilesystemType: fs.FilesystemTypeFake, + MarkerName: DefaultMarkerName, } - if err := cfg.CheckPath(); testcase.err != err { - t.Errorf("unexpected error in case %s: %s != %v", testcase.path, err, testcase.err) + if err := cfg.checkFilesystemPath(tmpFs, testcase.path); testcase.err != err { + t.Errorf("unexpected error in case; path: [%s] err [%s] expected err [%v]", testcase.path, err, testcase.err) } } } func TestNewSaveLoad(t *testing.T) { - path := "testdata/temp.xml" + path := "temp.xml" os.Remove(path) defer os.Remove(path) @@ -657,7 +643,7 @@ func TestPrepare(t *testing.T) { } func TestCopy(t *testing.T) { - wrapper, wrapperCancel, err := copyAndLoad("testdata/example.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad(testFs, "example.xml", device1) defer wrapperCancel() if err != nil { t.Fatal(err) @@ -690,14 +676,12 @@ func TestCopy(t *testing.T) { t.Error("Config should have changed") } if !bytes.Equal(bsOrig, bsCopy) { - // os.WriteFile("a", bsOrig, 0644) - // os.WriteFile("b", bsCopy, 0644) t.Error("Copy should be unchanged") } } func TestPullOrder(t *testing.T) { - wrapper, wrapperCleanup, err := copyAndLoad("testdata/pullorder.xml", device1) + wrapper, wrapperCleanup, err := copyAndLoad(testFs, "pullorder.xml", device1) defer wrapperCleanup() if err != nil { t.Fatal(err) @@ -750,7 +734,7 @@ func TestPullOrder(t *testing.T) { } func TestLargeRescanInterval(t *testing.T) { - wrapper, wrapperCancel, err := copyAndLoad("testdata/largeinterval.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad(testFs, "largeinterval.xml", device1) defer wrapperCancel() if err != nil { t.Fatal(err) @@ -810,7 +794,7 @@ func TestGUIPasswordHash(t *testing.T) { func TestDuplicateDevices(t *testing.T) { // Duplicate devices should be removed - wrapper, wrapperCancel, err := copyAndLoad("testdata/dupdevices.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad(testFs, "dupdevices.xml", device1) defer wrapperCancel() if err != nil { t.Fatal(err) @@ -829,7 +813,7 @@ func TestDuplicateDevices(t *testing.T) { func TestDuplicateFolders(t *testing.T) { // Duplicate folders are a loading error - _, _Cancel, err := copyAndLoad("testdata/dupfolders.xml", device1) + _, _Cancel, err := copyAndLoad(testFs, "dupfolders.xml", device1) defer _Cancel() if err == nil || !strings.Contains(err.Error(), errFolderIDDuplicate.Error() |