tests: Added unit tests for user password command#687
tests: Added unit tests for user password command#687rkcoder101 wants to merge 2 commits intogoharbor:mainfrom
Conversation
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds unit tests around the user password command flow and refactors the command implementation to make the logic injectable/testable without changing the underlying API behavior.
Changes:
- Refactored
UserPasswordChangeCmdinternals into aChangePasswordfunction driven by aUserPasswordChangerinterface. - Added unit tests for password change scenarios using a mock
UserPasswordChanger.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cmd/harbor/root/user/password.go | Refactors password reset command to use an injectable interface and extracted ChangePassword for testability. |
| cmd/harbor/root/user/password_test.go | Adds unit tests for password change flow using a mock implementation and captured log output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type MockUserPasswordChanger struct { | ||
| id map[string]int64 | ||
| passwords map[int64]string | ||
| userCnt int | ||
| expectAuthError bool | ||
| } |
There was a problem hiding this comment.
MockUserPasswordChanger.userCnt is never read in the test helper; keeping unused fields makes the mock harder to understand/maintain. Consider removing it, or using it (e.g., to drive GetUserIDFromUser behavior) so it provides value.
There was a problem hiding this comment.
it is used when populating the users in the init function
| setup: func() *MockUserPasswordChanger { | ||
| m := initMockUserPasswordChanger(5, false) | ||
| m.id["promptuser"] = 999 | ||
| m.passwords[999] = "InitialPass123" | ||
| return m |
There was a problem hiding this comment.
In the interactive prompt test setup, m.id["promptuser"] = 999 is unused because GetUserIDFromUser() always returns 999 and never consults m.id. Consider either removing these unused setup lines or updating GetUserIDFromUser() to derive the returned ID from the mock state to better reflect the intended scenario.
cmd/harbor/root/user/password.go
Outdated
| Use: "password", | ||
| Short: "Reset user password by name or id", | ||
| Long: "Allows admin to reset the password for a specified user or select interactively if no username is provided.", | ||
| Args: cobra.MinimumNArgs(0), |
There was a problem hiding this comment.
Args: cobra.MinimumNArgs(0) allows multiple positional args, but ChangePassword only uses args[0], silently ignoring the rest. Consider using cobra.MaximumNArgs(1) (or ExactArgs(1) if username is required) to prevent confusing/ignored input.
| Args: cobra.MinimumNArgs(0), | |
| Args: cobra.MaximumNArgs(1), |
| func UserPasswordChangeCmd() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "password", | ||
| Short: "Reset user password by name or id", | ||
| Long: "Allows admin to reset the password for a specified user or select interactively if no username is provided.", | ||
| Args: cobra.MinimumNArgs(0), | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| var userId int64 | ||
| var err error | ||
| resetView := &reset.PasswordChangeView{ | ||
| NewPassword: opts.NewPassword, | ||
| ConfirmPassword: opts.ConfirmPassword, | ||
| } | ||
|
|
||
| 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() | ||
| } | ||
|
|
||
| reset.ChangePasswordView(resetView) | ||
|
|
||
| err = api.ResetPassword(userId, opts) | ||
| if err != nil { | ||
| if isUnauthorizedError(err) { | ||
| log.Error("Permission denied: Admin privileges are required to execute this command.") | ||
| } else { | ||
| log.Errorf("failed to reset user password: %v", err) | ||
| } | ||
| } | ||
| d := &DefaultUserPasswordChanger{} | ||
| ChangePassword(args, d) | ||
| }, |
There was a problem hiding this comment.
PR description says the tests are for the "user list" command, but the changed files and new tests are for the user password command (cmd/harbor/root/user/password.go). Please update the PR description to match the actual scope to avoid confusion during review/release notes.
| if len(args) > 0 { | ||
| userID, err = passwordChanger.GetUserIDByName(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 | ||
| } |
There was a problem hiding this comment.
Test coverage gap: the production implementation api.GetUsersIdByName returns (0, nil) when the user is not found, and ChangePassword has a dedicated userID == 0 branch. The tests only cover the "not found" case via an error return, so the userID == 0 branch is currently untested.
| err = passwordChanger.ResetPassword(userID, *resetView) | ||
| if err != nil { | ||
| if isUnauthorizedError(err) { | ||
| log.Error("Permission denied: Admin privileges are required to execute this command.") | ||
| } else { | ||
| log.Errorf("failed to reset user password: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test coverage gap: there is no test asserting the non-403 error path for ResetPassword (the failed to reset user password log branch). Adding a test case where ResetPassword returns a non-"403" error would ensure this branch remains correct.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #687 +/- ##
=========================================
- Coverage 10.99% 7.31% -3.68%
=========================================
Files 173 260 +87
Lines 8671 12856 +4185
=========================================
- Hits 953 940 -13
- Misses 7612 11806 +4194
- Partials 106 110 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
1 participant
Description
This PR introduces unit tests for the user password command (
cmd/harbor/root/user/password.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 :)