From eec8942dc0b9b03fa74c2980b1963f0690639c3d Mon Sep 17 00:00:00 2001 From: Joey Clover Date: Wed, 20 Jan 2021 21:42:36 +0000 Subject: [PATCH] refactored sdk functions to always return &alpha.Bool{} instead of nil --- pkg/sdkserver/localsdk.go | 8 ++-- pkg/sdkserver/localsdk_test.go | 38 ++++++++++++++++++ pkg/sdkserver/sdkserver.go | 8 ++-- pkg/sdkserver/sdkserver_test.go | 71 +++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 8 deletions(-) diff --git a/pkg/sdkserver/localsdk.go b/pkg/sdkserver/localsdk.go index cbf10ad448..42686d1dc7 100644 --- a/pkg/sdkserver/localsdk.go +++ b/pkg/sdkserver/localsdk.go @@ -399,7 +399,7 @@ func (l *LocalSDKServer) stopReserveTimer() { // [FeatureFlag:PlayerTracking] func (l *LocalSDKServer) PlayerConnect(ctx context.Context, id *alpha.PlayerID) (*alpha.Bool, error) { if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { - return nil, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) + return &alpha.Bool{Bool: false}, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) } l.logger.WithField("playerID", id.PlayerID).Info("Player Connected") l.gsMutex.Lock() @@ -417,7 +417,7 @@ func (l *LocalSDKServer) PlayerConnect(ctx context.Context, id *alpha.PlayerID) } if l.gs.Status.Players.Count >= l.gs.Status.Players.Capacity { - return &alpha.Bool{}, errors.New("Players are already at capacity") + return &alpha.Bool{Bool: false}, errors.New("Players are already at capacity") } l.gs.Status.Players.Ids = append(l.gs.Status.Players.Ids, id.PlayerID) @@ -433,7 +433,7 @@ func (l *LocalSDKServer) PlayerConnect(ctx context.Context, id *alpha.PlayerID) // [FeatureFlag:PlayerTracking] func (l *LocalSDKServer) PlayerDisconnect(ctx context.Context, id *alpha.PlayerID) (*alpha.Bool, error) { if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { - return nil, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) + return &alpha.Bool{Bool: false}, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) } l.logger.WithField("playerID", id.PlayerID).Info("Player Disconnected") l.gsMutex.Lock() @@ -467,7 +467,7 @@ func (l *LocalSDKServer) PlayerDisconnect(ctx context.Context, id *alpha.PlayerI // [FeatureFlag:PlayerTracking] func (l *LocalSDKServer) IsPlayerConnected(c context.Context, id *alpha.PlayerID) (*alpha.Bool, error) { if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { - return nil, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) + return &alpha.Bool{Bool: false}, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) } result := &alpha.Bool{Bool: false} diff --git a/pkg/sdkserver/localsdk_test.go b/pkg/sdkserver/localsdk_test.go index e9fa73d753..0dfdcc3f53 100644 --- a/pkg/sdkserver/localsdk_test.go +++ b/pkg/sdkserver/localsdk_test.go @@ -344,6 +344,44 @@ func TestLocalSDKServerPlayerCapacity(t *testing.T) { assert.Equal(t, int64(10), gs.Status.Players.Capacity) } +func TestLocalSDKServerPlayerConnectAndDisconnectWithoutPlayerTracking(t *testing.T) { + t.Parallel() + runtime.FeatureTestMutex.Lock() + defer runtime.FeatureTestMutex.Unlock() + + assert.NoError(t, runtime.ParseFeatures(string(runtime.FeaturePlayerTracking)+"=false")) + + l, err := NewLocalSDKServer("") + assert.Nil(t, err) + + e := &alpha.Empty{} + capacity, err := l.GetPlayerCapacity(context.Background(), e) + assert.Nil(t, capacity) + assert.Error(t, err) + + count, err := l.GetPlayerCount(context.Background(), e) + assert.Error(t, err) + assert.Nil(t, count) + + list, err := l.GetConnectedPlayers(context.Background(), e) + assert.Error(t, err) + assert.Nil(t, list) + + id := &alpha.PlayerID{PlayerID: "test-player"} + + ok, err := l.PlayerConnect(context.Background(), id) + assert.Error(t, err) + assert.False(t, ok.Bool) + + ok, err = l.IsPlayerConnected(context.Background(), id) + assert.Error(t, err) + assert.False(t, ok.Bool) + + ok, err = l.PlayerDisconnect(context.Background(), id) + assert.Error(t, err) + assert.False(t, ok.Bool) +} + func TestLocalSDKServerPlayerConnectAndDisconnect(t *testing.T) { t.Parallel() diff --git a/pkg/sdkserver/sdkserver.go b/pkg/sdkserver/sdkserver.go index a7c0438d8e..eea78bd67d 100644 --- a/pkg/sdkserver/sdkserver.go +++ b/pkg/sdkserver/sdkserver.go @@ -568,7 +568,7 @@ func (s *SDKServer) stopReserveTimer() { // [FeatureFlag:PlayerTracking] func (s *SDKServer) PlayerConnect(ctx context.Context, id *alpha.PlayerID) (*alpha.Bool, error) { if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { - return nil, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) + return &alpha.Bool{Bool: false}, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) } s.logger.WithField("PlayerId", id.PlayerID).Debug("Player Connected") @@ -583,7 +583,7 @@ func (s *SDKServer) PlayerConnect(ctx context.Context, id *alpha.PlayerID) (*alp } if int64(len(s.gsConnectedPlayers)) >= s.gsPlayerCapacity { - return &alpha.Bool{}, errors.New("players are already at capacity") + return &alpha.Bool{Bool: false}, errors.New("players are already at capacity") } // let's retain the original order, as it should be a smaller patch on data change @@ -598,7 +598,7 @@ func (s *SDKServer) PlayerConnect(ctx context.Context, id *alpha.PlayerID) (*alp // [FeatureFlag:PlayerTracking] func (s *SDKServer) PlayerDisconnect(ctx context.Context, id *alpha.PlayerID) (*alpha.Bool, error) { if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { - return nil, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) + return &alpha.Bool{Bool: false}, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) } s.logger.WithField("PlayerId", id.PlayerID).Debug("Player Disconnected") @@ -629,7 +629,7 @@ func (s *SDKServer) PlayerDisconnect(ctx context.Context, id *alpha.PlayerID) (* // [FeatureFlag:PlayerTracking] func (s *SDKServer) IsPlayerConnected(ctx context.Context, id *alpha.PlayerID) (*alpha.Bool, error) { if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { - return nil, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) + return &alpha.Bool{Bool: false}, errors.Errorf("%s not enabled", runtime.FeaturePlayerTracking) } s.gsUpdateMutex.RLock() defer s.gsUpdateMutex.RUnlock() diff --git a/pkg/sdkserver/sdkserver_test.go b/pkg/sdkserver/sdkserver_test.go index 1c8120fd82..3a5ee2ec6d 100644 --- a/pkg/sdkserver/sdkserver_test.go +++ b/pkg/sdkserver/sdkserver_test.go @@ -1083,6 +1083,77 @@ func TestSDKServerPlayerCapacity(t *testing.T) { agtesting.AssertEventContains(t, m.FakeRecorder.Events, "PlayerCapacity Set to 20") } +func TestSDKServerPlayerConnectAndDisconnectWithoutPlayerTracking(t *testing.T) { + t.Parallel() + agruntime.FeatureTestMutex.Lock() + defer agruntime.FeatureTestMutex.Unlock() + + err := agruntime.ParseFeatures(string(agruntime.FeaturePlayerTracking) + "=false") + require.NoError(t, err, "Can not parse FeaturePlayerTracking feature") + + fixture := &agonesv1.GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + Status: agonesv1.GameServerStatus{ + State: agonesv1.GameServerStateReady, + }, + } + + m := agtesting.NewMocks() + m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, &agonesv1.GameServerList{Items: []agonesv1.GameServer{*fixture}}, nil + }) + + stop := make(chan struct{}) + defer close(stop) + + sc, err := defaultSidecar(m) + require.NoError(t, err) + + sc.informerFactory.Start(stop) + assert.True(t, cache.WaitForCacheSync(stop, sc.gameServerSynced)) + + go func() { + err = sc.Run(stop) + assert.NoError(t, err) + }() + + // check initial value comes through + // async, so check after a period + e := &alpha.Empty{} + err = wait.Poll(time.Second, 10*time.Second, func() (bool, error) { + count, err := sc.GetPlayerCapacity(context.Background(), e) + + assert.Nil(t, count) + return false, err + }) + assert.Error(t, err) + + count, err := sc.GetPlayerCount(context.Background(), e) + require.Error(t, err) + assert.Nil(t, count) + + list, err := sc.GetConnectedPlayers(context.Background(), e) + require.Error(t, err) + assert.Nil(t, list) + + id := &alpha.PlayerID{PlayerID: "test-player"} + + ok, err := sc.PlayerConnect(context.Background(), id) + require.Error(t, err) + assert.False(t, ok.Bool) + + ok, err = sc.IsPlayerConnected(context.Background(), id) + require.Error(t, err) + assert.False(t, ok.Bool) + + ok, err = sc.PlayerDisconnect(context.Background(), id) + require.Error(t, err) + assert.False(t, ok.Bool) +} + func TestSDKServerPlayerConnectAndDisconnect(t *testing.T) { t.Parallel() agruntime.FeatureTestMutex.Lock()