From f89d17ab1b6422d55484d13cb4cc04f452507be2 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 23 Jul 2018 17:02:33 +0200 Subject: [PATCH 1/7] integration tests: run sequentially when updating test cases When running "internal" and "external" versions of the same test case in parallel, the test case data must be read-only to avoid race conditions. When it is read/write because the tests were asked to update the reference files with "-update", running in parallel must be disabled. This broke in practice when updating entire directory trees. The risk of hitting the race is smaller when only updating single files, but it is still a race. --- cmd/dep/integration_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cmd/dep/integration_test.go b/cmd/dep/integration_test.go index 0a4667cff2..6cff1d6a15 100644 --- a/cmd/dep/integration_test.go +++ b/cmd/dep/integration_test.go @@ -44,7 +44,12 @@ func TestIntegration(t *testing.T) { parse := strings.Split(path, string(filepath.Separator)) testName := strings.Join(parse[2:len(parse)-1], "/") t.Run(testName, func(t *testing.T) { - t.Parallel() + // When updating test case files we can't run in parallel, because then + // we would have a race condition (each test case is used twice, once + // for "internal" and again for "external"). + if !*test.UpdateGolden { + t.Parallel() + } t.Run("external", testIntegration(testName, relPath, wd, execCmd)) t.Run("internal", testIntegration(testName, relPath, wd, runMain)) From 89ddb570b7122132b87430957a71150a4a9f4381 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 23 Jul 2018 14:46:27 +0200 Subject: [PATCH 2/7] document how to run "dlv" for harness tests Debugging a failing test with dlv is possible, but in contrast to "go test", "dlv test" doesn't change the current working directory, which is necessary for the test to find its test data. --- cmd/dep/testdata/harness_tests/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/dep/testdata/harness_tests/README.md b/cmd/dep/testdata/harness_tests/README.md index 379caaa2db..60029d7cfc 100644 --- a/cmd/dep/testdata/harness_tests/README.md +++ b/cmd/dep/testdata/harness_tests/README.md @@ -30,6 +30,8 @@ the test case directory itself. In the above example, the test name would be `category1/subcategory1/case1`, and could be singled out with the `-run` option of `go test` (i.e. `go test github.com/golang/dep/cmd/dep -run Integration/category1/subcategory1/case1`). +`dlv` can also be used when explicitly setting the right directory +(`dlv test --wd cmd/dep ./cmd/dep -test.run Integration/category1/subcategory1/case1`). New tests can be added simply by adding a new directory with the json file to the `testdata` tree. There is no need for code modification - the new test will be included automatically. From 5bdcd16c99e20eece00b5e3fcbbf5253c4996184 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 3 Jan 2019 16:40:54 +0100 Subject: [PATCH 3/7] testing: replace CopyTree with fs.CopyDir The functionality is the same and some minor differences (target directory must not exist when using fs.CopyDir) can be handled by being careful about creating the temporary directories. --- internal/test/integration/testproj.go | 50 +++++++++------------------ 1 file changed, 16 insertions(+), 34 deletions(-) diff --git a/internal/test/integration/testproj.go b/internal/test/integration/testproj.go index 1d8cc729d7..7b9c941d35 100644 --- a/internal/test/integration/testproj.go +++ b/internal/test/integration/testproj.go @@ -16,6 +16,7 @@ import ( "strings" "testing" + "github.com/golang/dep/internal/fs" "github.com/golang/dep/internal/test" "github.com/pkg/errors" ) @@ -56,8 +57,22 @@ func NewTestProject(t *testing.T, initPath, wd string, run RunFunc) *TestProject run: run, } new.makeRootTempDir() + new.TempDir(projectRoot) + dst := filepath.Join(new.tempdir, projectRoot) + // Not all test projects have an "initial" directory. + if _, err := os.Lstat(initPath); !os.IsNotExist(err) { + // Everything except for the inner destination + // directory must exist when calling CopyDir. It was + // created earlier by new.TempDir, so we have to + // remove it again here. + if err := os.Remove(dst); err != nil { + t.Fatalf("Remove(%s): %s", dst, err) + } + if err := fs.CopyDir(initPath, dst); err != nil { + t.Fatalf("CopyDir(%s, %s): %s", initPath, dst, err) + } + } new.TempDir(projectRoot, "vendor") - new.CopyTree(initPath) new.Setenv("GOPATH", new.tempdir) @@ -197,39 +212,6 @@ func (p *TestProject) DoRun(args []string) error { return status } -// CopyTree recursively copies a source directory into the test project's directory. -func (p *TestProject) CopyTree(src string) { - filepath.Walk(src, - func(path string, info os.FileInfo, err error) error { - if path != src { - localpath := path[len(src)+1:] - if info.IsDir() { - p.TempDir(projectRoot, localpath) - } else { - destpath := filepath.Join(p.ProjPath(), localpath) - copyFile(destpath, path) - } - } - return nil - }) -} - -func copyFile(dest, src string) { - in, err := os.Open(src) - if err != nil { - panic(err) - } - defer in.Close() - - out, err := os.Create(dest) - if err != nil { - panic(err) - } - defer out.Close() - - io.Copy(out, in) -} - // GetVendorPaths collects final vendor paths at a depth of three levels. func (p *TestProject) GetVendorPaths() []string { vendorPath := p.ProjPath("vendor") From cf7f7b97b97ee5ef92f8a7b426bf1314f00c77be Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 23 Jul 2018 16:16:57 +0200 Subject: [PATCH 4/7] testing: compare entire 'final' directory So far, only the list of vendored projects and the top-level Gopk.toml/lock files were compared. This way it was impossible to write integration tests that also check the content of the vendored projects. Now a test case can specify that it's 'final' directory contains a complete set of reference files, and then all files are checked respectively updated (when --updated is given). Directories and file meta still aren't checked. The reference files of existing tests can be changed on a case-by-case basis. This new comparison mode cannot be enabled unconditionally because a few tests then fail because each test run creates _vendor- directories which are named differently in each test run. --- cmd/dep/integration_test.go | 65 +++++++++++++++++++++++---- internal/test/integration/testcase.go | 5 +++ internal/test/integration/testproj.go | 2 +- 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/cmd/dep/integration_test.go b/cmd/dep/integration_test.go index 6cff1d6a15..3c449aaac4 100644 --- a/cmd/dep/integration_test.go +++ b/cmd/dep/integration_test.go @@ -16,6 +16,7 @@ import ( "testing" "github.com/golang/dep" + "github.com/golang/dep/internal/fs" "github.com/golang/dep/internal/test" "github.com/golang/dep/internal/test/integration" ) @@ -215,18 +216,66 @@ func testIntegration(name, relPath, wd string, run integration.RunFunc) func(t * testCase.CompareOutput(testProj.GetStdout()) } + // Determine how the test case specifies the expected + // content: either it lists just some projects, or it + // provides a complete reference directory. + reference := len(testCase.VendorFinal) == 1 && testCase.VendorFinal[0] == "compare" + // Check vendor paths testProj.CompareImportPaths() - testCase.CompareVendorPaths(testProj.GetVendorPaths()) + if !reference { + testCase.CompareVendorPaths(testProj.GetVendorPaths()) + } - if *test.UpdateGolden { - // Update manifest and lock - testCase.UpdateFile(dep.ManifestName, testProj.ProjPath(dep.ManifestName)) - testCase.UpdateFile(dep.LockName, testProj.ProjPath(dep.LockName)) + if reference { + // Check all files. + if *test.UpdateGolden { + // Update all files in the 'final' directory, removing those which + // no longer should exist. + if err := os.RemoveAll(testCase.FinalPath()); err != nil { + t.Fatalf("error removing 'final' directory: %s", err) + } + if err := fs.CopyDir(testProj.ProjPath(), testCase.FinalPath()); err != nil { + t.Fatalf("error copying into 'final' directory: %s", err) + } + } else { + // Compare all files from either of the two trees. + files := make(map[string]bool) + findFiles := func(dir string) error { + return filepath.Walk(dir, + func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if dir == path { + return nil + } + if !info.IsDir() { + localpath := path[len(dir)+1:] + files[localpath] = true + } + return nil + }) + } + findFiles(testCase.FinalPath()) + findFiles(testProj.ProjPath()) + + t.Logf("checking against %s", testCase.FinalPath()) + for localpath := range files { + t.Logf("comparing %s", localpath) + testCase.CompareFile(localpath, testProj.ProjPath(localpath)) + } + } } else { - // Check final manifest and lock - testCase.CompareFile(dep.ManifestName, testProj.ProjPath(dep.ManifestName)) - testCase.CompareFile(dep.LockName, testProj.ProjPath(dep.LockName)) + if *test.UpdateGolden { + // Update manifest and lock + testCase.UpdateFile(dep.ManifestName, testProj.ProjPath(dep.ManifestName)) + testCase.UpdateFile(dep.LockName, testProj.ProjPath(dep.LockName)) + } else { + // Check final manifest and lock + testCase.CompareFile(dep.ManifestName, testProj.ProjPath(dep.ManifestName)) + testCase.CompareFile(dep.LockName, testProj.ProjPath(dep.LockName)) + } } } } diff --git a/internal/test/integration/testcase.go b/internal/test/integration/testcase.go index cb7f60fed0..29409ea98a 100644 --- a/internal/test/integration/testcase.go +++ b/internal/test/integration/testcase.go @@ -65,6 +65,11 @@ func (tc *TestCase) InitialPath() string { return tc.initialPath } +// FinalPath represents the final set of files in a project. +func (tc *TestCase) FinalPath() string { + return tc.finalPath +} + // UpdateFile updates the golden file with the working result. func (tc *TestCase) UpdateFile(goldenPath, workingPath string) { exists, working, err := getFile(workingPath) diff --git a/internal/test/integration/testproj.go b/internal/test/integration/testproj.go index 7b9c941d35..c47d26a1d6 100644 --- a/internal/test/integration/testproj.go +++ b/internal/test/integration/testproj.go @@ -187,7 +187,7 @@ func (p *TestProject) GetVendorGit(ip string) { // DoRun executes the integration test command against the test project. func (p *TestProject) DoRun(args []string) error { if *test.PrintLogs { - p.t.Logf("running testdep %v", args) + p.t.Logf("running testdep %v in %s", args, p.ProjPath("")) } prog := filepath.Join(p.origWd, "testdep"+test.ExeSuffix) From d624ca1cd983159d7203eb2eace5733903d780a6 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 25 Jun 2018 15:59:11 +0200 Subject: [PATCH 5/7] gps: export content of git submodules The "git checkout-index" command is unaware of git submodules, therefore their content was missed when exporting files. With "git submodule foreach" the same operation (read-tree + checkout-index) can also be done for all submodules, recursively. The downside is that "foreach" invokes a shell command with certain variables set. To achieve the desired functionality, we have to rely on POSIX shell support. OTOH, the command seems to get embedded by git in a larger shell script because error messages when there are syntax errors show that the command gets invoked via `eval`, so git already depends on a POSIX shell. --- CHANGELOG.md | 1 + gps/vcs_source.go | 80 ++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ef764f4ae..a4160161d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ IMPROVEMENTS: * Don't exclude `Godeps` folder ([#1822](https://github.com/golang/dep/issues/1822)). * Add project-package relationship graph support in graphviz ([#1588](https://github.com/golang/dep/pull/1588)). * Limit concurrency of `dep status` to avoid hitting open file limits ([#1923](https://github.com/golang/dep/issue/1923)). +* Export the content of git submodules to the vendor directory ([#1909](https://github.com/golang/dep/pull/1909)). WIP: * Enable importing external configuration from dependencies during init (#1277). This is feature flagged and disabled by default. diff --git a/gps/vcs_source.go b/gps/vcs_source.go index 1e04b7b8c4..1e286840fd 100644 --- a/gps/vcs_source.go +++ b/gps/vcs_source.go @@ -159,28 +159,94 @@ type gitSource struct { baseVCSSource } +// gitSubmoduleStatusRE matches the output of "git submodule status" which has lines like this: +// f273d8faee1af72e63c0c949287a9265af6635f9 pkg (heads/master) +var gitSubmoduleStatusRE = regexp.MustCompile(`^.([a-fA-F0-9]+) (.*) \(.*?\)$`) + func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to string) error { r := s.repo + if err := s.exportTreeTo(ctx, filepath.Join(r.LocalPath(), ".git"), r.LocalPath(), rev.String(), to); err != nil { + return errors.Wrap(err, "exporting main repo content") + } + + // Now do the same for any submodule, using a shell command that "git submodule" + // iterates over. + { + cmd := commandContext(ctx, "git", "submodule", "status", "--recursive") + cmd.SetDir(r.LocalPath()) + out, err := cmd.CombinedOutput() + if err != nil { + return errors.Wrapf(err, "git submodule status in %s: %s", r.LocalPath(), string(out)) + } + + // The output is something like this: + // f273d8faee1af72e63c0c949287a9265af6635f9 pkg (heads/new-master2) + // 4782e9497d6177ebd53bf080198a7fb8320847f8 pkg/foo (heads/new-master2) + // + // We only need the paths. There's no good way to reliably distinguish + // paths (which may contain brackets) from the "git describe" output at the end + // (which might contain tags with spaces), but those pathological cases + // hopefully get avoided by developers. + for _, line := range strings.Split(string(out), "\n") { + if line == "" { + continue + } + match := gitSubmoduleStatusRE.FindStringSubmatch(line) + if match == nil { + return errors.Errorf("unexpected 'git submodule status' output line: %q", line) + } + hash := match[1] + relPath := match[2] + absPath := filepath.Join(r.LocalPath(), relPath) + // Each submodule path has .git text + // file which contains a relative link to the + // git directory. We need to follow that and + // there do the same "git read-tree + git + // checkout-index" with a temporary index as + // for the main repo. + // + // Instead of parsing that .git text file + // ourselves, we use "git rev-parse --git-dir". + var gitPath string + { + cmd := commandContext(ctx, "git", "rev-parse", "--git-dir") + cmd.SetDir(absPath) + out, err := cmd.CombinedOutput() + if err != nil { + return errors.Wrapf(err, "git rev-parse --git-dir in %s: %s", absPath, string(out)) + } + gitPath = strings.TrimRight(string(out), "\n") + } + if err := s.exportTreeTo(ctx, gitPath, absPath, hash, filepath.Join(to, relPath)); err != nil { + return errors.Wrapf(err, "exporting submodule %s repo content", relPath) + } + } + } + + return nil +} + +func (s *gitSource) exportTreeTo(ctx context.Context, gitDir, workDir, hash, to string) error { if err := os.MkdirAll(to, 0777); err != nil { return err } // Back up original index - idx, bak := filepath.Join(r.LocalPath(), ".git", "index"), filepath.Join(r.LocalPath(), ".git", "origindex") + idx, bak := filepath.Join(gitDir, "index"), filepath.Join(gitDir, "origindex") err := fs.RenameWithFallback(idx, bak) if err != nil { - return err + return errors.Wrapf(err, "rename index in %s", gitDir) } // could have an err here...but it's hard to imagine how? defer fs.RenameWithFallback(bak, idx) { - cmd := commandContext(ctx, "git", "read-tree", rev.String()) - cmd.SetDir(r.LocalPath()) + cmd := commandContext(ctx, "git", "read-tree", hash) + cmd.SetDir(workDir) if out, err := cmd.CombinedOutput(); err != nil { - return errors.Wrap(err, string(out)) + return errors.Wrapf(err, "git read-tree %s in %s: %s", workDir, hash, string(out)) } } @@ -197,9 +263,9 @@ func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to strin // index and HEAD. { cmd := commandContext(ctx, "git", "checkout-index", "-a", "--prefix="+to) - cmd.SetDir(r.LocalPath()) + cmd.SetDir(workDir) if out, err := cmd.CombinedOutput(); err != nil { - return errors.Wrap(err, string(out)) + return errors.Wrapf(err, "git checkout-index in %s to %q: %s", workDir, to, string(out)) } } From d338aaffd69d8cb63dc5258daa6ced02aa76777f Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 3 Jan 2019 15:56:27 +0100 Subject: [PATCH 6/7] dep: add submodule testcase This new test fails if the content of two recursive submodules is not exported. Verification depends on comparing the entire "final" directory. --- .../ensure/submodules/case1/final/Gopkg.lock | 17 ++++++++++ .../ensure/submodules/case1/final/Gopkg.toml | 34 +++++++++++++++++++ .../ensure/submodules/case1/final/main.go | 8 +++++ .../pohly/deptestmodules/pkg/foo/foo.go | 7 ++++ .../ensure/submodules/case1/initial/main.go | 8 +++++ .../ensure/submodules/case1/testcase.json | 9 +++++ 6 files changed, 83 insertions(+) create mode 100644 cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/Gopkg.lock create mode 100644 cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/Gopkg.toml create mode 100644 cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/main.go create mode 100644 cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/vendor/github.com/pohly/deptestmodules/pkg/foo/foo.go create mode 100644 cmd/dep/testdata/harness_tests/ensure/submodules/case1/initial/main.go create mode 100644 cmd/dep/testdata/harness_tests/ensure/submodules/case1/testcase.json diff --git a/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/Gopkg.lock b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/Gopkg.lock new file mode 100644 index 0000000000..e134cde91e --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/Gopkg.lock @@ -0,0 +1,17 @@ +# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. + + +[[projects]] + branch = "master" + digest = "1:62b5039d111b756e045e4d0f74545bfa9db8b49137f00bfe87e7f403023f500e" + name = "github.com/pohly/deptestmodules" + packages = ["pkg/foo"] + pruneopts = "UT" + revision = "797a8d2c23e060c7d84f3e3afcda9128d4853648" + +[solve-meta] + analyzer-name = "dep" + analyzer-version = 1 + input-imports = ["github.com/pohly/deptestmodules/pkg/foo"] + solver-name = "gps-cdcl" + solver-version = 1 diff --git a/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/Gopkg.toml b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/Gopkg.toml new file mode 100644 index 0000000000..a550140ec6 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/Gopkg.toml @@ -0,0 +1,34 @@ +# Gopkg.toml example +# +# Refer to https://golang.github.io/dep/docs/Gopkg.toml.html +# for detailed Gopkg.toml documentation. +# +# required = ["github.com/user/thing/cmd/thing"] +# ignored = ["github.com/user/project/pkgX", "bitbucket.org/user/project/pkgA/pkgY"] +# +# [[constraint]] +# name = "github.com/user/project" +# version = "1.0.0" +# +# [[constraint]] +# name = "github.com/user/project2" +# branch = "dev" +# source = "github.com/myfork/project2" +# +# [[override]] +# name = "github.com/x/y" +# version = "2.4.0" +# +# [prune] +# non-go = false +# go-tests = true +# unused-packages = true + + +[[constraint]] + branch = "master" + name = "github.com/pohly/deptestmodules" + +[prune] + go-tests = true + unused-packages = true diff --git a/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/main.go b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/main.go new file mode 100644 index 0000000000..0f125e8351 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/main.go @@ -0,0 +1,8 @@ +package main + +import ( + _ "github.com/pohly/deptestmodules/pkg/foo" +) + +func main() { +} diff --git a/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/vendor/github.com/pohly/deptestmodules/pkg/foo/foo.go b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/vendor/github.com/pohly/deptestmodules/pkg/foo/foo.go new file mode 100644 index 0000000000..b549aef878 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/final/vendor/github.com/pohly/deptestmodules/pkg/foo/foo.go @@ -0,0 +1,7 @@ +/* +Copyright 2018 Intel Corporation. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package foo diff --git a/cmd/dep/testdata/harness_tests/ensure/submodules/case1/initial/main.go b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/initial/main.go new file mode 100644 index 0000000000..0f125e8351 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/initial/main.go @@ -0,0 +1,8 @@ +package main + +import ( + _ "github.com/pohly/deptestmodules/pkg/foo" +) + +func main() { +} diff --git a/cmd/dep/testdata/harness_tests/ensure/submodules/case1/testcase.json b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/testcase.json new file mode 100644 index 0000000000..0789716aef --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/submodules/case1/testcase.json @@ -0,0 +1,9 @@ +{ + "commands": [ + ["init"], + ["ensure"] + ], + "vendor-final": [ + "compare" + ] +} From 8447e4c615f0b4eb31eb216fe151e23d2ec40f45 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 23 Jul 2018 21:34:44 +0200 Subject: [PATCH 7/7] appvoyer: verbose test execution Without the logs it is impossible to understand why certain commands fail. --- appveyor.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/appveyor.yml b/appveyor.yml index 26d1728eec..008f80352e 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -31,4 +31,5 @@ deploy: false test_script: - go build github.com/golang/dep/cmd/dep + - go test ./cmd/dep -args -logs - for /f "" %%G in ('go list github.com/golang/dep/...') do ( go test %%G & IF ERRORLEVEL == 1 EXIT 1)