From 46146f6f3dadc84301b86ce8492adfa06171d6c3 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 3 Apr 2018 17:50:22 -0700 Subject: [PATCH] Remove 'teststale' As of go 1.10, go's own build cache will ensure we do not rebuild or relink tests. --- hack/BUILD | 1 - hack/cmd/teststale/BUILD | 39 ---- hack/cmd/teststale/teststale.go | 209 ----------------- hack/cmd/teststale/teststale_test.go | 325 --------------------------- hack/lib/golang.sh | 29 +-- hack/make-rules/test.sh | 18 +- test/test_owners.csv | 1 - 7 files changed, 3 insertions(+), 619 deletions(-) delete mode 100644 hack/cmd/teststale/BUILD delete mode 100644 hack/cmd/teststale/teststale.go delete mode 100644 hack/cmd/teststale/teststale_test.go diff --git a/hack/BUILD b/hack/BUILD index d562a44883b..9b7375e88d5 100644 --- a/hack/BUILD +++ b/hack/BUILD @@ -19,7 +19,6 @@ filegroup( srcs = [ ":package-srcs", "//hack/boilerplate:all-srcs", - "//hack/cmd/teststale:all-srcs", "//hack/e2e-internal:all-srcs", "//hack/lib:all-srcs", "//hack/make-rules:all-srcs", diff --git a/hack/cmd/teststale/BUILD b/hack/cmd/teststale/BUILD deleted file mode 100644 index 0fab86f81f5..00000000000 --- a/hack/cmd/teststale/BUILD +++ /dev/null @@ -1,39 +0,0 @@ -package(default_visibility = ["//visibility:public"]) - -load( - "@io_bazel_rules_go//go:def.bzl", - "go_binary", - "go_library", - "go_test", -) - -go_binary( - name = "teststale", - embed = [":go_default_library"], -) - -go_test( - name = "go_default_test", - srcs = ["teststale_test.go"], - embed = [":go_default_library"], -) - -go_library( - name = "go_default_library", - srcs = ["teststale.go"], - importpath = "k8s.io/kubernetes/hack/cmd/teststale", - deps = ["//vendor/github.com/golang/glog:go_default_library"], -) - -filegroup( - name = "package-srcs", - srcs = glob(["**"]), - tags = ["automanaged"], - visibility = ["//visibility:private"], -) - -filegroup( - name = "all-srcs", - srcs = [":package-srcs"], - tags = ["automanaged"], -) diff --git a/hack/cmd/teststale/teststale.go b/hack/cmd/teststale/teststale.go deleted file mode 100644 index 45cbc6a3f99..00000000000 --- a/hack/cmd/teststale/teststale.go +++ /dev/null @@ -1,209 +0,0 @@ -/* -Copyright 2016 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// teststale checks the staleness of a test binary. go test -c builds a test -// binary but it does no staleness check. In other words, every time one runs -// go test -c, it compiles the test packages and links the binary even when -// nothing has changed. This program helps to mitigate that problem by allowing -// to check the staleness of a given test package and its binary. -package main - -import ( - "encoding/json" - "flag" - "fmt" - "io" - "os" - "os/exec" - "path/filepath" - "time" - - "github.com/golang/glog" -) - -const usageHelp = "" + - `This program checks the staleness of a given test package and its test -binary so that one can make a decision about re-building the test binary. - -Usage: - teststale -binary=/path/to/test/binary -package=package - -Example: - teststale -binary="$HOME/gosrc/bin/e2e.test" -package="k8s.io/kubernetes/test/e2e" - -` - -var ( - binary = flag.String("binary", "", "filesystem path to the test binary file. Example: \"$HOME/gosrc/bin/e2e.test\"") - pkgPath = flag.String("package", "", "import path of the test package in the format used while importing packages. Example: \"k8s.io/kubernetes/test/e2e\"") -) - -func usage() { - fmt.Fprintln(os.Stderr, usageHelp) - fmt.Fprintln(os.Stderr, "Flags:") - flag.PrintDefaults() - os.Exit(2) -} - -// golist is an interface emulating the `go list` command to get package information. -// TODO: Evaluate using `go/build` package instead. It doesn't provide staleness -// information, but we can probably run `go list` and `go/build.Import()` concurrently -// in goroutines and merge the results. Evaluate if that's faster. -type golist interface { - pkgInfo(pkgPaths []string) ([]pkg, error) -} - -// execmd implements the `golist` interface. -type execcmd struct { - cmd string - args []string - env []string -} - -func (e *execcmd) pkgInfo(pkgPaths []string) ([]pkg, error) { - args := append(e.args, pkgPaths...) - cmd := exec.Command(e.cmd, args...) - cmd.Env = e.env - - stdout, err := cmd.StdoutPipe() - if err != nil { - return nil, fmt.Errorf("failed to obtain the metadata output stream: %v", err) - } - - dec := json.NewDecoder(stdout) - - // Start executing the command - if err := cmd.Start(); err != nil { - return nil, fmt.Errorf("command did not start: %v", err) - } - - var pkgs []pkg - for { - var p pkg - if err := dec.Decode(&p); err == io.EOF { - break - } else if err != nil { - return nil, fmt.Errorf("failed to unmarshal metadata for package %s: %v", p.ImportPath, err) - } - pkgs = append(pkgs, p) - } - - if err := cmd.Wait(); err != nil { - return nil, fmt.Errorf("command did not complete: %v", err) - } - return pkgs, nil -} - -type pkg struct { - Dir string - ImportPath string - Target string - Stale bool - TestGoFiles []string - TestImports []string - XTestGoFiles []string - XTestImports []string -} - -func (p *pkg) isNewerThan(cmd golist, buildTime time.Time) bool { - // If the package itself is stale, then we have to rebuild the whole thing anyway. - if p.Stale { - return true - } - - // Test for file staleness - for _, f := range p.TestGoFiles { - if isNewerThan(filepath.Join(p.Dir, f), buildTime) { - glog.V(4).Infof("test Go file %s is stale", f) - return true - } - } - for _, f := range p.XTestGoFiles { - if isNewerThan(filepath.Join(p.Dir, f), buildTime) { - glog.V(4).Infof("external test Go file %s is stale", f) - return true - } - } - - imps := []string{} - imps = append(imps, p.TestImports...) - imps = append(imps, p.XTestImports...) - - // This calls `go list` the second time. This is required because the first - // call to `go list` checks the staleness of the package in question by - // looking the non-test dependencies, but it doesn't look at the test - // dependencies. However, it returns the list of test dependencies. This - // second call to `go list` checks the staleness of all the test - // dependencies. - pkgs, err := cmd.pkgInfo(imps) - if err != nil || len(pkgs) < 1 { - glog.V(4).Infof("failed to obtain metadata for packages %s: %v", imps, err) - return true - } - - for _, p := range pkgs { - if p.Stale { - glog.V(4).Infof("import %q is stale", p.ImportPath) - return true - } - } - - return false -} - -func isNewerThan(filename string, buildTime time.Time) bool { - stat, err := os.Stat(filename) - if err != nil { - return true - } - return stat.ModTime().After(buildTime) -} - -// isTestStale checks if the test binary is stale and needs to rebuilt. -// Some of the ideas here are inspired by how Go does staleness checks. -func isTestStale(cmd golist, binPath, pkgPath string) bool { - bStat, err := os.Stat(binPath) - if err != nil { - glog.V(4).Infof("Couldn't obtain the modified time of the binary %s: %v", binPath, err) - return true - } - buildTime := bStat.ModTime() - - pkgs, err := cmd.pkgInfo([]string{pkgPath}) - if err != nil || len(pkgs) < 1 { - glog.V(4).Infof("Couldn't retrieve test package information for package %s: %v", pkgPath, err) - return false - } - - return pkgs[0].isNewerThan(cmd, buildTime) -} - -func main() { - flag.Usage = usage - flag.Parse() - - cmd := &execcmd{ - cmd: "go", - args: []string{ - "list", - "-json", - }, - env: os.Environ(), - } - if !isTestStale(cmd, *binary, *pkgPath) { - os.Exit(1) - } -} diff --git a/hack/cmd/teststale/teststale_test.go b/hack/cmd/teststale/teststale_test.go deleted file mode 100644 index aa4e4fc21f8..00000000000 --- a/hack/cmd/teststale/teststale_test.go +++ /dev/null @@ -1,325 +0,0 @@ -/* -Copyright 2016 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package main - -import ( - "fmt" - "io/ioutil" - "math/rand" - "os" - "path" - "path/filepath" - "testing" - "time" -) - -const ( - // seed for rand.Source to generate data for files - seed int64 = 42 - - // 1K binary file - binLen = 1024 - - // Directory of the test package relative to $GOPATH - testImportDir = "example.com/proj/pkg" -) - -var ( - pastHour = time.Now().Add(-1 * time.Hour) - - // The test package we are testing against - testPkg = path.Join(testImportDir, "test") -) - -// fakegolist implements the `golist` interface providing fake package information for testing. -type fakegolist struct { - dir string - importMap map[string]pkg - testFiles []string - binfile string -} - -func newFakegolist() (*fakegolist, error) { - dir, err := ioutil.TempDir("", "teststale") - if err != nil { - // test can't proceed without a temp directory. - return nil, fmt.Errorf("failed to create a temp directory for testing: %v", err) - } - - // Set the temp directory as the $GOPATH - if err := os.Setenv("GOPATH", dir); err != nil { - // can't proceed without pointing the $GOPATH to the temp directory. - return nil, fmt.Errorf("failed to set \"$GOPATH\" pointing to %q: %v", dir, err) - } - - // Setup $GOPATH directory layout. - // Yeah! I am bored of repeatedly writing "if err != nil {}"! - if os.MkdirAll(filepath.Join(dir, "bin"), 0750) != nil || - os.MkdirAll(filepath.Join(dir, "pkg", "linux_amd64"), 0750) != nil || - os.MkdirAll(filepath.Join(dir, "src"), 0750) != nil { - return nil, fmt.Errorf("failed to setup the $GOPATH directory structure") - } - - // Create a temp file to represent the test binary. - binfile, err := ioutil.TempFile("", "testbin") - if err != nil { - return nil, fmt.Errorf("failed to create the temp file to represent the test binary: %v", err) - } - - // Could have used crypto/rand instead, but it doesn't matter. - rr := rand.New(rand.NewSource(42)) - bin := make([]byte, binLen) - if _, err = rr.Read(bin); err != nil { - return nil, fmt.Errorf("couldn't read from the random source: %v", err) - } - if _, err := binfile.Write(bin); err != nil { - return nil, fmt.Errorf("couldn't write to the binary file %q: %v", binfile.Name(), err) - } - if err := binfile.Close(); err != nil { - // It is arguable whether this should be fatal. - return nil, fmt.Errorf("failed to close the binary file %q: %v", binfile.Name(), err) - } - - if err := os.Chtimes(binfile.Name(), time.Now(), time.Now()); err != nil { - return nil, fmt.Errorf("failed to modify the mtime of the binary file %q: %v", binfile.Name(), err) - } - - // Create test source files directory. - testdir := filepath.Join(dir, "src", testPkg) - if err := os.MkdirAll(testdir, 0750); err != nil { - return nil, fmt.Errorf("failed to create test source directory %q: %v", testdir, err) - } - - fgl := &fakegolist{ - dir: dir, - importMap: map[string]pkg{ - "example.com/proj/pkg/test": { - Dir: path.Join(dir, "src", testPkg), - ImportPath: testPkg, - Target: path.Join(dir, "pkg", "linux_amd64", testImportDir, "test.a"), - Stale: false, - TestGoFiles: []string{ - "foo_test.go", - "bar_test.go", - }, - TestImports: []string{ - "example.com/proj/pkg/p1", - "example.com/proj/pkg/p1/c11", - "example.com/proj/pkg/p2", - "example.com/proj/cmd/p3/c12/c23", - "strings", - "testing", - }, - XTestGoFiles: []string{ - "xfoo_test.go", - "xbar_test.go", - "xbaz_test.go", - }, - XTestImports: []string{ - "example.com/proj/pkg/test", - "example.com/proj/pkg/p1", - "example.com/proj/cmd/p3/c12/c23", - "os", - "testing", - }, - }, - "example.com/proj/pkg/p1": {Stale: false}, - "example.com/proj/pkg/p1/c11": {Stale: false}, - "example.com/proj/pkg/p2": {Stale: false}, - "example.com/proj/cmd/p3/c12/c23": {Stale: false}, - "strings": {Stale: false}, - "testing": {Stale: false}, - "os": {Stale: false}, - }, - testFiles: []string{ - "foo_test.go", - "bar_test.go", - "xfoo_test.go", - "xbar_test.go", - "xbaz_test.go", - }, - binfile: binfile.Name(), - } - - // Create test source files. - for _, fn := range fgl.testFiles { - fp := filepath.Join(testdir, fn) - if _, err := os.Create(fp); err != nil { - return nil, fmt.Errorf("failed to create the test file %q: %v", fp, err) - } - if err := os.Chtimes(fp, time.Now(), pastHour); err != nil { - return nil, fmt.Errorf("failed to modify the mtime of the test file %q: %v", binfile.Name(), err) - } - } - - return fgl, nil -} - -func (fgl *fakegolist) pkgInfo(pkgPaths []string) ([]pkg, error) { - var pkgs []pkg - for _, path := range pkgPaths { - p, ok := fgl.importMap[path] - if !ok { - return nil, fmt.Errorf("package %q not found", path) - } - pkgs = append(pkgs, p) - } - return pkgs, nil -} - -func (fgl *fakegolist) chMtime(filename string, mtime time.Time) error { - for _, fn := range fgl.testFiles { - if fn == filename { - fp := filepath.Join(fgl.dir, "src", testPkg, fn) - if err := os.Chtimes(fp, time.Now(), mtime); err != nil { - return fmt.Errorf("failed to modify the mtime of %q: %v", filename, err) - } - return nil - } - } - return fmt.Errorf("file %q not found", filename) -} - -func (fgl *fakegolist) chStale(pkg string, stale bool) error { - if p, ok := fgl.importMap[pkg]; ok { - p.Stale = stale - fgl.importMap[pkg] = p - return nil - } - return fmt.Errorf("package %q not found", pkg) -} - -func (fgl *fakegolist) cleanup() { - os.RemoveAll(fgl.dir) - os.Remove(fgl.binfile) -} - -func TestIsTestStale(t *testing.T) { - cases := []struct { - fileMtime map[string]time.Time - pkgStaleness map[string]bool - result bool - }{ - // Basic test: binary is fresh, all modifications were before the binary was built. - { - result: false, - }, - // A local test file is new, hence binary must be stale. - { - fileMtime: map[string]time.Time{ - "foo_test.go": time.Now().Add(1 * time.Hour), - }, - result: true, - }, - // Test package is new, so binary must be stale. - { - pkgStaleness: map[string]bool{ - "example.com/proj/pkg/test": true, - }, - result: true, - }, - // Test package dependencies are new, so binary must be stale. - { - pkgStaleness: map[string]bool{ - "example.com/proj/cmd/p3/c12/c23": true, - "strings": true, - }, - result: true, - }, - // External test files are new, hence binary must be stale. - { - fileMtime: map[string]time.Time{ - "xfoo_test.go": time.Now().Add(1 * time.Hour), - "xbar_test.go": time.Now().Add(2 * time.Hour), - }, - result: true, - }, - // External test dependency is new, so binary must be stale. - { - pkgStaleness: map[string]bool{ - "os": true, - }, - result: true, - }, - // Multiple source files and dependencies are new, so binary must be stale. - { - fileMtime: map[string]time.Time{ - "foo_test.go": time.Now().Add(1 * time.Hour), - "xfoo_test.go": time.Now().Add(2 * time.Hour), - "xbar_test.go": time.Now().Add(3 * time.Hour), - }, - pkgStaleness: map[string]bool{ - "example.com/proj/pkg/p1": true, - "example.com/proj/pkg/p1/c11": true, - "example.com/proj/pkg/p2": true, - "example.com/proj/cmd/p3/c12/c23": true, - "strings": true, - "os": true, - }, - result: true, - }, - // Everything is new, so binary must be stale. - { - fileMtime: map[string]time.Time{ - "foo_test.go": time.Now().Add(3 * time.Hour), - "bar_test.go": time.Now().Add(1 * time.Hour), - "xfoo_test.go": time.Now().Add(2 * time.Hour), - "xbar_test.go": time.Now().Add(1 * time.Hour), - "xbaz_test.go": time.Now().Add(2 * time.Hour), - }, - pkgStaleness: map[string]bool{ - "example.com/proj/pkg/p1": true, - "example.com/proj/pkg/p1/c11": true, - "example.com/proj/pkg/p2": true, - "example.com/proj/cmd/p3/c12/c23": true, - "example.com/proj/pkg/test": true, - "strings": true, - "testing": true, - "os": true, - }, - result: true, - }, - } - - for _, tc := range cases { - fgl, err := newFakegolist() - if err != nil { - t.Fatalf("failed to setup the test: %v", err) - } - defer fgl.cleanup() - - for fn, mtime := range tc.fileMtime { - if err := fgl.chMtime(fn, mtime); err != nil { - t.Fatalf("failed to change the mtime of %q: %v", fn, err) - } - } - - for pkg, stale := range tc.pkgStaleness { - if err := fgl.chStale(pkg, stale); err != nil { - t.Fatalf("failed to change the staleness of %q: %v", pkg, err) - } - } - - if tc.result != isTestStale(fgl, fgl.binfile, testPkg) { - if tc.result { - t.Errorf("Expected test package %q to be stale", testPkg) - } else { - t.Errorf("Expected test package %q to be not stale", testPkg) - } - } - } -} diff --git a/hack/lib/golang.sh b/hack/lib/golang.sh index a6ffd3ad944..14266742016 100755 --- a/hack/lib/golang.sh +++ b/hack/lib/golang.sh @@ -423,7 +423,6 @@ kube::golang::place_bins() { # Ideally, not a shell script because testing shell scripts is painful. kube::golang::build_kube_toolchain() { local targets=( - hack/cmd/teststale vendor/github.com/jteeuwen/go-bindata/go-bindata ) @@ -513,33 +512,9 @@ kube::golang::build_binaries_for_platform() { local testpkg="$(dirname ${test})" - # Staleness check always happens on the host machine, so we don't - # have to locate the `teststale` binaries for the other platforms. - # Since we place the host binaries in `$KUBE_GOPATH/bin`, we can - # assume that the binary exists there, if it exists at all. - # Otherwise, something has gone wrong with building the `teststale` - # binary and we should safely proceed building the test binaries - # assuming that they are stale. There is no good reason to error - # out. - if test -x "${KUBE_GOPATH}/bin/teststale" && ! "${KUBE_GOPATH}/bin/teststale" -binary "${outfile}" -package "${testpkg}" - then - continue - fi - - # `go test -c` below directly builds the binary. It builds the packages, - # but it never installs them. `go test -i` only installs the dependencies - # of the test, but not the test package itself. So neither `go test -c` - # nor `go test -i` installs, for example, test/e2e.a. And without that, - # doing a staleness check on k8s.io/kubernetes/test/e2e package always - # returns true (always stale). And that's why we need to install the - # test package. - go install "${goflags[@]:+${goflags[@]}}" \ - -gcflags "${gogcflags}" \ - -ldflags "${goldflags}" \ - "${testpkg}" - mkdir -p "$(dirname ${outfile})" - go test -i -c \ + + go test -c \ "${goflags[@]:+${goflags[@]}}" \ -gcflags "${gogcflags}" \ -ldflags "${goldflags}" \ diff --git a/hack/make-rules/test.sh b/hack/make-rules/test.sh index 54feeea8ea8..14090ed8a3c 100755 --- a/hack/make-rules/test.sh +++ b/hack/make-rules/test.sh @@ -278,12 +278,6 @@ runTests() { # command, which is much faster. if [[ ! ${KUBE_COVER} =~ ^[yY]$ ]]; then kube::log::status "Running tests without code coverage" - # `go test` does not install the things it builds. `go test -i` installs - # the build artifacts but doesn't run the tests. The two together provide - # a large speedup for tests that do not need to be rebuilt. - go test -i "${goflags[@]:+${goflags[@]}}" \ - ${KUBE_RACE} ${KUBE_TIMEOUT} "${@}" \ - "${testargs[@]:+${testargs[@]}}" go test "${goflags[@]:+${goflags[@]}}" \ ${KUBE_RACE} ${KUBE_TIMEOUT} "${@}" \ "${testargs[@]:+${testargs[@]}}" \ @@ -319,21 +313,11 @@ runTests() { for path in $(echo $cover_ignore_dirs | sed 's/|/ /g'); do echo -e "skipped\tk8s.io/kubernetes/$path" done - # - # `go test` does not install the things it builds. `go test -i` installs - # the build artifacts but doesn't run the tests. The two together provide - # a large speedup for tests that do not need to be rebuilt. + printf "%s\n" "${@}" \ | grep -Ev $cover_ignore_dirs \ | xargs -I{} -n 1 -P ${KUBE_COVERPROCS} \ bash -c "set -o pipefail; _pkg=\"\$0\"; _pkg_out=\${_pkg//\//_}; \ - go test -i ${goflags[@]:+${goflags[@]}} \ - ${KUBE_RACE} \ - ${KUBE_TIMEOUT} \ - -cover -covermode=\"${KUBE_COVERMODE}\" \ - -coverprofile=\"${cover_report_dir}/\${_pkg}/${cover_profile}\" \ - \"\${_pkg}\" \ - ${testargs[@]:+${testargs[@]}} go test ${goflags[@]:+${goflags[@]}} \ ${KUBE_RACE} \ ${KUBE_TIMEOUT} \ diff --git a/test/test_owners.csv b/test/test_owners.csv index 0d30b8f08bc..3ea2d31fbc7 100644 --- a/test/test_owners.csv +++ b/test/test_owners.csv @@ -544,7 +544,6 @@ k8s.io/kubernetes/cmd/libs/go2idl/go-to-protobuf/protobuf,smarterclayton,0, k8s.io/kubernetes/cmd/libs/go2idl/openapi-gen/generators,davidopp,1, k8s.io/kubernetes/examples,Random-Liu,0, k8s.io/kubernetes/hack,thockin,1, -k8s.io/kubernetes/hack/cmd/teststale,thockin,1, k8s.io/kubernetes/pkg/api,Q-Lee,1, k8s.io/kubernetes/pkg/api/endpoints,cjcullen,1, k8s.io/kubernetes/pkg/api/events,jlowdermilk,1,