tests: Add unit tests for project create command#694
tests: Add unit tests for project create command#694rkcoder101 wants to merge 6 commits intogoharbor:mainfrom
Conversation
…and fillCreateView(), and wrote unit tests for all Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #694 +/- ##
=========================================
- Coverage 10.99% 7.46% -3.53%
=========================================
Files 173 260 +87
Lines 8671 12872 +4201
=========================================
+ Hits 953 961 +8
- Misses 7612 11802 +4190
- Partials 106 109 +3 ☔ 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 adds unit tests around the harbor project create command flow by refactoring cmd/harbor/root/project/create.go to make project creation and interactive view filling injectable/mocked.
Changes:
- Introduces a
ProjectCreatorinterface +DefaultProjectCreatorimplementation and extracts core logic intoCreateProject(w, projectCreator, opts, args). - Renames the interactive helper to
fillCreateView(...)and routes interactive filling through the injected interface. - Adds
create_test.gowith unit tests forCreateProject(),fillCreateView(), andCreateProjectCommand().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cmd/harbor/root/project/create.go | Refactors command logic for testability (injectable creator + writer), and renames interactive helper. |
| cmd/harbor/root/project/create_test.go | Adds unit tests covering flag vs interactive paths and command/flag wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/harbor/root/project/create.go
Outdated
| @@ -90,10 +108,6 @@ func createProjectView(createView *create.CreateView) error { | |||
| StorageLimit: "-1", | |||
| } | |||
| } | |||
There was a problem hiding this comment.
fillCreateView accepts a nil createView and allocates a new one, but because the pointer is passed by value and the function only returns error, the caller can never observe the newly-created view. Either remove the nil-handling (and require non-nil input) or change the function signature to return the (possibly newly-allocated) *create.CreateView.
There was a problem hiding this comment.
I have removed the nil handling (actually removed the whole function) as it is just a wrapper on the projectCreator.FillProjectView() function
| name: "interactive view error propagates", | ||
| setup: func() ([]input, *mockProjectCreator) { | ||
| return []input{ | ||
| {opts: &create.CreateView{}}, | ||
| }, &mockProjectCreator{ | ||
| projectName: make(map[string]struct{}), | ||
| errInFilling: true, | ||
| } | ||
| }, |
There was a problem hiding this comment.
This test case's setup function is missing a trailing comma after the function literal, which will prevent the test file from compiling. Add the comma after the closing } of the setup function.
There was a problem hiding this comment.
tests compile just fine
| assert.NoError(t, err) | ||
| if tt.input != nil { | ||
| assert.Equal(t, tt.expectName, tt.input.ProjectName) | ||
| assert.Equal(t, tt.expectLimit, tt.input.StorageLimit) | ||
| } | ||
| } |
There was a problem hiding this comment.
In TestFillCreateView, the nil createView gets defaults then filled case doesn't assert anything because assertions are gated on tt.input != nil. Either assert on a returned value (if fillCreateView is changed to return the view) or restructure the test so the nil/default path is verifiable.
There was a problem hiding this comment.
this test and the corresponding function have been removed as mentioned earlier
cmd/harbor/root/project/create.go
Outdated
| // CreateProjectCommand creates a new `harbor create project` command | ||
| func CreateProjectCommand() *cobra.Command { | ||
| var opts create.CreateView | ||
| type ProjectCreator interface { | ||
| CreateProject(opts create.CreateView) error | ||
| FillProjectView(createView *create.CreateView) error | ||
| } | ||
| type DefaultProjectCreator struct{} |
There was a problem hiding this comment.
The doc comment above ProjectCreator still says CreateProjectCommand creates ..., but it now documents the interface rather than the command function. This will mislead readers and may trip golint. Move the comment to CreateProjectCommand and add an appropriate comment for ProjectCreator (and/or DefaultProjectCreator).
| func CreateProject(w io.Writer, projectCreator ProjectCreator, opts *create.CreateView, args []string) error { | ||
| var ProjectName string | ||
| var createView *create.CreateView | ||
| if len(args) > 0 { | ||
| opts.ProjectName = args[0] |
There was a problem hiding this comment.
Local variable ProjectName is capitalized, which in Go signals an exported identifier and is inconsistent with other locals. Rename it to projectName (or inline createView.ProjectName) to follow Go naming conventions and avoid confusion.
cmd/harbor/root/project/create.go
Outdated
| } | ||
| err := fillCreateView(projectCreator, createView) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to get the required params to create project:%w", err) |
There was a problem hiding this comment.
Error string "Failed to get the required params to create project:%w" has inconsistent capitalization and is missing a space before the wrapped error. Consider using a lower-case message consistent with the rest of the CLI and formatting like ... project: %w for readability.
| return fmt.Errorf("Failed to get the required params to create project:%w", err) | |
| return fmt.Errorf("failed to get the required params to create project: %w", err) |
|
Hi @qcserestipy @bupd @Vad1mo @NucleoFusion kindly give these tests a review. Thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/harbor/root/project/create.go
Outdated
| return fmt.Errorf("proxy cache selected but no registry ID provided. Use --registry-id") | ||
| } | ||
| func CreateProject(w io.Writer, projectCreator ProjectCreator, opts *create.CreateView, args []string) error { | ||
| var ProjectName string |
There was a problem hiding this comment.
The variable name "ProjectName" uses PascalCase instead of camelCase. Go convention for local variables is to use camelCase. This should be renamed to "projectName" for consistency with Go naming conventions.
cmd/harbor/root/project/create.go
Outdated
| var ProjectName string | ||
| var createView *create.CreateView | ||
| if len(args) > 0 { | ||
| opts.ProjectName = args[0] | ||
| } | ||
|
|
||
| if !opts.ProxyCache && opts.RegistryID != "" { | ||
| return fmt.Errorf("registry ID should only be provided when proxy-cache is enabled") | ||
| } | ||
| if opts.ProxyCache && opts.RegistryID == "" { | ||
| return fmt.Errorf("proxy cache selected but no registry ID provided. Use --registry-id") | ||
| } | ||
|
|
||
| if opts.ProjectName != "" && opts.StorageLimit != "" { | ||
| log.Debug("Attempting to create project using flags...") | ||
| err = api.CreateProject(opts) | ||
| ProjectName = opts.ProjectName | ||
| } else { | ||
| log.Debug("Switching to interactive view...") | ||
| createView := &create.CreateView{ | ||
| ProjectName: opts.ProjectName, | ||
| Public: opts.Public, | ||
| RegistryID: opts.RegistryID, | ||
| StorageLimit: opts.StorageLimit, | ||
| ProxyCache: opts.ProxyCache, | ||
| } | ||
| if !opts.ProxyCache && opts.RegistryID != "" { | ||
| return fmt.Errorf("registry ID should only be provided when proxy-cache is enabled") | ||
| } | ||
|
|
||
| err = createProjectView(createView) | ||
| ProjectName = createView.ProjectName | ||
| } | ||
| if opts.ProjectName == "" || opts.StorageLimit == "" { | ||
| log.Debug("Switching to interactive view...") | ||
| createView = &create.CreateView{ | ||
| ProjectName: opts.ProjectName, | ||
| Public: opts.Public, | ||
| RegistryID: opts.RegistryID, | ||
| StorageLimit: opts.StorageLimit, | ||
| ProxyCache: opts.ProxyCache, | ||
| } | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to create project: %v", utils.ParseHarborErrorMsg(err)) | ||
| } | ||
| err := fillCreateView(projectCreator, createView) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to get the required params to create project:%w", err) | ||
| } | ||
| } else { | ||
| createView = opts | ||
| } | ||
|
|
||
| fmt.Printf("Project '%s' created successfully\n", ProjectName) | ||
| return nil | ||
| ProjectName = createView.ProjectName | ||
| if err := projectCreator.CreateProject(*createView); err != nil { | ||
| return fmt.Errorf("failed to create project: %v", utils.ParseHarborErrorMsg(err)) | ||
| } | ||
| fmt.Fprintf(w, "Project '%s' created successfully\n", ProjectName) |
There was a problem hiding this comment.
The variable declaration can be simplified. The variable ProjectName is declared at line 43 but only assigned at line 75 and used at line 79. Consider removing the declaration at line 43 and directly using createView.ProjectName in the fmt.Fprintf call at line 79, eliminating the unnecessary intermediate variable.
There was a problem hiding this comment.
dont think this matters much
| { | ||
| name: "nil createView gets defaults then filled", | ||
| input: nil, | ||
| expectName: "testProject999", | ||
| expectLimit: "-1", | ||
| }, | ||
| { | ||
| name: "empty createView gets filled", | ||
| input: &create.CreateView{ | ||
| ProjectName: "", | ||
| StorageLimit: "", | ||
| }, | ||
| expectName: "testProject999", | ||
| expectLimit: "-1", | ||
| }, | ||
| { | ||
| name: "error in FillProjectView propagates", | ||
| input: &create.CreateView{}, | ||
| errInFilling: true, | ||
| expectError: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| mock := &mockProjectCreator{ | ||
| projectName: make(map[string]struct{}), | ||
| errInFilling: tt.errInFilling, | ||
| } | ||
|
|
||
| err := fillCreateView(mock, tt.input) | ||
|
|
||
| if tt.expectError { | ||
| assert.Error(t, err) | ||
| } else { | ||
| assert.NoError(t, err) | ||
| if tt.input != nil { | ||
| assert.Equal(t, tt.expectName, tt.input.ProjectName) | ||
| assert.Equal(t, tt.expectLimit, tt.input.StorageLimit) | ||
| } |
There was a problem hiding this comment.
The test case "nil createView gets defaults then filled" doesn't actually verify the expected behavior. When input is nil, the assertions at lines 94-95 are skipped due to the condition at line 93. While this prevents the test from failing, it also means the test isn't validating that the nil case works correctly. Either remove this test case or modify the test to properly verify the nil case behavior (though note that the nil handling in fillCreateView has other issues as noted separately).
There was a problem hiding this comment.
this part has been removed as mentioned earlier
| @@ -0,0 +1,330 @@ | |||
| // Copyright Project Harbor Authors | |||
There was a problem hiding this comment.
The PR description states "This PR introduces unit tests for the user create command" but the tests are actually for the project create command. This appears to be a copy-paste error in the PR description.
| projectsAreEqual := func(u1, u2 []*create.CreateView) bool { | ||
| if len(u1) != len(u2) { | ||
| return false | ||
| } | ||
| mp := make(map[string]int) | ||
| for _, proj := range u1 { | ||
| mp[proj.ProjectName]++ | ||
| } | ||
| for _, proj := range u2 { |
There was a problem hiding this comment.
The parameter names "u1" and "u2" in the projectsAreEqual helper function appear to be remnants from a user-related test (u for user) and don't make sense in the context of comparing projects. Consider renaming them to "p1" and "p2" or "projects1" and "projects2" for clarity.
| projectsAreEqual := func(u1, u2 []*create.CreateView) bool { | |
| if len(u1) != len(u2) { | |
| return false | |
| } | |
| mp := make(map[string]int) | |
| for _, proj := range u1 { | |
| mp[proj.ProjectName]++ | |
| } | |
| for _, proj := range u2 { | |
| projectsAreEqual := func(p1, p2 []*create.CreateView) bool { | |
| if len(p1) != len(p2) { | |
| return false | |
| } | |
| mp := make(map[string]int) | |
| for _, proj := range p1 { | |
| mp[proj.ProjectName]++ | |
| } | |
| for _, proj := range p2 { |
cmd/harbor/root/project/create.go
Outdated
|
|
||
| fmt.Printf("Project '%s' created successfully\n", ProjectName) | ||
| return nil | ||
| ProjectName = createView.ProjectName |
There was a problem hiding this comment.
The variable name "ProjectName" uses PascalCase instead of camelCase. Go convention for local variables is to use camelCase. This should be renamed to "projectName" for consistency with Go naming conventions.
cmd/harbor/root/project/create.go
Outdated
| } | ||
| err := fillCreateView(projectCreator, createView) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to get the required params to create project:%w", err) |
There was a problem hiding this comment.
The error message has inconsistent capitalization and formatting. It starts with uppercase "Failed" while other error messages in the same function (lines 50, 54, 77) start with lowercase. Additionally, there should be a space after the colon before %w. Go convention is to use lowercase for error messages unless they start with a proper noun or acronym. Change to: "failed to get the required params to create project: %w"
| return fmt.Errorf("Failed to get the required params to create project:%w", err) | |
| return fmt.Errorf("failed to get the required params to create project: %w", err) |
cmd/harbor/root/project/create.go
Outdated
| @@ -24,52 +26,68 @@ import ( | |||
| ) | |||
|
|
|||
| // CreateProjectCommand creates a new `harbor create project` command | |||
There was a problem hiding this comment.
The comment "CreateProjectCommand creates a new harbor create project command" is misplaced. It appears above the ProjectCreator interface definition but actually describes the CreateProjectCommand function further down. Move this comment to line 82 above the CreateProjectCommand function, or update it to describe the ProjectCreator interface instead.
…per on another function. createview is never going to be nil so we need not worry about nil pointer Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
|
@bupd I have resolved the issues pointed out by copilot |
Description
This PR introduces unit tests for the user create command (
cmd/harbor/root/project/create.go)Solves a part of #591
Functions covered: