Fix a flaky test (#511)

* Fix a flaky test: When the test generates the application packages for local/remote services, there's a potential race if two separate packages are generated with the same contents. The issue is with file/directory item timestamps that can make the two packages differ. Instead, generate a single application package and replicate it in both services. * Address review comments * Update Makefile to enable testing of all relevant packages. Fix updateCommandWithFlags to properly handle arguments (use the value instead of their name).
gravitational · Jul 17, 2019 · 354669b · 354669b
1 parent 3411fe8
commit 354669b
Show file tree

Hide file tree

Showing 31 changed files with 1,283 additions and 158 deletions.
diff --git a/Gopkg.lock b/Gopkg.lock
diff --git a/Gopkg.toml b/Gopkg.toml
@@ -271,3 +271,8 @@ ignored = [
 [[constraint]]
   name = "github.com/gizak/termui"
   version = "=v2.3.0"
+
+[[constraint]]
+  name = "gopkg.in/check.v1"
+  branch = "v1"
+
diff --git a/build.assets/Makefile b/build.assets/Makefile
@@ -80,6 +80,9 @@ RELEASE_OUT := $(GRAVITY_BUILDDIR)/$(RELEASE_TARBALL_NAME)
 
 BASH ?= /bin/bash
 
+# packages to test
+TEST_PACKAGES ?= $(GRAVITY_PKG_PATH)/lib/... $(GRAVITY_PKG_PATH)/tool/...
+
 # Export variables for recursive make invocations
 export GRAVITY_PKG_PATH
 
@@ -102,13 +105,14 @@ test: buildbox test-etcd
 	docker run --net=host --rm=true $(NOROOT) \
 		-v $(TOP):$(SRCDIR) \
 		-e "GRAVITY_PKG_PATH=$(GRAVITY_PKG_PATH)" \
+		-e "TEST_PACKAGES=$(TEST_PACKAGES)" \
 		-t $(BBOX) \
 		dumb-init make -C $(SRCDIR)/build.assets FLAGS='-cover -race' TEST_ETCD=true TEST_K8S=$(TEST_K8S) test-inside-container
 
 
 .PHONY: test-inside-container
 test-inside-container:
-	TEST_ETCD=$(TEST_ETCD) TEST_ETCD_CONFIG=$(TEST_ETCD_CONFIG) TEST_K8S=$(TEST_K8S) go test $(GRAVITY_PKG_PATH)/lib/... $(FLAGS)
+	TEST_ETCD=$(TEST_ETCD) TEST_ETCD_CONFIG=$(TEST_ETCD_CONFIG) TEST_K8S=$(TEST_K8S) go test $(TEST_PACKAGES) $(FLAGS)
 # TODO: It turns out "go vet" never really worked because we used incorrect package path
 # and now that I've fixed it, it produces a gazillion of warnings. So I'm commenting this
 # out for the time being, should uncomment once we've fixed the warnings.

diff --git a/e b/e
diff --git a/lib/app/service/pull_test.go b/lib/app/service/pull_test.go
@@ -120,7 +120,7 @@ dependencies:
   - example.com/new:0.0.2
   - example.com/existing:0.0.1
 `
-	apptest.CreateDummyApplication2(s.srcApp, locator, dependencies, c)
+	apptest.CreateDummyApplicationWithDependencies(s.srcApp, locator, dependencies, c)
 
 	pulled, err := PullApp(AppPullRequest{
 		SrcPack:  s.srcPack,

diff --git a/lib/app/service/resources_test.go b/lib/app/service/resources_test.go
@@ -30,7 +30,7 @@ func (r *ResourceSuite) TestTranslatesResources(c *C) {
 	runtimePackage := loc.MustParseLocator("gravitational.io/planet:0.0.1")
 	apptest.CreatePackage(r.pack, runtimePackage, nil, c)
 	apptest.CreateRuntimeApplication(r.apps, c)
-	apptest.CreateDummyApplication(r.apps, appPackage, c)
+	apptest.CreateDummyApplication(appPackage, c, r.apps)
 
 	_, reader, err := r.pack.ReadPackage(appPackage)
 	c.Assert(err, IsNil)

diff --git a/lib/app/service/test/builder.go b/lib/app/service/test/builder.go
@@ -43,17 +43,23 @@ func CreateDummyPackageWithContents(locator loc.Locator, items []*archive.Item,
 	data := CreatePackageData(items, c)
 	err := packages.UpsertRepository(locator.Repository, time.Time{})
 	c.Assert(err, IsNil)
-	_, err = packages.CreatePackage(locator, data)
+	_, err = packages.CreatePackage(locator, &data)
 	c.Assert(err, IsNil)
 }
 
-// CreateDummyApplication creates app with valid app manifest, but fake content
-func CreateDummyApplication(apps app.Applications, loc loc.Locator, c *C) *app.Application {
-	return createApp(loc, "", apps, c)
+// CreateDummyApplication creates app with valid app manifest, but fake content.
+// It returns the application created in the last service specified with services
+func CreateDummyApplication(locator loc.Locator, c *C, services ...app.Applications) (app *app.Application) {
+	data := createAppData(locator, "", c)
+	for _, service := range services {
+		app = CreateApplicationFromBinaryData(service, locator, data, c)
+	}
+	return app
 }
 
-// CreateDummyApplication2 creates a test application with a valid manifest and specified dependencies
-func CreateDummyApplication2(apps app.Applications, loc loc.Locator, dependencies string, c *C) *app.Application {
+// CreateDummyApplicationWithDependencies creates a test application with a valid manifest
+// and specified dependencies
+func CreateDummyApplicationWithDependencies(apps app.Applications, loc loc.Locator, dependencies string, c *C) *app.Application {
 	return createApp(loc, dependencies, apps, c)
 }
 
@@ -99,6 +105,11 @@ func CreateAppWithDeps(apps app.Applications, packages pack.PackageService, c *C
 }
 
 func createApp(loc loc.Locator, dependencies string, apps app.Applications, c *C) *app.Application {
+	data := createAppData(loc, dependencies, c)
+	return CreateApplicationFromBinaryData(apps, loc, data, c)
+}
+
+func createAppData(loc loc.Locator, dependencies string, c *C) bytes.Buffer {
 	const manifestTemplate = `
 apiVersion: bundle.gravitational.io/v2
 kind: Bundle
@@ -170,8 +181,7 @@ spec:
 		archive.DirItem("registry/docker"),
 		archive.ItemFromString("registry/docker/TODO", ""),
 	}
-
-	return CreateApplicationFromData(apps, loc, files, c)
+	return CreatePackageData(files, c)
 }
 
 func CreateApplication(apps app.Applications, locator loc.Locator, files []*archive.Item, c *C) *app.Application {
@@ -180,9 +190,12 @@ func CreateApplication(apps app.Applications, locator loc.Locator, files []*arch
 
 func CreateApplicationFromData(apps app.Applications, locator loc.Locator, files []*archive.Item, c *C) *app.Application {
 	data := CreatePackageData(files, c)
+	return CreateApplicationFromBinaryData(apps, locator, data, c)
+}
 
+func CreateApplicationFromBinaryData(apps app.Applications, locator loc.Locator, data bytes.Buffer, c *C) *app.Application {
 	var labels map[string]string
-	app, err := apps.CreateApp(locator, data, labels)
+	app, err := apps.CreateApp(locator, &data, labels)
 	c.Assert(err, IsNil)
 	c.Assert(app, NotNil)
 
@@ -218,7 +231,7 @@ func CreatePackage(packages pack.PackageService, locator loc.Locator, files []*a
 
 	c.Assert(packages.UpsertRepository(locator.Repository, time.Time{}), IsNil)
 
-	app, err := packages.CreatePackage(locator, input)
+	app, err := packages.CreatePackage(locator, &input)
 	c.Assert(err, IsNil)
 	c.Assert(app, NotNil)
 
@@ -230,9 +243,9 @@ func CreatePackage(packages pack.PackageService, locator loc.Locator, files []*a
 	return envelope
 }
 
-func CreatePackageData(items []*archive.Item, c *C) *bytes.Buffer {
-	buf := &bytes.Buffer{}
-	archive := archive.NewTarAppender(buf)
+func CreatePackageData(items []*archive.Item, c *C) bytes.Buffer {
+	var buf bytes.Buffer
+	archive := archive.NewTarAppender(&buf)
 
 	c.Assert(archive.Add(items...), IsNil)
 	archive.Close()

diff --git a/lib/app/suite/suite.go b/lib/app/suite/suite.go
@@ -95,7 +95,7 @@ func (r *AppsSuite) ValidatesManifest(c *C) {
 	errorc := make(chan error, 1)
 	progressc := make(chan *app.ProgressEntry)
 	op, err := apps.CreateImportOperation(&app.ImportRequest{
-		Source:         ioutil.NopCloser(input),
+		Source:         ioutil.NopCloser(&input),
 		Repository:     "example.com",
 		PackageName:    "app",
 		PackageVersion: "0.0.1",
@@ -223,7 +223,7 @@ spec:
 	errorc := make(chan error, 1)
 	progressc := make(chan *app.ProgressEntry)
 	op, err := apps.CreateImportOperation(&app.ImportRequest{
-		Source:           ioutil.NopCloser(f),
+		Source:           ioutil.NopCloser(&f),
 		Repository:       "invalid---", // specify invalid name intentionally so import fails
 		PackageName:      "app",
 		PackageVersion:   "0.0.1",
@@ -387,7 +387,7 @@ metadata:
 	data := apptest.CreatePackageData(items, c)
 
 	var labels map[string]string
-	app, err := apps.CreateAppWithManifest(locator, []byte(manifest), ioutil.NopCloser(data), labels)
+	app, err := apps.CreateAppWithManifest(locator, []byte(manifest), ioutil.NopCloser(&data), labels)
 	c.Assert(err, IsNil)
 	c.Assert(app, NotNil)
 
@@ -403,14 +403,14 @@ func (r *AppsSuite) CreatesApplication(c *C) {
 	apps := r.NewService(c, nil, nil)
 	apptest.CreateRuntimeApplication(apps, c)
 	app := loc.MustParseLocator("example.com/example-app:0.0.1")
-	apptest.CreateDummyApplication(apps, app, c)
+	apptest.CreateDummyApplication(app, c, apps)
 }
 
 func (r *AppsSuite) DeletesApplication(c *C) {
 	apps := r.NewService(c, nil, nil)
 	apptest.CreateRuntimeApplication(apps, c)
 	loc := loc.MustParseLocator("example.com/example-app:0.0.1")
-	application := apptest.CreateDummyApplication(apps, loc, c)
+	application := apptest.CreateDummyApplication(loc, c, apps)
 
 	c.Assert(apps.DeleteApp(app.DeleteRequest{Package: application.Package}), IsNil)
 
@@ -569,7 +569,7 @@ metadata:
 	data := apptest.CreatePackageData(items, c)
 
 	var labels map[string]string
-	application, err := apps.CreateAppWithManifest(locator, manifestBytes, ioutil.NopCloser(data), labels)
+	application, err := apps.CreateAppWithManifest(locator, manifestBytes, ioutil.NopCloser(&data), labels)
 	c.Assert(err, IsNil)
 	c.Assert(application, NotNil)
 
@@ -771,7 +771,8 @@ func (r *AppsSuite) importApplicationWithResources(apps app.Applications, vendor
 	}
 	service.PostProcessManifest(manifest)
 
-	input := ioutil.NopCloser(apptest.CreatePackageData(resources, c))
+	buf := apptest.CreatePackageData(resources, c)
+	input := ioutil.NopCloser(&buf)
 
 	errorc := make(chan error, 1)
 	progressc := make(chan *app.ProgressEntry)

diff --git a/lib/blob/fs/fs.go b/lib/blob/fs/fs.go
@@ -28,8 +28,8 @@ import (
 	"github.com/gravitational/gravity/lib/blob"
 	"github.com/gravitational/gravity/lib/defaults"
 
-	log "github.com/sirupsen/logrus"
 	"github.com/gravitational/trace"
+	log "github.com/sirupsen/logrus"
 )
 
 func New(path string) (blob.Objects, error) {

diff --git a/tool/gravity/cli/utils.go b/tool/gravity/cli/utils.go
@@ -308,7 +308,7 @@ func updateCommandWithFlags(command []string, parser ArgsParser, flagsToAdd []fl
 	for _, el := range ctx.Elements {
 		switch c := el.Clause.(type) {
 		case *kingpin.ArgClause:
-			args = append(args, strconv.Quote(c.Model().Name))
+			args = append(args, strconv.Quote(*el.Value))
 		case *kingpin.FlagClause:
 			if _, ok := c.Model().Value.(boolFlag); ok {
 				args = append(args, fmt.Sprint("--", c.Model().Name))

diff --git a/tool/gravity/cli/utils_test.go b/tool/gravity/cli/utils_test.go
@@ -56,14 +56,14 @@ func (*S) TestUpdatesCommandLine(c *check.C) {
 		},
 		{
 			comment:   "Quotes flags and args",
-			inputArgs: []string{"install", `--token=some token`, "appdir"},
+			inputArgs: []string{"install", `--token=some token`, "/path/to/data"},
 			flags: []flag{
 				{
 					name: "advertise-addr", value: "localhost:8080",
 				},
 			},
 			outputArgs: []string{
-				"install", "--token", `"some token"`, `"appdir"`, "--advertise-addr", `"localhost:8080"`,
+				"install", "--token", `"some token"`, `"/path/to/data"`, "--advertise-addr", `"localhost:8080"`,
 			},
 		},
 	}

diff --git a/vendor/github.com/kr/pretty/.gitignore b/vendor/github.com/kr/pretty/.gitignore
diff --git a/vendor/github.com/kr/pretty/License b/vendor/github.com/kr/pretty/License
diff --git a/vendor/github.com/kr/pretty/Readme b/vendor/github.com/kr/pretty/Readme