From 0564fec22094b7c994c5951f42fbb67616c1ad90 Mon Sep 17 00:00:00 2001 From: Rayyan Khan Date: Sat, 7 Feb 2026 13:04:33 +0530 Subject: [PATCH 1/3] test/refactored user delete cmd and wrote unit tests for the same Signed-off-by: Rayyan Khan --- cmd/harbor/root/user/delete.go | 116 +++++++++++-------- cmd/harbor/root/user/delete_test.go | 167 ++++++++++++++++++++++++++++ 2 files changed, 236 insertions(+), 47 deletions(-) create mode 100644 cmd/harbor/root/user/delete_test.go diff --git a/cmd/harbor/root/user/delete.go b/cmd/harbor/root/user/delete.go index eaa2b1f4c..da6651371 100644 --- a/cmd/harbor/root/user/delete.go +++ b/cmd/harbor/root/user/delete.go @@ -22,59 +22,81 @@ import ( "github.com/spf13/cobra" ) -// UserDeleteCmd defines the "delete" command for user deletion. -func UserDeleteCmd() *cobra.Command { - cmd := &cobra.Command{ - Use: "delete", - Short: "delete user by name or id", - Args: cobra.MinimumNArgs(0), - Run: func(cmd *cobra.Command, args []string) { - // If there are command line arguments, process them concurrently. - if len(args) > 0 { - var wg sync.WaitGroup - errChan := make(chan error, len(args)) // Channel to collect errors +type UserDeleter interface { + UserDelete(userID int64) error + GetUserIDByName(username string) (int64, error) + GetUserIDFromUser() int64 +} +type DefaultUserDeleter struct{} - for _, arg := range args { - // Retrieve user ID by name. - userID, err := api.GetUsersIdByName(arg) - if err != nil { - log.Errorf("failed to get user id for '%s': %v", arg, err) - continue - } - wg.Add(1) - go func(userID int64) { - defer wg.Done() - if err := api.DeleteUser(userID); err != nil { - errChan <- err - } - }(userID) - } +func (d *DefaultUserDeleter) UserDelete(userID int64) error { + return api.DeleteUser(userID) +} +func (d *DefaultUserDeleter) GetUserIDByName(username string) (int64, error) { + return api.GetUsersIdByName(username) +} +func (d *DefaultUserDeleter) GetUserIDFromUser() int64 { + return prompt.GetUserIdFromUser() +} - // Wait for all goroutines to finish and then close the error channel. - go func() { - wg.Wait() - close(errChan) - }() +func DeleteUser(args []string, userDeleter UserDeleter) { + // If there are command line arguments, process them concurrently. + if len(args) > 0 { + var wg sync.WaitGroup + errChan := make(chan error, len(args)) // Channel to collect errors - // Process errors from the goroutines. - for err := range errChan { - if isUnauthorizedError(err) { - log.Error("Permission denied: Admin privileges are required to execute this command.") - } else { - log.Errorf("failed to delete user: %v", err) - } + for _, arg := range args { + // Retrieve user ID by name. + userID, err := userDeleter.GetUserIDByName(arg) + if err != nil { + log.Errorf("failed to get user id for '%s': %v", arg, err) + continue + } + wg.Add(1) + go func(userID int64) { + defer wg.Done() + if err := userDeleter.UserDelete(userID); err != nil { + errChan <- err } + }(userID) + } + + // Wait for all goroutines to finish and then close the error channel. + go func() { + wg.Wait() + close(errChan) + }() + + // Process errors from the goroutines. + for err := range errChan { + if isUnauthorizedError(err) { + log.Error("Permission denied: Admin privileges are required to execute this command.") } else { - // Interactive mode: get the user ID from the prompt. - userID := prompt.GetUserIdFromUser() - if err := api.DeleteUser(userID); err != nil { - if isUnauthorizedError(err) { - log.Error("Permission denied: Admin privileges are required to execute this command.") - } else { - log.Errorf("failed to delete user: %v", err) - } - } + log.Errorf("failed to delete user: %v", err) + } + } + } else { + // Interactive mode: get the user ID from the prompt. + userID := userDeleter.GetUserIDFromUser() + if err := userDeleter.UserDelete(userID); err != nil { + if isUnauthorizedError(err) { + log.Error("Permission denied: Admin privileges are required to execute this command.") + } else { + log.Errorf("failed to delete user: %v", err) } + } + } +} + +// UserDeleteCmd defines the "delete" command for user deletion. +func UserDeleteCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "delete", + Short: "delete user by name or id", // nope it only deletes by name + Args: cobra.MinimumNArgs(0), + Run: func(cmd *cobra.Command, args []string) { + d := &DefaultUserDeleter{} + DeleteUser(args, d) }, } diff --git a/cmd/harbor/root/user/delete_test.go b/cmd/harbor/root/user/delete_test.go new file mode 100644 index 000000000..125e4622a --- /dev/null +++ b/cmd/harbor/root/user/delete_test.go @@ -0,0 +1,167 @@ +// Copyright Project Harbor Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package user + +import ( + "bytes" + "fmt" + "testing" + + log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" +) + +type MockUserDeleter struct { + id map[string]int64 + user map[int64]string + userCnt int + expectAuthError bool +} + +func (m *MockUserDeleter) UserDelete(userID int64) error { + if m.expectAuthError { + return fmt.Errorf("403") + } + if v, ok := m.user[userID]; ok { + delete(m.id, v) + delete(m.user, userID) + return nil + } + return fmt.Errorf("user %d not found", userID) +} +func (m *MockUserDeleter) GetUserIDByName(username string) (int64, error) { + if v, ok := m.id[username]; ok { + return v, nil + } else { + return 0, fmt.Errorf(`Username %s not found`, username) + } +} +func (m *MockUserDeleter) GetUserIDFromUser() int64 { + return 999 +} + +func initMockUserDeleter(userCnt int, expectAuthError bool) *MockUserDeleter { + m := &MockUserDeleter{ + userCnt: userCnt, + expectAuthError: expectAuthError, + id: make(map[string]int64), + user: make(map[int64]string), + } + for i := 0; i < userCnt; i++ { + m.id[fmt.Sprintf("test%d", i+1)] = int64(i + 1) + m.user[int64(i+1)] = fmt.Sprintf("test%d", i+1) + } + return m +} +func TestDeleteUser(t *testing.T) { + tests := []struct { + name string + setup func() *MockUserDeleter + args []string + notExpectedID []int64 + expectedErr string + }{ + { + name: "successfully delete single user", + setup: func() *MockUserDeleter { + return initMockUserDeleter(5, false) + }, + args: []string{"test1"}, + notExpectedID: []int64{1}, + expectedErr: "", + }, + { + name: "successfully delete multiple users", + setup: func() *MockUserDeleter { + return initMockUserDeleter(5, false) + }, + args: []string{"test1", "test3", "test5"}, + notExpectedID: []int64{1, 3, 5}, + expectedErr: "", + }, + { + name: "delete non-existent user logs error", + setup: func() *MockUserDeleter { + return initMockUserDeleter(5, false) + }, + args: []string{"nonexistent"}, + notExpectedID: []int64{}, + expectedErr: "failed to get user id", + }, + { + name: "permission denied error", + setup: func() *MockUserDeleter { + return initMockUserDeleter(5, true) + }, + args: []string{"test1"}, + notExpectedID: []int64{}, + expectedErr: "Permission denied", + }, + { + name: "mixed existing and non-existing users", + setup: func() *MockUserDeleter { + return initMockUserDeleter(5, false) + }, + args: []string{"test1", "nonexistent", "test3"}, + notExpectedID: []int64{1, 3}, + expectedErr: "failed to get user id", + }, + { + name: "delete with empty args does not error", + setup: func() *MockUserDeleter { + m := initMockUserDeleter(5, false) + m.user[999] = "promptuser" + m.id["promptuser"] = 999 + return m + }, + args: []string{}, + notExpectedID: []int64{999}, + expectedErr: "", + }, + { + name: "delete all users", + setup: func() *MockUserDeleter { + return initMockUserDeleter(3, false) + }, + args: []string{"test1", "test2", "test3"}, + notExpectedID: []int64{1, 2, 3}, + expectedErr: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + originalLogOutput := log.StandardLogger().Out + log.SetOutput(&buf) + defer log.SetOutput(originalLogOutput) + + m := tt.setup() + DeleteUser(tt.args, m) + + logs := buf.String() + + if tt.expectedErr != "" { + assert.Contains(t, logs, tt.expectedErr, "Expected error logs to contain %s but got %s", tt.expectedErr, logs) + } else { + assert.Empty(t, logs, "Expected no error logs but got: %s", logs) + } + + for _, id := range tt.notExpectedID { + _, ok := m.user[id] + assert.False(t, ok, "User with ID %d should have been deleted", id) + } + }) + } +} From ee9bfef4a9759f59509f19a6cc41d2e25685bd84 Mon Sep 17 00:00:00 2001 From: Rayyan Khan Date: Sat, 7 Feb 2026 23:36:29 +0530 Subject: [PATCH 2/3] fix/added mutex lock to mock UserDelete and mock GetUserIDByName to prevent flaky tests (as suggested in copilot review) Signed-off-by: Rayyan Khan --- cmd/harbor/root/user/delete_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cmd/harbor/root/user/delete_test.go b/cmd/harbor/root/user/delete_test.go index 125e4622a..d78b0c8ae 100644 --- a/cmd/harbor/root/user/delete_test.go +++ b/cmd/harbor/root/user/delete_test.go @@ -16,6 +16,7 @@ package user import ( "bytes" "fmt" + "sync" "testing" log "github.com/sirupsen/logrus" @@ -23,6 +24,7 @@ import ( ) type MockUserDeleter struct { + mu sync.Mutex id map[string]int64 user map[int64]string userCnt int @@ -30,6 +32,8 @@ type MockUserDeleter struct { } func (m *MockUserDeleter) UserDelete(userID int64) error { + m.mu.Lock() + defer m.mu.Unlock() if m.expectAuthError { return fmt.Errorf("403") } @@ -41,6 +45,8 @@ func (m *MockUserDeleter) UserDelete(userID int64) error { return fmt.Errorf("user %d not found", userID) } func (m *MockUserDeleter) GetUserIDByName(username string) (int64, error) { + m.mu.Lock() + defer m.mu.Unlock() if v, ok := m.id[username]; ok { return v, nil } else { From 386bb92c74a1b0d1536074b2779028195e14e2db Mon Sep 17 00:00:00 2001 From: Rayyan Khan Date: Tue, 24 Feb 2026 12:38:22 +0530 Subject: [PATCH 3/3] refactor: changed the test design to use global functional variables instead of interfaces Signed-off-by: Rayyan Khan --- cmd/harbor/root/user/delete.go | 34 ++++++----------- cmd/harbor/root/user/delete_test.go | 57 +++++++++++++++++------------ 2 files changed, 44 insertions(+), 47 deletions(-) diff --git a/cmd/harbor/root/user/delete.go b/cmd/harbor/root/user/delete.go index da6651371..16a5c5da1 100644 --- a/cmd/harbor/root/user/delete.go +++ b/cmd/harbor/root/user/delete.go @@ -22,24 +22,13 @@ import ( "github.com/spf13/cobra" ) -type UserDeleter interface { - UserDelete(userID int64) error - GetUserIDByName(username string) (int64, error) - GetUserIDFromUser() int64 -} -type DefaultUserDeleter struct{} - -func (d *DefaultUserDeleter) UserDelete(userID int64) error { - return api.DeleteUser(userID) -} -func (d *DefaultUserDeleter) GetUserIDByName(username string) (int64, error) { - return api.GetUsersIdByName(username) -} -func (d *DefaultUserDeleter) GetUserIDFromUser() int64 { - return prompt.GetUserIdFromUser() -} +var ( + userDeleteAPI = api.DeleteUser + getUserIDByName = api.GetUsersIdByName + getUserIDFromUser = prompt.GetUserIdFromUser +) -func DeleteUser(args []string, userDeleter UserDeleter) { +func DeleteUser(args []string) { // If there are command line arguments, process them concurrently. if len(args) > 0 { var wg sync.WaitGroup @@ -47,7 +36,7 @@ func DeleteUser(args []string, userDeleter UserDeleter) { for _, arg := range args { // Retrieve user ID by name. - userID, err := userDeleter.GetUserIDByName(arg) + userID, err := getUserIDByName(arg) if err != nil { log.Errorf("failed to get user id for '%s': %v", arg, err) continue @@ -55,7 +44,7 @@ func DeleteUser(args []string, userDeleter UserDeleter) { wg.Add(1) go func(userID int64) { defer wg.Done() - if err := userDeleter.UserDelete(userID); err != nil { + if err := userDeleteAPI(userID); err != nil { errChan <- err } }(userID) @@ -77,8 +66,8 @@ func DeleteUser(args []string, userDeleter UserDeleter) { } } else { // Interactive mode: get the user ID from the prompt. - userID := userDeleter.GetUserIDFromUser() - if err := userDeleter.UserDelete(userID); err != nil { + userID := getUserIDFromUser() + if err := userDeleteAPI(userID); err != nil { if isUnauthorizedError(err) { log.Error("Permission denied: Admin privileges are required to execute this command.") } else { @@ -95,8 +84,7 @@ func UserDeleteCmd() *cobra.Command { Short: "delete user by name or id", // nope it only deletes by name Args: cobra.MinimumNArgs(0), Run: func(cmd *cobra.Command, args []string) { - d := &DefaultUserDeleter{} - DeleteUser(args, d) + DeleteUser(args) }, } diff --git a/cmd/harbor/root/user/delete_test.go b/cmd/harbor/root/user/delete_test.go index d78b0c8ae..c53ea9b29 100644 --- a/cmd/harbor/root/user/delete_test.go +++ b/cmd/harbor/root/user/delete_test.go @@ -23,15 +23,14 @@ import ( "github.com/stretchr/testify/assert" ) -type MockUserDeleter struct { +type mockUserDeleter struct { mu sync.Mutex id map[string]int64 user map[int64]string - userCnt int expectAuthError bool } -func (m *MockUserDeleter) UserDelete(userID int64) error { +func (m *mockUserDeleter) userDelete(userID int64) error { m.mu.Lock() defer m.mu.Unlock() if m.expectAuthError { @@ -44,7 +43,7 @@ func (m *MockUserDeleter) UserDelete(userID int64) error { } return fmt.Errorf("user %d not found", userID) } -func (m *MockUserDeleter) GetUserIDByName(username string) (int64, error) { +func (m *mockUserDeleter) getUserIDByName(username string) (int64, error) { m.mu.Lock() defer m.mu.Unlock() if v, ok := m.id[username]; ok { @@ -53,13 +52,12 @@ func (m *MockUserDeleter) GetUserIDByName(username string) (int64, error) { return 0, fmt.Errorf(`Username %s not found`, username) } } -func (m *MockUserDeleter) GetUserIDFromUser() int64 { +func (m *mockUserDeleter) getUserIDFromUser() int64 { return 999 } -func initMockUserDeleter(userCnt int, expectAuthError bool) *MockUserDeleter { - m := &MockUserDeleter{ - userCnt: userCnt, +func newMockUserDeleter(userCnt int, expectAuthError bool) *mockUserDeleter { + m := &mockUserDeleter{ expectAuthError: expectAuthError, id: make(map[string]int64), user: make(map[int64]string), @@ -68,20 +66,31 @@ func initMockUserDeleter(userCnt int, expectAuthError bool) *MockUserDeleter { m.id[fmt.Sprintf("test%d", i+1)] = int64(i + 1) m.user[int64(i+1)] = fmt.Sprintf("test%d", i+1) } + getUserIDByName = m.getUserIDByName + getUserIDFromUser = m.getUserIDFromUser + userDeleteAPI = m.userDelete return m } func TestDeleteUser(t *testing.T) { + origUserDelete := userDeleteAPI + origGetUserIDByName := getUserIDByName + origGetUserIDFromUser := getUserIDFromUser + defer func() { + userDeleteAPI = origUserDelete + getUserIDByName = origGetUserIDByName + getUserIDFromUser = origGetUserIDFromUser + }() tests := []struct { name string - setup func() *MockUserDeleter + setup func() *mockUserDeleter args []string notExpectedID []int64 expectedErr string }{ { name: "successfully delete single user", - setup: func() *MockUserDeleter { - return initMockUserDeleter(5, false) + setup: func() *mockUserDeleter { + return newMockUserDeleter(5, false) }, args: []string{"test1"}, notExpectedID: []int64{1}, @@ -89,8 +98,8 @@ func TestDeleteUser(t *testing.T) { }, { name: "successfully delete multiple users", - setup: func() *MockUserDeleter { - return initMockUserDeleter(5, false) + setup: func() *mockUserDeleter { + return newMockUserDeleter(5, false) }, args: []string{"test1", "test3", "test5"}, notExpectedID: []int64{1, 3, 5}, @@ -98,8 +107,8 @@ func TestDeleteUser(t *testing.T) { }, { name: "delete non-existent user logs error", - setup: func() *MockUserDeleter { - return initMockUserDeleter(5, false) + setup: func() *mockUserDeleter { + return newMockUserDeleter(5, false) }, args: []string{"nonexistent"}, notExpectedID: []int64{}, @@ -107,8 +116,8 @@ func TestDeleteUser(t *testing.T) { }, { name: "permission denied error", - setup: func() *MockUserDeleter { - return initMockUserDeleter(5, true) + setup: func() *mockUserDeleter { + return newMockUserDeleter(5, true) }, args: []string{"test1"}, notExpectedID: []int64{}, @@ -116,8 +125,8 @@ func TestDeleteUser(t *testing.T) { }, { name: "mixed existing and non-existing users", - setup: func() *MockUserDeleter { - return initMockUserDeleter(5, false) + setup: func() *mockUserDeleter { + return newMockUserDeleter(5, false) }, args: []string{"test1", "nonexistent", "test3"}, notExpectedID: []int64{1, 3}, @@ -125,8 +134,8 @@ func TestDeleteUser(t *testing.T) { }, { name: "delete with empty args does not error", - setup: func() *MockUserDeleter { - m := initMockUserDeleter(5, false) + setup: func() *mockUserDeleter { + m := newMockUserDeleter(5, false) m.user[999] = "promptuser" m.id["promptuser"] = 999 return m @@ -137,8 +146,8 @@ func TestDeleteUser(t *testing.T) { }, { name: "delete all users", - setup: func() *MockUserDeleter { - return initMockUserDeleter(3, false) + setup: func() *mockUserDeleter { + return newMockUserDeleter(3, false) }, args: []string{"test1", "test2", "test3"}, notExpectedID: []int64{1, 2, 3}, @@ -154,7 +163,7 @@ func TestDeleteUser(t *testing.T) { defer log.SetOutput(originalLogOutput) m := tt.setup() - DeleteUser(tt.args, m) + DeleteUser(tt.args) logs := buf.String()