From 3e5e5b90c35668a79c2961854e8135bffb805d78 Mon Sep 17 00:00:00 2001 From: Anthony Pynes Date: Wed, 13 Dec 2017 14:05:36 -0600 Subject: [PATCH 1/3] ISSUE-955 Fix "Error handling nested vendor folders" - Refactor to allow testing of the walk function - Tests for walk function - Return 'filepath.SkipDir' when removing a directory tree under the path as docs recommend - Cleaning up some logic in walk function --- path/strip.go | 48 +++++++-------- path/strip_test.go | 141 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+), 23 deletions(-) create mode 100644 path/strip_test.go diff --git a/path/strip.go b/path/strip.go index a4c4c284..bae8777f 100644 --- a/path/strip.go +++ b/path/strip.go @@ -8,18 +8,9 @@ import ( "github.com/Masterminds/glide/msg" ) -// StripVendor removes nested vendor and Godeps/_workspace/ directories. -func StripVendor() error { - searchPath, _ := Vendor() - if _, err := os.Stat(searchPath); err != nil { - if os.IsNotExist(err) { - msg.Debug("Vendor directory does not exist.") - } - - return err - } - - err := filepath.Walk(searchPath, func(path string, info os.FileInfo, err error) error { +func getWalkFunction(searchPath string, removeAll func(p string) error) func(path string, + info os.FileInfo, err error) error { + return func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -29,20 +20,31 @@ func StripVendor() error { return nil } - name := info.Name() - if name == "vendor" { - if _, err := os.Stat(path); err == nil { - if info.IsDir() { - msg.Info("Removing: %s", path) - return CustomRemoveAll(path) - } - - msg.Debug("%s is not a directory. Skipping removal", path) - return nil + if info.Name() == "vendor" && info.IsDir() { + msg.Info("Removing: %s", path) + err = removeAll(path) + if nil != err { + return err } + return filepath.SkipDir } return nil - }) + } +} + +// StripVendor removes nested vendor and Godeps/_workspace/ directories. +func StripVendor() error { + searchPath, _ := Vendor() + if _, err := os.Stat(searchPath); err != nil { + if os.IsNotExist(err) { + msg.Debug("Vendor directory does not exist.") + } + + return err + } + + err := filepath.Walk(searchPath, getWalkFunction(searchPath, CustomRemoveAll)) + if err != nil { return err } diff --git a/path/strip_test.go b/path/strip_test.go new file mode 100644 index 00000000..3ce24973 --- /dev/null +++ b/path/strip_test.go @@ -0,0 +1,141 @@ +package path + +import ( + "errors" + "os" + "path/filepath" + "reflect" + "testing" + "time" +) + +type mockFileInfo struct { + name string + isDir bool +} + +func (mfi *mockFileInfo) Name() string { + return mfi.name +} + +func (mfi *mockFileInfo) Size() int64 { + panic("not implemented") +} + +func (mfi *mockFileInfo) Mode() os.FileMode { + panic("not implemented") +} + +func (mfi *mockFileInfo) ModTime() time.Time { + panic("not implemented") +} + +func (mfi *mockFileInfo) IsDir() bool { + return mfi.isDir +} + +func (mfi *mockFileInfo) Sys() interface{} { + panic("not implemented") +} + +type removeAll struct { + calledWith string + err error +} + +func (rah *removeAll) removeAll(p string) error { + rah.calledWith = p + return rah.err +} + +func TestWalkFunction(t *testing.T) { + type args struct { + searchPath string + removeAll *removeAll + path string + info os.FileInfo + err error + } + tests := []struct { + name string + args args + want error + wantCalledWith string + }{ + { + name: "WalkFunctionSkipsNonVendor", + args: args{searchPath: "foo", + removeAll: &removeAll{}, + path: "foo/bar", + info: &mockFileInfo{name: "bar", isDir: true}, + err: nil, + }, + want: nil, + wantCalledWith: "", + }, + { + name: "WalkFunctionSkipsNonDir", + args: args{searchPath: "foo", + removeAll: &removeAll{}, + path: "foo/vendor", + info: &mockFileInfo{name: "vendor", isDir: false}, + err: nil, + }, + want: nil, + wantCalledWith: "", + }, + { + name: "WalkFunctionDeletesVendor", + args: args{searchPath: "foo", + removeAll: &removeAll{}, + path: "foo/vendor", + info: &mockFileInfo{name: "vendor", isDir: true}, + err: nil, + }, + want: filepath.SkipDir, + wantCalledWith: "foo/vendor", + }, + { + name: "WalkFunctionReturnsPassedError", + args: args{searchPath: "foo", + removeAll: &removeAll{}, + path: "foo/vendor", + info: &mockFileInfo{name: "vendor", isDir: true}, + err: errors.New("expected"), + }, + want: errors.New("expected"), + wantCalledWith: "", + }, + { + name: "WalkFunctionReturnsRemoveAllError", + args: args{searchPath: "foo", + removeAll: &removeAll{err: errors.New("expected")}, + path: "foo/vendor", + info: &mockFileInfo{name: "vendor", isDir: true}, + err: nil, + }, + want: errors.New("expected"), + wantCalledWith: "foo/vendor", + }, + { + name: "WalkFunctionSkipsBaseDir", + args: args{searchPath: "vendor", + removeAll: &removeAll{}, + path: "vendor", + info: &mockFileInfo{name: "vendor", isDir: true}, + err: nil, + }, + want: nil, + wantCalledWith: "", + }, + } + for _, test := range tests { + walkFunction := getWalkFunction(test.args.searchPath, test.args.removeAll.removeAll) + if actual := walkFunction(test.args.path, test.args.info, test.args.err); !reflect.DeepEqual(actual, test.want) { + t.Errorf("walkFunction() = %v, want %v", actual, test.want) + } + if test.args.removeAll.calledWith != test.wantCalledWith { + t.Errorf("removeAll argument = \"%s\", want \"%s\"", test.args.removeAll.calledWith, test.wantCalledWith) + } + } +} From 87bb0fbcf3bd5b6b768783811213c422808fb9b3 Mon Sep 17 00:00:00 2001 From: Anthony Heidenreich Date: Wed, 13 Dec 2017 14:10:11 -0600 Subject: [PATCH 2/3] ISSUE-955: added integration tests that reproduce the problem --- path/strip_int_test.go | 45 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 path/strip_int_test.go diff --git a/path/strip_int_test.go b/path/strip_int_test.go new file mode 100644 index 00000000..c0fdfd11 --- /dev/null +++ b/path/strip_int_test.go @@ -0,0 +1,45 @@ +package path + +import ( + "io/ioutil" + "os" + "path" + "testing" +) + +func generateTestDirectory(t *testing.T) string { + t.Helper() + baseDir, err := ioutil.TempDir(os.TempDir(), "mgt") + if nil != err { + t.Error("Unable to create temp directory: ", err.Error()) + } + paths := map[string][]string{ + "github.com/fake/log": {"log.go"}, + "github.com/phoney/foo": {"bar.go"}, + "github.com/phoney/foo/vendor": {"test.go", "foo.bar"}, + "github.com/aws/aws-sdk-go/awsmigrate/awsmigrate-renamer/vendor": {}, + "github.com/aws/aws-sdk-go/awsmigrate/awsmigrate-renamer/vendor/golang.org/x/tools/go/buildutil": {"allpackages.go", "tags.go", "fakecontext.go"}, + "github.com/aws/aws-sdk-go/vendor": {"key_test.go", "key.go"}, + "github.com/aws/aws-sdk-go/vendor/github.com/go-ini/ini": {"struct_test.go", "error.go", "ini_test.go"}, + } + os.OpenFile(path.Join(baseDir, "glide.yaml"), os.O_RDONLY|os.O_CREATE, 0666) + for p, files := range paths { + p = path.Join(baseDir, "vendor", p) + if err = os.MkdirAll(p, 0777); nil != err { + t.Errorf("Unable to create vendor dir: %s\n%s", p, err.Error()) + } + for _, f := range files { + os.OpenFile(path.Join(p, f), os.O_RDONLY|os.O_CREATE, 0666) + } + } + return baseDir +} + +func TestNestVendorNoError(t *testing.T) { + workingDir := generateTestDirectory(t) + os.Chdir(workingDir) + err := StripVendor() + if nil != err { + t.Errorf("Unexpected error in StripVendor: %s", err.Error()) + } +} From 07c3b6ed829c1f557eb5617003bc23a275dfe207 Mon Sep 17 00:00:00 2001 From: Anthony Heidenreich Date: Wed, 13 Dec 2017 14:18:12 -0600 Subject: [PATCH 3/3] removed the go1.9 dependency --- path/strip_int_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/path/strip_int_test.go b/path/strip_int_test.go index c0fdfd11..a6ad4bcd 100644 --- a/path/strip_int_test.go +++ b/path/strip_int_test.go @@ -8,7 +8,6 @@ import ( ) func generateTestDirectory(t *testing.T) string { - t.Helper() baseDir, err := ioutil.TempDir(os.TempDir(), "mgt") if nil != err { t.Error("Unable to create temp directory: ", err.Error())