From 6262cd7539fe6da5d831e0650815e4cd7485f06f Mon Sep 17 00:00:00 2001 From: Anton Korotkov <106995168+korotkov-aerospike@users.noreply.github.com> Date: Wed, 3 Apr 2024 14:42:42 +0300 Subject: [PATCH] Improve test coverage and fix warnings (#180) * add os accessor tests * remove unused method * remove unused method and fix warnings * add retry timer test * add TestCalculateConfigurationBackupPath * add os accessor tests * add restore test * fix imports * add state read test --- internal/server/server.go | 6 +- internal/util/util.go | 38 +--- modules/aerospike-tools-backup | 2 +- pkg/model/aerospike_cluster_test.go | 15 +- pkg/model/time_bounds.go | 5 - pkg/service/backup_backend.go | 2 +- pkg/service/os_accessor_test.go | 325 ++++++++++++++++++++++++++++ pkg/service/restore_memory_test.go | 86 +++++++- pkg/service/retry_service_test.go | 32 ++- pkg/service/s3_context_test.go | 14 +- pkg/util/log.go | 2 +- 11 files changed, 461 insertions(+), 66 deletions(-) diff --git a/internal/server/server.go b/internal/server/server.go index 23bd19fb..b4692843 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -85,6 +85,8 @@ type HTTPServer struct { backupBackends service.BackendsHolder } +// NewHTTPServer returns a new instance of HTTPServer. +// // Annotations to generate OpenAPI description (https://github.com/swaggo/swag) // @title Backup Service REST API Specification // @version 0.3.0 @@ -95,8 +97,6 @@ type HTTPServer struct { // // @externalDocs.description OpenAPI // @externalDocs.url https://swagger.io/resources/open-api/ -// -// NewHTTPServer returns a new instance of HTTPServer. func NewHTTPServer(backends service.BackendsHolder, config *model.Config, scheduler quartz.Scheduler) *HTTPServer { serverConfig := config.ServiceConfig.HTTPServer @@ -211,7 +211,7 @@ func (ws *HTTPServer) Start() { ws.server.Handler = ws.rateLimiterMiddleware(mux) err := ws.server.ListenAndServe() - if strings.Contains(err.Error(), "Server closed") { + if err != nil && strings.Contains(err.Error(), "Server closed") { slog.Info(err.Error()) } else { panic(err) diff --git a/internal/util/util.go b/internal/util/util.go index 7052da55..6208cf6c 100644 --- a/internal/util/util.go +++ b/internal/util/util.go @@ -1,7 +1,6 @@ package util import ( - "bytes" "fmt" "io" "log/slog" @@ -16,7 +15,7 @@ import ( // LogHandler returns the application log handler with the // configured level. func LogHandler(config *model.LoggerConfig) slog.Handler { - addSource := true + const addSource = true writer := logWriter(config) switch strings.ToUpper(config.GetFormatOrDefault()) { case "PLAIN": @@ -80,43 +79,10 @@ func logLevel(level string) slog.Level { } } -// Check panics if the error is not nil. -func Check(e error) { - if e != nil { - panic(e) - } -} - -// Returns an exit value for the error. +// ToExitVal returns an exit value for the error. func ToExitVal(err error) int { if err != nil { return 1 } return 0 } - -// CaptureStdout returns the stdout output written during the -// given function execution. -func CaptureStdout(f func()) string { - old := os.Stdout // keep backup of the real stdout - r, w, _ := os.Pipe() - os.Stdout = w - - f() // run the function - - outC := make(chan string) - // copy the output in a separate goroutine so printing - // can't block indefinitely - go func() { - var buf bytes.Buffer - _, err := io.Copy(&buf, r) - Check(err) - outC <- buf.String() - }() - - // back to normal state - _ = w.Close() - os.Stdout = old // restoring the real stdout - out := <-outC - return out -} diff --git a/modules/aerospike-tools-backup b/modules/aerospike-tools-backup index 26a01477..5ae83290 160000 --- a/modules/aerospike-tools-backup +++ b/modules/aerospike-tools-backup @@ -1 +1 @@ -Subproject commit 26a014776810671ba0c249ec3b030d3dd0c2dd9a +Subproject commit 5ae83290b4ac023f6bbf3fd0f5ac3fafa2d2fd66 diff --git a/pkg/model/aerospike_cluster_test.go b/pkg/model/aerospike_cluster_test.go index 409b4ffb..a3ac4c19 100644 --- a/pkg/model/aerospike_cluster_test.go +++ b/pkg/model/aerospike_cluster_test.go @@ -68,7 +68,7 @@ func TestAerospikeCluster_GetPassword(t *testing.T) { if test.expectedErr { assert.Nil(t, password) } - os.RemoveAll(testdataFolder) + _ = os.RemoveAll(testdataFolder) }) } } @@ -87,7 +87,7 @@ func TestAerospikeCluster_GetPasswordCaching(t *testing.T) { assert.Equal(t, ptr.String("password"), password) // remove file to ensure second call will not read it - os.RemoveAll(testdataFolder) + _ = os.RemoveAll(testdataFolder) // Make a second call to GetPassword and check if the returned passwords are same passwordAfterCache := cluster.GetPassword() @@ -108,8 +108,13 @@ func TestAerospikeCluster_GetPasswordFromCredentials(t *testing.T) { func createValidFile() { text := []byte("password") - os.MkdirAll(testdataFolder, 0744) + _ = os.MkdirAll(testdataFolder, 0744) f, _ := os.OpenFile(passwordPath, os.O_WRONLY|os.O_CREATE, 0644) - defer f.Close() - f.Write(text) + defer func(f *os.File) { + err := f.Close() + if err != nil { + panic(err) + } + }(f) + _, _ = f.Write(text) } diff --git a/pkg/model/time_bounds.go b/pkg/model/time_bounds.go index d59be101..364ee0d9 100644 --- a/pkg/model/time_bounds.go +++ b/pkg/model/time_bounds.go @@ -30,11 +30,6 @@ func NewTimeBoundsTo(toTime int64) (*TimeBounds, error) { return NewTimeBounds(nil, &toTime) } -// NewTimeBoundsFrom creates a new TimeBounds from the provided fromTime. -func NewTimeBoundsFrom(toTime int64) (*TimeBounds, error) { - return NewTimeBounds(nil, &toTime) -} - // NewTimeBoundsFromString creates a TimeBounds from the string representation of // time boundaries. func NewTimeBoundsFromString(from, to string) (*TimeBounds, error) { diff --git a/pkg/service/backup_backend.go b/pkg/service/backup_backend.go index ed59f617..57f8d26a 100644 --- a/pkg/service/backup_backend.go +++ b/pkg/service/backup_backend.go @@ -112,7 +112,7 @@ func (b *BackupBackend) FullBackupList(timebounds *model.TimeBounds) ([]model.Ba func (b *BackupBackend) detailsFromPaths(timebounds *model.TimeBounds, useCache bool, paths ...string) []model.BackupDetails { // each path contains a backup of specific time - backupDetails := []model.BackupDetails{} + backupDetails := make([]model.BackupDetails, 0, len(paths)) for _, path := range paths { namespaces, err := b.lsDir(filepath.Join(path, model.DataDirectory)) if err != nil { diff --git a/pkg/service/os_accessor_test.go b/pkg/service/os_accessor_test.go index 305b10ff..eade995e 100644 --- a/pkg/service/os_accessor_test.go +++ b/pkg/service/os_accessor_test.go @@ -1,8 +1,18 @@ package service import ( + "encoding/json" + "fmt" + "io/fs" "os" + "path/filepath" + "reflect" + "syscall" "testing" + "time" + + "github.com/aerospike/backup/pkg/model" + "github.com/stretchr/testify/assert" ) func TestDeleteFolder(t *testing.T) { @@ -28,3 +38,318 @@ func TestDeleteFolder(t *testing.T) { _ = os.RemoveAll(tempFolder) }) } + +func TestLsDir(t *testing.T) { + testCases := []struct { + name string + setup func() string + expected []string + }{ + { + name: "Existing directory", + setup: func() string { + dir := t.TempDir() + subDir1 := filepath.Join(dir, "subDir1") + subDir2 := filepath.Join(dir, "subDir2") + _ = os.MkdirAll(subDir1, 0755) + _ = os.MkdirAll(subDir2, 0755) + return dir + }, + expected: []string{"subDir1", "subDir2"}, + }, + { + name: "Non existing directory", + setup: func() string { + dir := filepath.Join(t.TempDir(), "non-existing-dir") + return dir + }, + expected: []string{}, + }, + { + name: "File instead of directory", + setup: func() string { + dir := t.TempDir() + file := filepath.Join(dir, "file") + _ = os.WriteFile(file, []byte("test content"), 0644) + return dir + }, + expected: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + dir := tc.setup() + result, err := NewOSDiskAccessor().lsDir(dir) + + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if len(result) != len(tc.expected) { + t.Fatalf("Unexpected results \nExpected: %v \nGot: %v", tc.expected, result) + } + }) + } +} + +func TestLsFiles(t *testing.T) { + testCases := []struct { + name string + setup func() string + expected []string + }{ + { + name: "Existing directory", + setup: func() string { + dir := t.TempDir() + subDir1 := filepath.Join(dir, "subDir1") + subDir2 := filepath.Join(dir, "subDir2") + _ = os.MkdirAll(subDir1, 0755) + _ = os.MkdirAll(subDir2, 0755) + return dir + }, + expected: nil, + }, + { + name: "Non existing directory", + setup: func() string { + dir := filepath.Join(t.TempDir(), "non-existing-dir") + return dir + }, + expected: []string{}, + }, + { + name: "File instead of directory", + setup: func() string { + dir := t.TempDir() + file := filepath.Join(dir, "file") + _ = os.WriteFile(file, []byte("test content"), 0644) + return dir + }, + expected: []string{"file"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + dir := tc.setup() + result, err := NewOSDiskAccessor().lsFiles(dir) + + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if len(result) != len(tc.expected) { + t.Fatalf("Unexpected results \nExpected: %v \nGot: %v", tc.expected, result) + } + }) + } +} + +func TestValidatePathContainsBackup(t *testing.T) { + dir := t.TempDir() + + // Prepare some test cases + testCases := []struct { + name string + path string + file bool + err error + }{ + { + name: "PathDoesNotExist", + path: "/invalid/path", + file: false, + err: &os.PathError{ + Op: "stat", + Path: "/invalid/path", + Err: syscall.ENOENT, + }, + }, + { + name: "PathExistsButNoBackupFile", + path: dir, + file: false, + err: fmt.Errorf("no backup files found in %s", dir), + }, + { + name: "PathExistsWithBackupFile", + path: dir, + file: true, + err: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // If necessary, create a backup file in the test directory + if tc.file { + _, err := os.Create(dir + "/testfile.asb") + if err != nil { + t.Fatalf("Failed to create test file: %s", err) + } + } + + // Run the function and check its output + err := validatePathContainsBackup(tc.path) + if reflect.TypeOf(err) != reflect.TypeOf(tc.err) { + t.Errorf("Expected error %v, but got %v", tc.err, err) + } + }) + } +} + +func TestOSDiskAccessor_CreateFolder(t *testing.T) { + testCases := []struct { + name string + parent string + success bool + }{ + { + name: "Successful directory creation", + parent: t.TempDir(), + success: true, + }, + { + name: "Attempting to create a directory with invalid path", + parent: "/", + success: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + path := tc.parent + "/test" + NewOSDiskAccessor().CreateFolder(path) + stats, err := os.Stat(path) + if stats == nil && tc.success { + t.Fatalf("Expected to create folder, got error %v", err) + } + if stats != nil && !tc.success { + t.Fatalf("Expected to faile create folder") + } + }) + } +} + +func TestReadBackupDetails(t *testing.T) { + accessor := NewOSDiskAccessor() + + path := filepath.Join(os.TempDir(), "test.yaml") + _ = os.MkdirAll(path, fs.ModePerm) + metadata := model.BackupMetadata{ + Created: time.Now(), + Namespace: "test-backup", + } + + data, _ := json.Marshal(metadata) + _ = accessor.write(filepath.Join(path, metadataFile), data) + + details, err := accessor.readBackupDetails(path, true) + assert.NoError(t, err) + assert.True(t, metadata.Created.Equal(details.BackupMetadata.Created)) + assert.Equal(t, path, *details.Key) +} + +func TestReadBackupDetailsNegative(t *testing.T) { + + accessor := &OSDiskAccessor{} + tests := []struct { + name string + setup func() string + }{ + { + name: "NonExistentDir", + setup: func() string { + return "nonexistentdir" + }, + }, + { + name: "EmptyDir", + setup: func() string { + return t.TempDir() + }, + }, + { + name: "InvalidMetadata", + setup: func() string { + dir := t.TempDir() + _ = accessor.write(filepath.Join(dir, metadataFile), []byte{1, 2, 3}) + return dir + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := tt.setup() + _, err := accessor.readBackupDetails(dir, false) + assert.Error(t, err) + }) + } +} + +func TestReadState(t *testing.T) { + accessor := NewOSDiskAccessor() + + dir := os.TempDir() + _ = os.MkdirAll(dir, fs.ModePerm) + path := filepath.Join(dir, "test_state.yaml") + expected := &model.BackupState{ + Performed: 10, + } + data, _ := json.Marshal(expected) + _ = accessor.write(path, data) + + state := &model.BackupState{} + err := accessor.readBackupState(path, state) + assert.NoError(t, err) + assert.Equal(t, expected, state) +} + +func TestReadStateNegative(t *testing.T) { + accessor := &OSDiskAccessor{} + tests := []struct { + name string + setup func() string + ignoreErr bool + }{ + { + name: "NonExistentDir", + setup: func() string { + return "nonexistentdir" + }, + ignoreErr: true, // when state file not exists, default is returned. + }, + { + name: "EmptyDir", + setup: func() string { + return t.TempDir() + }, + }, + { + name: "InvalidState", + setup: func() string { + dir := t.TempDir() + path := filepath.Join(dir, "test_state.yaml") + _ = accessor.write(path, []byte{1, 2, 3}) + return path + }, + ignoreErr: true, // when state file corrupted, default is returned. + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := tt.setup() + state := &model.BackupState{} + err := accessor.readBackupState(dir, state) + if tt.ignoreErr { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + }) + } +} diff --git a/pkg/service/restore_memory_test.go b/pkg/service/restore_memory_test.go index bc24641b..79918d48 100644 --- a/pkg/service/restore_memory_test.go +++ b/pkg/service/restore_memory_test.go @@ -10,6 +10,7 @@ import ( "github.com/aerospike/backup/pkg/model" "github.com/aerospike/backup/pkg/util" "github.com/aws/smithy-go/ptr" + "github.com/stretchr/testify/assert" ) var restoreService = makeTestRestoreService() @@ -69,7 +70,7 @@ type BackendMock struct { } func (m *BackendMock) ReadClusterConfiguration(_ string) ([]byte, error) { - return nil, nil + return []byte{}, nil } func (*BackendMock) FullBackupList(_ *model.TimeBounds) ([]model.BackupDetails, error) { @@ -102,7 +103,7 @@ type BackendFailMock struct { } func (m *BackendFailMock) ReadClusterConfiguration(_ string) ([]byte, error) { - return nil, nil + return nil, errors.New("mock error") } func (*BackendFailMock) FullBackupList(_ *model.TimeBounds) ([]model.BackupDetails, error) { @@ -341,3 +342,84 @@ func Test_restoreTimestampFail(t *testing.T) { t.Errorf("Expected restore job status to be Failed, but got %s", status.Status) } } + +func Test_RetrieveConfiguration(t *testing.T) { + tests := []struct { + name string + routine string + timestamp int64 + wantErr bool + }{ + { + name: "normal", + routine: "routine", + timestamp: 10, + wantErr: false, + }, + { + name: "wrong time", + routine: "routine", + timestamp: 1, + wantErr: true, + }, + { + name: "wrong routine", + routine: "routine_fail_read", + timestamp: 10, + wantErr: true, + }, + { + name: "routine not found", + routine: "routine not found", + timestamp: 10, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res, err := restoreService.RetrieveConfiguration(tt.routine, tt.timestamp) + assert.Equal(t, tt.wantErr, err != nil, "Unexpected error presence, got: %v", err) + + if !tt.wantErr { + assert.NotNil(t, res, "Expected non-nil result, got nil.") + } else { + assert.Nil(t, res, "Expected nil result as an error was expected.") + } + }) + } +} + +func Test_CalculateConfigurationBackupPath(t *testing.T) { + tests := []struct { + name string + path string + want string + wantErr bool + }{ + { + name: "NormalPath", + path: "backup/12345/data/ns1", + want: "backup/12345/configuration", + wantErr: false, + }, + { + name: "InvalidPath", + path: "://", + want: "", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := calculateConfigurationBackupPath(tt.path) + if (err != nil) != tt.wantErr { + t.Errorf("calculateConfigurationBackupPath() error = %v, wantErr %v", err, tt.wantErr) + return + } + if result != tt.want { + t.Errorf("calculateConfigurationBackupPath() got = %v, want %v", result, tt.want) + } + }) + } +} diff --git a/pkg/service/retry_service_test.go b/pkg/service/retry_service_test.go index b63c8467..d57779fd 100644 --- a/pkg/service/retry_service_test.go +++ b/pkg/service/retry_service_test.go @@ -7,6 +7,8 @@ import ( "time" ) +const timeout = 100 * time.Millisecond + func Test_timer(t *testing.T) { r := NewRetryService("test") counterLock := sync.Mutex{} @@ -19,9 +21,9 @@ func Test_timer(t *testing.T) { return errors.New("mock error") } return nil - }, time.Second, 3) + }, timeout, 3) - time.Sleep(5 * time.Second) + time.Sleep(1 * time.Second) counterLock.Lock() defer counterLock.Unlock() if retryCounter != 0 { @@ -29,6 +31,26 @@ func Test_timer(t *testing.T) { } } +func Test_timer_expires(t *testing.T) { + r := NewRetryService("test") + counterLock := sync.Mutex{} + retryCounter := 0 + const attempts = 3 + r.retry(func() error { + counterLock.Lock() + defer counterLock.Unlock() + retryCounter++ + return errors.New("mock error") + }, timeout, attempts-1) + + time.Sleep(1 * time.Second) + counterLock.Lock() + defer counterLock.Unlock() + if retryCounter != attempts { + t.Errorf("Expected retryCounter %d, got %d", attempts, retryCounter) + } +} + func Test_timerRunTwice(t *testing.T) { r := NewRetryService("test") counterLock := sync.Mutex{} @@ -42,10 +64,10 @@ func Test_timerRunTwice(t *testing.T) { } return nil } - r.retry(f, time.Second, 3) - r.retry(f, time.Second, 3) + r.retry(f, timeout, 3) + r.retry(f, timeout, 3) - time.Sleep(5 * time.Second) + time.Sleep(1 * time.Second) counterLock.Lock() defer counterLock.Unlock() if retryCounter != 0 { diff --git a/pkg/service/s3_context_test.go b/pkg/service/s3_context_test.go index 86b2ab57..8126fcbd 100644 --- a/pkg/service/s3_context_test.go +++ b/pkg/service/s3_context_test.go @@ -46,10 +46,10 @@ func runReadWriteState(t *testing.T, context S3Context) { Namespace: "testNS", Created: time.Now(), } - context.writeYaml("backup_path/"+metadataFile, metadataWrite) + _ = context.writeYaml("backup_path/"+metadataFile, metadataWrite) metadataRead := model.BackupMetadata{} - context.readFile("backup_path/"+metadataFile, &metadataRead) + _ = context.readFile("backup_path/"+metadataFile, &metadataRead) if metadataWrite.Namespace != metadataRead.Namespace { t.Errorf("namespace different, expected %s, got %s", metadataWrite.Namespace, metadataRead.Namespace) } @@ -70,15 +70,15 @@ func TestS3Context_DeleteFile(t *testing.T) { } func runDeleteFileTest(t *testing.T, context S3Context) { - context.writeYaml("incremental/file.txt", "data") - context.writeYaml("incremental/file2.txt", "data") + _ = context.writeYaml("incremental/file.txt", "data") + _ = context.writeYaml("incremental/file2.txt", "data") if files, _ := context.lsFiles("incremental"); len(files) != 2 { t.Error("files not created") } // DeleteFolder requires full path - context.DeleteFolder("incremental") + _ = context.DeleteFolder("incremental") if files, _ := context.lsFiles("incremental"); len(files) > 0 { t.Error("files not deleted") @@ -100,8 +100,8 @@ func runDeleteFolderTest(t *testing.T, context S3Context) { parent := "storage1/minioIncremental" folder1 := parent + "/source-ns1" folder2 := parent + "/source-ns16" - context.writeYaml(folder1+"/file1.txt", "data") - context.writeYaml(folder2+"/file2.txt", "data") + _ = context.writeYaml(folder1+"/file1.txt", "data") + _ = context.writeYaml(folder2+"/file2.txt", "data") err := context.DeleteFolder(folder1) if err != nil { diff --git a/pkg/util/log.go b/pkg/util/log.go index abcac04a..44c63b22 100644 --- a/pkg/util/log.go +++ b/pkg/util/log.go @@ -44,7 +44,7 @@ func LogCaptured(out string) { slog.Debug(groups[4]) } } else { // print to stderr - fmt.Fprintln(os.Stderr, entry) + _, _ = fmt.Fprintln(os.Stderr, entry) } } }