From 17e023eeae8e0ec73c7d0e3d1d6f348f020fce4d Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Sat, 5 Oct 2024 20:40:11 +1000 Subject: [PATCH 01/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit adds test coverage helper functions in Util Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 389 +++++++++++++++++++++++++++++++++- 1 file changed, 388 insertions(+), 1 deletion(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index b65127a2a3..695d5bdaf8 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -4,17 +4,404 @@ package util_test import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" "strconv" "testing" + "time" _ "github.com/lib/pq" "github.com/spf13/cobra" "github.com/spf13/viper" + pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/mindersec/minder/internal/util" ) + "google.golang.org/protobuf/proto" +) + +// TestGetConfigDirPath tests the GetConfigDirPath function +func TestGetConfigDirPath(t *testing.T) { + t.Parallel() + tests := []struct { + name string + envVar string + expectedPath string + expectingError bool + }{ + { + name: "XDG_CONFIG_HOME set", + envVar: "/custom/config", + expectedPath: "/custom/config/minder", + expectingError: false, + }, + { + name: "XDG_CONFIG_HOME is not set", + envVar: "", + expectedPath: filepath.Join(os.Getenv("HOME"), ".config", "minder"), + expectingError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + os.Setenv("XDG_CONFIG_HOME", tt.envVar) + path, err := GetConfigDirPath() + if (err != nil) != tt.expectingError { + t.Errorf("expected error: %v, got: %v", tt.expectingError, err) + } + if path != tt.expectedPath { + t.Errorf("expected path: %s, got: %s", tt.expectedPath, path) + } + }) + } +} + +// TestGetGrpcConnection tests the GetGrpcConnection function +func TestGetGrpcConnection(t *testing.T) { + t.Parallel() + tests := []struct { + name string + grpcHost string + grpcPort int + allowInsecure bool + issuerUrl string + clientId string + envToken string + expectedError bool + }{ + { + name: "Valid GRPC connection to localhost with secure connection", + grpcHost: "127.0.0.1", + grpcPort: 8090, + allowInsecure: false, + issuerUrl: "http://localhost:8081", + clientId: "minder-cli", + envToken: "MINDER_AUTH_TOKEN", + expectedError: false, + }, + { + name: "Valid GRPC connection to localhost with insecure connection", + grpcHost: "127.0.0.1", + grpcPort: 8090, + allowInsecure: true, + issuerUrl: "http://localhost:8081", + clientId: "minder-cli", + envToken: "MINDER_AUTH_TOKEN", + expectedError: false, + }, + { + name: "Valid GRPC connection to localhost without passing MINDER_AUTH_TOKEN as an argument", + grpcHost: "127.0.0.1", + grpcPort: 8090, + allowInsecure: false, + issuerUrl: "http://localhost:8081", + clientId: "minder-cli", + envToken: "", + expectedError: false, + }, + { + name: "Valid GRPC connection to remote server with secure connection", + grpcHost: "api.stacklok.com", + grpcPort: 443, + allowInsecure: false, + issuerUrl: "https://auth.stacklok.com", + clientId: "minder-cli", + envToken: "MINDER_AUTH_TOKEN", + expectedError: false, + }, + { + name: "Valid GRPC connection to remote server with insecure connection", + grpcHost: "api.stacklok.com", + grpcPort: 443, + allowInsecure: true, + issuerUrl: "https://auth.stacklok.com", + clientId: "minder-cli", + envToken: "MINDER_AUTH_TOKEN", + expectedError: false, + }, + { + name: "Valid GRPC connection to remote server without passing MINDER_AUTH_TOKEN as an argument", + grpcHost: "api.stacklok.com", + grpcPort: 443, + allowInsecure: true, + issuerUrl: "https://auth.stacklok.com", + clientId: "minder-cli", + envToken: "", + expectedError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + os.Setenv(MinderAuthTokenEnvVar, tt.envToken) + conn, err := 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) + } + if conn != nil { + conn.Close() + } + }) + } +} + +// TestSaveCredentials tests the SaveCredentials function +func TestSaveCredentials(t *testing.T) { + tokens := OpenIdCredentials{ + AccessToken: "test_access_token", + RefreshToken: "test_refresh_token", + AccessTokenExpiresAt: time.Time{}.Add(7 * 24 * time.Hour), + } + + cfgPath, err := GetConfigDirPath() + + if err != nil { + t.Fatalf("error getting config path: %v", err) + } + + expectedFilePath := filepath.Join(cfgPath, "credentials.json") + + filePath, err := SaveCredentials(tokens) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if filePath != expectedFilePath { + t.Errorf("expected file path %v, got %v", expectedFilePath, filePath) + } + + // Verify the file content + credsJSON, err := json.Marshal(tokens) + if err != nil { + t.Fatalf("error marshaling credentials: %v", err) + } + + content, err := os.ReadFile(filePath) + if err != nil { + t.Fatalf("error reading file: %v", err) + } + + if string(content) != string(credsJSON) { + t.Errorf("expected file content %v, got %v", string(credsJSON), string(content)) + } + + // Clean up + os.Remove(filePath) +} + +// TestRemoveCredentials tests the RemoveCredentials function +func TestRemoveCredentials(t *testing.T) { + 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") + } + + filePath := filepath.Join(xdgConfigHome, "minder", "credentials.json") + + // Create a dummy credentials file + err := os.MkdirAll(filepath.Dir(filePath), 0750) + if err != nil { + t.Fatalf("error creating directory: %v", err) + } + + err = os.WriteFile(filePath, []byte(`{"access_token":"test_access_token","refresh_token":"test_refresh_token","access_token_expires_at":1234567890}`), 0600) + if err != nil { + t.Fatalf("error writing credentials to file: %v", err) + } + + err = RemoveCredentials() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + // Verify the file is removed + if _, err := os.Stat(filePath); !os.IsNotExist(err) { + t.Errorf("expected file to be removed, but it still exists") + } +} + +// TestRefreshCredentials tests the RefreshCredentials function +func TestRefreshCredentials(t *testing.T) { + tests := []struct { + name string + refreshToken string + issuerUrl string + clientId string + responseBody string + expectedError string + expectedResult OpenIdCredentials + }{ + { + name: "Successful refresh with local server", + refreshToken: "valid_refresh_token", + issuerUrl: "http://localhost:8081", + clientId: "minder-cli", + responseBody: `{"access_token":"new_access_token","refresh_token":"new_refresh_token","expires_in":3600}`, + expectedResult: OpenIdCredentials{ + AccessToken: "new_access_token", + RefreshToken: "new_refresh_token", + AccessTokenExpiresAt: time.Now().Add(3600 * time.Second), + }, + }, + { + name: "Error fetching new credentials (responseBody is missing) rwith local server", + refreshToken: "valid_refresh_token", + issuerUrl: "http://localhost:8081", + clientId: "minder-cli", + expectedError: "error unmarshaling credentials: EOF", + }, + { + name: "Error unmarshaling credentials with local server", + refreshToken: "valid_refresh_token", + issuerUrl: "http://localhost:8081", + clientId: "minder-cli", + responseBody: `invalid_json`, + expectedError: "error unmarshaling credentials: invalid character 'i' looking for beginning of value", + }, + { + name: "Error refreshing credentials with local server", + refreshToken: "valid_refresh_token", + issuerUrl: "http://localhost:8081", + clientId: "minder-cli", + responseBody: `{"error":"invalid_grant","error_description":"Invalid refresh token"}`, + expectedError: "error refreshing credentials: invalid_grant: Invalid refresh token", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintln(w, tt.responseBody) + })) + defer server.Close() + + parsedURL, _ := url.Parse(server.URL) + tt.issuerUrl = parsedURL.String() + + result, err := 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) + } + } else { + if err != nil { + t.Errorf("expected no error, got %v", err) + } + if result.AccessToken != tt.expectedResult.AccessToken || result.RefreshToken != tt.expectedResult.RefreshToken { + t.Errorf("expected result %v, got %v", tt.expectedResult, result) + } + } + }) + } +} + +// TestLoadCredentials tests the LoadCredentials function +func TestLoadCredentials(t *testing.T) { +} + +// TestRevokeToken tests the RevokeToken function +func TestRevokeToken(t *testing.T) { + +} + +// TestGetJsonFromProto tests the GetJsonFromProto function +func TestGetJsonFromProto(t *testing.T) { + tests := []struct { + name string + input proto.Message + expectedJson string + expectedError bool + }{ + { + name: "Valid proto message", + input: &pb.Repository{ + Owner: "repoOwner", + Name: "repoName", + }, + expectedJson: `{ + "owner": "repoOwner", + "name": "repoName" +}`, + expectedError: false, + }, + { + name: "Nil proto message", + input: nil, + expectedJson: `{}`, + expectedError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + jsonResult, err := GetJsonFromProto(tt.input) + if (err != nil) != tt.expectedError { + t.Errorf("GetJsonFromProto() error = %v, expectedError %v", err, tt.expectedError) + return + } + if jsonResult != tt.expectedJson { + t.Errorf("GetJsonFromProto() = %v, expected %v", jsonResult, tt.expectedJson) + } + }) + } +} + +// TestGetYamlFromProto tests the GetYamlFromProto function +func TestGetYamlFromProto(t *testing.T) { + tests := []struct { + name string + input proto.Message + expectedYaml string + expectedError bool + }{ + { + name: "Valid proto message", + input: &pb.Repository{ + Owner: "repoOwner", + Name: "repoName", + }, + expectedYaml: `name: repoName +owner: repoOwner +`, + expectedError: false, + }, + { + name: "Nil proto message", + input: nil, + expectedYaml: `{} +`, + expectedError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + yamlResult, err := GetYamlFromProto(tt.input) + if (err != nil) != tt.expectedError { + t.Errorf("GetYamlFromProto() error = %v, expectedError %v", err, tt.expectedError) + return + } + if yamlResult != tt.expectedYaml { + t.Errorf("GetYamlFromProto() = %v, expected %v", yamlResult, tt.expectedYaml) + } + }) + } +} func TestGetConfigValue(t *testing.T) { t.Parallel() @@ -166,7 +553,7 @@ func TestInt32FromString(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got, err := util.Int32FromString(tt.args.v) + got, err := Int32FromString(tt.args.v) if tt.wantErr { require.Error(t, err, "expected error") require.Zero(t, got, "expected zero") From 75e0c98db6468d30c9e434118c60c1b9ee09f29c Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Sun, 6 Oct 2024 11:23:27 +1100 Subject: [PATCH 02/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the lint errors and handles errors. Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 695d5bdaf8..cd2bbe1c4d 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -53,7 +53,10 @@ func TestGetConfigDirPath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - os.Setenv("XDG_CONFIG_HOME", tt.envVar) + err := os.Setenv("XDG_CONFIG_HOME", tt.envVar) + if err != nil { + t.Errorf("error setting XDG_CONFIG_HOME: %v", err) + } path, err := GetConfigDirPath() if (err != nil) != tt.expectingError { t.Errorf("expected error: %v, got: %v", tt.expectingError, err) @@ -143,13 +146,19 @@ func TestGetGrpcConnection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - os.Setenv(MinderAuthTokenEnvVar, tt.envToken) + err := os.Setenv(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) if (err != nil) != tt.expectedError { t.Errorf("expected error: %v, got: %v", tt.expectedError, err) } if conn != nil { - conn.Close() + err = conn.Close() + if err != nil { + t.Errorf("error closing connection: %v", err) + } } }) } @@ -157,6 +166,7 @@ func TestGetGrpcConnection(t *testing.T) { // TestSaveCredentials tests the SaveCredentials function func TestSaveCredentials(t *testing.T) { + t.Parallel() tokens := OpenIdCredentials{ AccessToken: "test_access_token", RefreshToken: "test_refresh_token", @@ -186,7 +196,8 @@ func TestSaveCredentials(t *testing.T) { t.Fatalf("error marshaling credentials: %v", err) } - content, err := os.ReadFile(filePath) + fpath := filepath.Clean(filePath) + content, err := os.ReadFile(fpath) if err != nil { t.Fatalf("error reading file: %v", err) } @@ -196,11 +207,15 @@ func TestSaveCredentials(t *testing.T) { } // Clean up - os.Remove(filePath) + err = os.Remove(filePath) + if err != nil { + t.Fatalf("error removing file: %v", err) + } } // TestRemoveCredentials tests the RemoveCredentials function func TestRemoveCredentials(t *testing.T) { + t.Parallel() xdgConfigHome := os.Getenv("XDG_CONFIG_HOME") if xdgConfigHome == "" { homeDir, err := os.UserHomeDir() @@ -236,6 +251,7 @@ func TestRemoveCredentials(t *testing.T) { // TestRefreshCredentials tests the RefreshCredentials function func TestRefreshCredentials(t *testing.T) { + t.Parallel() tests := []struct { name string refreshToken string @@ -284,6 +300,7 @@ func TestRefreshCredentials(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") fmt.Fprintln(w, tt.responseBody) @@ -321,6 +338,7 @@ func TestRevokeToken(t *testing.T) { // TestGetJsonFromProto tests the GetJsonFromProto function func TestGetJsonFromProto(t *testing.T) { + t.Parallel() tests := []struct { name string input proto.Message @@ -349,6 +367,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) if (err != nil) != tt.expectedError { t.Errorf("GetJsonFromProto() error = %v, expectedError %v", err, tt.expectedError) @@ -363,6 +382,7 @@ func TestGetJsonFromProto(t *testing.T) { // TestGetYamlFromProto tests the GetYamlFromProto function func TestGetYamlFromProto(t *testing.T) { + t.Parallel() tests := []struct { name string input proto.Message @@ -391,6 +411,7 @@ owner: repoOwner for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() yamlResult, err := GetYamlFromProto(tt.input) if (err != nil) != tt.expectedError { t.Errorf("GetYamlFromProto() error = %v, expectedError %v", err, tt.expectedError) From 5b4f2b407d5ad1c2021cf2675f73745b0f8d8612 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Sun, 6 Oct 2024 12:27:01 +1100 Subject: [PATCH 03/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit - fixes the lint errors and handles errors. - adds test coverage for OpenFileArg, ExpandFileArgs, functions Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 194 ++++++++++++++++++++++++++++++++++ 1 file changed, 194 insertions(+) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index cd2bbe1c4d..98d720799f 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -6,12 +6,15 @@ package util_test import ( "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "net/url" "os" "path/filepath" + "reflect" "strconv" + "strings" "testing" "time" @@ -424,6 +427,197 @@ owner: repoOwner } } +// TestOpenFileArg tests the OpenFileArg function +func TestOpenFileArg(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + filePath string + dashOpen io.Reader + expectedDesc string + expectError bool + }{ + { + name: "Dash as file path", + filePath: "-", + dashOpen: strings.NewReader("dash input"), + expectedDesc: "dash input", + expectError: false, + }, + { + name: "Valid file path", + filePath: "testfile.txt", + dashOpen: nil, + expectedDesc: "test content", + expectError: false, + }, + { + name: "Invalid file path", + filePath: "nonexistent.txt", + dashOpen: nil, + expectedDesc: "", + expectError: true, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // Create a temporary file for testing + if tc.filePath == "testfile.txt" { + err := os.WriteFile(tc.filePath, []byte(tc.expectedDesc), 0600) + assert.NoError(t, err) + defer os.Remove(tc.filePath) + } + + desc, closer, err := OpenFileArg(tc.filePath, tc.dashOpen) + if closer != nil { + defer closer() + } + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + if tc.filePath == "-" || tc.filePath != "-" { + buf := new(strings.Builder) + _, err := io.Copy(buf, desc) + assert.NoError(t, err) + assert.Equal(t, tc.expectedDesc, buf.String()) + } + } + }) + } +} + +// TestExpandFileArgs tests the ExpandFileArgs function. +func TestExpandFileArgs(t *testing.T) { + t.Parallel() // Ensure the test function itself runs in parallel + + tests := []struct { + name string + files []string + expected []string + wantErr bool + }{ + { + name: "Single file", + files: []string{"testfile.txt"}, + expected: []string{"testfile.txt"}, + wantErr: false, + }, + { + name: "Single directory", + files: []string{"testdir"}, + expected: []string{"testdir/file1.txt", "testdir/file2.txt"}, + wantErr: false, + }, + { + name: "File and directory", + files: []string{"testfile.txt", "testdir"}, + expected: []string{"testfile.txt", "testdir/file1.txt", "testdir/file2.txt"}, + wantErr: false, + }, + { + name: "File with '-'", + files: []string{"-"}, + expected: []string{"-"}, + wantErr: false, + }, + { + name: "Non-existent file", + files: []string{"nonexistent.txt"}, + expected: nil, + 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)) + 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) + } + + got, err := ExpandFileArgs(tt.files) + if (err != nil) != tt.wantErr { + t.Errorf("ExpandFileArgs() error = %v, wantErr %v", err, tt.wantErr) + } + if !reflect.DeepEqual(got, tt.expected) { + t.Errorf("ExpandFileArgs() = %v, want %v", got, tt.expected) + } + }) + } +} + +// sanitizeTestName replaces spaces and special characters in test names to create valid directory names. +func sanitizeTestName(name string) string { + return strings.ReplaceAll(name, " ", "_") +} + +// setupTestFiles creates test files and directories for the unit tests. +func setupTestFiles(testDir string) error { + if err := os.MkdirAll(fmt.Sprintf("%s/testdir", testDir), 0750); err != nil { + return fmt.Errorf("failed to create directory '%s/testdir': %w", testDir, err) + } + + if err := os.WriteFile(fmt.Sprintf("%s/testfile.txt", testDir), []byte("test file"), 0600); err != nil { + return fmt.Errorf("failed to create file '%s/testfile.txt': %w", testDir, err) + } + + if err := os.WriteFile(fmt.Sprintf("%s/testdir/file1.txt", testDir), []byte("file 1"), 0600); err != nil { + return fmt.Errorf("failed to create file '%s/testdir/file1.txt': %w", testDir, err) + } + + if err := os.WriteFile(fmt.Sprintf("%s/testdir/file2.txt", testDir), []byte("file 2"), 0600); err != nil { + return fmt.Errorf("failed to create file '%s/testdir/file2.txt': %w", testDir, err) + } + + // Create a file named "-" + if err := os.WriteFile(fmt.Sprintf("%s/-", testDir), []byte("dash file"), 0600); err != nil { + return fmt.Errorf("failed to create file '%s/-': %w", testDir, err) + } + + 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() From a802549df749fb2d6373df861e217a2475267d46 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Sun, 6 Oct 2024 13:30:51 +1100 Subject: [PATCH 04/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage. This commit - fixes the lint errors and handles errors. - Adds test coverage for LoadCredentials function Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 79 ++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 2 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 98d720799f..aad17b74b5 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -304,7 +304,7 @@ func TestRefreshCredentials(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") fmt.Fprintln(w, tt.responseBody) })) @@ -332,6 +332,81 @@ func TestRefreshCredentials(t *testing.T) { // TestLoadCredentials tests the LoadCredentials function func TestLoadCredentials(t *testing.T) { + t.Parallel() + tests := []struct { + name string + fileContent string + expectedError string + expectedResult OpenIdCredentials + }{ + { + name: "Successful load", + fileContent: `{"access_token":"access_token","refresh_token":"refresh_token","expiry":"2024-10-05T17:46:27+10:00"}`, + expectedResult: OpenIdCredentials{ + AccessToken: "access_token", + RefreshToken: "refresh_token", + AccessTokenExpiresAt: time.Date(2024, 10, 5, 17, 46, 27, 0, time.FixedZone("AEST", 10*60*60)), + }, + }, + { + name: "Error unmarshaling credentials", + fileContent: `invalid_json`, + expectedError: "error unmarshaling credentials: invalid character 'i' looking for beginning of value", + }, + } + + for _, tt := range tests { + 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) + + // Create the minder directory inside the temp directory + minderDir := filepath.Join(tempDir, "minder") + err = os.MkdirAll(minderDir, 0750) + if err != nil { + t.Fatalf("failed to create minder directory: %v", err) + } + + filePath := filepath.Join(minderDir, "credentials.json") + + if tt.fileContent != "" { + // Create a temporary file with the specified content + err := os.WriteFile(filePath, []byte(tt.fileContent), 0600) + if err != nil { + t.Fatalf("failed to write test file: %v", err) + } + } + + // Temporarily override the environment variable for the test + originalEnv := os.Getenv("XDG_CONFIG_HOME") + err = os.Setenv("XDG_CONFIG_HOME", tempDir) + if err != nil { + t.Errorf("error setting XDG_CONFIG_HOME: %v", err) + } + defer os.Setenv("XDG_CONFIG_HOME", originalEnv) + + result, err := LoadCredentials() + if tt.expectedError != "" { + if err == nil || err.Error() != tt.expectedError { + t.Errorf("expected error %v, got %v", tt.expectedError, err) + } + } else { + if err != nil { + t.Errorf("expected no error, got %v", err) + } + if result.AccessToken != tt.expectedResult.AccessToken || result.RefreshToken != tt.expectedResult.RefreshToken || !result.AccessTokenExpiresAt.Equal(tt.expectedResult.AccessTokenExpiresAt) { + t.Errorf("expected result %v, got %v", tt.expectedResult, result) + } + } + }) + } } // TestRevokeToken tests the RevokeToken function @@ -496,7 +571,7 @@ func TestOpenFileArg(t *testing.T) { // TestExpandFileArgs tests the ExpandFileArgs function. func TestExpandFileArgs(t *testing.T) { - t.Parallel() // Ensure the test function itself runs in parallel + t.Parallel() tests := []struct { name string From 50c5d2563591dd34fb708a5900cd817ded430173 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Mon, 7 Oct 2024 20:47:37 +1100 Subject: [PATCH 05/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the lint errors after fixing lint version. Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index aad17b74b5..444c5460cc 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -21,13 +21,14 @@ import ( _ "github.com/lib/pq" "github.com/spf13/cobra" "github.com/spf13/viper" - pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" "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" ) // TestGetConfigDirPath tests the GetConfigDirPath function @@ -410,9 +411,9 @@ func TestLoadCredentials(t *testing.T) { } // TestRevokeToken tests the RevokeToken function -func TestRevokeToken(t *testing.T) { +// func TestRevokeToken(t *testing.T) { -} +// } // TestGetJsonFromProto tests the GetJsonFromProto function func TestGetJsonFromProto(t *testing.T) { From 64279004596adae8a72df511283bceefb1af1195 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Tue, 8 Oct 2024 23:14:02 +1100 Subject: [PATCH 06/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit adds test coverage for RevokeToken function. Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 73 ++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 444c5460cc..65a701de65 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -411,9 +411,78 @@ func TestLoadCredentials(t *testing.T) { } // TestRevokeToken tests the RevokeToken function -// func TestRevokeToken(t *testing.T) { +func TestRevokeToken(t *testing.T) { + t.Parallel() + tests := []struct { + name string + token string + issuerUrl string + clientId string + tokenHint string + expectedPath string + expectError bool + }{ + { + name: "Valid token revocation", + token: "test-token", + issuerUrl: "http://localhost:8081", + clientId: "minder-cli", + tokenHint: "refresh_token", + expectedPath: "/realms/stacklok/protocol/openid-connect/revoke", + expectError: false, + }, + { + name: "Invalid issuer URL", + token: "test-token", + issuerUrl: "://invalid-url", + clientId: "minder-cli", + tokenHint: "refresh_token", + expectedPath: "", + expectError: true, + }, + } -// } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var server *httptest.Server + if tt.name != "Invalid issuer URL" { + server = httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + if r.URL.Path != tt.expectedPath { + t.Errorf("expected path %s, got %s", tt.expectedPath, r.URL.Path) + } + + if err := r.ParseForm(); err != nil { + t.Fatalf("error parsing form: %v", err) + } + + if r.Form.Get("client_id") != tt.clientId { + t.Errorf("expected client_id %s, got %s", tt.clientId, r.Form.Get("client_id")) + } + + if r.Form.Get("token") != tt.token { + t.Errorf("expected token %s, got %s", tt.token, r.Form.Get("token")) + } + + if r.Form.Get("token_type_hint") != tt.tokenHint { + t.Errorf("expected token_type_hint %s, got %s", tt.tokenHint, r.Form.Get("token_type_hint")) + } + })) + defer server.Close() + } + + issuerUrl := tt.issuerUrl + if tt.name != "Invalid issuer URL" { + issuerUrl = server.URL + } + + err := RevokeToken(tt.token, issuerUrl, tt.clientId, tt.tokenHint) + if (err != nil) != tt.expectError { + t.Errorf("RevokeToken() error = %v, expectError %v", err, tt.expectError) + } + }) + } +} // TestGetJsonFromProto tests the GetJsonFromProto function func TestGetJsonFromProto(t *testing.T) { From 13fd878c5aeb0828de0bdc45afe697efb5f8a98d Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Sat, 18 Jan 2025 20:08:03 +1100 Subject: [PATCH 07/17] 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") From 23b284165135d0a58b13a5e9d1816b0ac8edb7c4 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Sun, 26 Jan 2025 17:09:15 +1100 Subject: [PATCH 08/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - TestGetConfigDirPath; Change from mu to envLock - TestGetGrpcConnection: Moved deferred call up next to the os.Setenv call to reset env variable. - TestSaveCredentials: Add lock so that config dir path is retrieved correctly. - TestRemoveCredentials: Added deferred call up next to the os.Setenv call to reset env variable. - TestLoadCredentials: Removed the statement that capture range variable and removed commented code, moved the deferred call up next to the resource it creates. - TestOpenFileArg: Add t.TempDir() to create test the file. - TestRevokeToken: Add a func(t *testing.T) *httptest.Server in the test cases which constructs the server. - ExpandFileArgs: Using slices.EqualFunc with an anonymous function Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 131 ++++++++++++++++------------------ 1 file changed, 61 insertions(+), 70 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 2fc15ef47f..332015b0cf 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -12,6 +12,7 @@ import ( "net/url" "os" "path/filepath" + "slices" "strconv" "strings" "sync" @@ -32,7 +33,7 @@ import ( // TestGetConfigDirPath tests the GetConfigDirPath function func TestGetConfigDirPath(t *testing.T) { t.Parallel() - var mu sync.Mutex + var envLock sync.Mutex tests := []struct { name string envVar string @@ -56,8 +57,8 @@ func TestGetConfigDirPath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - mu.Lock() - defer mu.Unlock() + envLock.Lock() + defer envLock.Unlock() err := os.Setenv("XDG_CONFIG_HOME", tt.envVar) if err != nil { t.Errorf("error setting XDG_CONFIG_HOME: %v", err) @@ -153,6 +154,8 @@ func TestGetGrpcConnection(t *testing.T) { t.Parallel() originalEnvToken := os.Getenv(util.MinderAuthTokenEnvVar) err := os.Setenv(util.MinderAuthTokenEnvVar, tt.envToken) + // reset this the environment variable when complete. + defer os.Setenv(util.MinderAuthTokenEnvVar, originalEnvToken) if err != nil { t.Errorf("error setting MinderAuthTokenEnvVar: %v", err) } @@ -166,7 +169,7 @@ func TestGetGrpcConnection(t *testing.T) { t.Errorf("error closing connection: %v", err) } } - defer os.Setenv(util.MinderAuthTokenEnvVar, originalEnvToken) + }) } } @@ -174,6 +177,9 @@ func TestGetGrpcConnection(t *testing.T) { // TestSaveCredentials tests the SaveCredentials function func TestSaveCredentials(t *testing.T) { t.Parallel() + var envLock sync.Mutex + envLock.Lock() + defer envLock.Unlock() tokens := util.OpenIdCredentials{ AccessToken: "test_access_token", RefreshToken: "test_refresh_token", @@ -226,10 +232,13 @@ func TestRemoveCredentials(t *testing.T) { // Create a temporary directory testDir := t.TempDir() + originalEnv := os.Getenv("XDG_CONFIG_HOME") err := os.Setenv("XDG_CONFIG_HOME", testDir) if err != nil { t.Errorf("error setting XDG_CONFIG_HOME: %v", err) } + // reset this the environment variable when complete. + defer os.Setenv("XDG_CONFIG_HOME", originalEnv) xdgConfigHome := os.Getenv("XDG_CONFIG_HOME") @@ -367,18 +376,11 @@ func TestLoadCredentials(t *testing.T) { } for _, tt := range tests { - 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 := t.TempDir() - + defer os.RemoveAll(tempDir) // Create the minder directory inside the temp directory minderDir := filepath.Join(tempDir, "minder") err := os.MkdirAll(minderDir, 0750) @@ -407,6 +409,8 @@ func TestLoadCredentials(t *testing.T) { if err != nil { t.Errorf("error setting XDG_CONFIG_HOME: %v", err) } + // reset this the environment variable when complete. + defer os.Setenv("XDG_CONFIG_HOME", originalEnv) result, err := util.LoadCredentials() if tt.expectedError != "" { @@ -421,25 +425,38 @@ 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) + }) } } +type TestCase struct { + name string + token string + issuerUrl string + clientId string + tokenHint string + expectedPath string + expectError bool + createServer func(t *testing.T, tt TestCase) *httptest.Server +} + +func createTestServer(t *testing.T, tt TestCase) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + err := r.ParseForm() + require.NoError(t, err, "error parsing form") + + require.Equal(t, tt.clientId, r.Form.Get("client_id")) + require.Equal(t, tt.token, r.Form.Get("token")) + require.Equal(t, tt.tokenHint, r.Form.Get("token_type_hint")) + })) +} + // TestRevokeToken tests the RevokeToken function func TestRevokeToken(t *testing.T) { t.Parallel() - tests := []struct { - name string - token string - issuerUrl string - clientId string - tokenHint string - expectedPath string - expectError bool - }{ + tests := []TestCase{ { name: "Valid token revocation", token: "test-token", @@ -448,6 +465,7 @@ func TestRevokeToken(t *testing.T) { tokenHint: "refresh_token", expectedPath: "/realms/stacklok/protocol/openid-connect/revoke", expectError: false, + createServer: createTestServer, }, { name: "Invalid issuer URL", @@ -457,49 +475,28 @@ func TestRevokeToken(t *testing.T) { tokenHint: "refresh_token", expectedPath: "", expectError: true, + createServer: nil, }, } for _, tt := range tests { + tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { t.Parallel() var server *httptest.Server - if tt.name != "Invalid issuer URL" { - server = httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - if r.URL.Path != tt.expectedPath { - t.Errorf("expected path %s, got %s", tt.expectedPath, r.URL.Path) - } - - if err := r.ParseForm(); err != nil { - t.Fatalf("error parsing form: %v", err) - } - - if r.Form.Get("client_id") != tt.clientId { - t.Errorf("expected client_id %s, got %s", tt.clientId, r.Form.Get("client_id")) - } - - if r.Form.Get("token") != tt.token { - t.Errorf("expected token %s, got %s", tt.token, r.Form.Get("token")) - } - - if r.Form.Get("token_type_hint") != tt.tokenHint { - t.Errorf("expected token_type_hint %s, got %s", tt.tokenHint, r.Form.Get("token_type_hint")) - } - })) + if tt.createServer != nil { + server = tt.createServer(t, tt) defer server.Close() + tt.issuerUrl = server.URL } - issuerUrl := tt.issuerUrl - if tt.name != "Invalid issuer URL" { - issuerUrl = server.URL - } - - err := util.RevokeToken(tt.token, issuerUrl, tt.clientId, tt.tokenHint) + err := util.RevokeToken(tt.token, tt.issuerUrl, tt.clientId, tt.tokenHint) if (err != nil) != tt.expectError { t.Errorf("RevokeToken() error = %v, expectError %v", err, tt.expectError) } }) } + } // TestGetJsonFromProto tests the GetJsonFromProto function @@ -626,18 +623,22 @@ func TestOpenFileArg(t *testing.T) { for _, tc := range testCases { tc := tc + testDir := t.TempDir() + var tempFilePath string t.Run(tc.name, func(t *testing.T) { t.Parallel() - // Create a temporary file for testing if tc.filePath == "testfile.txt" { - err := os.WriteFile(tc.filePath, []byte(tc.expectedDesc), 0600) + tempFilePath = filepath.Join(testDir, tc.filePath) + err := os.WriteFile(tempFilePath, []byte(tc.expectedDesc), 0600) assert.NoError(t, err) - defer os.Remove(tc.filePath) + defer os.Remove(tempFilePath) + } else { + // When handling the dash (-) as a file path, it should read from the provided io.Reader (tc.dashOpen) instead of trying to open a file + tempFilePath = tc.filePath } - - desc, closer, err := util.OpenFileArg(tc.filePath, tc.dashOpen) + desc, closer, err := util.OpenFileArg(tempFilePath, tc.dashOpen) if closer != nil { defer closer() } @@ -719,7 +720,9 @@ func TestExpandFileArgs(t *testing.T) { if (err != nil) != tt.wantErr { t.Errorf("ExpandFileArgs() error = %v, wantErr %v", err, tt.wantErr) } - if !compareExpandedFiles(got, tt.expected) { + if !slices.EqualFunc(got, tt.expected, func(a, b util.ExpandedFile) bool { + return a.Path == b.Path && a.Expanded == b.Expanded + }) { t.Errorf("ExpandFileArgs() = %v, want %v", got, tt.expected) } defer os.RemoveAll(testDir) @@ -727,18 +730,6 @@ func TestExpandFileArgs(t *testing.T) { } } -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. func setupTestFiles(testDir string) error { if err := os.MkdirAll(fmt.Sprintf("%s/testdir", testDir), 0750); err != nil { From c5888ed3dd34cbb7a914c2511013e95dce32c697 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Mon, 27 Jan 2025 08:44:54 +1100 Subject: [PATCH 09/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - Add envLock mutex to ensure that all the tests that run t.SetEnv("XDG_CONFIG...") need to be prevented from running at the same time as each other. - TestGetConfigDirPath: Added deferred call up next to the os.Setenv call to reset env variable. - TestOpenFileArg* Removed if condition. Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 43 +++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 332015b0cf..e107ff1a28 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -30,10 +30,14 @@ import ( pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" ) +var ( + // envLock is a mutex to ensure that all the tests that run os.SetEnv("XDG_CONFIG...") need to be prevented from running at the same time as each other. + envLock = &sync.Mutex{} +) + // TestGetConfigDirPath tests the GetConfigDirPath function func TestGetConfigDirPath(t *testing.T) { - t.Parallel() - var envLock sync.Mutex + tests := []struct { name string envVar string @@ -59,10 +63,14 @@ func TestGetConfigDirPath(t *testing.T) { t.Parallel() envLock.Lock() defer envLock.Unlock() + originalEnv := os.Getenv("XDG_CONFIG_HOME") err := os.Setenv("XDG_CONFIG_HOME", tt.envVar) if err != nil { t.Errorf("error setting XDG_CONFIG_HOME: %v", err) } + // reset this the environment variable when complete. + defer os.Setenv("XDG_CONFIG_HOME", originalEnv) + path, err := util.GetConfigDirPath() if (err != nil) != tt.expectingError { t.Errorf("expected error: %v, got: %v", tt.expectingError, err) @@ -177,7 +185,6 @@ func TestGetGrpcConnection(t *testing.T) { // TestSaveCredentials tests the SaveCredentials function func TestSaveCredentials(t *testing.T) { t.Parallel() - var envLock sync.Mutex envLock.Lock() defer envLock.Unlock() tokens := util.OpenIdCredentials{ @@ -186,7 +193,17 @@ func TestSaveCredentials(t *testing.T) { AccessTokenExpiresAt: time.Time{}.Add(7 * 24 * time.Hour), } - cfgPath, err := util.GetConfigDirPath() + // Create a temporary directory + testDir := t.TempDir() + + originalEnv := os.Getenv("XDG_CONFIG_HOME") + err := os.Setenv("XDG_CONFIG_HOME", testDir) + if err != nil { + t.Errorf("error setting XDG_CONFIG_HOME: %v", err) + } + // reset this the environment variable when complete. + defer os.Setenv("XDG_CONFIG_HOME", originalEnv) + cfgPath := filepath.Join(testDir, "minder") if err != nil { t.Fatalf("error getting config path: %v", err) @@ -229,6 +246,8 @@ func TestSaveCredentials(t *testing.T) { // TestRemoveCredentials tests the RemoveCredentials function func TestRemoveCredentials(t *testing.T) { t.Parallel() + envLock.Lock() + defer envLock.Unlock() // Create a temporary directory testDir := t.TempDir() @@ -270,6 +289,8 @@ func TestRemoveCredentials(t *testing.T) { // TestRefreshCredentials tests the RefreshCredentials function func TestRefreshCredentials(t *testing.T) { t.Parallel() + envLock.Lock() + defer envLock.Unlock() tests := []struct { name string refreshToken string @@ -319,6 +340,8 @@ func TestRefreshCredentials(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() + envLock.Lock() + defer envLock.Unlock() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") fmt.Fprintln(w, tt.responseBody) @@ -647,12 +670,12 @@ func TestOpenFileArg(t *testing.T) { assert.Error(t, err) } else { assert.NoError(t, err) - if tc.filePath == "-" || tc.filePath != "-" { - buf := new(strings.Builder) - _, err := io.Copy(buf, desc) - assert.NoError(t, err) - assert.Equal(t, tc.expectedDesc, buf.String()) - } + + buf := new(strings.Builder) + _, err := io.Copy(buf, desc) + assert.NoError(t, err) + assert.Equal(t, tc.expectedDesc, buf.String()) + } }) } From 39faa7904407a1d30604ab714ba70e719aa40f83 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Mon, 27 Jan 2025 09:39:54 +1100 Subject: [PATCH 10/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - Add a helper function SetEnvVar just to make sure that mutex lock handle things consistently - Fix all test functions that require mutex with above helper function Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 91 +++++++++++++---------------------- 1 file changed, 34 insertions(+), 57 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index e107ff1a28..ed1bb55510 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -35,9 +35,21 @@ var ( envLock = &sync.Mutex{} ) +func SetEnvVar(t *testing.T, env string, value string) { + t.Helper() // Keep golangci-lint happy + envLock.Lock() + t.Cleanup(envLock.Unlock) + + err := os.Setenv(env, value) + if err != nil { + t.Errorf("error setting %v: %v", env, err) + } + +} + // TestGetConfigDirPath tests the GetConfigDirPath function func TestGetConfigDirPath(t *testing.T) { - + t.Parallel() tests := []struct { name string envVar string @@ -61,16 +73,7 @@ func TestGetConfigDirPath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - envLock.Lock() - defer envLock.Unlock() - originalEnv := os.Getenv("XDG_CONFIG_HOME") - err := os.Setenv("XDG_CONFIG_HOME", tt.envVar) - if err != nil { - t.Errorf("error setting XDG_CONFIG_HOME: %v", err) - } - // reset this the environment variable when complete. - defer os.Setenv("XDG_CONFIG_HOME", originalEnv) - + SetEnvVar(t, "XDG_CONFIG_HOME", tt.envVar) path, err := util.GetConfigDirPath() if (err != nil) != tt.expectingError { t.Errorf("expected error: %v, got: %v", tt.expectingError, err) @@ -164,9 +167,7 @@ func TestGetGrpcConnection(t *testing.T) { err := os.Setenv(util.MinderAuthTokenEnvVar, tt.envToken) // reset this the environment variable when complete. defer os.Setenv(util.MinderAuthTokenEnvVar, originalEnvToken) - if err != nil { - t.Errorf("error setting MinderAuthTokenEnvVar: %v", err) - } + 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) @@ -185,8 +186,7 @@ func TestGetGrpcConnection(t *testing.T) { // TestSaveCredentials tests the SaveCredentials function func TestSaveCredentials(t *testing.T) { t.Parallel() - envLock.Lock() - defer envLock.Unlock() + tokens := util.OpenIdCredentials{ AccessToken: "test_access_token", RefreshToken: "test_refresh_token", @@ -196,18 +196,9 @@ func TestSaveCredentials(t *testing.T) { // Create a temporary directory testDir := t.TempDir() - originalEnv := os.Getenv("XDG_CONFIG_HOME") - err := os.Setenv("XDG_CONFIG_HOME", testDir) - if err != nil { - t.Errorf("error setting XDG_CONFIG_HOME: %v", err) - } - // reset this the environment variable when complete. - defer os.Setenv("XDG_CONFIG_HOME", originalEnv) - cfgPath := filepath.Join(testDir, "minder") + SetEnvVar(t, "XDG_CONFIG_HOME", testDir) - if err != nil { - t.Fatalf("error getting config path: %v", err) - } + cfgPath := filepath.Join(testDir, "minder") expectedFilePath := filepath.Join(cfgPath, "credentials.json") @@ -246,25 +237,17 @@ func TestSaveCredentials(t *testing.T) { // TestRemoveCredentials tests the RemoveCredentials function func TestRemoveCredentials(t *testing.T) { t.Parallel() - envLock.Lock() - defer envLock.Unlock() + // Create a temporary directory testDir := t.TempDir() - originalEnv := os.Getenv("XDG_CONFIG_HOME") - err := os.Setenv("XDG_CONFIG_HOME", testDir) - if err != nil { - t.Errorf("error setting XDG_CONFIG_HOME: %v", err) - } - // reset this the environment variable when complete. - defer os.Setenv("XDG_CONFIG_HOME", originalEnv) - + SetEnvVar(t, "XDG_CONFIG_HOME", testDir) 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) @@ -289,8 +272,10 @@ func TestRemoveCredentials(t *testing.T) { // TestRefreshCredentials tests the RefreshCredentials function func TestRefreshCredentials(t *testing.T) { t.Parallel() - envLock.Lock() - defer envLock.Unlock() + // Create a temporary directory + testDir := t.TempDir() + + SetEnvVar(t, "XDG_CONFIG_HOME", testDir) tests := []struct { name string refreshToken string @@ -340,8 +325,7 @@ func TestRefreshCredentials(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - envLock.Lock() - defer envLock.Unlock() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") fmt.Fprintln(w, tt.responseBody) @@ -371,6 +355,7 @@ func TestRefreshCredentials(t *testing.T) { // TestLoadCredentials tests the LoadCredentials function func TestLoadCredentials(t *testing.T) { t.Parallel() + tests := []struct { name string fileContent string @@ -402,10 +387,10 @@ func TestLoadCredentials(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - tempDir := t.TempDir() - defer os.RemoveAll(tempDir) + testDir := t.TempDir() + SetEnvVar(t, "XDG_CONFIG_HOME", testDir) // Create the minder directory inside the temp directory - minderDir := filepath.Join(tempDir, "minder") + minderDir := filepath.Join(testDir, "minder") err := os.MkdirAll(minderDir, 0750) if err != nil { t.Fatalf("failed to create minder directory: %v", err) @@ -426,15 +411,6 @@ func TestLoadCredentials(t *testing.T) { 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 - originalEnv := os.Getenv("XDG_CONFIG_HOME") - err = os.Setenv("XDG_CONFIG_HOME", tempDir) - if err != nil { - t.Errorf("error setting XDG_CONFIG_HOME: %v", err) - } - // reset this the environment variable when complete. - defer os.Setenv("XDG_CONFIG_HOME", originalEnv) - result, err := util.LoadCredentials() if tt.expectedError != "" { if err == nil || !strings.HasPrefix(err.Error(), tt.expectedError) { @@ -554,13 +530,14 @@ func TestGetJsonFromProto(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - jsonResult, err := util.GetJsonFromProto(tt.input) + jsonStr, err := util.GetJsonFromProto(tt.input) + if (err != nil) != tt.expectedError { t.Errorf("GetJsonFromProto() error = %v, expectedError %v", err, tt.expectedError) return } - if jsonResult != tt.expectedJson { - t.Errorf("GetJsonFromProto() = %v, expected %v", jsonResult, tt.expectedJson) + if strings.TrimSpace(jsonStr) != strings.TrimSpace(tt.expectedJson) { + t.Errorf("GetJsonFromProto() = %v, expected %v", jsonStr, tt.expectedJson) } }) } From 7b95216dfe9f3844128597a0a516809d82828de2 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Mon, 27 Jan 2025 09:49:02 +1100 Subject: [PATCH 11/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - Fix lint errors Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index ed1bb55510..91d568e593 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -165,6 +165,9 @@ func TestGetGrpcConnection(t *testing.T) { t.Parallel() originalEnvToken := os.Getenv(util.MinderAuthTokenEnvVar) err := os.Setenv(util.MinderAuthTokenEnvVar, tt.envToken) + if err != nil { + t.Errorf("error setting %v: %v", util.MinderAuthTokenEnvVar, err) + } // reset this the environment variable when complete. defer os.Setenv(util.MinderAuthTokenEnvVar, originalEnvToken) @@ -442,6 +445,7 @@ type TestCase struct { } func createTestServer(t *testing.T, tt TestCase) *httptest.Server { + t.Helper() return httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { err := r.ParseForm() require.NoError(t, err, "error parsing form") From 6c53d65e16a4b96d424fa13ab651a27fc95ba382 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Mon, 27 Jan 2025 09:57:16 +1100 Subject: [PATCH 12/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - Remove the line tt := tt to capture the range variable in the functions. - Normalize JSON strings by removing all whitespaces and new lines in TestGetYamlFromProto Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 91d568e593..a149276e61 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -483,7 +483,7 @@ func TestRevokeToken(t *testing.T) { } for _, tt := range tests { - tt := tt // capture range variable + t.Run(tt.name, func(t *testing.T) { t.Parallel() var server *httptest.Server @@ -584,8 +584,12 @@ owner: repoOwner t.Errorf("GetYamlFromProto() error = %v, expectedError %v", err, tt.expectedError) return } - if yamlResult != tt.expectedYaml { - t.Errorf("GetYamlFromProto() = %v, expected %v", yamlResult, tt.expectedYaml) + // Normalize JSON strings by removing all whitespaces and new lines + normalizedResult := strings.Join(strings.Fields(yamlResult), "") + normalizedExpected := strings.Join(strings.Fields(tt.expectedYaml), "") + + if normalizedResult != normalizedExpected { + t.Errorf("GetJsonFromProto() = %v, expected %v", normalizedResult, normalizedExpected) } }) } @@ -704,7 +708,6 @@ func TestExpandFileArgs(t *testing.T) { }, } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() // Create a temporary directory @@ -906,7 +909,6 @@ func TestInt32FromString(t *testing.T) { }, } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() From 7a031061464b228e5a6a4d6b890e0c53de3379c1 Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Mon, 27 Jan 2025 11:17:23 +1100 Subject: [PATCH 13/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - TestOpenFileArg: create testfile.txt outside the test case code, to avoid needing as much conditional logic in the test. Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index a149276e61..bfaf5c2154 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -599,6 +599,13 @@ owner: repoOwner func TestOpenFileArg(t *testing.T) { t.Parallel() + testDir := t.TempDir() + tempFilePath := filepath.Join(testDir, "testfile.txt") + err := os.WriteFile(tempFilePath, []byte("test content"), 0600) + if err != nil { + t.Fatalf("Failed to create test file: %v", err) + } + testCases := []struct { name string filePath string @@ -615,7 +622,7 @@ func TestOpenFileArg(t *testing.T) { }, { name: "Valid file path", - filePath: "testfile.txt", + filePath: tempFilePath, dashOpen: nil, expectedDesc: "test content", expectError: false, @@ -630,23 +637,9 @@ func TestOpenFileArg(t *testing.T) { } for _, tc := range testCases { - tc := tc - testDir := t.TempDir() - var tempFilePath string - t.Run(tc.name, func(t *testing.T) { t.Parallel() - // Create a temporary file for testing - if tc.filePath == "testfile.txt" { - tempFilePath = filepath.Join(testDir, tc.filePath) - err := os.WriteFile(tempFilePath, []byte(tc.expectedDesc), 0600) - assert.NoError(t, err) - defer os.Remove(tempFilePath) - } else { - // When handling the dash (-) as a file path, it should read from the provided io.Reader (tc.dashOpen) instead of trying to open a file - tempFilePath = tc.filePath - } - desc, closer, err := util.OpenFileArg(tempFilePath, tc.dashOpen) + desc, closer, err := util.OpenFileArg(tc.filePath, tc.dashOpen) if closer != nil { defer closer() } @@ -660,7 +653,6 @@ func TestOpenFileArg(t *testing.T) { _, err := io.Copy(buf, desc) assert.NoError(t, err) assert.Equal(t, tc.expectedDesc, buf.String()) - } }) } From af6a40cec3520716dfa06b27c419112387a343ac Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Mon, 27 Jan 2025 12:21:33 +1100 Subject: [PATCH 14/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - TestExpandFileArgs: Removed os.RemoveAll(testDir) line - Changed name to setEnvVar and fixed all references - TestGetGrpcConnection: use setEnvVar to set env variable - TestGetJsonFromProto: Normalize json to handle empty space - remved helper function createTestServer Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 93 +++++++++++++---------------------- 1 file changed, 35 insertions(+), 58 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index bfaf5c2154..66cad91d7a 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -9,7 +9,6 @@ import ( "io" "net/http" "net/http/httptest" - "net/url" "os" "path/filepath" "slices" @@ -35,16 +34,17 @@ var ( envLock = &sync.Mutex{} ) -func SetEnvVar(t *testing.T, env string, value string) { +func setEnvVar(t *testing.T, env string, value string) { t.Helper() // Keep golangci-lint happy envLock.Lock() t.Cleanup(envLock.Unlock) + originalEnvToken := os.Getenv(env) err := os.Setenv(env, value) if err != nil { t.Errorf("error setting %v: %v", env, err) } - + defer os.Setenv(env, originalEnvToken) } // TestGetConfigDirPath tests the GetConfigDirPath function @@ -73,7 +73,7 @@ func TestGetConfigDirPath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - SetEnvVar(t, "XDG_CONFIG_HOME", tt.envVar) + setEnvVar(t, "XDG_CONFIG_HOME", tt.envVar) path, err := util.GetConfigDirPath() if (err != nil) != tt.expectingError { t.Errorf("expected error: %v, got: %v", tt.expectingError, err) @@ -163,13 +163,8 @@ func TestGetGrpcConnection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - originalEnvToken := os.Getenv(util.MinderAuthTokenEnvVar) - err := os.Setenv(util.MinderAuthTokenEnvVar, tt.envToken) - if err != nil { - t.Errorf("error setting %v: %v", util.MinderAuthTokenEnvVar, err) - } - // reset this the environment variable when complete. - defer os.Setenv(util.MinderAuthTokenEnvVar, originalEnvToken) + + setEnvVar(t, util.MinderAuthTokenEnvVar, tt.envToken) conn, err := util.GetGrpcConnection(tt.grpcHost, tt.grpcPort, tt.allowInsecure, tt.issuerUrl, tt.clientId) if (err != nil) != tt.expectedError { @@ -199,16 +194,14 @@ func TestSaveCredentials(t *testing.T) { // Create a temporary directory testDir := t.TempDir() - SetEnvVar(t, "XDG_CONFIG_HOME", testDir) + setEnvVar(t, "XDG_CONFIG_HOME", testDir) cfgPath := filepath.Join(testDir, "minder") expectedFilePath := filepath.Join(cfgPath, "credentials.json") filePath, err := util.SaveCredentials(tokens) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } + require.NoError(t, err) if filePath != expectedFilePath { t.Errorf("expected file path %v, got %v", expectedFilePath, filePath) @@ -216,25 +209,16 @@ func TestSaveCredentials(t *testing.T) { // Verify the file content credsJSON, err := json.Marshal(tokens) - if err != nil { - t.Fatalf("error marshaling credentials: %v", err) - } + require.NoError(t, err) - fpath := filepath.Clean(filePath) - content, err := os.ReadFile(fpath) - if err != nil { - t.Fatalf("error reading file: %v", err) - } + cleanPath := filepath.Clean(filePath) + content, err := os.ReadFile(cleanPath) + require.NoError(t, err) if string(content) != string(credsJSON) { t.Errorf("expected file content %v, got %v", string(credsJSON), string(content)) } - // Clean up - err = os.Remove(filePath) - if err != nil { - t.Fatalf("error removing file: %v", err) - } } // TestRemoveCredentials tests the RemoveCredentials function @@ -244,7 +228,7 @@ func TestRemoveCredentials(t *testing.T) { // Create a temporary directory testDir := t.TempDir() - SetEnvVar(t, "XDG_CONFIG_HOME", testDir) + setEnvVar(t, "XDG_CONFIG_HOME", testDir) xdgConfigHome := os.Getenv("XDG_CONFIG_HOME") filePath := filepath.Join(xdgConfigHome, "minder", "credentials.json") @@ -278,7 +262,7 @@ func TestRefreshCredentials(t *testing.T) { // Create a temporary directory testDir := t.TempDir() - SetEnvVar(t, "XDG_CONFIG_HOME", testDir) + setEnvVar(t, "XDG_CONFIG_HOME", testDir) tests := []struct { name string refreshToken string @@ -291,7 +275,6 @@ func TestRefreshCredentials(t *testing.T) { { name: "Successful refresh with local server", refreshToken: "valid_refresh_token", - issuerUrl: "http://localhost:8081", clientId: "minder-cli", responseBody: `{"access_token":"new_access_token","refresh_token":"new_refresh_token","expires_in":3600}`, expectedResult: util.OpenIdCredentials{ @@ -303,14 +286,12 @@ func TestRefreshCredentials(t *testing.T) { { name: "Error fetching new credentials (responseBody is missing) rwith local server", refreshToken: "valid_refresh_token", - issuerUrl: "http://localhost:8081", clientId: "minder-cli", expectedError: "error unmarshaling credentials: EOF", }, { name: "Error unmarshaling credentials with local server", refreshToken: "valid_refresh_token", - issuerUrl: "http://localhost:8081", clientId: "minder-cli", responseBody: `invalid_json`, expectedError: "error unmarshaling credentials: invalid character 'i' looking for beginning of value", @@ -318,7 +299,6 @@ func TestRefreshCredentials(t *testing.T) { { name: "Error refreshing credentials with local server", refreshToken: "valid_refresh_token", - issuerUrl: "http://localhost:8081", clientId: "minder-cli", responseBody: `{"error":"invalid_grant","error_description":"Invalid refresh token"}`, expectedError: "error refreshing credentials: invalid_grant: Invalid refresh token", @@ -335,8 +315,7 @@ func TestRefreshCredentials(t *testing.T) { })) defer server.Close() - parsedURL, _ := url.Parse(server.URL) - tt.issuerUrl = parsedURL.String() + tt.issuerUrl = server.URL result, err := util.RefreshCredentials(tt.refreshToken, tt.issuerUrl, tt.clientId) if tt.expectedError != "" { @@ -391,7 +370,7 @@ func TestLoadCredentials(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() testDir := t.TempDir() - SetEnvVar(t, "XDG_CONFIG_HOME", testDir) + setEnvVar(t, "XDG_CONFIG_HOME", testDir) // Create the minder directory inside the temp directory minderDir := filepath.Join(testDir, "minder") err := os.MkdirAll(minderDir, 0750) @@ -403,10 +382,7 @@ func TestLoadCredentials(t *testing.T) { if tt.fileContent != "" { // Create a temporary file with the specified content - err := os.WriteFile(filePath, []byte(tt.fileContent), 0600) - if err != nil { - t.Fatalf("failed to write test file: %v", err) - } + require.NoError(t, os.WriteFile(filePath, []byte(tt.fileContent), 0600)) // 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 { @@ -433,6 +409,7 @@ func TestLoadCredentials(t *testing.T) { } } +// TestCase struct for holding test case data type TestCase struct { name string token string @@ -444,18 +421,6 @@ type TestCase struct { createServer func(t *testing.T, tt TestCase) *httptest.Server } -func createTestServer(t *testing.T, tt TestCase) *httptest.Server { - t.Helper() - return httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - err := r.ParseForm() - require.NoError(t, err, "error parsing form") - - require.Equal(t, tt.clientId, r.Form.Get("client_id")) - require.Equal(t, tt.token, r.Form.Get("token")) - require.Equal(t, tt.tokenHint, r.Form.Get("token_type_hint")) - })) -} - // TestRevokeToken tests the RevokeToken function func TestRevokeToken(t *testing.T) { t.Parallel() @@ -468,7 +433,16 @@ func TestRevokeToken(t *testing.T) { tokenHint: "refresh_token", expectedPath: "/realms/stacklok/protocol/openid-connect/revoke", expectError: false, - createServer: createTestServer, + createServer: func(t *testing.T, tt TestCase) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + err := r.ParseForm() + require.NoError(t, err, "error parsing form") + require.Equal(t, tt.clientId, r.Form.Get("client_id")) + require.Equal(t, tt.token, r.Form.Get("token")) + require.Equal(t, tt.tokenHint, r.Form.Get("token_type_hint")) + })) + }, }, { name: "Invalid issuer URL", @@ -499,7 +473,6 @@ func TestRevokeToken(t *testing.T) { } }) } - } // TestGetJsonFromProto tests the GetJsonFromProto function @@ -540,8 +513,13 @@ func TestGetJsonFromProto(t *testing.T) { t.Errorf("GetJsonFromProto() error = %v, expectedError %v", err, tt.expectedError) return } - if strings.TrimSpace(jsonStr) != strings.TrimSpace(tt.expectedJson) { - t.Errorf("GetJsonFromProto() = %v, expected %v", jsonStr, tt.expectedJson) + + // Normalize JSON strings by removing all whitespaces and new lines + normalizedResult := strings.Join(strings.Fields(jsonStr), "") + normalizedExpected := strings.Join(strings.Fields(tt.expectedJson), "") + + if normalizedResult != normalizedExpected { + t.Errorf("GetJsonFromProto() = %v, expected %v", normalizedResult, normalizedExpected) } }) } @@ -724,7 +702,6 @@ func TestExpandFileArgs(t *testing.T) { }) { t.Errorf("ExpandFileArgs() = %v, want %v", got, tt.expected) } - defer os.RemoveAll(testDir) }) } } From a4ce30bf2cc6dffdf0e825761005a7f3ef5b915d Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Tue, 28 Jan 2025 07:48:24 +1100 Subject: [PATCH 15/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - setEnvVar: Fixed setting env to original value using t.Cleanup() Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 66cad91d7a..2a8de6322e 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -39,12 +39,14 @@ func setEnvVar(t *testing.T, env string, value string) { envLock.Lock() t.Cleanup(envLock.Unlock) - originalEnvToken := os.Getenv(env) + originalEnvVal := os.Getenv(env) err := os.Setenv(env, value) if err != nil { t.Errorf("error setting %v: %v", env, err) } - defer os.Setenv(env, originalEnvToken) + + t.Cleanup(func() { _ = os.Setenv(env, originalEnvVal) }) + } // TestGetConfigDirPath tests the GetConfigDirPath function @@ -163,9 +165,7 @@ func TestGetGrpcConnection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - setEnvVar(t, util.MinderAuthTokenEnvVar, tt.envToken) - 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) From 798b21c09ecca47e96ee3a5f97d40690fe58e8bb Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Thu, 30 Jan 2025 05:33:20 +1100 Subject: [PATCH 16/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - introduced two separate mutex locks such as envLockXdgConfigHome, envLockMinderAuthToken - setEnvVar: parameterise mutex lock Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 2a8de6322e..1772e6dad2 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -30,11 +30,20 @@ import ( ) var ( - // envLock is a mutex to ensure that all the tests that run os.SetEnv("XDG_CONFIG...") need to be prevented from running at the same time as each other. - envLock = &sync.Mutex{} + + // envLockXdgConfigHome is a mutex to ensure that all the tests that run os.SetEnv("XDG_CONFIG_HOME") need to be prevented from running at the same time as each other. + envLockXdgConfigHome = &sync.Mutex{} + + // envLockXdgConfig is a mutex to ensure that all the tests that run os.SetEnv("MINDER_AUTH_TOKEN") need to be prevented from running at the same time as each other. + envLockMinderAuthToken = &sync.Mutex{} + + XdgConfigHomeEnvVar = "XDG_CONFIG_HOME" + + MinderAuthTokenEnvVar = "MINDER_AUTH_TOKEN" ) -func setEnvVar(t *testing.T, env string, value string) { +// Based on tests, seemed to need one mutex per env var. +func setEnvVar(t *testing.T, envLock *sync.Mutex, env string, value string) { t.Helper() // Keep golangci-lint happy envLock.Lock() t.Cleanup(envLock.Unlock) @@ -75,7 +84,7 @@ func TestGetConfigDirPath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - setEnvVar(t, "XDG_CONFIG_HOME", tt.envVar) + setEnvVar(t, envLockXdgConfigHome, XdgConfigHomeEnvVar, tt.envVar) path, err := util.GetConfigDirPath() if (err != nil) != tt.expectingError { t.Errorf("expected error: %v, got: %v", tt.expectingError, err) @@ -165,7 +174,7 @@ func TestGetGrpcConnection(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - setEnvVar(t, util.MinderAuthTokenEnvVar, tt.envToken) + setEnvVar(t, envLockMinderAuthToken, MinderAuthTokenEnvVar, tt.envToken) 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) @@ -194,7 +203,7 @@ func TestSaveCredentials(t *testing.T) { // Create a temporary directory testDir := t.TempDir() - setEnvVar(t, "XDG_CONFIG_HOME", testDir) + setEnvVar(t, envLockXdgConfigHome, XdgConfigHomeEnvVar, testDir) cfgPath := filepath.Join(testDir, "minder") @@ -227,9 +236,8 @@ func TestRemoveCredentials(t *testing.T) { // Create a temporary directory testDir := t.TempDir() - - setEnvVar(t, "XDG_CONFIG_HOME", testDir) - xdgConfigHome := os.Getenv("XDG_CONFIG_HOME") + setEnvVar(t, envLockXdgConfigHome, XdgConfigHomeEnvVar, testDir) + xdgConfigHome := os.Getenv(XdgConfigHomeEnvVar) filePath := filepath.Join(xdgConfigHome, "minder", "credentials.json") @@ -262,7 +270,7 @@ func TestRefreshCredentials(t *testing.T) { // Create a temporary directory testDir := t.TempDir() - setEnvVar(t, "XDG_CONFIG_HOME", testDir) + setEnvVar(t, envLockXdgConfigHome, XdgConfigHomeEnvVar, testDir) tests := []struct { name string refreshToken string @@ -370,7 +378,7 @@ func TestLoadCredentials(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() testDir := t.TempDir() - setEnvVar(t, "XDG_CONFIG_HOME", testDir) + setEnvVar(t, envLockXdgConfigHome, XdgConfigHomeEnvVar, testDir) // Create the minder directory inside the temp directory minderDir := filepath.Join(testDir, "minder") err := os.MkdirAll(minderDir, 0750) From 50087af354aabcf4014ef8208e949e224443540d Mon Sep 17 00:00:00 2001 From: Kugamoorthy Gajananan Date: Thu, 30 Jan 2025 05:45:04 +1100 Subject: [PATCH 17/17] Add additional test coverage for helper functions in Util Currently helper functions in Util lacks test coverage This commit fixes the following functions by addressing the review feedback. - Added lint comment //nolint:gosec // This is not a hardcoded credential Signed-off-by: Kugamoorthy Gajananan --- internal/util/helpers_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/util/helpers_test.go b/internal/util/helpers_test.go index 1772e6dad2..9350d7fc87 100644 --- a/internal/util/helpers_test.go +++ b/internal/util/helpers_test.go @@ -36,9 +36,9 @@ var ( // envLockXdgConfig is a mutex to ensure that all the tests that run os.SetEnv("MINDER_AUTH_TOKEN") need to be prevented from running at the same time as each other. envLockMinderAuthToken = &sync.Mutex{} - + //nolint:gosec // This is not a hardcoded credential XdgConfigHomeEnvVar = "XDG_CONFIG_HOME" - + //nolint:gosec // This is not a hardcoded credential MinderAuthTokenEnvVar = "MINDER_AUTH_TOKEN" )