tests: Added unit tests for user delete command#689
tests: Added unit tests for user delete command#689rkcoder101 wants to merge 1 commit intogoharbor:mainfrom
Conversation
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds unit tests around the user delete CLI command by refactoring the delete implementation to be dependency-injectable, enabling mock-based testing without hitting real API/prompt code.
Changes:
- Added a new unit test suite for delete behavior, including multi-user and error paths.
- Refactored
user deletecommand logic into a helper function and introduced a deleter abstraction for mocking.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/harbor/root/user/delete.go | Refactors delete command to call an injected deleter implementation, enabling unit testing. |
| cmd/harbor/root/user/delete_test.go | Adds table-driven unit tests for delete behavior using a mock deleter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 |
There was a problem hiding this comment.
DeleteUser deletes users concurrently, but MockUserDeleter.UserDelete mutates Go maps (m.id, m.user) without synchronization. When multiple usernames are passed (e.g. the multi-user test cases), this can trigger data races and/or a runtime panic due to concurrent map writes. Make the mock thread-safe (e.g., guard map access with a sync.Mutex or use sync.Map), or change the mock to avoid shared mutable state across goroutines.
| 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. |
There was a problem hiding this comment.
UserDeleter, DefaultUserDeleter, and DeleteUser are exported but appear to be used only within this package (and by tests in the same package). Consider making them unexported (e.g., userDeleter, defaultUserDeleter, deleteUser) to avoid unintentionally expanding the public surface area of cmd/harbor/root/user.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #689 +/- ##
=========================================
- Coverage 10.99% 7.36% -3.63%
=========================================
Files 173 260 +87
Lines 8671 12857 +4186
=========================================
- Hits 953 947 -6
- Misses 7612 11800 +4188
- Partials 106 110 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Description
This PR introduces unit tests for the user delete command (
cmd/harbor/root/user/delete.go)Relates to #591
Is a sub-part of #653
Note
No changes have been done to the core logic of the application, only a little refactoring for the purpose of writing some nice and clean test code :)