Skip to content
This repository has been archived by the owner on Jan 27, 2020. It is now read-only.

Make aggregateChanges more testable. #91

Merged
merged 8 commits into from
Jan 31, 2016
Merged

Make aggregateChanges more testable. #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
635c35a
aggregateChanges: Remove the callback agrument
plouj Jan 30, 2016
603b004
aggregateChanges: Remove unused argument folder.
plouj Jan 30, 2016
82ebcf0
aggregateChanges: Factor out sortedUniqueAndCleanPaths
plouj Jan 30, 2016
e7597a6
aggregateChanges: Clean paths before sorting
plouj Jan 30, 2016
f57bb37
aggregateChanges: Extract the os.Stat call into the statPathFunc argu…
plouj Jan 30, 2016
15bb6bd
syncwatcher_test: Slighly more output in case of errors.
plouj Jan 30, 2016
308ae31
aggregateChanges: More unit tests.
plouj Jan 30, 2016
9c174e6
aggregateChanges: Remove unnecessary two-value assignment
plouj Jan 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 51 additions & 23 deletions syncwatcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ func accumulateChanges(debounceTimeout time.Duration,
}

// Try to inform changes to syncthing and if succeeded, clean up
err = aggregateChanges(folder, folderPath, dirVsFiles, callback, paths)
err = callback(folder, aggregateChanges(folderPath, dirVsFiles, paths, currentPathStatus))
if err == nil {
for _, path := range paths {
delete(inProgress, path)
Expand Down Expand Up @@ -761,41 +761,69 @@ func accumulateChanges(debounceTimeout time.Duration,
}
}

func cleanPaths(paths []string) {
for i := range paths {
paths[i] = filepath.Clean(paths[i])
}
}

func sortedUniqueAndCleanPaths(paths []string) []string {
cleanPaths(paths)
sort.Strings(paths)
var new_paths []string
previousPath := ""
for _, path := range paths {
if path == "." {
path = ""
}
if path != previousPath {
new_paths = append(new_paths, path)
}
previousPath = path
}
return new_paths
}

type PathStatus int
const (
deletedPath PathStatus = iota
directoryPath
filePath
)

func currentPathStatus(path string) PathStatus {
fileinfo, _ := os.Stat(path)
if fileinfo == nil {
return deletedPath
} else if fileinfo.IsDir() {
return directoryPath
}
return filePath
}

type statPathFunc func(name string) (PathStatus)
// AggregateChanges optimises tracking in two ways:
// - If there are more than `dirVsFiles` changes in a directory, we inform Syncthing to scan the entire directory
// - Directories with parent directory changes are aggregated. If A/B has 3 changes and A/C has 8, A will have 11 changes and if this is bigger than dirVsFiles we will scan A.
func aggregateChanges(folder string, folderPath string, dirVsFiles int, callback InformCallback, paths []string) error {
if len(paths) == 0 {
return errors.New("No changes to aggregate")
}
func aggregateChanges(folderPath string, dirVsFiles int, paths []string, pathStatus statPathFunc) []string {
// Map paths to scores; if score == -1 the path is a filename
trackedPaths := make(map[string]int)
// Map of directories
trackedDirs := make(map[string]bool)
// Make sure parent paths are processed first
sort.Strings(paths)
// For removing duplicates in a sorted list
previousPath := ""
paths = sortedUniqueAndCleanPaths(paths)
// First we collect all paths and calculate scores for them
for i := range paths {
path := filepath.Clean(paths[i])
if path == "." {
path = ""
}
if path == previousPath {
continue
}
previousPath = path
fi, _ := os.Stat(path)
for _, path := range paths {
pathstatus := pathStatus(path)
path = strings.TrimPrefix(path, folderPath)
path = strings.TrimPrefix(path, pathSeparator)
var dir string
if fi == nil {
if pathstatus == deletedPath {
// Definitely inform if the path does not exist anymore
dir = path
trackedPaths[path] = dirVsFiles
Debug.Println("[AG] Not found:", path)
} else if fi.IsDir() {
} else if pathstatus == directoryPath {
// Definitely inform if a directory changed
dir = path
trackedPaths[path] = dirVsFiles
Expand Down Expand Up @@ -827,12 +855,12 @@ func aggregateChanges(folder string, folderPath string, dirVsFiles int, callback
keys = append(keys, k)
}
sort.Strings(keys) // Sort directories before their own files
previousPath = ""
previousPath := ""
var scans []string
// Decide if we should inform about particular path based on dirVsFiles
for i := range keys {
trackedPath := keys[i]
trackedPathScore, _ := trackedPaths[trackedPath]
trackedPathScore := trackedPaths[trackedPath]
if strings.HasPrefix(trackedPath, previousPath+pathSeparator) {
// Already informed parent directory change
continue
Expand All @@ -849,7 +877,7 @@ func aggregateChanges(folder string, folderPath string, dirVsFiles int, callback
break
}
}
return callback(folder, scans)
return scans
}

// watchSTEvents reads events from Syncthing. For events of type ItemStarted and ItemFinished it puts
Expand Down
82 changes: 66 additions & 16 deletions syncwatcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestDebouncedFileWatch(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 1 || sub[0] != testFile {
t.Error("Invalid result for file change: "+repo, sub)
t.Errorf("Invalid result for file change: (%v) %#v", repo, sub)
}
testOK = true
return nil
Expand Down Expand Up @@ -99,7 +99,7 @@ func TestDebouncedDirectoryWatch(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 1 || sub[0] != testFile {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
testOK = true
return nil
Expand Down Expand Up @@ -131,7 +131,7 @@ func TestDebouncedParentDirectoryWatch(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 1 || sub[0] != "a" {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
testOK = true
return nil
Expand Down Expand Up @@ -168,10 +168,10 @@ func TestDebouncedParentDirectoryWatch2(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 2 || sub[0] != "a" {
t.Error("Invalid result for directory change 1: "+repo, sub)
t.Errorf("Invalid result for directory change 1: (%v) %#v", repo, sub)
}
if repo != testRepo || sub[1] != "b" {
t.Error("Invalid result for directory change 2: "+repo, sub)
t.Errorf("Invalid result for directory change 2: (%v) %#v", repo, sub)
}
testOK = len(sub)
return nil
Expand Down Expand Up @@ -205,7 +205,7 @@ func TestDebouncedParentDirectoryWatch3(t *testing.T) {
}
for i, s := range sub {
if repo != testRepo || s != testFiles[i] {
t.Error("Invalid result for directory change " + strconv.Itoa(testOK) + ": " + repo + " " + s)
t.Errorf("Invalid result for directory change %t : (%v) %#v", testOK, repo, s)
}
}
testOK = len(sub)
Expand Down Expand Up @@ -242,10 +242,10 @@ func TestDebouncedParentDirectoryWatch4(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 2 || sub[0] != "a"+slash+"b" {
t.Error("Invalid result for directory change "+strconv.Itoa(testOK)+": "+repo, sub)
t.Errorf("Invalid result for directory change %t : (%v) %#v", testOK, repo, sub)
}
if repo != testRepo || sub[1] != "a"+slash+"e" {
t.Error("Invalid result for directory change "+strconv.Itoa(testOK)+": "+repo, sub)
t.Errorf("Invalid result for directory change %t : (%v) %#v", testOK, repo, sub)
}
testOK = len(sub)
return nil
Expand Down Expand Up @@ -280,7 +280,7 @@ func TestDebouncedParentDirectoryWatch5(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 1 || sub[0] != "" {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
testOK = true
return nil
Expand Down Expand Up @@ -315,7 +315,7 @@ func TestDebouncedParentDirectoryWatch6(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 1 || sub[0] != strings.TrimSuffix(testChangeDir, slash) {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
testOK += 1
return nil
Expand Down Expand Up @@ -348,7 +348,7 @@ func TestDebouncedParentDirectoryRemovedWatch(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 1 || sub[0] != "a" {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
testOK += 1
return nil
Expand Down Expand Up @@ -381,7 +381,7 @@ func TestSTEvents(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 0 {
t.Error("Invalid result for directory change: " + repo)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
testOK = false
return nil
Expand Down Expand Up @@ -418,7 +418,7 @@ func TestFilesAggregation(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 50 || sub[0] != "a/0" {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
if testOK {
t.Error("Callback triggered multiple times")
Expand Down Expand Up @@ -456,7 +456,7 @@ func TestManyFilesAggregation(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 1 || sub[0] != "" {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
if testOK {
t.Error("Callback triggered multiple times")
Expand Down Expand Up @@ -494,7 +494,7 @@ func TestDeletesAggregation(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 50 || sub[0] != "a/0" {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
if testOK {
t.Error("Callback triggered multiple times")
Expand Down Expand Up @@ -531,7 +531,7 @@ func TestManyDeletesAggregation(t *testing.T) {
return nil
}
if repo != testRepo || len(sub) != 1 || sub[0] != "" {
t.Error("Invalid result for directory change: "+repo, sub)
t.Errorf("Invalid result for directory change: (%v) %#v", repo, sub)
}
if testOK {
t.Error("Callback triggered multiple times")
Expand All @@ -550,3 +550,53 @@ func TestManyDeletesAggregation(t *testing.T) {
t.Error("Callback not triggered")
}
}

func slicesEqual(left, right []string) bool {
if left == nil && right == nil {
return true;
}
if left == nil || right == nil {
return false;
}
if len(left) != len(right) {
return false
}
for i := range left {
if left[i] != right[i] {
return false
}
}
return true
}

func TestAggregateChanges(t *testing.T) {
pathStat := func(path string) (PathStatus) {
if strings.Contains(path, "deleted") {
return deletedPath
} else if strings.Contains(path, "file") {
return filePath
} else {
return directoryPath
}
}
checkAggregation := func(dirVsFiles int, paths []string, expected []string) {
changes := aggregateChanges("/path/to/folder", dirVsFiles, paths, pathStat)
if !slicesEqual(changes, expected) {
t.Errorf("Expected: %#v, got: %#v", expected, changes)
}
}

checkAggregation(3, nil, nil)
checkAggregation(3, []string{}, nil)
checkAggregation(3, []string{"file1"}, []string{"file1"})
checkAggregation(3, []string{"a/file1"}, []string{"a/file1"})
checkAggregation(3, []string{"a/file1", "a/file2", "a/file3",
"b/file1", "b/file2"}, []string{"a", "b/file1", "b/file2"})
checkAggregation(3, []string{"a/deleted1", "a/deleted2", "a/deleted3", "a/deleted4",
"b/deleted1", "b/deleted2"}, []string{"a/deleted1", "a/deleted2", "a/deleted3",
"a/deleted4", "b/deleted1", "b/deleted2"})
checkAggregation(3, []string{"file1", "file2"}, []string{"file1", "file2"})
checkAggregation(3, []string{"file1", "file2", "file3", "file4"}, []string{""})
checkAggregation(3, []string{"file1", "file2", "file3", "file4",
"a/file1", "a/file2"}, []string{""})
}