Skip to content

Commit

Permalink
Merge pull request #157 from dnephin/fix-watch
Browse files Browse the repository at this point in the history
filewatcher: fix a bug with maxDepth and comparing event.Op
  • Loading branch information
dnephin committed Oct 25, 2020
2 parents a94234d + d0804e6 commit cde872b
Show file tree
Hide file tree
Showing 4 changed files with 204 additions and 29 deletions.
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ replace github.com/spf13/pflag => github.com/dnephin/pflag v0.0.0-20200521001137
require (
github.com/fatih/color v1.9.0
github.com/fsnotify/fsnotify v1.4.9
github.com/google/go-cmp v0.3.0
github.com/google/go-cmp v0.4.0
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
github.com/jonboulle/clockwork v0.1.0
github.com/mattn/go-colorable v0.1.6 // indirect
Expand All @@ -15,7 +15,7 @@ require (
golang.org/x/sync v0.0.0-20190423024810-112230192c58
golang.org/x/sys v0.0.0-20201009025420-dfb3f7c4e634
golang.org/x/tools v0.0.0-20190624222133-a101b041ded4
gotest.tools/v3 v3.0.2
gotest.tools/v3 v3.0.3
)

go 1.11
10 changes: 6 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ github.com/fatih/color v1.9.0 h1:8xPHl4/q1VyqGIPif1F+1V3Y3lSmrq01EabUW3CoW5s=
github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL+zU=
github.com/fsnotify/fsnotify v1.4.9 h1:hsms1Qyu0jgnwNXIxa+/V/PDsU6CfLf6CNO8H7IWoS4=
github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ=
github.com/google/go-cmp v0.3.0 h1:crn/baboCvb5fXaQ0IJ1SGTsTVrWpDsCWC8EGETZijY=
github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
github.com/google/go-cmp v0.4.0 h1:xsAVV57WRhGj6kEIi8ReJzQlHHqcBYCElAvkovg3B/4=
github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaUGG7oYTSPP8MxqL4YI3kZKwcP4=
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ=
github.com/jonboulle/clockwork v0.1.0 h1:VKV+ZcuP6l3yW9doeqz6ziZGgcynBVQO+obU0+0hcPo=
Expand Down Expand Up @@ -42,5 +42,7 @@ golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/tools v0.0.0-20190624222133-a101b041ded4 h1:1mMox4TgefDwqluYCv677yNXwlfTkija4owZve/jr78=
golang.org/x/tools v0.0.0-20190624222133-a101b041ded4/go.mod h1:/rFqwRUd4F7ZHNgwSSTFct+R/Kf4OFW1sUzUTQQTgfc=
gotest.tools/v3 v3.0.2 h1:kG1BFyqVHuQoVQiR1bWGnfz/fmHvvuiSPIV7rvl360E=
gotest.tools/v3 v3.0.2/go.mod h1:3SzNCllyD9/Y+b5r9JIKQ474KzkZyqLqEfYqMsX94Bk=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
gotest.tools/v3 v3.0.3 h1:4AuOwCGf4lLR9u3YOe2awrHygurzhO/HeQ6laiA6Sx0=
gotest.tools/v3 v3.0.3/go.mod h1:Z7Lb0S5l+klDB31fvDQX8ss/FlKDxtlFlw3Oa8Ymbl8=
60 changes: 37 additions & 23 deletions internal/filewatcher/watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ func Watch(dirs []string, run func(pkg string) error) error {
case <-timer.C:
return fmt.Errorf("exceeded idle timeout while watching files")
case event := <-watcher.Events:
if !timer.Stop() {
<-timer.C
}
log.Debugf("handling event %v", event)

if handleDirCreated(watcher, event) {
Expand All @@ -54,9 +57,28 @@ func Watch(dirs []string, run func(pkg string) error) error {
}
}

func findAllDirs(dirs []string, depth int) []string {
var output []string
func findAllDirs(dirs []string, maxDepth int) []string {
if len(dirs) == 0 {
dirs = []string{"./..."}
}

var output []string // nolint: prealloc
for _, dir := range dirs {
const recur = "/..."
if strings.HasSuffix(dir, recur) {
dir = strings.TrimSuffix(dir, recur)
output = append(output, findSubDirs(dir, maxDepth)...)
continue
}
output = append(output, dir)
}
return output
}

func findSubDirs(rootDir string, maxDepth int) []string {
var output []string
// add root dir depth so that maxDepth is relative to the root dir
maxDepth += pathDepth(rootDir)
walker := func(path string, info os.FileInfo, err error) error {
if err != nil {
log.Warnf("failed to watch %v: %v", path, err)
Expand All @@ -65,32 +87,24 @@ func findAllDirs(dirs []string, depth int) []string {
if !info.IsDir() {
return nil
}
if isMaxDepth(path, depth) || exclude(path) {
if pathDepth(path) > maxDepth || exclude(path) {
log.Debugf("Ignoring %v because of max depth or exclude list", path)
return filepath.SkipDir
}
if noGoFiles(path) {
if !hasGoFiles(path) {
log.Debugf("Ignoring %v because it has no .go files", path)
return nil
}
output = append(output, path)
return nil
}

if len(dirs) == 0 {
dirs = []string{"."}
}

for _, dir := range dirs {
dir = strings.TrimSuffix(dir, "/...")
// nolint: errcheck // error is handled by walker func
filepath.Walk(dir, walker)
}
// nolint: errcheck // error is handled by walker func
filepath.Walk(rootDir, walker)
return output
}

func isMaxDepth(path string, depth int) bool {
return strings.Count(filepath.Clean(path), string(filepath.Separator)) >= depth
func pathDepth(path string) int {
return strings.Count(filepath.Clean(path), string(filepath.Separator))
}

// return true if path is vendor, testdata, or starts with a dot
Expand All @@ -105,31 +119,31 @@ func exclude(path string) bool {
return false
}

func noGoFiles(path string) bool {
func hasGoFiles(path string) bool {
fh, err := os.Open(path)
if err != nil {
return true
return false
}

for {
names, err := fh.Readdirnames(20)
switch {
case err == io.EOF:
return true
return false
case err != nil:
log.Warnf("failed to read directory %v: %v", path, err)
return true
return false
}

for _, name := range names {
if strings.HasSuffix(name, ".go") {
return false
return true
}
}
}
}

func handleDirCreated(watcher *fsnotify.Watcher, event fsnotify.Event) bool {
func handleDirCreated(watcher *fsnotify.Watcher, event fsnotify.Event) (handled bool) {
if event.Op&fsnotify.Create != fsnotify.Create {
return false
}
Expand Down Expand Up @@ -158,7 +172,7 @@ type handler struct {
const floodThreshold = 250 * time.Millisecond

func (h *handler) handleEvent(event fsnotify.Event) error {
if event.Op&fsnotify.Write|fsnotify.Create == 0 {
if event.Op&(fsnotify.Write|fsnotify.Create) == 0 {
return nil
}

Expand Down
159 changes: 159 additions & 0 deletions internal/filewatcher/watch_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
package filewatcher

import (
"fmt"
"path/filepath"
"testing"
"time"

"github.com/fsnotify/fsnotify"
"gotest.tools/v3/assert"
"gotest.tools/v3/env"
"gotest.tools/v3/fs"
)

func TestHandler_HandleEvent(t *testing.T) {
type testCase struct {
name string
last time.Time
expectedRun bool
event fsnotify.Event
}

fn := func(t *testing.T, tc testCase) {
var ran bool
run := func(pkg string) error {
ran = true
return nil
}

h := handler{last: tc.last, fn: run}
err := h.handleEvent(tc.event)
assert.NilError(t, err)
assert.Equal(t, ran, tc.expectedRun)
if tc.expectedRun {
assert.Assert(t, !h.last.IsZero())
}
}

var testCases = []testCase{
{
name: "Op is rename",
event: fsnotify.Event{Op: fsnotify.Rename, Name: "file_test.go"},
},
{
name: "Op is remove",
event: fsnotify.Event{Op: fsnotify.Remove, Name: "file_test.go"},
},
{
name: "Op is chmod",
event: fsnotify.Event{Op: fsnotify.Chmod, Name: "file_test.go"},
},
{
name: "Op is write+chmod",
event: fsnotify.Event{Op: fsnotify.Write | fsnotify.Chmod, Name: "file_test.go"},
expectedRun: true,
},
{
name: "Op is write",
event: fsnotify.Event{Op: fsnotify.Write, Name: "file_test.go"},
expectedRun: true,
},
{
name: "Op is create",
event: fsnotify.Event{Op: fsnotify.Create, Name: "file_test.go"},
expectedRun: true,
},
{
name: "file is not a go file",
event: fsnotify.Event{Op: fsnotify.Write, Name: "readme.md"},
},
{
name: "under flood threshold",
event: fsnotify.Event{Op: fsnotify.Create, Name: "file_test.go"},
last: time.Now(),
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
fn(t, tc)
})
}
}

func TestHasGoFiles(t *testing.T) {
t.Run("none", func(t *testing.T) {
tmpDir := fs.NewDir(t, t.Name(), fs.WithFile("readme.md", ""))
defer tmpDir.Remove()
assert.Assert(t, !hasGoFiles(tmpDir.Path()))
})
t.Run("empty", func(t *testing.T) {
tmpDir := fs.NewDir(t, t.Name())
defer tmpDir.Remove()
assert.Assert(t, !hasGoFiles(tmpDir.Path()))
})
t.Run("some go files", func(t *testing.T) {
tmpDir := fs.NewDir(t, t.Name(), fs.WithFile("main.go", ""))
defer tmpDir.Remove()
assert.Assert(t, hasGoFiles(tmpDir.Path()))
})
t.Run("many go files", func(t *testing.T) {
tmpDir := fs.NewDir(t, t.Name())
for i := 0; i < 47; i++ {
fs.Apply(t, tmpDir, fs.WithFile(fmt.Sprintf("file%d.go", i), ""))
}
defer tmpDir.Remove()
assert.Assert(t, hasGoFiles(tmpDir.Path()))
})
}

func TestFindAllDirs(t *testing.T) {
goFile := fs.WithFile("file.go", "")
dirOne := fs.NewDir(t, t.Name(),
goFile,
fs.WithFile("not-a-dir", ""),
fs.WithDir("no-go-files"),
fs.WithDir(".starts-with-dot", goFile))
defer dirOne.Remove()
var path string
for i := 1; i <= 10; i++ {
path = filepath.Join(path, fmt.Sprintf("%d", i))
var ops []fs.PathOp
if i != 4 && i != 5 {
ops = []fs.PathOp{goFile}
}
fs.Apply(t, dirOne, fs.WithDir(path, ops...))
}

dirTwo := fs.NewDir(t, t.Name(),
goFile,
// subdir should be ignored, dirTwo is used without /... suffix
fs.WithDir("subdir", goFile))
defer dirTwo.Remove()

dirs := findAllDirs([]string{dirOne.Path() + "/...", dirTwo.Path()}, maxDepth)
expected := []string{
dirOne.Path(),
dirOne.Join("1"),
dirOne.Join("1/2"),
dirOne.Join("1/2/3"),
dirOne.Join("1/2/3/4/5/6"),
dirOne.Join("1/2/3/4/5/6/7"),
dirTwo.Path(),
}
assert.DeepEqual(t, dirs, expected)
}

func TestFindAllDirs_DefaultPath(t *testing.T) {
goFile := fs.WithFile("file.go", "")
dirOne := fs.NewDir(t, t.Name(),
goFile,
fs.WithDir("a", goFile),
fs.WithDir("b", goFile))
defer dirOne.Remove()

defer env.ChangeWorkingDir(t, dirOne.Path())()
dirs := findAllDirs([]string{}, maxDepth)
expected := []string{".", "a", "b"}
assert.DeepEqual(t, dirs, expected)
}

0 comments on commit cde872b

Please sign in to comment.