From dcaf005ffe8ad2271455043e74e2cd934db8a5af Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Tue, 26 Apr 2016 23:13:14 -0700 Subject: [PATCH 1/7] Improve the speed of do-nothing build. As thockin found out here https://github.com/kubernetes/kubernetes/issues/24518, vast majority of the do-nothing build time is spent in rebuilding the test binaries. There is no staleness check support for test binaries. This commit implements the staleness checks for test binaries and uses them while building packages. Tests are TBD. I am still trying to figure out how to test this. --- hack/cmd/teststale/teststale.go | 203 ++++++++++++++++++++++++++++++++ hack/lib/golang.sh | 13 +- 2 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 hack/cmd/teststale/teststale.go diff --git a/hack/cmd/teststale/teststale.go b/hack/cmd/teststale/teststale.go new file mode 100644 index 00000000000..e78f75e7a98 --- /dev/null +++ b/hack/cmd/teststale/teststale.go @@ -0,0 +1,203 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +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 ( + "bufio" + "flag" + "fmt" + "log" + "os" + "os/exec" + "path/filepath" + "strings" + "time" +) + +var ( + binary = flag.String("binary", "", "absolute filesystem path to the test binary") + pkgPath = flag.String("package", "", "test package import path in the format used in the import statements without the $GOPATH prefix") +) + +type pkg struct { + dir string + target string + stale bool + testGoFiles []string + testImports []string + xTestGoFiles []string + xTestImports []string +} + +func newCmd(format string, pkgPaths []string) *exec.Cmd { + args := []string{ + "list", + "-f", + format, + } + args = append(args, pkgPaths...) + cmd := exec.Command("go", args...) + cmd.Env = os.Environ() + return cmd +} + +func newPkg(path string) (*pkg, error) { + format := "Dir: {{println .Dir}}Target: {{println .Target}}Stale: {{println .Stale}}TestGoFiles: {{println .TestGoFiles}}TestImports: {{println .TestImports}}XTestGoFiles: {{println .XTestGoFiles}}XTestImports: {{println .XTestImports}}" + cmd := newCmd(format, []string{path}) + stdout, err := cmd.StdoutPipe() + if err != nil { + return nil, fmt.Errorf("could not pipe STDOUT: %v", err) + } + + // Start executing the command + if err := cmd.Start(); err != nil { + return nil, fmt.Errorf("command did not start: %v", err) + } + + // Parse the command output + scanner := bufio.NewScanner(stdout) + scanner.Split(bufio.ScanLines) + + // To be conservative, default to package to be stale + p := &pkg{ + stale: true, + } + + // TODO: avoid this stupid code repetition by iterating through struct fields. + scanner.Scan() + p.dir = strings.TrimPrefix(scanner.Text(), "Dir: ") + scanner.Scan() + p.target = strings.TrimPrefix(scanner.Text(), "Target: ") + scanner.Scan() + if strings.TrimPrefix(scanner.Text(), "Stale: ") == "false" { + p.stale = false + } + p.testGoFiles = scanLineList(scanner, "TestGoFiles: ") + p.testImports = scanLineList(scanner, "TestImports: ") + p.xTestGoFiles = scanLineList(scanner, "XTestGoFiles: ") + p.xTestImports = scanLineList(scanner, "XTestImports: ") + + if err := cmd.Wait(); err != nil { + return nil, fmt.Errorf("command did not complete: %v", err) + } + return p, nil +} + +func (p *pkg) isStale(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 isStale(buildTime, filepath.Join(p.dir, f)) { + log.Printf("test Go file %s is stale", f) + return true + } + } + for _, f := range p.xTestGoFiles { + if isStale(buildTime, filepath.Join(p.dir, f)) { + log.Printf("external test Go file %s is stale", f) + return true + } + } + + format := "{{.Stale}}" + imps := []string{} + imps = append(imps, p.testImports...) + imps = append(imps, p.xTestImports...) + + cmd := newCmd(format, imps) + stdout, err := cmd.StdoutPipe() + if err != nil { + log.Printf("unexpected error with creating stdout pipe: %v", err) + return true + } + // Start executing the command + if err := cmd.Start(); err != nil { + log.Printf("unexpected error executing command: %v", err) + return true + } + + // Parse the command output + scanner := bufio.NewScanner(stdout) + scanner.Split(bufio.ScanLines) + + for i := 0; scanner.Scan(); i++ { + if out := scanner.Text(); out != "false" { + log.Printf("import %q is stale: %s", imps[i], out) + return true + } + } + + if err := cmd.Wait(); err != nil { + log.Printf("unexpected error waiting to finish: %v", err) + return true + } + return false +} + +// scanLineList scans a line, removes the prefix and splits the remaining line into +// individual strings. +// TODO: There are ton of intermediate strings being created here. Convert this to +// a bufio.SplitFunc instead. +func scanLineList(scanner *bufio.Scanner, prefix string) []string { + scanner.Scan() + list := strings.TrimPrefix(scanner.Text(), prefix) + line := strings.Trim(list, "[]") + if len(line) == 0 { + return []string{} + } + return strings.Split(line, " ") +} + +func isStale(buildTime time.Time, filename string) 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(binPath, pkgPath string) bool { + bStat, err := os.Stat(binPath) + if err != nil { + log.Printf("Couldn't obtain the modified time of the binary: %v", err) + return true + } + buildTime := bStat.ModTime() + + p, err := newPkg(pkgPath) + if err != nil { + log.Printf("Couldn't retrieve the test package information: %v", err) + return false + } + + return p.isStale(buildTime) +} + +func main() { + flag.Parse() + if isTestStale(*binary, *pkgPath) { + fmt.Println("true") + } else { + fmt.Println("false") + } +} diff --git a/hack/lib/golang.sh b/hack/lib/golang.sh index 95e1ce8c790..be147ae1450 100755 --- a/hack/lib/golang.sh +++ b/hack/lib/golang.sh @@ -114,6 +114,7 @@ kube::golang::test_targets() { cmd/linkcheck examples/k8petstore/web-server/src vendor/github.com/onsi/ginkgo/ginkgo + hack/cmd/teststale test/e2e/e2e.test test/e2e_node/e2e_node.test ) @@ -443,15 +444,25 @@ kube::golang::build_binaries_for_platform() { fi fi + teststale=$(kube::golang::output_filename_for_binary "hack/cmd/teststale" "${platform}") for test in "${tests[@]:+${tests[@]}}"; do local outfile=$(kube::golang::output_filename_for_binary "${test}" \ "${platform}") + + local testpkg="$(dirname ${test})" + if [[ "$(${teststale} -binary "${outfile}" -package "${testpkg}")" == "false" ]]; then + continue + fi + go install "${goflags[@]:+${goflags[@]}}" \ + -ldflags "${goldflags}" \ + "${testpkg}" + mkdir -p "$(dirname ${outfile})" go test -c \ "${goflags[@]:+${goflags[@]}}" \ -ldflags "${goldflags}" \ -o "${outfile}" \ - "$(dirname ${test})" + "${testpkg}" done } From dd154350e888b7d9ee444faaa78c452f7c0daced Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Tue, 10 May 2016 13:43:33 -0700 Subject: [PATCH 2/7] Address review comments. --- hack/cmd/teststale/teststale.go | 179 +++++++++++++++----------------- hack/lib/golang.sh | 9 +- 2 files changed, 90 insertions(+), 98 deletions(-) diff --git a/hack/cmd/teststale/teststale.go b/hack/cmd/teststale/teststale.go index e78f75e7a98..48974b61371 100644 --- a/hack/cmd/teststale/teststale.go +++ b/hack/cmd/teststale/teststale.go @@ -14,159 +14,145 @@ 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 ( - "bufio" + "encoding/json" "flag" "fmt" + "io" "log" "os" "os/exec" "path/filepath" - "strings" "time" ) +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", "", "absolute filesystem path to the test binary") - pkgPath = flag.String("package", "", "test package import path in the format used in the import statements without the $GOPATH prefix") + 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\"") ) -type pkg struct { - dir string - target string - stale bool - testGoFiles []string - testImports []string - xTestGoFiles []string - xTestImports []string +func usage() { + fmt.Fprintln(os.Stderr, usageHelp) + fmt.Fprintln(os.Stderr, "Flags:") + flag.PrintDefaults() + os.Exit(2) } -func newCmd(format string, pkgPaths []string) *exec.Cmd { +type pkg struct { + Dir string + ImportPath string + Target string + Stale bool + TestGoFiles []string + TestImports []string + XTestGoFiles []string + XTestImports []string +} + +func pkgInfo(pkgPaths []string) ([]pkg, error) { args := []string{ "list", - "-f", - format, + "-json", } args = append(args, pkgPaths...) cmd := exec.Command("go", args...) cmd.Env = os.Environ() - return cmd -} -func newPkg(path string) (*pkg, error) { - format := "Dir: {{println .Dir}}Target: {{println .Target}}Stale: {{println .Stale}}TestGoFiles: {{println .TestGoFiles}}TestImports: {{println .TestImports}}XTestGoFiles: {{println .XTestGoFiles}}XTestImports: {{println .XTestImports}}" - cmd := newCmd(format, []string{path}) stdout, err := cmd.StdoutPipe() if err != nil { - return nil, fmt.Errorf("could not pipe STDOUT: %v", err) + 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) } - // Parse the command output - scanner := bufio.NewScanner(stdout) - scanner.Split(bufio.ScanLines) - - // To be conservative, default to package to be stale - p := &pkg{ - stale: true, + 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) } - // TODO: avoid this stupid code repetition by iterating through struct fields. - scanner.Scan() - p.dir = strings.TrimPrefix(scanner.Text(), "Dir: ") - scanner.Scan() - p.target = strings.TrimPrefix(scanner.Text(), "Target: ") - scanner.Scan() - if strings.TrimPrefix(scanner.Text(), "Stale: ") == "false" { - p.stale = false - } - p.testGoFiles = scanLineList(scanner, "TestGoFiles: ") - p.testImports = scanLineList(scanner, "TestImports: ") - p.xTestGoFiles = scanLineList(scanner, "XTestGoFiles: ") - p.xTestImports = scanLineList(scanner, "XTestImports: ") - if err := cmd.Wait(); err != nil { return nil, fmt.Errorf("command did not complete: %v", err) } - return p, nil + return pkgs, nil } -func (p *pkg) isStale(buildTime time.Time) bool { +func (p *pkg) isNewerThan(buildTime time.Time) bool { // If the package itself is stale, then we have to rebuild the whole thing anyway. - if p.stale { + if p.Stale { return true } // Test for file staleness - for _, f := range p.testGoFiles { - if isStale(buildTime, filepath.Join(p.dir, f)) { + for _, f := range p.TestGoFiles { + if isNewerThan(filepath.Join(p.Dir, f), buildTime) { log.Printf("test Go file %s is stale", f) return true } } - for _, f := range p.xTestGoFiles { - if isStale(buildTime, filepath.Join(p.dir, f)) { + for _, f := range p.XTestGoFiles { + if isNewerThan(filepath.Join(p.Dir, f), buildTime) { log.Printf("external test Go file %s is stale", f) return true } } - format := "{{.Stale}}" imps := []string{} - imps = append(imps, p.testImports...) - imps = append(imps, p.xTestImports...) + imps = append(imps, p.TestImports...) + imps = append(imps, p.XTestImports...) - cmd := newCmd(format, imps) - stdout, err := cmd.StdoutPipe() - if err != nil { - log.Printf("unexpected error with creating stdout pipe: %v", err) - return true - } - // Start executing the command - if err := cmd.Start(); err != nil { - log.Printf("unexpected error executing command: %v", err) + // 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 := pkgInfo(imps) + if err != nil || len(pkgs) < 1 { + log.Printf("failed to obtain metadata for packages %s: %v", imps, err) return true } - // Parse the command output - scanner := bufio.NewScanner(stdout) - scanner.Split(bufio.ScanLines) - - for i := 0; scanner.Scan(); i++ { - if out := scanner.Text(); out != "false" { - log.Printf("import %q is stale: %s", imps[i], out) + for _, p := range pkgs { + if p.Stale { + log.Printf("import %q is stale", p.ImportPath) return true } } - if err := cmd.Wait(); err != nil { - log.Printf("unexpected error waiting to finish: %v", err) - return true - } return false } -// scanLineList scans a line, removes the prefix and splits the remaining line into -// individual strings. -// TODO: There are ton of intermediate strings being created here. Convert this to -// a bufio.SplitFunc instead. -func scanLineList(scanner *bufio.Scanner, prefix string) []string { - scanner.Scan() - list := strings.TrimPrefix(scanner.Text(), prefix) - line := strings.Trim(list, "[]") - if len(line) == 0 { - return []string{} - } - return strings.Split(line, " ") -} - -func isStale(buildTime time.Time, filename string) bool { +func isNewerThan(filename string, buildTime time.Time) bool { stat, err := os.Stat(filename) if err != nil { return true @@ -174,30 +160,29 @@ func isStale(buildTime time.Time, filename string) bool { return stat.ModTime().After(buildTime) } -// IsTestStale checks if the test binary is stale and needs to rebuilt. +// 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(binPath, pkgPath string) bool { bStat, err := os.Stat(binPath) if err != nil { - log.Printf("Couldn't obtain the modified time of the binary: %v", err) + log.Printf("Couldn't obtain the modified time of the binary %s: %v", binPath, err) return true } buildTime := bStat.ModTime() - p, err := newPkg(pkgPath) - if err != nil { - log.Printf("Couldn't retrieve the test package information: %v", err) + pkgs, err := pkgInfo([]string{pkgPath}) + if err != nil || len(pkgs) < 1 { + log.Printf("Couldn't retrieve test package information for package %s: %v", pkgPath, err) return false } - return p.isStale(buildTime) + return pkgs[0].isNewerThan(buildTime) } func main() { + flag.Usage = usage flag.Parse() - if isTestStale(*binary, *pkgPath) { - fmt.Println("true") - } else { - fmt.Println("false") + if !isTestStale(*binary, *pkgPath) { + os.Exit(1) } } diff --git a/hack/lib/golang.sh b/hack/lib/golang.sh index be147ae1450..846de5a7924 100755 --- a/hack/lib/golang.sh +++ b/hack/lib/golang.sh @@ -450,9 +450,16 @@ kube::golang::build_binaries_for_platform() { "${platform}") local testpkg="$(dirname ${test})" - if [[ "$(${teststale} -binary "${outfile}" -package "${testpkg}")" == "false" ]]; then + if ! ${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[@]}}" \ -ldflags "${goldflags}" \ "${testpkg}" From 704ef5bb6ec1fdddc9e5608953c3bad3e2aaa564 Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Tue, 10 May 2016 15:51:15 -0700 Subject: [PATCH 3/7] Replace log with glog. Show log messages to users is unnecessary. It is only for debugging purposes. --- hack/cmd/teststale/teststale.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/hack/cmd/teststale/teststale.go b/hack/cmd/teststale/teststale.go index 48974b61371..8dcbddb79bb 100644 --- a/hack/cmd/teststale/teststale.go +++ b/hack/cmd/teststale/teststale.go @@ -26,11 +26,12 @@ import ( "flag" "fmt" "io" - "log" "os" "os/exec" "path/filepath" "time" + + "github.com/golang/glog" ) const usageHelp = "" + @@ -115,13 +116,13 @@ func (p *pkg) isNewerThan(buildTime time.Time) bool { // Test for file staleness for _, f := range p.TestGoFiles { if isNewerThan(filepath.Join(p.Dir, f), buildTime) { - log.Printf("test Go file %s is stale", f) + 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) { - log.Printf("external test Go file %s is stale", f) + glog.V(4).Infof("external test Go file %s is stale", f) return true } } @@ -138,13 +139,13 @@ func (p *pkg) isNewerThan(buildTime time.Time) bool { // dependencies. pkgs, err := pkgInfo(imps) if err != nil || len(pkgs) < 1 { - log.Printf("failed to obtain metadata for packages %s: %v", imps, err) + glog.V(4).Infof("failed to obtain metadata for packages %s: %v", imps, err) return true } for _, p := range pkgs { if p.Stale { - log.Printf("import %q is stale", p.ImportPath) + glog.V(4).Infof("import %q is stale", p.ImportPath) return true } } @@ -165,14 +166,14 @@ func isNewerThan(filename string, buildTime time.Time) bool { func isTestStale(binPath, pkgPath string) bool { bStat, err := os.Stat(binPath) if err != nil { - log.Printf("Couldn't obtain the modified time of the binary %s: %v", binPath, err) + glog.V(4).Infof("Couldn't obtain the modified time of the binary %s: %v", binPath, err) return true } buildTime := bStat.ModTime() pkgs, err := pkgInfo([]string{pkgPath}) if err != nil || len(pkgs) < 1 { - log.Printf("Couldn't retrieve test package information for package %s: %v", pkgPath, err) + glog.V(4).Infof("Couldn't retrieve test package information for package %s: %v", pkgPath, err) return false } From 55589e070b3196f6c0af8d237952f07c4173d7f0 Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Sat, 14 May 2016 05:03:15 -0700 Subject: [PATCH 4/7] Virtualize the constructs a bit to make the code testable. --- hack/cmd/teststale/teststale.go | 66 +++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/hack/cmd/teststale/teststale.go b/hack/cmd/teststale/teststale.go index 8dcbddb79bb..2da1bb85677 100644 --- a/hack/cmd/teststale/teststale.go +++ b/hack/cmd/teststale/teststale.go @@ -58,25 +58,25 @@ func usage() { os.Exit(2) } -type pkg struct { - Dir string - ImportPath string - Target string - Stale bool - TestGoFiles []string - TestImports []string - XTestGoFiles []string - XTestImports []string +// 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) } -func pkgInfo(pkgPaths []string) ([]pkg, error) { - args := []string{ - "list", - "-json", - } - args = append(args, pkgPaths...) - cmd := exec.Command("go", args...) - cmd.Env = os.Environ() +// 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 { @@ -107,7 +107,18 @@ func pkgInfo(pkgPaths []string) ([]pkg, error) { return pkgs, nil } -func (p *pkg) isNewerThan(buildTime time.Time) bool { +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 @@ -137,7 +148,7 @@ func (p *pkg) isNewerThan(buildTime time.Time) bool { // 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 := pkgInfo(imps) + 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 @@ -163,7 +174,7 @@ func isNewerThan(filename string, buildTime time.Time) bool { // 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(binPath, pkgPath string) bool { +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) @@ -171,19 +182,28 @@ func isTestStale(binPath, pkgPath string) bool { } buildTime := bStat.ModTime() - pkgs, err := pkgInfo([]string{pkgPath}) + 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(buildTime) + return pkgs[0].isNewerThan(cmd, buildTime) } func main() { flag.Usage = usage flag.Parse() - if !isTestStale(*binary, *pkgPath) { + + cmd := &execcmd{ + cmd: "go", + args: []string{ + "list", + "-json", + }, + env: os.Environ(), + } + if !isTestStale(cmd, *binary, *pkgPath) { os.Exit(1) } } From 317a7d2e522ddaaca23fd64865f98c7db9520067 Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Sat, 14 May 2016 05:05:01 -0700 Subject: [PATCH 5/7] Write the first test: Isn't it strange that the elaborate setup that is required alone is somewhat longer than the actual code to do the staleness check\!? --- hack/cmd/teststale/teststale_test.go | 199 +++++++++++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 hack/cmd/teststale/teststale_test.go diff --git a/hack/cmd/teststale/teststale_test.go b/hack/cmd/teststale/teststale_test.go new file mode 100644 index 00000000000..90dd47bd908 --- /dev/null +++ b/hack/cmd/teststale/teststale_test.go @@ -0,0 +1,199 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +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) cleanup() { + os.RemoveAll(fgl.dir) + os.Remove(fgl.binfile) +} + +func TestIsTestStale(t *testing.T) { + fgl, err := newFakegolist() + if err != nil { + t.Fatalf("failed to setup the test: %v", err) + } + defer fgl.cleanup() + + if isTestStale(fgl, fgl.binfile, testPkg) { + t.Errorf("Expected test package %q to be not stale", testPkg) + } +} From 7e486bd777936ee3e71e628baf3c6954110070a9 Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Sat, 14 May 2016 13:17:14 -0700 Subject: [PATCH 6/7] Build and use the teststale binary on only the host platform. --- hack/lib/golang.sh | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/hack/lib/golang.sh b/hack/lib/golang.sh index 846de5a7924..40b19d579dc 100755 --- a/hack/lib/golang.sh +++ b/hack/lib/golang.sh @@ -114,7 +114,6 @@ kube::golang::test_targets() { cmd/linkcheck examples/k8petstore/web-server/src vendor/github.com/onsi/ginkgo/ginkgo - hack/cmd/teststale test/e2e/e2e.test test/e2e_node/e2e_node.test ) @@ -375,6 +374,25 @@ kube::golang::fallback_if_stdlib_not_installable() { use_go_build=true } +# Builds the toolchain necessary for building kube. This needs to be +# built only on the host platform. +# TODO: This builds only the `teststale` binary right now. As we expand +# this function's capabilities we need to find this a right home. +# Ideally, not a shell script because testing shell scripts is painful. +kube::golang::build_kube_toolchain() { + local targets=( + hack/cmd/teststale + ) + + local binaries + binaries=($(kube::golang::binaries_from_targets "${targets[@]}")) + + kube::log::status "Building the toolchain targets:" "${binaries[@]}" + go install "${goflags[@]:+${goflags[@]}}" \ + -ldflags "${goldflags}" \ + "${binaries[@]:+${binaries[@]}}" +} + # Try and replicate the native binary placement of go install without # calling go install. kube::golang::output_filename_for_binary() { @@ -444,15 +462,25 @@ kube::golang::build_binaries_for_platform() { fi fi - teststale=$(kube::golang::output_filename_for_binary "hack/cmd/teststale" "${platform}") for test in "${tests[@]:+${tests[@]}}"; do local outfile=$(kube::golang::output_filename_for_binary "${test}" \ "${platform}") local testpkg="$(dirname ${test})" - if ! ${teststale} -binary "${outfile}" -package "${testpkg}"; then + + # 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` @@ -564,6 +592,9 @@ kube::golang::build_binaries() { fi fi + # First build the toolchain before building any other targets + kube::golang::build_kube_toolchain + if [[ "${parallel}" == "true" ]]; then kube::log::status "Building go targets for ${platforms[@]} in parallel (output will appear in a burst when complete):" "${targets[@]}" local platform From 47efee7ce4d5e10b89785af05abcb3b81a72f585 Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Sat, 14 May 2016 23:19:18 -0700 Subject: [PATCH 7/7] Add more tests. --- hack/cmd/teststale/teststale_test.go | 138 +++++++++++++++++++++++++-- 1 file changed, 132 insertions(+), 6 deletions(-) diff --git a/hack/cmd/teststale/teststale_test.go b/hack/cmd/teststale/teststale_test.go index 90dd47bd908..45096a8a8e2 100644 --- a/hack/cmd/teststale/teststale_test.go +++ b/hack/cmd/teststale/teststale_test.go @@ -181,19 +181,145 @@ func (fgl *fakegolist) pkgInfo(pkgPaths []string) ([]pkg, error) { 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) { - fgl, err := newFakegolist() - if err != nil { - t.Fatalf("failed to setup the test: %v", err) + 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, + }, } - defer fgl.cleanup() - if isTestStale(fgl, fgl.binfile, testPkg) { - t.Errorf("Expected test package %q to be not stale", testPkg) + 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) + } + } } }