From 7aa00e6f54266cf9828f93a016bf7a2c408283db Mon Sep 17 00:00:00 2001 From: Hugo Nijhuis-Mekkelholt Date: Tue, 30 Dec 2025 19:51:52 +0100 Subject: [PATCH] fix: address code review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix GetRaw() error handling to check status before reading body - Add context.Context support to all HTTP requests - Use consistent capitalized error messages - Update copyright year from 2024 to 2025 - Add unit tests for modules/api/client.go - Add tests for runs, jobs, logs commands 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- cmd/actions/jobs.go | 8 +- cmd/actions/jobs_test.go | 62 +++++++++++ cmd/actions/logs.go | 8 +- cmd/actions/logs_test.go | 62 +++++++++++ cmd/actions/runs.go | 4 +- cmd/actions/runs_test.go | 58 +++++++++++ modules/api/client.go | 32 +++--- modules/api/client_test.go | 203 +++++++++++++++++++++++++++++++++++++ modules/api/types.go | 2 +- 9 files changed, 413 insertions(+), 26 deletions(-) create mode 100644 cmd/actions/jobs_test.go create mode 100644 cmd/actions/logs_test.go create mode 100644 cmd/actions/runs_test.go create mode 100644 modules/api/client_test.go diff --git a/cmd/actions/jobs.go b/cmd/actions/jobs.go index 920b1f6..83e0332 100644 --- a/cmd/actions/jobs.go +++ b/cmd/actions/jobs.go @@ -1,4 +1,4 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. +// Copyright 2025 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package actions @@ -33,12 +33,12 @@ func RunActionJobs(ctx stdctx.Context, cmd *cli.Command) error { c.Ensure(context.CtxRequirement{RemoteRepo: true}) if cmd.Args().Len() < 1 { - return fmt.Errorf("run ID is required") + return fmt.Errorf("Run ID is required") } runID, err := utils.ArgToIndex(cmd.Args().First()) if err != nil { - return fmt.Errorf("invalid run ID: %w", err) + return fmt.Errorf("Invalid run ID: %w", err) } client := api.NewClient(c.Login) @@ -47,7 +47,7 @@ func RunActionJobs(ctx stdctx.Context, cmd *cli.Command) error { c.Owner, c.Repo, runID) var jobs api.ActionJobList - if _, err := client.Get(path, &jobs); err != nil { + if _, err := client.Get(ctx, path, &jobs); err != nil { return err } diff --git a/cmd/actions/jobs_test.go b/cmd/actions/jobs_test.go new file mode 100644 index 0000000..5a4f535 --- /dev/null +++ b/cmd/actions/jobs_test.go @@ -0,0 +1,62 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import ( + "testing" +) + +func TestJobsCommandFlags(t *testing.T) { + cmd := CmdActionsJobs + + // Test that required flags exist + expectedFlags := []string{"output", "remote", "login", "repo"} + + for _, flagName := range expectedFlags { + found := false + for _, flag := range cmd.Flags { + for _, name := range flag.Names() { + if name == flagName { + found = true + break + } + } + if found { + break + } + } + + if !found { + t.Errorf("Expected flag %s not found in CmdActionsJobs", flagName) + } + } +} + +func TestJobsCommandProperties(t *testing.T) { + cmd := CmdActionsJobs + + if cmd.Name != "jobs" { + t.Errorf("Expected command name 'jobs', got %s", cmd.Name) + } + + if len(cmd.Aliases) == 0 || cmd.Aliases[0] != "job" { + t.Errorf("Expected alias 'job' for jobs command") + } + + if cmd.Usage == "" { + t.Error("Jobs command should have usage text") + } + + if cmd.Description == "" { + t.Error("Jobs command should have description") + } + + if cmd.ArgsUsage != "" { + t.Errorf("Expected ArgsUsage '', got %s", cmd.ArgsUsage) + } + + if cmd.Action == nil { + t.Error("Jobs command should have an action") + } +} diff --git a/cmd/actions/logs.go b/cmd/actions/logs.go index 5d127ff..772ffff 100644 --- a/cmd/actions/logs.go +++ b/cmd/actions/logs.go @@ -1,4 +1,4 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. +// Copyright 2025 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package actions @@ -32,12 +32,12 @@ func RunActionLogs(ctx stdctx.Context, cmd *cli.Command) error { c.Ensure(context.CtxRequirement{RemoteRepo: true}) if cmd.Args().Len() < 1 { - return fmt.Errorf("job ID is required") + return fmt.Errorf("Job ID is required") } jobID, err := utils.ArgToIndex(cmd.Args().First()) if err != nil { - return fmt.Errorf("invalid job ID: %w", err) + return fmt.Errorf("Invalid job ID: %w", err) } client := api.NewClient(c.Login) @@ -45,7 +45,7 @@ func RunActionLogs(ctx stdctx.Context, cmd *cli.Command) error { path := fmt.Sprintf("/repos/%s/%s/actions/jobs/%d/logs", c.Owner, c.Repo, jobID) - logs, err := client.GetRaw(path) + logs, err := client.GetRaw(ctx, path) if err != nil { return err } diff --git a/cmd/actions/logs_test.go b/cmd/actions/logs_test.go new file mode 100644 index 0000000..bfdc2ca --- /dev/null +++ b/cmd/actions/logs_test.go @@ -0,0 +1,62 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import ( + "testing" +) + +func TestLogsCommandFlags(t *testing.T) { + cmd := CmdActionsLogs + + // Test that required flags exist + expectedFlags := []string{"output", "remote", "login", "repo"} + + for _, flagName := range expectedFlags { + found := false + for _, flag := range cmd.Flags { + for _, name := range flag.Names() { + if name == flagName { + found = true + break + } + } + if found { + break + } + } + + if !found { + t.Errorf("Expected flag %s not found in CmdActionsLogs", flagName) + } + } +} + +func TestLogsCommandProperties(t *testing.T) { + cmd := CmdActionsLogs + + if cmd.Name != "logs" { + t.Errorf("Expected command name 'logs', got %s", cmd.Name) + } + + if len(cmd.Aliases) == 0 || cmd.Aliases[0] != "log" { + t.Errorf("Expected alias 'log' for logs command") + } + + if cmd.Usage == "" { + t.Error("Logs command should have usage text") + } + + if cmd.Description == "" { + t.Error("Logs command should have description") + } + + if cmd.ArgsUsage != "" { + t.Errorf("Expected ArgsUsage '', got %s", cmd.ArgsUsage) + } + + if cmd.Action == nil { + t.Error("Logs command should have an action") + } +} diff --git a/cmd/actions/runs.go b/cmd/actions/runs.go index ad80ac4..7daabae 100644 --- a/cmd/actions/runs.go +++ b/cmd/actions/runs.go @@ -1,4 +1,4 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. +// Copyright 2025 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package actions @@ -41,7 +41,7 @@ func RunActionRuns(ctx stdctx.Context, cmd *cli.Command) error { flags.GetListOptions().PageSize) var runs api.ActionRunList - if _, err := client.Get(path, &runs); err != nil { + if _, err := client.Get(ctx, path, &runs); err != nil { return err } diff --git a/cmd/actions/runs_test.go b/cmd/actions/runs_test.go new file mode 100644 index 0000000..aad2ac1 --- /dev/null +++ b/cmd/actions/runs_test.go @@ -0,0 +1,58 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import ( + "testing" +) + +func TestRunsCommandFlags(t *testing.T) { + cmd := CmdActionsRuns + + // Test that required flags exist + expectedFlags := []string{"page", "limit", "output", "remote", "login", "repo"} + + for _, flagName := range expectedFlags { + found := false + for _, flag := range cmd.Flags { + for _, name := range flag.Names() { + if name == flagName { + found = true + break + } + } + if found { + break + } + } + + if !found { + t.Errorf("Expected flag %s not found in CmdActionsRuns", flagName) + } + } +} + +func TestRunsCommandProperties(t *testing.T) { + cmd := CmdActionsRuns + + if cmd.Name != "runs" { + t.Errorf("Expected command name 'runs', got %s", cmd.Name) + } + + if len(cmd.Aliases) == 0 || cmd.Aliases[0] != "run" { + t.Errorf("Expected alias 'run' for runs command") + } + + if cmd.Usage == "" { + t.Error("Runs command should have usage text") + } + + if cmd.Description == "" { + t.Error("Runs command should have description") + } + + if cmd.Action == nil { + t.Error("Runs command should have an action") + } +} diff --git a/modules/api/client.go b/modules/api/client.go index d079f5f..31b2a4d 100644 --- a/modules/api/client.go +++ b/modules/api/client.go @@ -1,9 +1,10 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. +// Copyright 2025 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package api import ( + "context" "crypto/tls" "encoding/json" "fmt" @@ -35,13 +36,13 @@ func NewClient(login *config.Login) *Client { } } -// Get makes an authenticated GET request to the API -func (c *Client) Get(path string, result interface{}) (*http.Response, error) { +// Get makes an authenticated GET request to the API and decodes the JSON response +func (c *Client) Get(ctx context.Context, path string, result interface{}) (*http.Response, error) { url := fmt.Sprintf("%s/api/v1%s", c.login.URL, path) - req, err := http.NewRequest("GET", url, nil) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create request: %w", err) } req.Header.Set("Authorization", "token "+c.login.Token) @@ -49,7 +50,7 @@ func (c *Client) Get(path string, result interface{}) (*http.Response, error) { resp, err := c.httpClient.Do(req) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to execute request: %w", err) } defer resp.Body.Close() @@ -68,29 +69,30 @@ func (c *Client) Get(path string, result interface{}) (*http.Response, error) { } // GetRaw makes an authenticated GET request and returns the raw response body -func (c *Client) GetRaw(path string) ([]byte, error) { +func (c *Client) GetRaw(ctx context.Context, path string) ([]byte, error) { url := fmt.Sprintf("%s/api/v1%s", c.login.URL, path) - req, err := http.NewRequest("GET", url, nil) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create request: %w", err) } req.Header.Set("Authorization", "token "+c.login.Token) resp, err := c.httpClient.Do(req) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to execute request: %w", err) } defer resp.Body.Close() - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err + if resp.StatusCode >= 400 { + body, _ := io.ReadAll(resp.Body) + return nil, fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, string(body)) } - if resp.StatusCode >= 400 { - return nil, fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, string(body)) + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response: %w", err) } return body, nil diff --git a/modules/api/client_test.go b/modules/api/client_test.go new file mode 100644 index 0000000..963d265 --- /dev/null +++ b/modules/api/client_test.go @@ -0,0 +1,203 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package api + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "code.gitea.io/tea/modules/config" +) + +func TestNewClient(t *testing.T) { + login := &config.Login{ + URL: "https://gitea.example.com", + Token: "test-token", + Insecure: false, + } + + client := NewClient(login) + if client == nil { + t.Fatal("NewClient returned nil") + } + + if client.login != login { + t.Error("Client login not set correctly") + } + + if client.httpClient == nil { + t.Error("Client httpClient not set") + } +} + +func TestNewClientInsecure(t *testing.T) { + login := &config.Login{ + URL: "https://gitea.example.com", + Token: "test-token", + Insecure: true, + } + + client := NewClient(login) + if client == nil { + t.Fatal("NewClient returned nil") + } + + // Verify that insecure transport is configured + if client.httpClient.Transport == nil { + t.Error("Expected custom transport for insecure client") + } +} + +func TestGet(t *testing.T) { + // Create a test server + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Verify request + if r.Method != "GET" { + t.Errorf("Expected GET request, got %s", r.Method) + } + + if r.Header.Get("Authorization") != "token test-token" { + t.Errorf("Expected authorization header, got %s", r.Header.Get("Authorization")) + } + + if r.Header.Get("Accept") != "application/json" { + t.Errorf("Expected accept header, got %s", r.Header.Get("Accept")) + } + + // Return test response + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]string{"status": "ok"}) + })) + defer server.Close() + + login := &config.Login{ + URL: server.URL, + Token: "test-token", + } + + client := NewClient(login) + + var result map[string]string + _, err := client.Get(context.Background(), "/test", &result) + if err != nil { + t.Fatalf("Get returned error: %v", err) + } + + if result["status"] != "ok" { + t.Errorf("Expected status 'ok', got %s", result["status"]) + } +} + +func TestGetError(t *testing.T) { + // Create a test server that returns an error + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte("not found")) + })) + defer server.Close() + + login := &config.Login{ + URL: server.URL, + Token: "test-token", + } + + client := NewClient(login) + + var result map[string]string + _, err := client.Get(context.Background(), "/test", &result) + if err == nil { + t.Fatal("Expected error for 404 response") + } + + expectedError := "API request failed with status 404" + if err.Error()[:len(expectedError)] != expectedError { + t.Errorf("Expected error starting with '%s', got '%s'", expectedError, err.Error()) + } +} + +func TestGetRaw(t *testing.T) { + expectedBody := "raw log content here" + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Errorf("Expected GET request, got %s", r.Method) + } + + if r.Header.Get("Authorization") != "token test-token" { + t.Errorf("Expected authorization header, got %s", r.Header.Get("Authorization")) + } + + w.Write([]byte(expectedBody)) + })) + defer server.Close() + + login := &config.Login{ + URL: server.URL, + Token: "test-token", + } + + client := NewClient(login) + + body, err := client.GetRaw(context.Background(), "/logs") + if err != nil { + t.Fatalf("GetRaw returned error: %v", err) + } + + if string(body) != expectedBody { + t.Errorf("Expected body '%s', got '%s'", expectedBody, string(body)) + } +} + +func TestGetRawError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("internal error")) + })) + defer server.Close() + + login := &config.Login{ + URL: server.URL, + Token: "test-token", + } + + client := NewClient(login) + + _, err := client.GetRaw(context.Background(), "/logs") + if err == nil { + t.Fatal("Expected error for 500 response") + } + + expectedError := "API request failed with status 500" + if err.Error()[:len(expectedError)] != expectedError { + t.Errorf("Expected error starting with '%s', got '%s'", expectedError, err.Error()) + } +} + +func TestGetWithContextCancellation(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // This should not be reached if context is cancelled + w.Write([]byte("ok")) + })) + defer server.Close() + + login := &config.Login{ + URL: server.URL, + Token: "test-token", + } + + client := NewClient(login) + + // Create a cancelled context + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var result map[string]string + _, err := client.Get(ctx, "/test", &result) + if err == nil { + t.Fatal("Expected error for cancelled context") + } +} diff --git a/modules/api/types.go b/modules/api/types.go index af479e6..92dd912 100644 --- a/modules/api/types.go +++ b/modules/api/types.go @@ -1,4 +1,4 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. +// Copyright 2025 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package api