From 13fd878c5aeb0828de0bdc45afe697efb5f8a98d Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Sat, 18 Jan 2025 20:08:03 +1100 Subject: [PATCH] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit adds test coverage for the following functions by addressing the review feedback - GetConfigDirPath - GetGrpcConnection - SaveCredentials - RemoveCredentials - RefreshCredentials - LoadCredentials - RevokeToken - GetJsonFromProto - GetYamlFromProto - TestOpenFileArg - ExpandFileArgs Signed-off-by: Kugamoorthy Gajananan Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 164 +++++++++++++++++----------------- 1 file changed, 83 insertions(+), 81 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 65a701de65..2fc15ef47f 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -12,9 +12,9 @@ import ( "net/url" "os" "path/filepath" - "reflect" "strconv" "strings" + "sync" "testing" "time" @@ -23,17 +23,16 @@ import ( "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/mindersec/minder/internal/util" -) "google.golang.org/protobuf/proto" - pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" + "github.com/mindersec/minder/internal/util" + pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" ) // TestGetConfigDirPath tests the GetConfigDirPath function func TestGetConfigDirPath(t *testing.T) { t.Parallel() + var mu sync.Mutex tests := []struct { name string envVar string @@ -57,11 +56,13 @@ func TestGetConfigDirPath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() + mu.Lock() + defer mu.Unlock() err := os.Setenv("XDG_CONFIG_HOME", tt.envVar) if err != nil { t.Errorf("error setting XDG_CONFIG_HOME: %v", err) } - path, err := GetConfigDirPath() + path, err := util.GetConfigDirPath() if (err != nil) != tt.expectingError { t.Errorf("expected error: %v, got: %v", tt.expectingError, err) } @@ -150,11 +151,12 @@ func TestGetGrpcConnection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := os.Setenv(MinderAuthTokenEnvVar, tt.envToken) + originalEnvToken := os.Getenv(util.MinderAuthTokenEnvVar) + err := os.Setenv(util.MinderAuthTokenEnvVar, tt.envToken) if err != nil { t.Errorf("error setting MinderAuthTokenEnvVar: %v", err) } - conn, err := GetGrpcConnection(tt.grpcHost, tt.grpcPort, tt.allowInsecure, tt.issuerUrl, tt.clientId) + conn, err := util.GetGrpcConnection(tt.grpcHost, tt.grpcPort, tt.allowInsecure, tt.issuerUrl, tt.clientId) if (err != nil) != tt.expectedError { t.Errorf("expected error: %v, got: %v", tt.expectedError, err) } @@ -164,6 +166,7 @@ func TestGetGrpcConnection(t *testing.T) { t.Errorf("error closing connection: %v", err) } } + defer os.Setenv(util.MinderAuthTokenEnvVar, originalEnvToken) }) } } @@ -171,13 +174,13 @@ func TestGetGrpcConnection(t *testing.T) { // TestSaveCredentials tests the SaveCredentials function func TestSaveCredentials(t *testing.T) { t.Parallel() - tokens := OpenIdCredentials{ + tokens := util.OpenIdCredentials{ AccessToken: "test_access_token", RefreshToken: "test_refresh_token", AccessTokenExpiresAt: time.Time{}.Add(7 * 24 * time.Hour), } - cfgPath, err := GetConfigDirPath() + cfgPath, err := util.GetConfigDirPath() if err != nil { t.Fatalf("error getting config path: %v", err) @@ -185,7 +188,7 @@ func TestSaveCredentials(t *testing.T) { expectedFilePath := filepath.Join(cfgPath, "credentials.json") - filePath, err := SaveCredentials(tokens) + filePath, err := util.SaveCredentials(tokens) if err != nil { t.Fatalf("expected no error, got %v", err) } @@ -220,19 +223,21 @@ func TestSaveCredentials(t *testing.T) { // TestRemoveCredentials tests the RemoveCredentials function func TestRemoveCredentials(t *testing.T) { t.Parallel() - xdgConfigHome := os.Getenv("XDG_CONFIG_HOME") - if xdgConfigHome == "" { - homeDir, err := os.UserHomeDir() - if err != nil { - t.Fatalf("error getting home directory: %v", err) - } - xdgConfigHome = filepath.Join(homeDir, ".config") + // Create a temporary directory + testDir := t.TempDir() + + err := os.Setenv("XDG_CONFIG_HOME", testDir) + if err != nil { + t.Errorf("error setting XDG_CONFIG_HOME: %v", err) } + xdgConfigHome := os.Getenv("XDG_CONFIG_HOME") + filePath := filepath.Join(xdgConfigHome, "minder", "credentials.json") // Create a dummy credentials file - err := os.MkdirAll(filepath.Dir(filePath), 0750) + err = os.MkdirAll(filepath.Dir(filePath), 0750) + if err != nil { t.Fatalf("error creating directory: %v", err) } @@ -242,7 +247,7 @@ func TestRemoveCredentials(t *testing.T) { t.Fatalf("error writing credentials to file: %v", err) } - err = RemoveCredentials() + err = util.RemoveCredentials() if err != nil { t.Fatalf("expected no error, got %v", err) } @@ -263,7 +268,7 @@ func TestRefreshCredentials(t *testing.T) { clientId string responseBody string expectedError string - expectedResult OpenIdCredentials + expectedResult util.OpenIdCredentials }{ { name: "Successful refresh with local server", @@ -271,7 +276,7 @@ func TestRefreshCredentials(t *testing.T) { issuerUrl: "http://localhost:8081", clientId: "minder-cli", responseBody: `{"access_token":"new_access_token","refresh_token":"new_refresh_token","expires_in":3600}`, - expectedResult: OpenIdCredentials{ + expectedResult: util.OpenIdCredentials{ AccessToken: "new_access_token", RefreshToken: "new_refresh_token", AccessTokenExpiresAt: time.Now().Add(3600 * time.Second), @@ -314,7 +319,7 @@ func TestRefreshCredentials(t *testing.T) { parsedURL, _ := url.Parse(server.URL) tt.issuerUrl = parsedURL.String() - result, err := RefreshCredentials(tt.refreshToken, tt.issuerUrl, tt.clientId) + result, err := util.RefreshCredentials(tt.refreshToken, tt.issuerUrl, tt.clientId) if tt.expectedError != "" { if err == nil || err.Error() != tt.expectedError { t.Errorf("expected error %v, got %v", tt.expectedError, err) @@ -338,12 +343,12 @@ func TestLoadCredentials(t *testing.T) { name string fileContent string expectedError string - expectedResult OpenIdCredentials + expectedResult util.OpenIdCredentials }{ { name: "Successful load", fileContent: `{"access_token":"access_token","refresh_token":"refresh_token","expiry":"2024-10-05T17:46:27+10:00"}`, - expectedResult: OpenIdCredentials{ + expectedResult: util.OpenIdCredentials{ AccessToken: "access_token", RefreshToken: "refresh_token", AccessTokenExpiresAt: time.Date(2024, 10, 5, 17, 46, 27, 0, time.FixedZone("AEST", 10*60*60)), @@ -352,7 +357,12 @@ func TestLoadCredentials(t *testing.T) { { name: "Error unmarshaling credentials", fileContent: `invalid_json`, - expectedError: "error unmarshaling credentials: invalid character 'i' looking for beginning of value", + expectedError: "error unmarshaling credentials: invalid character 'i'", + }, + { + name: "Error reading credentials file", + fileContent: "", + expectedError: "error reading credentials file", }, } @@ -360,17 +370,18 @@ func TestLoadCredentials(t *testing.T) { tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { t.Parallel() - // Create a unique temporary directory for the test - tempDir, err := os.MkdirTemp("", "test_load_credentials_"+tt.name) - if err != nil { - t.Fatalf("failed to create temp directory: %v", err) - } - defer os.RemoveAll(tempDir) + // tempDir, err := os.MkdirTemp("", "test_load_credentials_"+tt.name) + // if err != nil { + // t.Fatalf("failed to create temp directory: %v", err) + // } + // defer os.RemoveAll(tempDir) + + tempDir := t.TempDir() // Create the minder directory inside the temp directory minderDir := filepath.Join(tempDir, "minder") - err = os.MkdirAll(minderDir, 0750) + err := os.MkdirAll(minderDir, 0750) if err != nil { t.Fatalf("failed to create minder directory: %v", err) } @@ -383,6 +394,11 @@ func TestLoadCredentials(t *testing.T) { if err != nil { t.Fatalf("failed to write test file: %v", err) } + // Print the file path and content for debugging + t.Logf("Test %s: written file path %s with content: %s", tt.name, filePath, tt.fileContent) + } else { + // Print the file path for debugging + t.Logf("Test %s: file path %s not created as file content is empty", tt.name, filePath) } // Temporarily override the environment variable for the test @@ -391,12 +407,11 @@ func TestLoadCredentials(t *testing.T) { if err != nil { t.Errorf("error setting XDG_CONFIG_HOME: %v", err) } - defer os.Setenv("XDG_CONFIG_HOME", originalEnv) - result, err := LoadCredentials() + result, err := util.LoadCredentials() if tt.expectedError != "" { - if err == nil || err.Error() != tt.expectedError { - t.Errorf("expected error %v, got %v", tt.expectedError, err) + if err == nil || !strings.HasPrefix(err.Error(), tt.expectedError) { + t.Errorf("expected error matching %v, got %v", tt.expectedError, err) } } else { if err != nil { @@ -406,7 +421,10 @@ func TestLoadCredentials(t *testing.T) { t.Errorf("expected result %v, got %v", tt.expectedResult, result) } } + defer os.Setenv("XDG_CONFIG_HOME", originalEnv) + defer os.RemoveAll(tempDir) }) + } } @@ -476,7 +494,7 @@ func TestRevokeToken(t *testing.T) { issuerUrl = server.URL } - err := RevokeToken(tt.token, issuerUrl, tt.clientId, tt.tokenHint) + err := util.RevokeToken(tt.token, issuerUrl, tt.clientId, tt.tokenHint) if (err != nil) != tt.expectError { t.Errorf("RevokeToken() error = %v, expectError %v", err, tt.expectError) } @@ -516,7 +534,7 @@ func TestGetJsonFromProto(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - jsonResult, err := GetJsonFromProto(tt.input) + jsonResult, err := util.GetJsonFromProto(tt.input) if (err != nil) != tt.expectedError { t.Errorf("GetJsonFromProto() error = %v, expectedError %v", err, tt.expectedError) return @@ -560,7 +578,7 @@ owner: repoOwner for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - yamlResult, err := GetYamlFromProto(tt.input) + yamlResult, err := util.GetYamlFromProto(tt.input) if (err != nil) != tt.expectedError { t.Errorf("GetYamlFromProto() error = %v, expectedError %v", err, tt.expectedError) return @@ -619,7 +637,7 @@ func TestOpenFileArg(t *testing.T) { defer os.Remove(tc.filePath) } - desc, closer, err := OpenFileArg(tc.filePath, tc.dashOpen) + desc, closer, err := util.OpenFileArg(tc.filePath, tc.dashOpen) if closer != nil { defer closer() } @@ -642,35 +660,35 @@ func TestOpenFileArg(t *testing.T) { // TestExpandFileArgs tests the ExpandFileArgs function. func TestExpandFileArgs(t *testing.T) { t.Parallel() - tests := []struct { name string files []string - expected []string + expected []util.ExpandedFile wantErr bool }{ { name: "Single file", files: []string{"testfile.txt"}, - expected: []string{"testfile.txt"}, + expected: []util.ExpandedFile{{Path: "testfile.txt", Expanded: false}}, wantErr: false, }, + { name: "Single directory", files: []string{"testdir"}, - expected: []string{"testdir/file1.txt", "testdir/file2.txt"}, + expected: []util.ExpandedFile{{Path: "testdir/file1.txt", Expanded: true}, {Path: "testdir/file2.txt", Expanded: true}}, wantErr: false, }, { name: "File and directory", files: []string{"testfile.txt", "testdir"}, - expected: []string{"testfile.txt", "testdir/file1.txt", "testdir/file2.txt"}, + expected: []util.ExpandedFile{{Path: "testfile.txt", Expanded: false}, {Path: "testdir/file1.txt", Expanded: true}, {Path: "testdir/file2.txt", Expanded: true}}, wantErr: false, }, { name: "File with '-'", files: []string{"-"}, - expected: []string{"-"}, + expected: []util.ExpandedFile{{Path: "-", Expanded: false}}, wantErr: false, }, { @@ -680,45 +698,45 @@ func TestExpandFileArgs(t *testing.T) { wantErr: true, }, } - for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - - // Create a unique directory for each test - testDir := fmt.Sprintf("testdir_%s", sanitizeTestName(tt.name)) + // Create a temporary directory + testDir := t.TempDir() if err := setupTestFiles(testDir); err != nil { t.Fatalf("Failed to set up test files: %v", err) } - - // Ensure cleanup happens after the test - t.Cleanup(func() { - cleanupTestFiles(testDir) - }) - // Update file paths to include the unique directory for i, file := range tt.files { tt.files[i] = fmt.Sprintf("%s/%s", testDir, file) } for i, file := range tt.expected { - tt.expected[i] = fmt.Sprintf("%s/%s", testDir, file) + tt.expected[i].Path = fmt.Sprintf("%s/%s", testDir, file.Path) } - - got, err := ExpandFileArgs(tt.files) + combinedFiles := tt.files + got, err := util.ExpandFileArgs(combinedFiles...) if (err != nil) != tt.wantErr { t.Errorf("ExpandFileArgs() error = %v, wantErr %v", err, tt.wantErr) } - if !reflect.DeepEqual(got, tt.expected) { + if !compareExpandedFiles(got, tt.expected) { t.Errorf("ExpandFileArgs() = %v, want %v", got, tt.expected) } + defer os.RemoveAll(testDir) }) } } -// sanitizeTestName replaces spaces and special characters in test names to create valid directory names. -func sanitizeTestName(name string) string { - return strings.ReplaceAll(name, " ", "_") +func compareExpandedFiles(got, expected []util.ExpandedFile) bool { + if len(got) != len(expected) { + return false + } + for i := range got { + if got[i].Path != expected[i].Path || got[i].Expanded != expected[i].Expanded { + return false + } + } + return true } // setupTestFiles creates test files and directories for the unit tests. @@ -747,22 +765,6 @@ func setupTestFiles(testDir string) error { return nil } -// cleanupTestFiles removes test files and directories after the unit tests. -func cleanupTestFiles(testDir string) { - fmt.Printf("Cleaning up test files in %s...\n", testDir) - - retries := 3 - for i := 0; i < retries; i++ { - err := os.RemoveAll(testDir) - if err == nil || os.IsNotExist(err) { - break - } - time.Sleep(100 * time.Millisecond) // Wait before retrying - } - fmt.Printf("Removed directory '%s'\n", testDir) - -} - func TestGetConfigValue(t *testing.T) { t.Parallel() @@ -913,7 +915,7 @@ func TestInt32FromString(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got, err := Int32FromString(tt.args.v) + got, err := util.Int32FromString(tt.args.v) if tt.wantErr { require.Error(t, err, "expected error") require.Zero(t, got, "expected zero")