test/Unit tests for user command #653
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #653 +/- ##
=========================================
- Coverage 10.99% 8.37% -2.62%
=========================================
Files 173 260 +87
Lines 8671 12902 +4231
=========================================
+ Hits 953 1080 +127
- Misses 7612 11704 +4092
- Partials 106 118 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rkcoder101
changed the title
Refactored user list command (UserListCmd) to separate out PrintUsers and GetUsers logic, and wrote unit tests for PrintUsers. Refactored user list view (ListUsers) to separate out MakeUserRows logic and wrote unit tests for it. Made some minor refactory changes in pkg/utils/helper.go and pkg/utils/utils.go to enable writing clean test code Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
…st commit) in cmd/harbor/root/user/list.go, setup interface for mocking the API Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
…y been tested Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
…unit tests for all the functions related to user create command. Refactored only where needed Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
…ests for DeleteUser() Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
…ts for it Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
rkcoder101
changed the title
…nit tests Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds unit tests around the harbor user CLI commands and refactors a few command/view/util functions to make them testable via dependency injection and io.Writer output control.
Changes:
- Added unit tests for
usersubcommands (list/create/delete/elevate/password) and the user list view row builder. - Refactored user commands to extract core logic into functions with injectable interfaces (e.g., lister/creator/deleter/elevator/password changer).
- Introduced writer-based formatting/output helpers to enable deterministic output assertions in tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/views/user/list/view_test.go | Adds unit tests for MakeUserRows. |
| pkg/views/user/list/view.go | Splits row-building from rendering; makes list rendering writer-based and returns errors. |
| pkg/utils/utils.go | Adds FPrintPayloadInJSONFormat/YAMLFormat for writer injection; wraps old functions. |
| pkg/utils/helper.go | Adds FPrintFormat for writer injection; wraps old PrintFormat. |
| cmd/harbor/root/user/password_test.go | Adds unit tests for password reset behavior via injected dependency. |
| cmd/harbor/root/user/password.go | Refactors password reset flow behind a UserPasswordChanger interface. |
| cmd/harbor/root/user/list_test.go | Adds unit tests for user listing: output formats + paging/validation logic. |
| cmd/harbor/root/user/list.go | Refactors listing into GetUsers/PrintUsers and injects a UserLister. |
| cmd/harbor/root/user/elevate_test.go | Adds unit tests for elevation flow including confirm/unauthorized/error cases. |
| cmd/harbor/root/user/elevate.go | Refactors elevation flow behind a UserElevator interface. |
| cmd/harbor/root/user/delete_test.go | Adds unit tests for deletion behavior including concurrent deletes and error paths. |
| cmd/harbor/root/user/delete.go | Refactors deletion flow behind a UserDeleter interface. |
| cmd/harbor/root/user/create_test.go | Adds unit tests for creation flow including FillUser path and unauthorized handling. |
| cmd/harbor/root/user/create.go | Refactors user creation flow behind a UserCreator interface; hardens isUnauthorizedError(nil). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/harbor/root/user/list_test.go
Outdated
| /* | ||
| [X] make a test opts, only Page and PageSize fields needs to be there | ||
| [X] make a helper to populate the users | ||
| [X] mock api.List | ||
| [X] mock authentication (no need, just make the api mock to pass an error with 403 in case you need to check for unauthenticated user) | ||
| [X] test error logs | ||
| [X] check if the users received are correct | ||
|
|
||
| */ | ||
| /* | ||
| Logic for mocking api.ListUsers | ||
| -> should return (*user.ListUsersOK, error) ; Payload should be populated with the users | ||
| -> shoud return the PageSize*(Page-1) th user till PageSize*(Page) -1 th user (0 based indexing) | ||
| Helper for populating users: | ||
| -> models.UserResp ko append krna hoga api.ListFlags type ke payload me | ||
| */ |
There was a problem hiding this comment.
This large commented-out block (including non-English notes) looks like scratchpad/TODO content rather than a maintained test explanation. Please remove it or replace it with a brief, clear English comment describing the mock paging behavior the test relies on.
| /* | |
| [X] make a test opts, only Page and PageSize fields needs to be there | |
| [X] make a helper to populate the users | |
| [X] mock api.List | |
| [X] mock authentication (no need, just make the api mock to pass an error with 403 in case you need to check for unauthenticated user) | |
| [X] test error logs | |
| [X] check if the users received are correct | |
| */ | |
| /* | |
| Logic for mocking api.ListUsers | |
| -> should return (*user.ListUsersOK, error) ; Payload should be populated with the users | |
| -> shoud return the PageSize*(Page-1) th user till PageSize*(Page) -1 th user (0 based indexing) | |
| Helper for populating users: | |
| -> models.UserResp ko append krna hoga api.ListFlags type ke payload me | |
| */ | |
| // MockUserLister simulates the ListUsers API with simple paging behavior. | |
| // It generates a deterministic list of users with IDs from 1 to numberOfUsersforTesting | |
| // and, in UserList, returns only the slice for the requested Page and PageSize. | |
| // When expectAuthError is true, UserList returns a 403-like error instead of data. |
cmd/harbor/root/user/list_test.go
Outdated
| switch { | ||
| case len(allUsers) == 0: | ||
| if !strings.Contains(logs, "No users found") { | ||
| t.Errorf(`Expected logs to contain "No user found" but got: %s`, logs) |
There was a problem hiding this comment.
The failure message says "No user found" but the code is checking for "No users found". Please align the message with the actual expected log text to avoid confusion when the assertion fails.
| t.Errorf(`Expected logs to contain "No user found" but got: %s`, logs) | |
| t.Errorf(`Expected logs to contain "No users found" but got: %s`, logs) |
pkg/views/user/list/view_test.go
Outdated
| testDate, _ := strfmt.ParseDateTime(dateStr) | ||
| expectedTimeStr, _ := utils.FormatCreatedTime(dateStr) |
There was a problem hiding this comment.
The errors from strfmt.ParseDateTime and utils.FormatCreatedTime are ignored in the test. If these ever fail, the test will silently use zero values and can produce misleading failures. Please assert/require that these calls return no error.
| testDate, _ := strfmt.ParseDateTime(dateStr) | |
| expectedTimeStr, _ := utils.FormatCreatedTime(dateStr) | |
| testDate, err := strfmt.ParseDateTime(dateStr) | |
| if err != nil { | |
| t.Fatalf("failed to parse date %q: %v", dateStr, err) | |
| } | |
| expectedTimeStr, err := utils.FormatCreatedTime(dateStr) | |
| if err != nil { | |
| t.Fatalf("failed to format created time %q: %v", dateStr, err) | |
| } |
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
|
Hi @bupd @Vad1mo @qcserestipy kindly review this work whenever you guys get the time. Thanks! |
qcserestipy
left a comment
qcserestipy
left a comment
There was a problem hiding this comment.
Thanks so much for putting this work together! The test coverage you've added is solid and the implementation looks good. This is already ready to merge.
That said, I wanted to flag that there's still some untested functionality in the user command package that we should track:
Untested functions across the module:
github.com/goharbor/harbor-cli/cmd/harbor/root/user/cmd.go:20: User 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/create.go:32: FillUser 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/create.go:36: UserCreate 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/create.go:39: CreateUser 100.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/create.go:56: UserCreateCmd 81.8%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/create.go:78: isUnauthorizedError 100.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/delete.go:32: UserDelete 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/delete.go:35: GetUserIDByName 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/delete.go:38: GetUserIDFromUser 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/delete.go:42: DeleteUser 84.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/delete.go:92: UserDeleteCmd 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/elevate.go:33: GetUserIDByName 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/elevate.go:37: GetUserIDFromUser 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/elevate.go:41: ConfirmElevation 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/elevate.go:45: ElevateUser 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/elevate.go:49: ElevateUser 91.3%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/elevate.go:87: ElevateUserCmd 50.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/list.go:36: UserList 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/list.go:40: GetUsers 91.3%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/list.go:79: PrintUsers 81.8%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/list.go:97: UserListCmd 61.5%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/password.go:34: GetUserIDByName 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/password.go:38: GetUserIDFromUser 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/password.go:42: FillPasswordView 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/password.go:46: ResetPassword 0.0%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/password.go:50: ChangePassword 83.3%
github.com/goharbor/harbor-cli/cmd/harbor/root/user/password.go:81: UserPasswordChangeCmd 0.0%Would you be open to tackling these in a follow-up PR? You could also open a tracking issue if that helps keep this on the radar.
Thank you again for your contribution!
|
Thanks for the review! It means a lot to me |
|
Also while writing the unit tests I found some bugs and other things which can be improved on in the user command, I will open an issue regarding that too. It would be more efficient to implement those fixes as follow-up PRs once this foundational refactoring is merged. |
bupd
left a comment
bupd
left a comment
There was a problem hiding this comment.
I would suggest breaking this PR and basically focus on maybe like create / delete - like that.
added suggestions below for improvements please fix it. avoid using Fprint.
| } | ||
|
|
||
| fmt.Println(string(jsonStr)) | ||
| fmt.Fprint(w, string(jsonStr)) |
There was a problem hiding this comment.
| fmt.Fprint(w, string(jsonStr)) | |
| fmt.Println(string(jsonStr)) |
revert this back - our usecase doesn't fit for Fprint
There was a problem hiding this comment.
Oops I realised Fprint does not give a new line character at the end (like Println). Can I use Fprintln which enables both writer injection (and helps in testing) as well as gives \n at the end?
Actually I saw that PrintFormat() function is being used in many commands (example), and it would enable easy testing if we had a 'F' version of the function, through which we can inject a writer/buffer to capture and test the output.
|
|
||
| if err != nil { | ||
| if isUnauthorizedError(err) { | ||
| log.Error("Permission denied: Admin privileges are required to execute this command.") |
There was a problem hiding this comment.
robot accounts can also create users - this is not limited to admins.
There was a problem hiding this comment.
Ok I will correct this 👍
Sure I'll split this into separate PRs (create, delete, list, etc.). Just let me know if I can use FPrintln instead of FPrint. Thanks! |
3 participants
Description
This PR introduces unit tests for the user command (
cmd/harbor/root/user) and user view (pkg/views/user)Relates to #591
Test Coverage Checklist
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 :)
List of refactoring changes done till now (for writing unit tests)
cmd/harbor/root/user/list.go: SeparatedPrintUser()andGetUsers()fromUserListCmd()pkg/utils/helper.go: AddedFPrintFormat()to supportio.Writerinjection. The originalPrintFormat()remains unchanged, ensuring full backward compatibility with existing code.pkg/utils/utils.go: AddedFPrintPayloadInJSONFormat()andFPrintPayloadInYAMLFormat()to supportio.Writer. The originalPrintPayloadInJSONFormat()andPrintPayloadInYAMLFormat()remain unchanged.pkg/views/user/list/view.go: SeparatedMakeUserRows()fromListUsers()cmd/harbor/root/user/create.go: SeparatedCreateUser()fromUserCreateCmd()cmd/harbor/root/user/create.go: Removed the redundantcreateUserView()as it was just a wrapper which called two functionscmd/harbor/root/user/delete.go: SeparatedDeleteUser()fromUserDeleteCmd()cmd/harbor/root/user/elevate.go: SeparatedElevateUser()fromElevateUserCmd()cmd/harbor/root/user/elevate.go: SeparatedChangePassword()fromUserPasswordChangeCmd()