Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 59 additions & 8 deletions internal/providers/gitlab/properties_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@
},
},
{
name: "repository succeeds",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this name to empty-string?

args: args{
ctx: context.TODO(),
getByProps: properties.NewProperties(map[string]any{
Expand All @@ -143,19 +142,30 @@
properties.RepoPropertyIsPrivate: true,
properties.RepoPropertyIsArchived: false,
properties.RepoPropertyIsFork: false,
RepoPropertyLicense: "mit", // Uses the correct constant
}),
wantErr: false,
gitLabServerMockFunc: func(w http.ResponseWriter, _ *http.Request) {
gitLabServerMockFunc: func(w http.ResponseWriter, r *http.Request) {
// Verify the query parameter you added is being sent
if r.URL.Query().Get("license") != "true" {
t.Errorf("expected query param license=true, got %s", r.URL.RawQuery)
w.WriteHeader(http.StatusBadRequest)
return
}

resp := &gitlab.Project{
ID: 1,
Name: "project-1",
Description: "project-1 description",
Visibility: gitlab.PrivateVisibility,
Archived: false,
ForkedFromProject: nil,
ID: 1,

Check failure on line 157 in internal/providers/gitlab/properties_test.go

View workflow job for this annotation

GitHub Actions / lint / Run golangci-lint

File is not properly formatted (gci)
Name: "project-1",
Description: "project-1 description",
Visibility: gitlab.PrivateVisibility,
Archived: false,
ForkedFromProject: nil,
Namespace: &gitlab.ProjectNamespace{
Path: "group",
},
License: &gitlab.ProjectLicense{
Name: "mit", // Changed Key to Name to match repository_properties.go

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this comment make sense in 6 months?

},
}

w.Header().Set("Content-Type", "application/json")
Expand Down Expand Up @@ -397,3 +407,44 @@
cli: &http.Client{},
}
}


func TestGitlabProjectToProperties_License(t *testing.T) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this test cover that the other test doesn't?

In general, extra tests add maintenance burden when refactoring code. That burden pays off when the tests cover additional important code cases, but redundant tests add more maintenance cost without commensurate benefits.

t.Parallel()

// 1. Create a mock GitLab project with a license
mockProject := &gitlab.Project{
ID: 123,
Name: "test-repo",
Namespace: &gitlab.ProjectNamespace{
Path: "test-org",
},
License: &gitlab.ProjectLicense{
Name: "Apache-2.0",
},
Visibility: gitlab.PublicVisibility,
Archived: false,
DefaultBranch: "main",
}

// 2. Run the function we want to test
props, err := gitlabProjectToProperties(mockProject)
if err != nil {
t.Fatalf("unexpected error converting project to properties: %v", err)
}

// 3. Verify that the license was successfully populated and is not empty
licenseProp := props.GetProperty(RepoPropertyLicense)
if licenseProp == nil {
t.Fatal("expected license property to be populated, but got nil")
}

licenseVal, err := licenseProp.AsString()
if err != nil {
t.Fatalf("failed to read license property as string: %v", err)
}

if licenseVal != "Apache-2.0" {
t.Errorf("expected license 'Apache-2.0', got '%s'", licenseVal)
}
}
3 changes: 2 additions & 1 deletion internal/providers/gitlab/repository_properties.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@
) (*gitlab.Project, error) {
projectURLPath, err := url.JoinPath("projects", url.PathEscape(upstreamID))
if err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ?license=true is appended before the error check — the code appends the query param before checking if url.JoinPath returned an error. If JoinPath fails, projectURLPath would be an empty string and the append would produce just ?license=true. The correct order is:

projectURLPath, err := url.JoinPath("projects", url.PathEscape(upstreamID))
if err != nil {
    return nil, fmt.Errorf("failed to join URL path for project using upstream ID: %w", err)
}
projectURLPath = projectURLPath + "?license=true"

return nil, fmt.Errorf("failed to join URL path for project using upstream ID: %w", err)
return nil, fmt.Errorf("failed to join URL path for project using upstream ID: %w", err)

Check failure on line 54 in internal/providers/gitlab/repository_properties.go

View workflow job for this annotation

GitHub Actions / lint / Run golangci-lint

File is not properly formatted (gci)
}
projectURLPath = projectURLPath + "?license=true"

// NOTE: We're not using github.com/xanzy/go-gitlab to do the actual
// request here because of the way they form authentication for requests.
Expand Down
Loading