From efbed665829adcc87340c9094f513f25ec3d0ed7 Mon Sep 17 00:00:00 2001
From: Alexander Apalikov <alexander.apalikov@globant.com>
Date: Tue, 7 Jul 2020 22:00:37 +0300
Subject: [PATCH] Fix tests timeout, WatchGameServer recent feature (#1674)

* Fix tests timeout, WatchGameServer recent feature

Move GetGameServer() call out of streamMutex lock.
Add previously writtent tests to run without this feature, as they
timeouts.
---
 pkg/sdkserver/sdkserver.go      |  2 +-
 pkg/sdkserver/sdkserver_test.go | 34 ++++++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/pkg/sdkserver/sdkserver.go b/pkg/sdkserver/sdkserver.go
index 9ffe177d91..a7c0438d8e 100644
--- a/pkg/sdkserver/sdkserver.go
+++ b/pkg/sdkserver/sdkserver.go
@@ -497,7 +497,6 @@ func (s *SDKServer) GetGameServer(context.Context, *sdk.Empty) (*sdk.GameServer,
 // backing GameServer configuration / status
 func (s *SDKServer) WatchGameServer(_ *sdk.Empty, stream sdk.SDK_WatchGameServerServer) error {
 	s.logger.Debug("Received WatchGameServer request, adding stream to connectedStreams")
-	s.streamMutex.Lock()
 
 	if runtime.FeatureEnabled(runtime.FeatureSDKWatchSendOnExecute) {
 		gs, err := s.GetGameServer(context.Background(), &sdk.Empty{})
@@ -511,6 +510,7 @@ func (s *SDKServer) WatchGameServer(_ *sdk.Empty, stream sdk.SDK_WatchGameServer
 		}
 	}
 
+	s.streamMutex.Lock()
 	s.connectedStreams = append(s.connectedStreams, stream)
 	s.streamMutex.Unlock()
 	// don't exit until we shutdown, because that will close the stream
diff --git a/pkg/sdkserver/sdkserver_test.go b/pkg/sdkserver/sdkserver_test.go
index db18a34fd5..d9d812f5dc 100644
--- a/pkg/sdkserver/sdkserver_test.go
+++ b/pkg/sdkserver/sdkserver_test.go
@@ -648,6 +648,14 @@ func TestSDKServerGetGameServer(t *testing.T) {
 
 func TestSDKServerWatchGameServer(t *testing.T) {
 	t.Parallel()
+
+	agruntime.FeatureTestMutex.Lock()
+	defer agruntime.FeatureTestMutex.Unlock()
+	err := agruntime.ParseFeatures(string(agruntime.FeatureSDKWatchSendOnExecute) + "=false")
+	if err != nil {
+		assert.FailNow(t, "Can not parse FeatureSDKWatchSendOnExecute")
+	}
+
 	m := agtesting.NewMocks()
 	sc, err := defaultSidecar(m)
 	assert.Nil(t, err)
@@ -686,8 +694,8 @@ func TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute(t *testing.T) {
 	m.AgonesClient.AddWatchReactor("gameservers", k8stesting.DefaultWatchReactor(fakeWatch, nil))
 
 	err := agruntime.ParseFeatures(string(agruntime.FeatureSDKWatchSendOnExecute) + "=true")
-	if !assert.NoError(t, err) {
-		t.Fatal("Can not parse FeatureSDKWatchSendOnExecute")
+	if err != nil {
+		assert.FailNow(t, "Can not parse FeatureSDKWatchSendOnExecute")
 	}
 
 	sc, err := defaultSidecar(m)
@@ -745,6 +753,14 @@ func TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute(t *testing.T) {
 
 func TestSDKServerSendGameServerUpdate(t *testing.T) {
 	t.Parallel()
+
+	agruntime.FeatureTestMutex.Lock()
+	defer agruntime.FeatureTestMutex.Unlock()
+	err := agruntime.ParseFeatures(string(agruntime.FeatureSDKWatchSendOnExecute) + "=false")
+	if err != nil {
+		assert.FailNow(t, "Can not parse FeatureSDKWatchSendOnExecute")
+	}
+
 	m := agtesting.NewMocks()
 	sc, err := defaultSidecar(m)
 	assert.Nil(t, err)
@@ -774,8 +790,17 @@ func TestSDKServerSendGameServerUpdate(t *testing.T) {
 
 func TestSDKServerUpdateEventHandler(t *testing.T) {
 	t.Parallel()
-	m := agtesting.NewMocks()
 
+	// Acquire lock in order to be sure that
+	// no other parallel test turn on FeatureSDKWatchSendOnExecute
+	agruntime.FeatureTestMutex.Lock()
+	defer agruntime.FeatureTestMutex.Unlock()
+	err := agruntime.ParseFeatures(string(agruntime.FeatureSDKWatchSendOnExecute) + "=false")
+	if err != nil {
+		assert.FailNow(t, "Can not parse FeatureSDKWatchSendOnExecute")
+	}
+
+	m := agtesting.NewMocks()
 	fakeWatch := watch.NewFake()
 	m.AgonesClient.AddWatchReactor("gameservers", k8stesting.DefaultWatchReactor(fakeWatch, nil))
 
@@ -1271,6 +1296,9 @@ func waitConnectedStreamCount(sc *SDKServer, count int) error {
 }
 
 func asyncWatchGameServer(t *testing.T, sc *SDKServer, stream sdk.SDK_WatchGameServerServer) {
+	// Note that new FeatureSDKWatchSendOnExecute feature gate
+	// uses getGameServer() function and therefore WatchGameServer()
+	// would block if gsWaitForSync is not Done().
 	go func() {
 		err := sc.WatchGameServer(&sdk.Empty{}, stream)
 		assert.Nil(t, err)