tests: Added unit tests for user list command and user view#685
tests: Added unit tests for user list command and user view#685rkcoder101 wants to merge 1 commit intogoharbor:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves unit test coverage around the harbor user list command flow and the user list view rendering, with small refactors to make the logic more testable.
Changes:
- Added unit tests for
cmd/harbor/root/user/list.go(command flags, pagination behavior, and output modes). - Refactored the user list view to extract row-building into
MakeUserRowsand madeListUsersreturn an error instead of exiting. - Refactored user listing logic into
GetUsers/PrintUsersand introduced aUserListerinterface to allow mocking in tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
pkg/views/user/list/view.go |
Extracts row creation (MakeUserRows) and makes ListUsers return an error. |
pkg/views/user/list/view_test.go |
Adds tests for MakeUserRows. |
cmd/harbor/root/user/list.go |
Refactors command to separate fetching vs printing and adds UserLister for testability. |
cmd/harbor/root/user/list_test.go |
Adds unit tests for printing, pagination behavior, and command flag wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/goharbor/go-client/pkg/sdk/v2.0/client/user" | ||
| "github.com/goharbor/go-client/pkg/sdk/v2.0/models" | ||
| "github.com/goharbor/harbor-cli/pkg/api" |
There was a problem hiding this comment.
In this file’s package user, importing github.com/goharbor/go-client/pkg/sdk/v2.0/client/user as user is very easy to confuse with the current package name and makes types like *user.ListUsersOK hard to read. Consider aliasing the import (e.g., clientuser) and updating the interface return type accordingly.
| if formatFlag != "" { | ||
| err := utils.PrintFormat(allUsers, formatFlag) | ||
| if err != nil { | ||
| log.Error(err) |
There was a problem hiding this comment.
PrintUsers returns an error, but in the formatted-output branch a utils.PrintFormat failure is only logged and the function still returns nil. This makes the command exit successfully even though output failed; return the formatting error instead of swallowing it.
| log.Error(err) | |
| log.Error(err) | |
| return err |
| if isUnauthorizedError(err) { | ||
| return nil, fmt.Errorf("Permission denied: Admin privileges are required to execute this command.") | ||
| } | ||
| } else { | ||
| response, err := api.ListUsers(opts) | ||
| if err != nil { | ||
| if isUnauthorizedError(err) { | ||
| return fmt.Errorf("Permission denied: Admin privileges are required to execute this command.") | ||
| } | ||
| return fmt.Errorf("failed to list users: %v", err) | ||
| } | ||
| allUsers = response.Payload | ||
| return nil, fmt.Errorf("failed to list users: %v", err) | ||
| } |
There was a problem hiding this comment.
This error wrapping uses %v, which prevents callers from unwrapping/inspecting the underlying error. Prefer %w when re-wrapping (e.g., fmt.Errorf("failed to list users: %w", err)).
| if isUnauthorizedError(err) { | ||
| return nil, fmt.Errorf("Permission denied: Admin privileges are required to execute this command.") | ||
| } | ||
| formatFlag := viper.GetString("output-format") | ||
| if formatFlag != "" { | ||
| err := utils.PrintFormat(allUsers, formatFlag) | ||
| if err != nil { | ||
| log.Error(err) | ||
| } | ||
| } else { | ||
| list.ListUsers(allUsers) | ||
| return nil, fmt.Errorf("failed to list users: %v", err) | ||
| } |
There was a problem hiding this comment.
Same as above: use %w when re-wrapping err so it can be unwrapped by callers.
pkg/views/user/list/view_test.go
Outdated
| expectedTimeStr, err := utils.FormatCreatedTime(dateStr) | ||
| if err != nil { | ||
| t.Fatalf("failed to format created time %q: %v", dateStr, err) | ||
| } |
There was a problem hiding this comment.
This test asserts an exact string produced by FormatCreatedTime (which is based on time.Since). Because that value changes over time (and can flip at a day boundary), the assertion can be flaky. Prefer asserting the time column is non-empty or matches a suffix/pattern (e.g., contains "day ago").
cmd/harbor/root/user/list_test.go
Outdated
| origStdout := os.Stdout | ||
| defer func() { os.Stdout = origStdout }() | ||
|
|
||
| r, w, _ := os.Pipe() |
There was a problem hiding this comment.
os.Pipe()’s error return is ignored here. Even in tests, handle the error (and fail the test) so the helper doesn’t panic or behave unpredictably if pipe creation fails.
| r, w, _ := os.Pipe() | |
| r, w, err := os.Pipe() | |
| if err != nil { | |
| return "", err | |
| } |
cmd/harbor/root/user/list_test.go
Outdated
| r, w, _ := os.Pipe() | ||
| os.Stdout = w | ||
|
|
||
| if err := f(); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| w.Close() |
There was a problem hiding this comment.
If f() returns an error, this helper returns early without closing the pipe writer/reader, which can leak file descriptors across test cases. Use defer to close w/r (and restore stdout) so cleanup happens on all paths.
| r, w, _ := os.Pipe() | |
| os.Stdout = w | |
| if err := f(); err != nil { | |
| return "", err | |
| } | |
| w.Close() | |
| r, w, err := os.Pipe() | |
| if err != nil { | |
| return "", err | |
| } | |
| defer func() { | |
| _ = w.Close() | |
| _ = r.Close() | |
| }() | |
| os.Stdout = w | |
| if err := f(); err != nil { | |
| return "", err | |
| } | |
| if err := w.Close(); err != nil { | |
| return "", err | |
| } |
cmd/harbor/root/user/list_test.go
Outdated
| return buf.String(), nil | ||
| } | ||
| func TestPrintUsers(t *testing.T) { | ||
| testDate, _ := strfmt.ParseDateTime("2023-01-01T12:00:00Z") |
There was a problem hiding this comment.
The strfmt.ParseDateTime error is ignored here. Check err and t.Fatalf on failure to avoid masking unexpected parsing issues.
| testDate, _ := strfmt.ParseDateTime("2023-01-01T12:00:00Z") | |
| testDate, err := strfmt.ParseDateTime("2023-01-01T12:00:00Z") | |
| if err != nil { | |
| t.Fatalf("failed to parse test date: %v", err) | |
| } |
|
@bupd please review this whenever u get the time. Thanks! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #685 +/- ##
=========================================
- Coverage 10.99% 7.72% -3.27%
=========================================
Files 173 260 +87
Lines 8671 12925 +4254
=========================================
+ Hits 953 998 +45
- Misses 7612 11816 +4204
- Partials 106 111 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…g previous commits into one to resolve merge conflicts) Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
rkcoder101
force-pushed
the
test/user-list
branch
from
af74354 to
709ad8f
Compare
Description
This PR introduces unit tests for the user list command (
cmd/harbor/root/user/list.go) and user view (pkg/views/user)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 :)