diff --git a/.gitignore b/.gitignore index 60fb0d4..439a9bb 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ /review-bot +coverage.out diff --git a/Makefile b/Makefile new file mode 100644 index 0000000..6bb1ae0 --- /dev/null +++ b/Makefile @@ -0,0 +1,20 @@ +.PHONY: build test test-integration lint clean + +build: + go build -o review-bot ./cmd/review-bot/ + +test: + go test ./... + +test-integration: + go test -tags integration -v ./cmd/review-bot/ + +lint: + go vet ./... + +clean: + rm -f review-bot + +coverage: + go test -coverprofile=coverage.out ./... + go tool cover -func=coverage.out diff --git a/cmd/review-bot/integration_test.go b/cmd/review-bot/integration_test.go new file mode 100644 index 0000000..d0da818 --- /dev/null +++ b/cmd/review-bot/integration_test.go @@ -0,0 +1,161 @@ +//go:build integration + +package main + +import ( + "context" + "os" + "strconv" + "strings" + "testing" + + "gitea.weiker.me/rodin/review-bot/gitea" + "gitea.weiker.me/rodin/review-bot/llm" + "gitea.weiker.me/rodin/review-bot/review" +) + +// Integration test requires a running Gitea instance and LLM endpoint. +// Set environment variables: +// +// INTEGRATION_GITEA_URL - Gitea base URL +// INTEGRATION_GITEA_TOKEN - Gitea API token with repo access +// INTEGRATION_GITEA_REPO - owner/repo with an open PR +// INTEGRATION_PR_NUMBER - PR number to test against +// INTEGRATION_LLM_BASE_URL - LLM API base URL +// INTEGRATION_LLM_API_KEY - LLM API key +// INTEGRATION_LLM_MODEL - Model name +func TestIntegration_FullReviewFlow(t *testing.T) { + giteaURL := os.Getenv("INTEGRATION_GITEA_URL") + giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN") + giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO") + prNumStr := os.Getenv("INTEGRATION_PR_NUMBER") + llmBaseURL := os.Getenv("INTEGRATION_LLM_BASE_URL") + llmAPIKey := os.Getenv("INTEGRATION_LLM_API_KEY") + llmModel := os.Getenv("INTEGRATION_LLM_MODEL") + + if giteaURL == "" || giteaToken == "" || giteaRepo == "" || prNumStr == "" || + llmBaseURL == "" || llmAPIKey == "" || llmModel == "" { + t.Skip("Integration test env vars not set, skipping") + } + + prNumber, err := strconv.Atoi(prNumStr) + if err != nil { + t.Fatalf("Invalid PR number %q: %v", prNumStr, err) + } + + // Parse owner/repo + parts := strings.SplitN(giteaRepo, "/", 2) + if len(parts) != 2 { + t.Fatalf("Invalid repo format %q", giteaRepo) + } + owner, repoName := parts[0], parts[1] + if owner == "" || repoName == "" { + t.Fatalf("Invalid repo format %q", giteaRepo) + } + + ctx := context.Background() + + // Step 1: Fetch PR + giteaClient := gitea.NewClient(giteaURL, giteaToken) + pr, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber) + if err != nil { + t.Fatalf("GetPullRequest: %v", err) + } + t.Logf("PR: %s (sha: %s)", pr.Title, pr.Head.Sha) + + // Step 2: Fetch diff + diff, err := giteaClient.GetPullRequestDiff(ctx, owner, repoName, prNumber) + if err != nil { + t.Fatalf("GetPullRequestDiff: %v", err) + } + if diff == "" { + t.Fatal("diff is empty") + } + t.Logf("Diff size: %d bytes", len(diff)) + + // Step 3: Build prompts + systemPrompt := review.BuildSystemPrompt("", "") + userPrompt := review.BuildUserPrompt(pr.Title, pr.Body, diff, "", true, "") + + // Step 4: Call LLM + llmClient := llm.NewClient(llmBaseURL, llmAPIKey, llmModel) + response, err := llmClient.Complete(ctx, []llm.Message{ + {Role: "system", Content: systemPrompt}, + {Role: "user", Content: userPrompt}, + }) + if err != nil { + t.Fatalf("LLM Complete: %v", err) + } + t.Logf("LLM response: %d bytes", len(response)) + + // Step 5: Parse response + result, err := review.ParseResponse(response) + if err != nil { + t.Fatalf("ParseResponse: %v", err) + } + t.Logf("Verdict: %s, Findings: %d", result.Verdict, len(result.Findings)) + + // Step 6: Format (dry-run validation) + body := review.FormatMarkdown(result, "integration-test") + if body == "" { + t.Fatal("formatted review body is empty") + } + t.Logf("Review body:\n%s", body) +} + +func TestIntegration_PostAndCleanup(t *testing.T) { + giteaURL := os.Getenv("INTEGRATION_GITEA_URL") + giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN") + giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO") + prNumStr := os.Getenv("INTEGRATION_PR_NUMBER") + + if giteaURL == "" || giteaToken == "" || giteaRepo == "" || prNumStr == "" { + t.Skip("Integration test env vars not set, skipping") + } + + prNumber, err := strconv.Atoi(prNumStr) + if err != nil { + t.Fatalf("Invalid PR number %q: %v", prNumStr, err) + } + + parts := strings.SplitN(giteaRepo, "/", 2) + if len(parts) != 2 { + t.Fatalf("Invalid repo format %q", giteaRepo) + } + owner, repoName := parts[0], parts[1] + + ctx := context.Background() + giteaClient := gitea.NewClient(giteaURL, giteaToken) + + // Post a test review + sentinel := "" + testBody := "# Integration Test Review\n\nThis is a test review.\n\n" + sentinel + posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, nil) + if err != nil { + t.Fatalf("PostReview: %v", err) + } + t.Logf("Posted review ID: %d", posted.ID) + + // Verify it appears in listing + reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) + if err != nil { + t.Fatalf("ListReviews: %v", err) + } + + found := false + for _, r := range reviews { + if r.ID == posted.ID && strings.Contains(r.Body, sentinel) { + found = true + break + } + } + if !found { + t.Error("posted review not found in listing") + } + + // Cleanup: delete the test review + err = giteaClient.DeleteReview(ctx, owner, repoName, prNumber, posted.ID) + if err != nil { + t.Logf("Warning: could not delete test review %d: %v", posted.ID, err) + } +} diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 4577df5..e2f6dcb 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -2,7 +2,10 @@ package main import ( "bytes" + "flag" "log/slog" + "os" + "os/exec" "strings" "testing" @@ -365,3 +368,480 @@ func TestSetupLogger_Integration(t *testing.T) { setupLogger("text", "unknown") // should default to info setupLogger("invalid", "info") // should default to text } + +func TestIsPatternFile(t *testing.T) { + tests := []struct { + path string + want bool + }{ + {"README.md", true}, + {"docs/GUIDE.MD", true}, + {"config.yml", true}, + {"config.yaml", true}, + {"notes.txt", true}, + {"NOTES.TXT", true}, + {"main.go", false}, + {"lib.rs", false}, + {"index.js", false}, + {"Makefile", false}, + {"", false}, + {"doc.pdf", false}, + {"patterns.Yml", true}, + {"deep/path/file.yaml", true}, + } + + for _, tc := range tests { + t.Run(tc.path, func(t *testing.T) { + got := isPatternFile(tc.path) + if got != tc.want { + t.Errorf("isPatternFile(%q) = %v, want %v", tc.path, got, tc.want) + } + }) + } +} + +func TestEvaluateCIStatus(t *testing.T) { + tests := []struct { + name string + statuses []gitea.CommitStatus + wantPassed bool + wantSubstr string + }{ + { + name: "empty statuses", + statuses: nil, + wantPassed: true, + wantSubstr: "no CI statuses", + }, + { + name: "all success", + statuses: []gitea.CommitStatus{ + {Status: "success", Context: "ci/build", Description: "Build passed"}, + {Status: "success", Context: "ci/test", Description: "Tests passed"}, + }, + wantPassed: true, + wantSubstr: "all checks passed", + }, + { + name: "one failure", + statuses: []gitea.CommitStatus{ + {Status: "success", Context: "ci/build", Description: "Build passed"}, + {Status: "failure", Context: "ci/test", Description: "Tests failed"}, + }, + wantPassed: false, + wantSubstr: "ci/test", + }, + { + name: "error status", + statuses: []gitea.CommitStatus{ + {Status: "error", Context: "ci/lint", Description: "Lint error"}, + }, + wantPassed: false, + wantSubstr: "ci/lint", + }, + { + name: "pending treated as not-failed", + statuses: []gitea.CommitStatus{ + {Status: "pending", Context: "ci/build", Description: "In progress"}, + {Status: "success", Context: "ci/test", Description: "Tests passed"}, + }, + wantPassed: true, + wantSubstr: "all checks passed", + }, + { + name: "multiple failures", + statuses: []gitea.CommitStatus{ + {Status: "failure", Context: "ci/build", Description: "Build failed"}, + {Status: "failure", Context: "ci/test", Description: "Tests failed"}, + }, + wantPassed: false, + wantSubstr: "ci/build", + }, + { + name: "mixed with pending and failure", + statuses: []gitea.CommitStatus{ + {Status: "success", Context: "ci/build", Description: "Build passed"}, + {Status: "pending", Context: "ci/deploy", Description: "Deploying"}, + {Status: "failure", Context: "ci/test", Description: "Tests failed"}, + }, + wantPassed: false, + wantSubstr: "ci/test", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + passed, details := evaluateCIStatus(tc.statuses) + if passed != tc.wantPassed { + t.Errorf("evaluateCIStatus() passed = %v, want %v", passed, tc.wantPassed) + } + if !strings.Contains(details, tc.wantSubstr) { + t.Errorf("evaluateCIStatus() details = %q, want substring %q", details, tc.wantSubstr) + } + }) + } +} + +func TestEnvOrDefault(t *testing.T) { + // Test with unset env var + os.Unsetenv("TEST_ENV_OR_DEFAULT_UNSET") + got := envOrDefault("TEST_ENV_OR_DEFAULT_UNSET", "fallback") + if got != "fallback" { + t.Errorf("envOrDefault(unset) = %q, want %q", got, "fallback") + } + + // Test with set env var + os.Setenv("TEST_ENV_OR_DEFAULT_SET", "custom") + defer os.Unsetenv("TEST_ENV_OR_DEFAULT_SET") + got = envOrDefault("TEST_ENV_OR_DEFAULT_SET", "fallback") + if got != "custom" { + t.Errorf("envOrDefault(set) = %q, want %q", got, "custom") + } + + // Test with empty env var (should return default) + os.Setenv("TEST_ENV_OR_DEFAULT_EMPTY", "") + defer os.Unsetenv("TEST_ENV_OR_DEFAULT_EMPTY") + got = envOrDefault("TEST_ENV_OR_DEFAULT_EMPTY", "fallback") + if got != "fallback" { + t.Errorf("envOrDefault(empty) = %q, want %q", got, "fallback") + } +} + +func TestEnvOrDefaultFloat(t *testing.T) { + // Test with unset env var + os.Unsetenv("TEST_ENV_FLOAT_UNSET") + got := envOrDefaultFloat("TEST_ENV_FLOAT_UNSET", 1.5) + if got != 1.5 { + t.Errorf("envOrDefaultFloat(unset) = %f, want %f", got, 1.5) + } + + // Test with valid float + os.Setenv("TEST_ENV_FLOAT_SET", "2.7") + defer os.Unsetenv("TEST_ENV_FLOAT_SET") + got = envOrDefaultFloat("TEST_ENV_FLOAT_SET", 1.5) + if got != 2.7 { + t.Errorf("envOrDefaultFloat(set) = %f, want %f", got, 2.7) + } + + // Test with invalid float (should return default) + os.Setenv("TEST_ENV_FLOAT_INVALID", "not-a-number") + defer os.Unsetenv("TEST_ENV_FLOAT_INVALID") + got = envOrDefaultFloat("TEST_ENV_FLOAT_INVALID", 3.14) + if got != 3.14 { + t.Errorf("envOrDefaultFloat(invalid) = %f, want %f", got, 3.14) + } + + // Test with empty string (should return default) + os.Setenv("TEST_ENV_FLOAT_EMPTY", "") + defer os.Unsetenv("TEST_ENV_FLOAT_EMPTY") + got = envOrDefaultFloat("TEST_ENV_FLOAT_EMPTY", 0.5) + if got != 0.5 { + t.Errorf("envOrDefaultFloat(empty) = %f, want %f", got, 0.5) + } +} + +func TestEnvOrDefaultInt(t *testing.T) { + // Test with unset env var + os.Unsetenv("TEST_ENV_INT_UNSET") + got := envOrDefaultInt("TEST_ENV_INT_UNSET", 42) + if got != 42 { + t.Errorf("envOrDefaultInt(unset) = %d, want %d", got, 42) + } + + // Test with valid int + os.Setenv("TEST_ENV_INT_SET", "100") + defer os.Unsetenv("TEST_ENV_INT_SET") + got = envOrDefaultInt("TEST_ENV_INT_SET", 42) + if got != 100 { + t.Errorf("envOrDefaultInt(set) = %d, want %d", got, 100) + } + + // Test with invalid int (should return default) + os.Setenv("TEST_ENV_INT_INVALID", "abc") + defer os.Unsetenv("TEST_ENV_INT_INVALID") + got = envOrDefaultInt("TEST_ENV_INT_INVALID", 42) + if got != 42 { + t.Errorf("envOrDefaultInt(invalid) = %d, want %d", got, 42) + } + + // Test with empty string (should return default) + os.Setenv("TEST_ENV_INT_EMPTY", "") + defer os.Unsetenv("TEST_ENV_INT_EMPTY") + got = envOrDefaultInt("TEST_ENV_INT_EMPTY", 99) + if got != 99 { + t.Errorf("envOrDefaultInt(empty) = %d, want %d", got, 99) + } + + // Test with negative int + os.Setenv("TEST_ENV_INT_NEG", "-5") + defer os.Unsetenv("TEST_ENV_INT_NEG") + got = envOrDefaultInt("TEST_ENV_INT_NEG", 42) + if got != -5 { + t.Errorf("envOrDefaultInt(negative) = %d, want %d", got, -5) + } +} + +func TestEnvOrDefaultBool(t *testing.T) { + tests := []struct { + name string + envVal string + setEnv bool + defaultVal bool + want bool + }{ + {"unset returns default true", "", false, true, true}, + {"unset returns default false", "", false, false, false}, + {"true", "true", true, false, true}, + {"TRUE", "TRUE", true, false, true}, + {"True", "True", true, false, true}, + {"1", "1", true, false, true}, + {"yes", "yes", true, false, true}, + {"YES", "YES", true, false, true}, + {"false", "false", true, true, false}, + {"0", "0", true, true, false}, + {"no", "no", true, true, false}, + {"random string", "random", true, true, false}, + {"empty string returns default", "", true, true, true}, + {"whitespace true", " true ", true, false, true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + envKey := "TEST_ENV_BOOL_" + strings.ReplaceAll(tc.name, " ", "_") + if tc.setEnv { + os.Setenv(envKey, tc.envVal) + defer os.Unsetenv(envKey) + } else { + os.Unsetenv(envKey) + } + got := envOrDefaultBool(envKey, tc.defaultVal) + if got != tc.want { + t.Errorf("envOrDefaultBool(%q, %v) = %v, want %v", tc.envVal, tc.defaultVal, got, tc.want) + } + }) + } +} + +func TestExtractSentinelName_EdgeCases(t *testing.T) { + tests := []struct { + body string + want string + }{ + {" rest", "sonnet"}, + {" rest", "gpt-review"}, + {"no sentinel here", "unknown"}, + {" end", "abc"}, // embedded in text + } + + for _, tc := range tests { + got := extractSentinelName(tc.body) + if got != tc.want { + t.Errorf("extractSentinelName(%q) = %q, want %q", tc.body, got, tc.want) + } + } +} + +// TestMainSubprocess runs main() as a subprocess using the test binary itself. +// This allows coverage to be captured for main() code paths. +func TestMainSubprocess_Version(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + // Reset flags for main() + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", "--version"} + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_Version") + cmd.Env = append(os.Environ(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("--version subprocess failed: %v\n%s", err, out) + } + if !strings.Contains(string(out), "review-bot") { + t.Errorf("--version output = %q, want to contain 'review-bot'", string(out)) + } +} + +func TestMainSubprocess_MissingFlags(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + // Reset flags for main() + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot"} + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_MissingFlags") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit with no flags, got success") + } + if !strings.Contains(string(out), "missing required") { + t.Errorf("expected error about missing flags, got: %s", out) + } +} + +func TestMainSubprocess_InvalidReviewerName(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--gitea-url", "http://localhost", + "--repo", "owner/repo", + "--pr", "1", + "--reviewer-name", "invalid name", + "--reviewer-token", "tok", + "--llm-base-url", "http://localhost", + "--llm-api-key", "key", + "--llm-model", "model", + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidReviewerName") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit with invalid reviewer-name, got success") + } + if !strings.Contains(string(out), "invalid reviewer name") { + t.Errorf("expected error about invalid reviewer name, got: %s", out) + } +} + +func TestMainSubprocess_InvalidRepo(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--gitea-url", "http://localhost", + "--repo", "invalidrepo", + "--pr", "1", + "--reviewer-token", "tok", + "--llm-base-url", "http://localhost", + "--llm-api-key", "key", + "--llm-model", "model", + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidRepo") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit with invalid repo format") + } + if !strings.Contains(string(out), "invalid repo format") { + t.Errorf("expected error about invalid repo, got: %s", out) + } +} + +func TestMainSubprocess_InvalidPRNumber(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--gitea-url", "http://localhost", + "--repo", "owner/repo", + "--pr", "notanumber", + "--reviewer-token", "tok", + "--llm-base-url", "http://localhost", + "--llm-api-key", "key", + "--llm-model", "model", + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidPRNumber") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit with invalid PR number") + } + if !strings.Contains(string(out), "invalid PR number") { + t.Errorf("expected error about invalid PR number, got: %s", out) + } +} + +func TestMainSubprocess_InvalidTemperature(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--gitea-url", "http://localhost", + "--repo", "owner/repo", + "--pr", "1", + "--reviewer-token", "tok", + "--llm-base-url", "http://localhost", + "--llm-api-key", "key", + "--llm-model", "model", + "--llm-temperature", "5.0", + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidTemperature") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit with invalid temperature") + } + if !strings.Contains(string(out), "invalid LLM temperature") { + t.Errorf("expected error about invalid temperature, got: %s", out) + } +} + +func TestMainSubprocess_InvalidProvider(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--gitea-url", "http://localhost", + "--repo", "owner/repo", + "--pr", "1", + "--reviewer-token", "tok", + "--llm-base-url", "http://localhost", + "--llm-api-key", "key", + "--llm-model", "model", + "--llm-provider", "invalid-provider", + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidProvider") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit with invalid provider") + } + if !strings.Contains(string(out), "invalid LLM provider") { + t.Errorf("expected error about invalid provider, got: %s", out) + } +} + +// cleanEnv returns environ without any GITEA/LLM/REVIEWER env vars that would +// interfere with testing missing-flag scenarios. +func cleanEnv() []string { + var env []string + for _, e := range os.Environ() { + key := strings.SplitN(e, "=", 2)[0] + switch { + case strings.HasPrefix(key, "GITEA_"), + strings.HasPrefix(key, "LLM_"), + strings.HasPrefix(key, "REVIEWER_"), + strings.HasPrefix(key, "PR_"), + strings.HasPrefix(key, "LOG_"), + strings.HasPrefix(key, "CONVENTIONS_"), + strings.HasPrefix(key, "SYSTEM_PROMPT_"), + strings.HasPrefix(key, "PATTERNS_"), + strings.HasPrefix(key, "UPDATE_"): + continue + default: + env = append(env, e) + } + } + return env +}