Merge pull request #1080 from dgageot/improve-tests

Couple of improvements to the test phase

pkg/skaffold/runner/runner.go

@@ -190,7 +190,7 @@ func (r *SkaffoldRunner) Run(ctx context.Context, out io.Writer, artifacts []*la
 		return errors.Wrap(err, "build step")
 	}
 
-	if err = r.Test(out, bRes); err != nil {
+	if err = r.Test(ctx, out, bRes); err != nil {
 		return errors.Wrap(err, "test step")
 	}
 
@@ -257,7 +257,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 			}
 
 			r.updateBuiltImages(imageList, bRes)
-			if err := r.Test(out, bRes); err != nil {
+			if err := r.Test(ctx, out, bRes); err != nil {
 				logrus.Warnln("Skipping Deploy due to failed tests:", err)
 				return nil
 			}
@@ -267,7 +267,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 				return nil
 			}
 		case changed.needsRedeploy:
-			if err := r.Test(out, r.builds); err != nil {
+			if err := r.Test(ctx, out, r.builds); err != nil {
 				logrus.Warnln("Skipping Deploy due to failed tests:", err)
 				return nil
 			}
@@ -301,7 +301,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 
 	// Watch test configuration
 	if err := watcher.Register(
-		func() ([]string, error) { return r.TestDependencies(), nil },
+		func() ([]string, error) { return r.TestDependencies() },
 		func(watch.Events) { changed.needsRedeploy = true },
 	); err != nil {
 		return nil, errors.Wrap(err, "watching test files")
@@ -330,7 +330,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 	}
 
 	r.updateBuiltImages(imageList, bRes)
-	if err := r.Test(out, bRes); err != nil {
+	if err := r.Test(ctx, out, bRes); err != nil {
 		return nil, errors.Wrap(err, "exiting dev mode because the first test run failed")
 	}
 

pkg/skaffold/runner/runner_test.go

@@ -70,7 +70,7 @@ type TestTester struct {
 	errors []error
 }
 
-func (t *TestTester) Test(out io.Writer, builds []build.Artifact) error {
+func (t *TestTester) Test(ctx context.Context, out io.Writer, builds []build.Artifact) error {
 	if len(t.errors) > 0 {
 		err := t.errors[0]
 		t.errors = t.errors[1:]
@@ -79,8 +79,8 @@ func (t *TestTester) Test(out io.Writer, builds []build.Artifact) error {
 	return nil
 }
 
-func (t *TestTester) TestDependencies() []string {
-	return []string{}
+func (t *TestTester) TestDependencies() ([]string, error) {
+	return nil, nil
 }
 
 type TestDeployer struct {

pkg/skaffold/runner/timings.go

@@ -21,6 +21,8 @@ import (
 	"io"
 	"time"
 
+	"k8s.io/apimachinery/pkg/labels"
+
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
@@ -46,57 +48,58 @@ type withTimings struct {
 	deploy.Deployer
 }
 
+func (w withTimings) Labels() map[string]string {
+	return labels.Merge(w.Builder.Labels(), w.Deployer.Labels())
+}
+
 func (w withTimings) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*latest.Artifact) ([]build.Artifact, error) {
 	start := time.Now()
 	color.Default.Fprintln(out, "Starting build...")
 
 	bRes, err := w.Builder.Build(ctx, out, tagger, artifacts)
-	if err == nil {
-		color.Default.Fprintln(out, "Build complete in", time.Since(start))
+	if err != nil {
+		return nil, err
 	}
-	return bRes, err
+
+	color.Default.Fprintln(out, "Build complete in", time.Since(start))
+	return bRes, nil
 }
 
-func (w withTimings) Test(out io.Writer, builds []build.Artifact) error {
+func (w withTimings) Test(ctx context.Context, out io.Writer, builds []build.Artifact) error {
 	start := time.Now()
 	color.Default.Fprintln(out, "Starting test...")
 
-	err := w.Tester.Test(out, builds)
-	if err == nil {
-		color.Default.Fprintln(out, "Test complete in", time.Since(start))
+	err := w.Tester.Test(ctx, out, builds)
+	if err != nil {
+		return err
 	}
-	return err
+
+	color.Default.Fprintln(out, "Test complete in", time.Since(start))
+	return nil
 }
 
 func (w withTimings) Deploy(ctx context.Context, out io.Writer, builds []build.Artifact) ([]deploy.Artifact, error) {
 	start := time.Now()
 	color.Default.Fprintln(out, "Starting deploy...")
 
 	dRes, err := w.Deployer.Deploy(ctx, out, builds)
-	if err == nil {
-		color.Default.Fprintln(out, "Deploy complete in", time.Since(start))
+	if err != nil {
+		return nil, err
 	}
-	return dRes, err
-}
 
-func (w withTimings) Labels() map[string]string {
-	labels := map[string]string{}
-	for k, v := range w.Builder.Labels() {
-		labels[k] = v
-	}
-	for k, v := range w.Deployer.Labels() {
-		labels[k] = v
-	}
-	return labels
+	color.Default.Fprintln(out, "Deploy complete in", time.Since(start))
+	return dRes, nil
 }
 
 func (w withTimings) Cleanup(ctx context.Context, out io.Writer) error {
 	start := time.Now()
 	color.Default.Fprintln(out, "Cleaning up...")
 
 	err := w.Deployer.Cleanup(ctx, out)
-	if err == nil {
-		color.Default.Fprintln(out, "Cleanup complete in", time.Since(start))
+	if err != nil {
+		return err
 	}
-	return err
+
+	color.Default.Fprintln(out, "Cleanup complete in", time.Since(start))
+	return nil
 }

pkg/skaffold/test/structure/structure.go

@@ -17,23 +17,30 @@ limitations under the License.
 package structure
 
 import (
+	"context"
+	"io"
 	"os/exec"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
-
+	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
 
 // Test is the entrypoint for running structure tests
-func (tr *Runner) Test(image string) error {
-	logrus.Infof("running structure tests for files %v", tr.testFiles)
-	args := []string{"test", "--image", image}
+func (tr *Runner) Test(ctx context.Context, out io.Writer, image string) error {
+	logrus.Infof("Running structure tests for files %v", tr.testFiles)
+
+	args := []string{"test", "-v", "warn", "--image", image}
 	for _, f := range tr.testFiles {
 		args = append(args, "--config", f)
 	}
-	args = append(args, tr.testFiles...)
-	cmd := exec.Command("container-structure-test", args...)
 
-	_, err := util.RunCmdOut(cmd)
-	return err
+	cmd := exec.CommandContext(ctx, "container-structure-test", args...)
+	cmd.Stdout = out
+	cmd.Stderr = out
+
+	if err := cmd.Run(); err != nil {
+		return errors.Wrap(err, "running container-structure-test")
+	}
+
+	return nil
 }

pkg/skaffold/test/structure/types.go

@@ -20,8 +20,9 @@ type Runner struct {
 	testFiles []string
 }
 
-func NewStructureTestRunner(files []string) (*Runner, error) {
+// NewRunner creates a new structure.Runner.
+func NewRunner(files []string) *Runner {
 	return &Runner{
 		testFiles: files,
-	}, nil
+	}
 }

pkg/skaffold/test/test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package test
 
 import (
+	"context"
 	"io"
 	"os"
 
@@ -32,73 +33,72 @@ import (
 // and returns a Tester instance with all the necessary test runners
 // to run all specified tests.
 func NewTester(testCases *[]latest.TestCase) (Tester, error) {
-	testers := []*ArtifactTester{}
-	deps := []string{}
 	// TODO(nkubala): copied this from runner.getDeployer(), this should be moved somewhere else
 	cwd, err := os.Getwd()
 	if err != nil {
 		return nil, errors.Wrap(err, "finding current directory")
 	}
-	for _, testCase := range *testCases {
-		testRunner := &ArtifactTester{
-			ImageName: testCase.ImageName,
-		}
-		if testCase.StructureTests != nil {
-			stFiles, err := util.ExpandPathsGlob(cwd, testCase.StructureTests)
-			if err != nil {
-				return FullTester{}, errors.Wrap(err, "expanding test file paths")
-			}
-			stRunner, err := structure.NewStructureTestRunner(stFiles)
-			if err != nil {
-				return FullTester{}, errors.Wrap(err, "retrieving structure test runner")
-			}
-			testRunner.TestRunners = append(testRunner.TestRunners, stRunner)
-
-			deps = append(deps, stFiles...)
-		}
-		testers = append(testers, testRunner)
-	}
+
 	return FullTester{
-		ArtifactTesters: testers,
-		Dependencies:    deps,
+		testCases:  testCases,
+		workingDir: cwd,
 	}, nil
 }
 
 // TestDependencies returns the watch dependencies to the runner.
-func (t FullTester) TestDependencies() []string {
-	return t.Dependencies
+func (t FullTester) TestDependencies() ([]string, error) {
+	var deps []string
+
+	for _, test := range *t.testCases {
+		if test.StructureTests == nil {
+			continue
+		}
+
+		files, err := util.ExpandPathsGlob(t.workingDir, test.StructureTests)
+		if err != nil {
+			return nil, errors.Wrap(err, "expanding test file paths")
+		}
+
+		deps = append(deps, files...)
+	}
+
+	return deps, nil
 }
 
 // Test is the top level testing execution call. It serves as the
 // entrypoint to all individual tests.
-func (t FullTester) Test(out io.Writer, bRes []build.Artifact) error {
-	t.resolveArtifactImageTags(bRes)
-	for _, aTester := range t.ArtifactTesters {
-		if err := aTester.RunTests(); err != nil {
-			return err
+func (t FullTester) Test(ctx context.Context, out io.Writer, bRes []build.Artifact) error {
+	for _, test := range *t.testCases {
+		if err := t.runStructureTests(ctx, out, bRes, test); err != nil {
+			return errors.Wrap(err, "running structure tests")
 		}
 	}
+
 	return nil
 }
 
-// RunTests serves as the entrypoint to each group of
-// artifact-specific tests.
-func (a *ArtifactTester) RunTests() error {
-	for _, t := range a.TestRunners {
-		if err := t.Test(a.ImageName); err != nil {
-			return err
-		}
+func (t FullTester) runStructureTests(ctx context.Context, out io.Writer, bRes []build.Artifact, testCase latest.TestCase) error {
+	if len(testCase.StructureTests) == 0 {
+		return nil
 	}
-	return nil
+
+	files, err := util.ExpandPathsGlob(t.workingDir, testCase.StructureTests)
+	if err != nil {
+		return errors.Wrap(err, "expanding test file paths")
+	}
+
+	runner := structure.NewRunner(files)
+	fqn := resolveArtifactImageTag(testCase.ImageName, bRes)
+
+	return runner.Test(ctx, out, fqn)
 }
 
-// replace original test artifact images with tagged build artifact images
-func (t *FullTester) resolveArtifactImageTags(bRes []build.Artifact) {
-	for _, aTest := range t.ArtifactTesters {
-		for _, res := range bRes {
-			if aTest.ImageName == res.ImageName {
-				aTest.ImageName = res.Tag
-			}
+func resolveArtifactImageTag(imageName string, bRes []build.Artifact) string {
+	for _, res := range bRes {
+		if imageName == res.ImageName {
+			return res.Tag
 		}
 	}
+
+	return imageName
 }

pkg/skaffold/test/types.go

@@ -17,19 +17,21 @@ limitations under the License.
 package test
 
 import (
+	"context"
 	"io"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 )
 
 // Tester is the top level test executor in Skaffold.
 // A tester is really a collection of artifact-specific testers,
 // each of which contains one or more TestRunners which implements
 // a single test run.
 type Tester interface {
-	Test(io.Writer, []build.Artifact) error
+	Test(context.Context, io.Writer, []build.Artifact) error
 
-	TestDependencies() []string
+	TestDependencies() ([]string, error)
 }
 
 // FullTester serves as a holder for the individual artifact-specific
@@ -38,22 +40,15 @@ type Tester interface {
 // the FullTester actually handles the work.
 
 // FullTester should always be the ONLY implementation of the Tester interface;
-// newly added testing implementations should implement the TestRunner interface.
+// newly added testing implementations should implement the Runner interface.
 type FullTester struct {
-	ArtifactTesters []*ArtifactTester
-	Dependencies    []string
+	testCases  *[]latest.TestCase
+	workingDir string
 }
 
-// ArtifactTester is an artifact-specific test holder, which contains
-// tests runners to run all specified tests on an individual artifact.
-type ArtifactTester struct {
-	ImageName   string
-	TestRunners []Runner
-}
-
-// TestRunner is the lowest-level test executor in Skaffold, responsible for
+// Runner is the lowest-level test executor in Skaffold, responsible for
 // running a single test on a single artifact image and returning its result.
 // Any new test type should implement this interface.
 type Runner interface {
-	Test(image string) error
+	Test(ctx context.Context, out io.Writer, image string) error
 }