tests: Added unit tests for user elevate command#686
tests: Added unit tests for user elevate command#686rkcoder101 wants to merge 2 commits intogoharbor:mainfrom
Conversation
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
=========================================
- Coverage 10.99% 7.44% -3.55%
=========================================
Files 173 260 +87
Lines 8671 12859 +4188
=========================================
+ Hits 953 957 +4
- Misses 7612 11793 +4181
- Partials 106 109 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds unit tests around the user elevate CLI command and refactors the command implementation to be more testable by introducing an injectable dependency interface.
Changes:
- Introduces a
UserElevatorinterface +DefaultUserElevatorimplementation and extracts command logic intoElevateUser(...). - Adds table-driven unit tests covering success and several failure/confirmation flows for
ElevateUser, plus a basicElevateUserCmd()shape test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cmd/harbor/root/user/elevate.go |
Refactors elevate command logic into an injectable function to support unit testing. |
cmd/harbor/root/user/elevate_test.go |
Adds unit tests for elevate command logic and command definition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func ElevateUserCmd() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "elevate", | ||
| Short: "elevate user", | ||
| Long: "elevate user to admin role", | ||
| Args: cobra.MaximumNArgs(1), | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| var err error | ||
| var userId int64 | ||
| if len(args) > 0 { | ||
| userId, err = api.GetUsersIdByName(args[0]) | ||
| if err != nil { | ||
| log.Errorf("failed to get user id for '%s': %v", args[0], err) | ||
| return | ||
| } | ||
| if userId == 0 { | ||
| log.Errorf("User with name '%s' not found", args[0]) | ||
| return | ||
| } | ||
| } else { | ||
| userId = prompt.GetUserIdFromUser() | ||
| } | ||
| confirm, err := views.ConfirmElevation() | ||
| if err != nil { | ||
| log.Errorf("failed to confirm elevation: %v", err) | ||
| return | ||
| } | ||
| if !confirm { | ||
| log.Error("User did not confirm elevation. Aborting command.") | ||
| return | ||
| } | ||
|
|
||
| err = api.ElevateUser(userId) | ||
| if err != nil { | ||
| if isUnauthorizedError(err) { | ||
| log.Error("Permission denied: Admin privileges are required to execute this command.") | ||
| } else { | ||
| log.Errorf("failed to elevate user: %v", err) | ||
| } | ||
| } | ||
| d := &DefaultUserElevator{} | ||
| ElevateUser(args, d) | ||
| }, |
There was a problem hiding this comment.
The PR description says it adds unit tests for the "user list" command, but the actual changes are for the "user elevate" command (elevate.go / elevate_test.go). Please update the PR description/title to match the scope to avoid confusion when tracking coverage work.
| setup: func() *MockUserElevator { | ||
| m := initMockUserElevator(5, false, true, nil) | ||
| m.id["promptuser"] = 999 | ||
| return m |
There was a problem hiding this comment.
The "elevate user via interactive prompt" test setup writes to m.id["promptuser"], but MockUserElevator.GetUserIDFromUser() always returns 999 and never consults m.id. This line is currently redundant/misleading; either remove it or have the mock's GetUserIDFromUser return a configurable value so the test reflects the intended behavior.
| { | ||
| name: "user not found logs error", | ||
| setup: func() *MockUserElevator { | ||
| return initMockUserElevator(5, false, true, nil) | ||
| }, | ||
| args: []string{"nonexistent"}, | ||
| expectedAdminID: []int64{}, | ||
| expectedErr: "failed to get user id", | ||
| }, | ||
| { | ||
| name: "permission denied error", |
There was a problem hiding this comment.
The tests cover the GetUserIDByName error path, but not the (userID==0, err==nil) "not found" branch that ElevateUser explicitly handles. Since pkg/api.GetUsersIdByName returns (0, nil) when a user is not found, consider adding a test case where GetUserIDByName returns 0 with no error and assert the "User with name ... not found" log message.
| type UserElevator interface { | ||
| GetUserIDByName(username string) (int64, error) | ||
| GetUserIDFromUser() int64 | ||
| ConfirmElevation() (bool, error) | ||
| ElevateUser(userID int64) error | ||
| } | ||
|
|
||
| type DefaultUserElevator struct{} | ||
|
|
||
| func (d *DefaultUserElevator) GetUserIDByName(username string) (int64, error) { | ||
| return api.GetUsersIdByName(username) | ||
| } | ||
|
|
||
| func (d *DefaultUserElevator) GetUserIDFromUser() int64 { | ||
| return prompt.GetUserIdFromUser() | ||
| } | ||
|
|
||
| func (d *DefaultUserElevator) ConfirmElevation() (bool, error) { | ||
| return views.ConfirmElevation() | ||
| } | ||
|
|
||
| func (d *DefaultUserElevator) ElevateUser(userID int64) error { | ||
| return api.ElevateUser(userID) | ||
| } | ||
|
|
||
| func ElevateUser(args []string, userElevator UserElevator) { | ||
| var err error | ||
| var userID int64 | ||
|
|
There was a problem hiding this comment.
UserElevator, DefaultUserElevator, and ElevateUser are exported but appear to be used only within this package (and tests are in the same package). Consider making these identifiers unexported (e.g., userElevator/defaultUserElevator/elevateUser) to avoid expanding the public surface area of the cmd package unnecessarily.
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
1 participant
Description
This PR introduces unit tests for the user elevate command (
cmd/harbor/root/user/elevate.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 :)