tests: Added unit tests for user create command#688
tests: Added unit tests for user create command#688rkcoder101 wants to merge 3 commits intogoharbor:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds unit tests around the harbor user create command and introduces a small refactor in cmd/harbor/root/user/create.go to enable dependency injection for interactive prompting and API calls.
Changes:
- Added a new
create_test.gocovering user creation flows (success, duplicates, unauthorized, fill-missing-fields, and basic command/flag wiring). - Refactored
UserCreateCmdby extracting aCreateUserhelper and introducing aUserCreatorinterface with a default implementation. - Hardened
isUnauthorizedErrorto handlenilerrors safely.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/harbor/root/user/create_test.go | New unit tests for create-user behavior and command configuration. |
| cmd/harbor/root/user/create.go | Refactor to injectable user-creation logic + small nil guard in unauthorized detection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package user | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "cmp" | ||
| "fmt" |
There was a problem hiding this comment.
PR metadata/title says this adds unit tests for the user password command, but the changes in this PR add tests/refactoring for the user create command. Please update the PR title (or description) so it accurately reflects what’s being changed/reviewed.
cmd/harbor/root/user/create.go
Outdated
| type UserCreator interface { | ||
| FillUser(opts *create.CreateView) | ||
| UserCreate(opts create.CreateView) error | ||
| } | ||
| type DefaultUserCreator struct{} | ||
|
|
||
| func (d *DefaultUserCreator) FillUser(opts *create.CreateView) { | ||
| create.CreateUserView(opts) | ||
| } | ||
|
|
||
| func (d *DefaultUserCreator) UserCreate(opts create.CreateView) error { | ||
| return api.CreateUser(opts) | ||
| } | ||
| func CreateUser(opts *create.CreateView, userCreator UserCreator) { | ||
| var err error | ||
|
|
There was a problem hiding this comment.
UserCreator, DefaultUserCreator, and CreateUser are newly exported but appear to be internal test seams for UserCreateCmd. Exporting them expands the package API surface unnecessarily (and generally implies a stability/compatibility promise). Consider making these unexported (e.g., userCreator, defaultUserCreator, createUser) and keep UserCreateCmd as the exported entry point.
| if isUnauthorizedError(err) { | ||
| log.WithFields(log.Fields{ | ||
| "action": "user create", | ||
| }).Error("Permission denied: The current account does not have the required permissions to create users.") |
There was a problem hiding this comment.
The unauthorized/permission-denied message here differs from other user commands (most log/return "Permission denied: Admin privileges are required to execute this command."). This inconsistency can be confusing for users and makes it harder to match errors across commands/tests. Consider standardizing the message (and logging style) across the user subcommands.
| }).Error("Permission denied: The current account does not have the required permissions to create users.") | |
| }).Error("Permission denied: Admin privileges are required to execute this command.") |
| usersAreEqual := func(u1, u2 []*create.CreateView) bool { | ||
| if len(u1) != len(u2) { | ||
| return false | ||
| } | ||
| slices.SortFunc(u1, func(a, b *create.CreateView) int { | ||
| return cmp.Compare(a.Username, b.Username) | ||
| }) | ||
| slices.SortFunc(u2, func(a, b *create.CreateView) int { | ||
| return cmp.Compare(a.Username, b.Username) | ||
| }) | ||
| for i := 0; i < len(u1); i++ { | ||
| if !reflect.DeepEqual(u1[i], u2[i]) { | ||
| return false | ||
| } | ||
| } | ||
| return true |
There was a problem hiding this comment.
usersAreEqual sorts both input slices in-place, which mutates m.users and the table’s expectedUsers slice. This can make failures harder to diagnose and can introduce cross-test coupling if cases are ever re-used/expanded. Prefer comparing without mutation (e.g., copy before sorting, or use a matcher like assert.ElementsMatch).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #688 +/- ##
=========================================
- Coverage 10.99% 7.46% -3.53%
=========================================
Files 173 260 +87
Lines 8671 12910 +4239
=========================================
+ Hits 953 964 +11
- Misses 7612 11837 +4225
- Partials 106 109 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rkcoder101
changed the title
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
…rfaces Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
rkcoder101
force-pushed
the
test/user-create
branch
from
b72da56 to
d895b59
Compare
Description
This PR introduces unit tests for the user create command (
cmd/harbor/root/user/create.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 :)