Merge pull request #894 from laverya/fix-test-data-races

fix several races in unit tests

Author: laverya

Makefile

@@ -250,6 +250,13 @@ lint: vet ineffassign .state/lint
 
 test: lint .state/test
 
+.state/race: $(SRC)
+	go test --race ./pkg/...
+	@mkdir -p .state
+	@touch .state/race
+
+race: lint .state/race
+
 .state/coverage.out: $(SRC)
 	@mkdir -p .state/
 	#the reduced parallelism here is to avoid hitting the memory limits - we consistently did so with two threads on a 4gb instance

pkg/lifecycle/daemon/daemon_channel_test.go

@@ -57,6 +57,8 @@ func TestDaemonChannel(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			req := require.New(t)
+
 			v := viper.New()
 
 			port := rand.Intn(2000) + 33000
@@ -82,32 +84,36 @@ func TestDaemonChannel(t *testing.T) {
 			}
 
 			daemonCtx, daemonCancelFunc := context.WithCancel(context.Background())
-			defer daemonCancelFunc()
 
 			errChan := make(chan error)
 			log.Log("starting daemon")
-			go func() {
+			go func(errCh chan error) {
 				err := daemon.Serve(daemonCtx, test.release)
 				log.Log("daemon.error", err)
-				errChan <- err
-			}()
+				errCh <- err
+			}(errChan)
 
 			// sigh. Give the server a second to start up
 			time.Sleep(500 * time.Millisecond)
 
 			resp, err := http.Get(fmt.Sprintf("http://localhost:%d/api/v1/metadata", port))
-			require.NoError(t, err)
+			req.NoError(err)
 
 			body, err := ioutil.ReadAll(resp.Body)
-			require.NoError(t, err)
+			req.NoError(err)
 			log.Log("received body", fmt.Sprintf("\"%s\"", body))
 
+			daemonCancelFunc()
+
 			unmarshalled := make(map[string]string)
 			err = json.Unmarshal(body, &unmarshalled)
-			require.NoError(t, err)
+			req.NoError(err)
+
+			req.Equal(test.expectName, unmarshalled["name"])
+			req.Equal(test.expectIcon, unmarshalled["icon"])
 
-			require.Equal(t, test.expectName, unmarshalled["name"])
-			require.Equal(t, test.expectIcon, unmarshalled["icon"])
+			daemonErr := <-errChan
+			req.EqualError(daemonErr, "context canceled")
 		})
 	}
 }

pkg/lifecycle/daemon/routes_navcycle.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"sync"
 
 	"github.com/gin-gonic/gin"
 	"github.com/go-kit/kit/log"
@@ -54,6 +55,9 @@ type NavcycleRoutes struct {
 
 	// This isn't known at injection time, so we have to set in Register
 	Release *api.Release
+
+	// Prevent data races when registering preexecute functions
+	mut sync.Mutex
 }
 
 // Register registers routes
@@ -95,6 +99,7 @@ func (d *NavcycleRoutes) Register(group *gin.RouterGroup, release *api.Release)
 type preExecuteFunc func(context.Context, api.Step) error
 
 func (d *NavcycleRoutes) registerPreExecuteFuncs() {
+	d.mut.Lock()
 	preExecuteFuncMap := make(map[string]preExecuteFunc)
 
 	for _, step := range d.Release.Spec.Lifecycle.V1 {
@@ -104,6 +109,7 @@ func (d *NavcycleRoutes) registerPreExecuteFuncs() {
 	}
 
 	d.PreExecuteFuncMap = preExecuteFuncMap
+	d.mut.Unlock()
 }
 
 func (d *NavcycleRoutes) shutdown(c *gin.Context) {

pkg/lifecycle/render/config/daemonresolver_test.go

@@ -7,13 +7,14 @@ import (
 
 	"github.com/mitchellh/cli"
 	"github.com/replicatedhq/libyaml"
+	"github.com/spf13/afero"
+	"github.com/spf13/viper"
+	"github.com/stretchr/testify/require"
+
 	"github.com/replicatedhq/ship/pkg/api"
 	"github.com/replicatedhq/ship/pkg/lifecycle/daemon"
 	"github.com/replicatedhq/ship/pkg/lifecycle/kustomize"
 	"github.com/replicatedhq/ship/pkg/testing/logger"
-	"github.com/spf13/afero"
-	"github.com/spf13/viper"
-	"github.com/stretchr/testify/require"
 )
 
 type daemonResolverTestCase struct {
@@ -147,19 +148,24 @@ func TestDaemonResolver(t *testing.T) {
 			}
 
 			daemonCtx, daemonCancelFunc := context.WithCancel(context.Background())
-			defer daemonCancelFunc()
+			daemonCloseChan := make(chan struct{})
 
-			log.Log("starting daemon")
-			go func() {
+			require.NoError(t, log.Log("starting daemon"))
+			go func(closeChan chan struct{}) {
 				daemon.Serve(daemonCtx, test.release)
-			}()
+				closeChan <- struct{}{}
+			}(daemonCloseChan)
 
 			resolver := &DaemonResolver{log, daemon}
 
 			resolveContext, cancel := context.WithTimeout(context.Background(), 1*time.Second)
-			defer cancel()
 			config, err := resolver.ResolveConfig(resolveContext, test.release, test.inputContext)
 
+			daemonCancelFunc()
+			cancel()
+
+			<-daemonCloseChan
+
 			test.expect(t, config, err)
 
 			//req.NoError(err)