tests: Add unit tests for project list command#692
tests: Add unit tests for project list command#692rkcoder101 wants to merge 2 commits intogoharbor:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #692 +/- ##
=========================================
- Coverage 10.99% 7.95% -3.04%
=========================================
Files 173 260 +87
Lines 8671 12920 +4249
=========================================
+ Hits 953 1028 +75
- Misses 7612 11781 +4169
- Partials 106 111 +5 ☔ 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 improves unit test coverage for the harbor project list command by extracting testable helper functions from ListProjectCommand() and adding a dedicated test suite for listing, pagination, query building, output formatting, and command flag wiring.
Changes:
- Added
BuildListOptions()andPrintProjects()helpers to make list logic testable outside of Cobra’sRunE. - Refactored
ListProjectCommand()to use the new helpers. - Added
list_test.gocoveringfetchProjects(),BuildListOptions(),PrintProjects(), andListProjectCommand().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cmd/harbor/root/project/list.go |
Refactors list command to delegate option building and printing to helpers (enables unit testing). |
cmd/harbor/root/project/list_test.go |
Adds unit tests for pagination, query construction, output formatting paths, and Cobra flag defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @qcserestipy @bupd @Vad1mo @NucleoFusion kindly give these tests a review. Thanks! |
bupd
left a comment
There was a problem hiding this comment.
@rkcoder101 please resolve conflicts
and fix the suggestion.
| func PrintProjects(allProjects []*models.Project) error { | ||
| log.WithField("count", len(allProjects)).Debug("Number of projects fetched") | ||
| if len(allProjects) == 0 { | ||
| log.Info("No projects found") | ||
| return nil | ||
| } | ||
| formatFlag := viper.GetString("output-format") | ||
| if formatFlag != "" { | ||
| log.WithField("output_format", formatFlag).Debug("Output format selected") | ||
| err := utils.PrintFormat(allProjects, formatFlag) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| log.Debug("Listing projects using default view") | ||
| if err := list.ListProjects(allProjects); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I dont think we need this one. Isnt there a global util function used for printing tables and outputs. ??
There was a problem hiding this comment.
Actually this is not something which I have written; this piece of code was already written inside the RunE of ListProjectCommand. I just separated out the printing logic into a function for creating a testable unit. The util function which you are referring to (utils.PrintFormat()) is actually used in the above code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/harbor/root/project/list_test.go
Outdated
| return *res, fmt.Errorf("mock list error") | ||
| } | ||
| if len(opts) == 0 { | ||
| return *res, fmt.Errorf("No options passed") |
There was a problem hiding this comment.
The error message "No options passed" should be "no options passed" to follow Go's error message conventions (lowercase unless starting with proper nouns or exported identifiers).
| return *res, fmt.Errorf("No options passed") | |
| return *res, fmt.Errorf("no options passed") |
cmd/harbor/root/project/list_test.go
Outdated
| log "github.com/sirupsen/logrus" | ||
| "github.com/spf13/viper" | ||
| "github.com/stretchr/testify/assert" | ||
| "go.yaml.in/yaml/v4" |
There was a problem hiding this comment.
The codebase consistently uses gopkg.in/yaml.v2 for YAML parsing across all production code (e.g., cmd/harbor/root/configurations/apply.go:30, pkg/config/replication/policies.go:24, pkg/config/robot/robot.go:41). While go.yaml.in/yaml/v4 is available in go.mod, introducing it in tests creates an inconsistency and adds an additional dependency. Consider using gopkg.in/yaml.v2 to align with the existing codebase convention. If v4 is needed for specific features, this decision should be documented.
| "go.yaml.in/yaml/v4" | |
| "gopkg.in/yaml.v2" |
pkg/views/project/list/view.go
Outdated
| fmt.Println("Error running program:", err) | ||
| os.Exit(1) | ||
| if _, err := tea.NewProgram(m, tea.WithOutput(os.Stdout), tea.WithInput(nil)).Run(); err != nil { | ||
| return fmt.Errorf("Error running program:%w", err) |
There was a problem hiding this comment.
The error message format is missing a space before the colon. It should be "Error running program: %w" instead of "Error running program:%w" for consistent error formatting.
| return fmt.Errorf("Error running program:%w", err) | |
| return fmt.Errorf("Error running program: %w", err) |
cmd/harbor/root/project/list.go
Outdated
| if len(allProjects) == 0 { | ||
| log.Info("No projects found") | ||
| return nil | ||
| return fmt.Errorf("%s", utils.ParseHarborErrorMsg(err)) |
There was a problem hiding this comment.
Using fmt.Errorf("%s", ...) is an anti-pattern in Go. The format verb "%s" doesn't add any value here. Based on the codebase convention, this should be fmt.Errorf("failed to get projects list: %v", utils.ParseHarborErrorMsg(err)) to provide clearer context about what operation failed, similar to how it's done in other commands throughout the codebase.
| return fmt.Errorf("%s", utils.ParseHarborErrorMsg(err)) | |
| return fmt.Errorf("failed to get projects list: %v", utils.ParseHarborErrorMsg(err)) |
| @@ -0,0 +1,608 @@ | |||
| // Copyright Project Harbor Authors | |||
There was a problem hiding this comment.
The PR description states "This PR introduces unit tests for the user list command" but should say "project list command" based on the PR title and the actual code being tested. The description should be corrected for accuracy.
cmd/harbor/root/project/list.go
Outdated
| } | ||
|
|
||
| if private && public { | ||
| return nil, fmt.Errorf("Cannot specify both --private and --public flags") |
There was a problem hiding this comment.
The codebase predominantly uses lowercase error messages (e.g., "failed to..." instead of "Failed to..."). The error message "Cannot specify both --private and --public flags" should be "cannot specify both --private and --public flags" to follow the established convention seen throughout the codebase.
| return nil, fmt.Errorf("Cannot specify both --private and --public flags") | |
| return nil, fmt.Errorf("cannot specify both --private and --public flags") |
…commits squashed to resolve conflicts) Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
35a876d to
2efeb88
Compare
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Description
This PR introduces unit tests for the user list command (
cmd/harbor/root/project/list.go)Solves a part of #591
Functions covered: