From b1bfddbd1b506eb394dea607d1f7aac0f5828cff Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 4 Dec 2023 22:19:24 +0100 Subject: [PATCH 1/8] more tests --- pkg/aclmanager/aclmanager_test.go | 77 +++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/pkg/aclmanager/aclmanager_test.go b/pkg/aclmanager/aclmanager_test.go index e79b6d0..4dcb9b8 100644 --- a/pkg/aclmanager/aclmanager_test.go +++ b/pkg/aclmanager/aclmanager_test.go @@ -3,8 +3,10 @@ package aclmanager import ( "context" "fmt" + "github.com/spf13/viper" "reflect" "testing" + "time" "github.com/go-redis/redismock/v9" "github.com/stretchr/testify/assert" @@ -358,6 +360,81 @@ func TestCurrentFunction_Error(t *testing.T) { assert.Error(t, err) } +func TestAclManager_Loop(t *testing.T) { + viper.Set("syncInterval", 1) + tests := []struct { + name string + aclManager *AclManager + wantErr bool + expectError error + }{ + { + name: "primary node", + aclManager: &AclManager{ + Addr: "localhost:6379", + Password: "password", + Username: "username", + }, + wantErr: false, + }, + { + name: "follower node", + aclManager: &AclManager{ + Addr: "localhost:6379", + Password: "password", + Username: "username", + }, + wantErr: true, + expectError: fmt.Errorf("error syncing acls"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + redisClient, mock := redismock.NewClientMock() + tt.aclManager.RedisClient = redisClient + + if tt.wantErr == false { + if tt.name == "primary node" { + mock.ExpectInfo("replication").SetVal(primaryOutput) + } else { + mock.ExpectInfo("replication").SetVal(followerOutput) + mock.ExpectInfo("replication").SetVal(followerOutput) + } + } + + // Set up a cancellable context to control the loop + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Run Loop in a separate goroutine + done := make(chan error, 1) + go func() { + done <- tt.aclManager.Loop(ctx) + }() + + if tt.wantErr == false && tt.name == "follower node" { + time.Sleep(time.Second * 10) + } + + // Cancel the context to stop the loop + cancel() + + // Check for errors + err := <-done + if err != nil { + if !tt.wantErr { + t.Errorf("Expected no error, got: %v", err) + } + + if err.Error() != tt.expectError.Error() { + t.Errorf("Expected error: %v, got: %v", tt.expectError, err) + } + } + }) + } +} + func BenchmarkParseRedisOutputFollower(b *testing.B) { for i := 0; i < b.N; i++ { _, err := parseRedisOutput(followerOutput) From 91cd1a97f72a9c15e32b3f02edc59c0da0872d2d Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 4 Dec 2023 22:24:42 +0100 Subject: [PATCH 2/8] more tests --- pkg/aclmanager/aclmanager_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/aclmanager/aclmanager_test.go b/pkg/aclmanager/aclmanager_test.go index 4dcb9b8..aacaa49 100644 --- a/pkg/aclmanager/aclmanager_test.go +++ b/pkg/aclmanager/aclmanager_test.go @@ -377,6 +377,16 @@ func TestAclManager_Loop(t *testing.T) { }, wantErr: false, }, + { + name: "primary node with error", + aclManager: &AclManager{ + Addr: "localhost:6379", + Password: "password", + Username: "username", + }, + wantErr: true, + expectError: fmt.Errorf("error"), + }, { name: "follower node", aclManager: &AclManager{ @@ -401,6 +411,8 @@ func TestAclManager_Loop(t *testing.T) { mock.ExpectInfo("replication").SetVal(followerOutput) mock.ExpectInfo("replication").SetVal(followerOutput) } + } else { + mock.ExpectInfo("replication").SetErr(fmt.Errorf("error")) } // Set up a cancellable context to control the loop From ba42fb63cc3abd8e515e6aa194b8f39f0f8e6b59 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 4 Dec 2023 22:29:40 +0100 Subject: [PATCH 3/8] test close --- pkg/aclmanager/aclmanager_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/aclmanager/aclmanager_test.go b/pkg/aclmanager/aclmanager_test.go index aacaa49..20c3297 100644 --- a/pkg/aclmanager/aclmanager_test.go +++ b/pkg/aclmanager/aclmanager_test.go @@ -447,6 +447,18 @@ func TestAclManager_Loop(t *testing.T) { } } +func TestClose(t *testing.T) { + redisClient, _ := redismock.NewClientMock() + aclManager := AclManager{RedisClient: redisClient} + err := aclManager.Close() + assert.NoError(t, err) +} + +func TestClosePanic(t *testing.T) { + aclManager := AclManager{RedisClient: nil} + assert.Panics(t, func() { aclManager.Close() }) +} + func BenchmarkParseRedisOutputFollower(b *testing.B) { for i := 0; i < b.N; i++ { _, err := parseRedisOutput(followerOutput) From 330623da2590e46b333375f85f877bdae7b4828a Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 4 Dec 2023 22:34:16 +0100 Subject: [PATCH 4/8] error wrap --- pkg/aclmanager/aclmanager.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/aclmanager/aclmanager.go b/pkg/aclmanager/aclmanager.go index ee08af0..9304ad2 100644 --- a/pkg/aclmanager/aclmanager.go +++ b/pkg/aclmanager/aclmanager.go @@ -247,16 +247,17 @@ func (a *AclManager) Loop(ctx context.Context) (err error) { case <-ctx.Done(): return err case <-ticker.C: - function, err := a.CurrentFunction() + function, e := a.CurrentFunction() if err != nil { - slog.Warn("unable to check if it's a primary", "message", err) + slog.Warn("unable to check if it's a primary", "message", e) + err = fmt.Errorf("unable to check if it's a primary: %w", e) } if function == Follower { - err = a.SyncAcls() + e = a.SyncAcls() if err != nil { - slog.Warn("unable to sync acls from primary", "message", err) - return fmt.Errorf("error syncing acls: %v", err) + slog.Warn("unable to sync acls from primary", "message", e) } + err = fmt.Errorf("unable to check if it's a primary: %w", e) } } } From 74024b1b89c7ca83cc5ee85c2a56e1e4548cc841 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 4 Dec 2023 23:15:16 +0100 Subject: [PATCH 5/8] moar tests --- pkg/aclmanager/aclmanager_test.go | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/pkg/aclmanager/aclmanager_test.go b/pkg/aclmanager/aclmanager_test.go index 20c3297..98131bb 100644 --- a/pkg/aclmanager/aclmanager_test.go +++ b/pkg/aclmanager/aclmanager_test.go @@ -361,7 +361,7 @@ func TestCurrentFunction_Error(t *testing.T) { } func TestAclManager_Loop(t *testing.T) { - viper.Set("syncInterval", 1) + viper.Set("syncInterval", 5) tests := []struct { name string aclManager *AclManager @@ -395,8 +395,19 @@ func TestAclManager_Loop(t *testing.T) { Username: "username", }, wantErr: true, - expectError: fmt.Errorf("error syncing acls"), + expectError: fmt.Errorf("unable to check if it's a primary: error"), }, + // TODO: refactor to be able to test this case + //{ + // name: "follower node", + // aclManager: &AclManager{ + // Addr: "localhost:6379", + // Password: "password", + // Username: "username", + // }, + // wantErr: false, + // expectError: fmt.Errorf("error syncing acls"), + //}, } for _, tt := range tests { @@ -404,15 +415,20 @@ func TestAclManager_Loop(t *testing.T) { redisClient, mock := redismock.NewClientMock() tt.aclManager.RedisClient = redisClient - if tt.wantErr == false { + if tt.wantErr { + if tt.name == "primary node" { + mock.ExpectInfo("replication").SetErr(fmt.Errorf("error")) + } else { + mock.ExpectInfo("replication").SetVal(followerOutput) + mock.ExpectInfo("replication").SetErr(fmt.Errorf("error")) + } + } else { if tt.name == "primary node" { mock.ExpectInfo("replication").SetVal(primaryOutput) } else { mock.ExpectInfo("replication").SetVal(followerOutput) mock.ExpectInfo("replication").SetVal(followerOutput) } - } else { - mock.ExpectInfo("replication").SetErr(fmt.Errorf("error")) } // Set up a cancellable context to control the loop @@ -425,7 +441,10 @@ func TestAclManager_Loop(t *testing.T) { done <- tt.aclManager.Loop(ctx) }() - if tt.wantErr == false && tt.name == "follower node" { + if tt.name == "follower node" { + if tt.wantErr { + mock.ExpectInfo("replication").SetErr(fmt.Errorf("error")) + } time.Sleep(time.Second * 10) } From 68b0a06b5b75aed167654103d24d64b94e249f73 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 4 Dec 2023 23:19:32 +0100 Subject: [PATCH 6/8] remove --- pkg/aclmanager/aclmanager_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/aclmanager/aclmanager_test.go b/pkg/aclmanager/aclmanager_test.go index 98131bb..fded632 100644 --- a/pkg/aclmanager/aclmanager_test.go +++ b/pkg/aclmanager/aclmanager_test.go @@ -442,9 +442,6 @@ func TestAclManager_Loop(t *testing.T) { }() if tt.name == "follower node" { - if tt.wantErr { - mock.ExpectInfo("replication").SetErr(fmt.Errorf("error")) - } time.Sleep(time.Second * 10) } From 2340f1c0857b7356c6a59b2f6928f54b8c33f4b9 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 4 Dec 2023 23:22:23 +0100 Subject: [PATCH 7/8] adds the missing command mock --- pkg/aclmanager/aclmanager_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/aclmanager/aclmanager_test.go b/pkg/aclmanager/aclmanager_test.go index fded632..6e039f0 100644 --- a/pkg/aclmanager/aclmanager_test.go +++ b/pkg/aclmanager/aclmanager_test.go @@ -419,6 +419,7 @@ func TestAclManager_Loop(t *testing.T) { if tt.name == "primary node" { mock.ExpectInfo("replication").SetErr(fmt.Errorf("error")) } else { + mock.ExpectInfo("replication").SetVal(followerOutput) mock.ExpectInfo("replication").SetVal(followerOutput) mock.ExpectInfo("replication").SetErr(fmt.Errorf("error")) } From a724d9d743df8f1ac492de49d68a0a2450fff845 Mon Sep 17 00:00:00 2001 From: Juliano Martinez Date: Mon, 4 Dec 2023 23:27:13 +0100 Subject: [PATCH 8/8] set a bigger timeout --- pkg/aclmanager/aclmanager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/aclmanager/aclmanager_test.go b/pkg/aclmanager/aclmanager_test.go index 6e039f0..86ffc9f 100644 --- a/pkg/aclmanager/aclmanager_test.go +++ b/pkg/aclmanager/aclmanager_test.go @@ -361,7 +361,7 @@ func TestCurrentFunction_Error(t *testing.T) { } func TestAclManager_Loop(t *testing.T) { - viper.Set("syncInterval", 5) + viper.Set("syncInterval", 60) tests := []struct { name string aclManager *AclManager