tests: Added unit tests for user elevate command#686
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
=========================================
- Coverage 10.99% 7.79% -3.20%
=========================================
Files 173 265 +92
Lines 8671 13175 +4504
=========================================
+ Hits 953 1027 +74
- Misses 7612 12035 +4423
- Partials 106 113 +7 ☔ 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.
|
@bupd please review this whenever u get the time. Thanks! |
qcserestipy
left a comment
There was a problem hiding this comment.
Thank you for this contribution. The tests work. In general I think adding additional interfaces in the production code for testability only is technical overhead. Please restructure the test to work without this interface.
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| type UserElevator interface { |
There was a problem hiding this comment.
Introducing a UserElevator interface in production code solely for test mockability adds unnecessary complexity. There's only one real implementation (DefaultUserElevator), and this pattern isn't used elsewhere in the codebase. Consider restructuring the tests to avoid altering the production code's API surface, or extracting only the testable logic into a separate function.
There was a problem hiding this comment.
Hi @qcserestipy I have tried to adapt a different design of test code. I have tried using global functional variables. Kindly review it, and please let me know if there are any changes required in the approach. Thanks!
qcserestipy
left a comment
There was a problem hiding this comment.
@rkcoder101 Thank you for incorporating the changes! LGTM, but please fix the lint issues.
All the PRs opened lately are facing linting issues. Actually there have been some linting issues on the main recently #711 . Fellow contributors have raised PRs to solve them, I will rebase as soon as they get merged. |
bupd
left a comment
There was a problem hiding this comment.
https://github.com/goharbor/harbor-cli/actions/runs/22251606412/attempts/1#summary-64376129514
@rkcoder101 Please fix the lint issues - I believe we can make gosec nolint rule on all _test.go files
Please feel free to update it.
…commits) Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
526420b to
4eef00d
Compare
Signed-off-by: Nishchay Rajput <nishchayr@iitbhilai.ac.in>
Description
This PR introduces unit tests for the user elevate command (
cmd/harbor/root/user/elevate.go)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 :)