Rodin
282b6e0e86
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 22s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 40s
Address sonnet NIT: if --repo or --pr is ever removed from baseSubprocessArgs(), the mutation loop silently no-ops and the test becomes meaningless. Adding a found guard and t.Fatal makes the regression immediately visible.
1635 lines
49 KiB
Go
1635 lines
49 KiB
Go
package main
|
|
|
|
import (
|
|
"bytes"
|
|
"context"
|
|
"flag"
|
|
"fmt"
|
|
"log/slog"
|
|
"os"
|
|
"os/exec"
|
|
"path/filepath"
|
|
"strings"
|
|
"testing"
|
|
|
|
"gitea.weiker.me/rodin/review-bot/review"
|
|
)
|
|
|
|
func TestValidateReviewerName(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
input string
|
|
wantErr bool
|
|
}{
|
|
{"valid simple", "sonnet", false},
|
|
{"valid with dash", "code-review", false},
|
|
{"valid with underscore", "my_bot", false},
|
|
{"valid alphanumeric", "bot123", false},
|
|
{"valid uppercase", "MyBot", false},
|
|
{"empty is valid", "", false},
|
|
{"invalid html close", "foo-->", true},
|
|
{"invalid space", "my bot", true},
|
|
{"invalid dot", "my.bot", true},
|
|
{"invalid slash", "my/bot", true},
|
|
{"invalid angle", "bot<script>", true},
|
|
{"invalid colon", "bot:name", true},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
err := validateReviewerName(tc.input)
|
|
if tc.wantErr && err == nil {
|
|
t.Errorf("expected error for %q, got nil", tc.input)
|
|
}
|
|
if !tc.wantErr && err != nil {
|
|
t.Errorf("expected no error for %q, got %v", tc.input, err)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestValidateWorkspacePath(t *testing.T) {
|
|
// Create a temp directory as our workspace
|
|
tmpDir := t.TempDir()
|
|
|
|
// Create a valid file inside the workspace
|
|
validFile := filepath.Join(tmpDir, "valid.json")
|
|
if err := os.WriteFile(validFile, []byte("{}"), 0644); err != nil {
|
|
t.Fatalf("failed to create test file: %v", err)
|
|
}
|
|
|
|
// Create a subdirectory with a file
|
|
subDir := filepath.Join(tmpDir, "subdir")
|
|
if err := os.MkdirAll(subDir, 0755); err != nil {
|
|
t.Fatalf("failed to create subdir: %v", err)
|
|
}
|
|
nestedFile := filepath.Join(subDir, "nested.json")
|
|
if err := os.WriteFile(nestedFile, []byte("{}"), 0644); err != nil {
|
|
t.Fatalf("failed to create nested file: %v", err)
|
|
}
|
|
|
|
// Create a symlink pointing outside the workspace
|
|
symlinkPath := filepath.Join(tmpDir, "evil-symlink.json")
|
|
if err := os.Symlink("/etc/passwd", symlinkPath); err != nil {
|
|
t.Fatalf("failed to create symlink: %v", err)
|
|
}
|
|
|
|
// Save and restore GITHUB_WORKSPACE
|
|
origWorkspace := os.Getenv("GITHUB_WORKSPACE")
|
|
defer os.Setenv("GITHUB_WORKSPACE", origWorkspace)
|
|
|
|
tests := []struct {
|
|
name string
|
|
workspace string
|
|
path string
|
|
wantErr bool
|
|
errMatch string
|
|
}{
|
|
{
|
|
name: "valid relative path",
|
|
workspace: tmpDir,
|
|
path: "valid.json",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "valid nested path",
|
|
workspace: tmpDir,
|
|
path: "subdir/nested.json",
|
|
wantErr: false,
|
|
},
|
|
{
|
|
name: "path traversal attempt",
|
|
workspace: tmpDir,
|
|
path: "../../../etc/passwd",
|
|
wantErr: true,
|
|
errMatch: "resolves outside workspace",
|
|
},
|
|
{
|
|
name: "absolute path normalized to workspace-relative",
|
|
workspace: tmpDir,
|
|
path: "/etc/passwd",
|
|
wantErr: true,
|
|
// Go 1.21+ filepath.Join normalizes absolute paths: Join("/tmp/x", "/etc/passwd")
|
|
// becomes "/tmp/x/etc/passwd", which is within workspace but doesn't exist.
|
|
errMatch: "failed to resolve",
|
|
},
|
|
{
|
|
name: "nonexistent file",
|
|
workspace: tmpDir,
|
|
path: "nonexistent.json",
|
|
wantErr: true,
|
|
errMatch: "failed to resolve",
|
|
},
|
|
{
|
|
name: "symlink escaping workspace",
|
|
workspace: tmpDir,
|
|
path: "evil-symlink.json",
|
|
wantErr: true,
|
|
errMatch: "symlink resolves outside workspace",
|
|
},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
os.Setenv("GITHUB_WORKSPACE", tc.workspace)
|
|
resolved, err := validateWorkspacePath(tc.path, "test-file")
|
|
|
|
if tc.wantErr {
|
|
if err == nil {
|
|
t.Errorf("expected error for %q, got nil", tc.path)
|
|
} else if tc.errMatch != "" && !strings.Contains(err.Error(), tc.errMatch) {
|
|
t.Errorf("error %q should contain %q", err.Error(), tc.errMatch)
|
|
}
|
|
} else {
|
|
if err != nil {
|
|
t.Errorf("expected no error for %q, got %v", tc.path, err)
|
|
}
|
|
if resolved == "" {
|
|
t.Error("expected non-empty resolved path")
|
|
}
|
|
// Verify resolved path is within workspace
|
|
if !strings.HasPrefix(resolved, tc.workspace) {
|
|
t.Errorf("resolved path %q not within workspace %q", resolved, tc.workspace)
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func makeReview(id int64, login, state string, _ bool, body string) vcsReview {
|
|
r := vcsReview{
|
|
ID: id,
|
|
Body: body,
|
|
State: state,
|
|
}
|
|
r.User.Login = login
|
|
return r
|
|
}
|
|
|
|
func TestBuildSupersededBody(t *testing.T) {
|
|
original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
|
|
sentinel := "<!-- review-bot:sonnet -->"
|
|
newURL := "https://gitea.example.com/owner/repo/pulls/1#pullrequestreview-99"
|
|
|
|
result := buildSupersededBody(original, "abcdef1234567890", newURL, sentinel)
|
|
|
|
// Should contain the struck-through banner
|
|
if !strings.Contains(result, "~~Original review~~") {
|
|
t.Error("missing struck-through banner")
|
|
}
|
|
// Should contain superseded notice with link
|
|
if !strings.Contains(result, "**Superseded**") {
|
|
t.Error("missing superseded notice")
|
|
}
|
|
if !strings.Contains(result, "[see current review]("+newURL+")") {
|
|
t.Error("missing link to new review")
|
|
}
|
|
// Should contain collapsed original
|
|
if !strings.Contains(result, "<details>") {
|
|
t.Error("missing details/collapse")
|
|
}
|
|
// Should contain short commit SHA
|
|
if !strings.Contains(result, "abcdef12") {
|
|
t.Error("missing short SHA")
|
|
}
|
|
// Should NOT contain full SHA
|
|
if strings.Contains(result, "abcdef1234567890") {
|
|
t.Error("should truncate SHA to 8 chars")
|
|
}
|
|
// Should contain the original body inside details
|
|
if !strings.Contains(result, original) {
|
|
t.Error("original body not preserved in collapsed section")
|
|
}
|
|
// Should end with sentinel
|
|
if !strings.Contains(result, sentinel) {
|
|
t.Error("missing sentinel")
|
|
}
|
|
}
|
|
|
|
func TestBuildSupersededBodyShortSHA(t *testing.T) {
|
|
// Short SHA should pass through without panic
|
|
result := buildSupersededBody("body", "abc", "https://example.com/review", "<!-- review-bot:x -->")
|
|
if !strings.Contains(result, "abc") {
|
|
t.Error("short SHA not preserved")
|
|
}
|
|
}
|
|
|
|
func TestFindOwnReview(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
reviews []vcsReview
|
|
sentinel string
|
|
wantID int64
|
|
wantNil bool
|
|
}{
|
|
{
|
|
name: "no reviews",
|
|
reviews: nil,
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
wantNil: true,
|
|
},
|
|
{
|
|
name: "found by sentinel",
|
|
reviews: []vcsReview{
|
|
makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
wantID: 42,
|
|
},
|
|
{
|
|
name: "wrong sentinel",
|
|
reviews: []vcsReview{
|
|
makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
wantNil: true,
|
|
},
|
|
{
|
|
name: "multiple reviews, returns first match",
|
|
reviews: []vcsReview{
|
|
makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
|
|
makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
wantID: 20,
|
|
},
|
|
{
|
|
name: "skips superseded review",
|
|
reviews: []vcsReview{
|
|
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n<!-- review-bot:sonnet -->"),
|
|
makeReview(20, "bot", "APPROVED", false, "fresh review\n<!-- review-bot:sonnet -->"),
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
wantID: 20,
|
|
},
|
|
{
|
|
name: "only superseded reviews exist",
|
|
reviews: []vcsReview{
|
|
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
wantNil: true,
|
|
},
|
|
{
|
|
name: "picks highest ID among matches",
|
|
reviews: []vcsReview{
|
|
makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
|
|
makeReview(30, "bot", "APPROVED", false, "v0\n<!-- review-bot:sonnet -->"),
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
wantID: 50,
|
|
},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
got := findOwnReview(tc.reviews, tc.sentinel)
|
|
if tc.wantNil {
|
|
if got != nil {
|
|
t.Errorf("findOwnReview() = %v, want nil", got)
|
|
}
|
|
} else {
|
|
if got == nil {
|
|
t.Fatal("findOwnReview() = nil, want non-nil")
|
|
}
|
|
if got.ID != tc.wantID {
|
|
t.Errorf("findOwnReview().ID = %d, want %d", got.ID, tc.wantID)
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestHasSharedToken(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
reviews []vcsReview
|
|
sentinel string
|
|
want bool
|
|
}{
|
|
{
|
|
name: "no reviews",
|
|
reviews: nil,
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
want: false,
|
|
},
|
|
{
|
|
name: "no own review yet - cannot detect",
|
|
reviews: []vcsReview{
|
|
{ID: 1, User: struct{ Login string }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
want: false,
|
|
},
|
|
{
|
|
name: "separate users - no shared token",
|
|
reviews: []vcsReview{
|
|
{ID: 1, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
|
{ID: 2, User: struct{ Login string }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"},
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
want: false,
|
|
},
|
|
{
|
|
name: "shared token detected - same user different sentinels",
|
|
reviews: []vcsReview{
|
|
{ID: 1, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
|
{ID: 2, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"},
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
want: true,
|
|
},
|
|
{
|
|
name: "three roles same user",
|
|
reviews: []vcsReview{
|
|
{ID: 1, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
|
{ID: 2, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
|
|
{ID: 3, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
|
|
},
|
|
sentinel: "<!-- review-bot:sonnet -->",
|
|
want: true,
|
|
},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
got := hasSharedToken(tc.reviews, tc.sentinel)
|
|
if got != tc.want {
|
|
t.Errorf("hasSharedToken() = %v, want %v", got, tc.want)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestExtractSentinelName(t *testing.T) {
|
|
tests := []struct {
|
|
body string
|
|
want string
|
|
}{
|
|
{"<!-- review-bot:sonnet --> rest", "sonnet"},
|
|
{"<!-- review-bot:security --> rest", "security"},
|
|
{"no sentinel here", "unknown"},
|
|
{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
got := extractSentinelName(tc.body)
|
|
if got != tc.want {
|
|
t.Errorf("extractSentinelName(%q) = %q, want %q", tc.body, got, tc.want)
|
|
}
|
|
}
|
|
}
|
|
|
|
func TestSetupLogger_JSONFormat(t *testing.T) {
|
|
// Capture output by creating a logger manually with the same logic
|
|
var buf bytes.Buffer
|
|
opts := &slog.HandlerOptions{Level: slog.LevelInfo}
|
|
handler := slog.NewJSONHandler(&buf, opts)
|
|
logger := slog.New(handler)
|
|
|
|
logger.Info("test message", "key", "value")
|
|
|
|
output := buf.String()
|
|
if !strings.Contains(output, `"msg":"test message"`) {
|
|
t.Errorf("expected JSON msg field, got: %s", output)
|
|
}
|
|
if !strings.Contains(output, `"key":"value"`) {
|
|
t.Errorf("expected JSON key field, got: %s", output)
|
|
}
|
|
if !strings.Contains(output, `"level":"INFO"`) {
|
|
t.Errorf("expected JSON level field, got: %s", output)
|
|
}
|
|
}
|
|
|
|
func TestSetupLogger_TextFormat(t *testing.T) {
|
|
var buf bytes.Buffer
|
|
opts := &slog.HandlerOptions{Level: slog.LevelInfo}
|
|
handler := slog.NewTextHandler(&buf, opts)
|
|
logger := slog.New(handler)
|
|
|
|
logger.Info("test message", "key", "value")
|
|
|
|
output := buf.String()
|
|
if !strings.Contains(output, "msg=\"test message\"") && !strings.Contains(output, "msg=test") {
|
|
t.Errorf("expected text msg field, got: %s", output)
|
|
}
|
|
if !strings.Contains(output, "key=value") {
|
|
t.Errorf("expected text key field, got: %s", output)
|
|
}
|
|
}
|
|
|
|
func TestSetupLogger_LevelFiltering(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
verbosity string
|
|
logLevel slog.Level
|
|
expected bool // should the message appear
|
|
}{
|
|
{"info logger shows info", "info", slog.LevelInfo, true},
|
|
{"info logger hides debug", "info", slog.LevelDebug, false},
|
|
{"debug logger shows debug", "debug", slog.LevelDebug, true},
|
|
{"warn logger hides info", "warn", slog.LevelInfo, false},
|
|
{"warn logger shows warn", "warn", slog.LevelWarn, true},
|
|
{"error logger hides warn", "error", slog.LevelWarn, false},
|
|
{"error logger shows error", "error", slog.LevelError, true},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
var level slog.Level
|
|
switch tc.verbosity {
|
|
case "debug":
|
|
level = slog.LevelDebug
|
|
case "info":
|
|
level = slog.LevelInfo
|
|
case "warn":
|
|
level = slog.LevelWarn
|
|
case "error":
|
|
level = slog.LevelError
|
|
}
|
|
|
|
var buf bytes.Buffer
|
|
opts := &slog.HandlerOptions{Level: level}
|
|
handler := slog.NewTextHandler(&buf, opts)
|
|
logger := slog.New(handler)
|
|
|
|
logger.Log(nil, tc.logLevel, "test")
|
|
|
|
hasOutput := buf.Len() > 0
|
|
if hasOutput != tc.expected {
|
|
t.Errorf("verbosity=%s, logLevel=%s: got output=%v, want %v",
|
|
tc.verbosity, tc.logLevel, hasOutput, tc.expected)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestSetupLogger_Integration(t *testing.T) {
|
|
// Test that setupLogger doesn't panic for valid inputs
|
|
setupLogger("text", "info")
|
|
setupLogger("json", "debug")
|
|
setupLogger("text", "warn")
|
|
setupLogger("json", "error")
|
|
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)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// TestBuildPatternPaths verifies the path-building logic for fetchPatterns.
|
|
// Empty patternsFiles means "fetch all from root" (represented as [""]).
|
|
func TestBuildPatternPaths(t *testing.T) {
|
|
buildPaths := func(patternsFiles string) []string {
|
|
if patternsFiles == "" {
|
|
return []string{""}
|
|
}
|
|
var paths []string
|
|
for _, p := range strings.Split(patternsFiles, ",") {
|
|
p = strings.TrimSpace(p)
|
|
if p != "" {
|
|
paths = append(paths, p)
|
|
}
|
|
}
|
|
return paths
|
|
}
|
|
|
|
tests := []struct {
|
|
name string
|
|
input string
|
|
want []string
|
|
}{
|
|
{"empty fetches root", "", []string{""}},
|
|
{"single file", "README.md", []string{"README.md"}},
|
|
{"multiple files", "README.md,PATTERNS.md", []string{"README.md", "PATTERNS.md"}},
|
|
{"trims whitespace", " foo.md , bar.md ", []string{"foo.md", "bar.md"}},
|
|
{"skips empty between commas", "foo.md,,bar.md", []string{"foo.md", "bar.md"}},
|
|
{"directory path", "patterns/", []string{"patterns/"}},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
got := buildPaths(tc.input)
|
|
if len(got) != len(tc.want) {
|
|
t.Errorf("buildPaths(%q) = %v, want %v", tc.input, got, tc.want)
|
|
return
|
|
}
|
|
for i := range got {
|
|
if got[i] != tc.want[i] {
|
|
t.Errorf("buildPaths(%q)[%d] = %q, want %q", tc.input, i, got[i], tc.want[i])
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestEvaluateCIStatus(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
statuses []vcsCommitStatus
|
|
wantPassed bool
|
|
wantSubstr string
|
|
}{
|
|
{
|
|
name: "empty statuses",
|
|
statuses: nil,
|
|
wantPassed: true,
|
|
wantSubstr: "no CI statuses",
|
|
},
|
|
{
|
|
name: "all success",
|
|
statuses: []vcsCommitStatus{
|
|
{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: []vcsCommitStatus{
|
|
{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: []vcsCommitStatus{
|
|
{Status: "error", Context: "ci/lint", Description: "Lint error"},
|
|
},
|
|
wantPassed: false,
|
|
wantSubstr: "ci/lint",
|
|
},
|
|
{
|
|
name: "pending treated as not-failed",
|
|
statuses: []vcsCommitStatus{
|
|
{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: []vcsCommitStatus{
|
|
{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: []vcsCommitStatus{
|
|
{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 TestGithubAPIURL(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
input string
|
|
want string
|
|
}{
|
|
{
|
|
name: "empty string defaults to api.github.com",
|
|
input: "",
|
|
want: "https://api.github.com",
|
|
},
|
|
{
|
|
name: "github.com maps to api.github.com",
|
|
input: "https://github.com",
|
|
want: "https://api.github.com",
|
|
},
|
|
{
|
|
name: "github.com with trailing slash maps to api.github.com",
|
|
input: "https://github.com/",
|
|
want: "https://api.github.com",
|
|
},
|
|
{
|
|
name: "GHES host gets /api/v3 suffix",
|
|
input: "https://ghe.example.com",
|
|
want: "https://ghe.example.com/api/v3",
|
|
},
|
|
{
|
|
name: "GHES concur domain does not map to api.github.com",
|
|
input: "https://github.concur.com",
|
|
want: "https://github.concur.com/api/v3",
|
|
},
|
|
}
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
got := githubAPIURL(tt.input)
|
|
if got != tt.want {
|
|
t.Errorf("githubAPIURL(%q) = %q, want %q", tt.input, got, tt.want)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
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
|
|
}{
|
|
{"<!-- review-bot:sonnet --> rest", "sonnet"},
|
|
{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
|
|
{"no sentinel here", "unknown"},
|
|
{"<!-- review-bot:", "unknown"}, // prefix but no suffix
|
|
{"prefix <!-- review-bot:abc --> 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 = append(baseSubprocessArgs(),
|
|
"--reviewer-name", "invalid name",
|
|
)
|
|
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)
|
|
args := baseSubprocessArgs()
|
|
// Replace the canonical --repo value with an invalid one.
|
|
found := false
|
|
for i, a := range args {
|
|
if a == "--repo" && i+1 < len(args) {
|
|
args[i+1] = "invalidrepo"
|
|
found = true
|
|
break
|
|
}
|
|
}
|
|
if !found {
|
|
t.Fatal("baseSubprocessArgs() does not contain --repo; test is broken")
|
|
}
|
|
os.Args = args
|
|
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)
|
|
args := baseSubprocessArgs()
|
|
// Replace the canonical --pr value with a non-numeric string.
|
|
found := false
|
|
for i, a := range args {
|
|
if a == "--pr" && i+1 < len(args) {
|
|
args[i+1] = "notanumber"
|
|
found = true
|
|
break
|
|
}
|
|
}
|
|
if !found {
|
|
t.Fatal("baseSubprocessArgs() does not contain --pr; test is broken")
|
|
}
|
|
os.Args = args
|
|
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 = append(baseSubprocessArgs(),
|
|
"--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 = append(baseSubprocessArgs(),
|
|
"--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)
|
|
}
|
|
}
|
|
|
|
// baseSubprocessArgs returns the base set of required flags for subprocess tests
|
|
// that need a fully-configured main() invocation. Each test appends its own
|
|
// test-specific flags on top of this base.
|
|
//
|
|
// Using a helper here means that when the set of required flags changes, only
|
|
// this function needs updating (instead of every test that passes all flags).
|
|
func baseSubprocessArgs() []string {
|
|
return []string{
|
|
"review-bot",
|
|
"--vcs-url", "https://gitea.example.com",
|
|
"--repo", "owner/repo",
|
|
"--pr", "1",
|
|
"--reviewer-token", "tok",
|
|
"--llm-base-url", "https://api.example.com",
|
|
"--llm-api-key", "key",
|
|
"--llm-model", "gpt-4",
|
|
}
|
|
}
|
|
|
|
// cleanEnv returns environ without any GITEA/LLM/REVIEWER/VCS 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_"),
|
|
strings.HasPrefix(key, "VCS_"):
|
|
continue
|
|
default:
|
|
env = append(env, e)
|
|
}
|
|
}
|
|
return env
|
|
}
|
|
|
|
func TestFindAllOwnReviews(t *testing.T) {
|
|
reviews := []vcsReview{
|
|
{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
|
|
{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
|
|
{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
|
|
{ID: 4, Body: "~~Original review~~\n<!-- review-bot:sonnet -->\nsuperseded"},
|
|
{ID: 5, Body: "<!-- review-bot:sonnet -->\nthird review"},
|
|
}
|
|
|
|
got := findAllOwnReviews(reviews, "<!-- review-bot:sonnet -->")
|
|
if len(got) != 3 {
|
|
t.Fatalf("findAllOwnReviews() returned %d, want 3", len(got))
|
|
}
|
|
wantIDs := []int64{1, 3, 5}
|
|
for i, r := range got {
|
|
if r.ID != wantIDs[i] {
|
|
t.Errorf("got[%d].ID = %d, want %d", i, r.ID, wantIDs[i])
|
|
}
|
|
}
|
|
}
|
|
|
|
func TestShouldSkipStaleReview(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
evaluatedSHA string
|
|
currentSHA string
|
|
wantSkip bool
|
|
}{
|
|
{
|
|
name: "matching SHAs",
|
|
evaluatedSHA: "abc123def456",
|
|
currentSHA: "abc123def456",
|
|
wantSkip: false,
|
|
},
|
|
{
|
|
name: "different SHAs",
|
|
evaluatedSHA: "abc123def456",
|
|
currentSHA: "xyz789abc123",
|
|
wantSkip: true,
|
|
},
|
|
{
|
|
name: "empty current SHA (re-fetch failed)",
|
|
evaluatedSHA: "abc123def456",
|
|
currentSHA: "",
|
|
wantSkip: false,
|
|
},
|
|
{
|
|
name: "both empty (edge case)",
|
|
evaluatedSHA: "",
|
|
currentSHA: "",
|
|
wantSkip: false,
|
|
},
|
|
{
|
|
name: "only current empty",
|
|
evaluatedSHA: "abc123",
|
|
currentSHA: "",
|
|
wantSkip: false,
|
|
},
|
|
}
|
|
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
got := shouldSkipStaleReview(tc.evaluatedSHA, tc.currentSHA)
|
|
if got != tc.wantSkip {
|
|
t.Errorf("shouldSkipStaleReview(%q, %q) = %v, want %v",
|
|
tc.evaluatedSHA, tc.currentSHA, got, tc.wantSkip)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// ============================================================
|
|
// Mock vcsClient for unit tests
|
|
// ============================================================
|
|
|
|
// mockVCSClient is a minimal mock of vcsClient for testing helper functions.
|
|
// Only the methods exercised by the test code need implementations; all others
|
|
// panic with a clear message to catch accidental calls.
|
|
type mockVCSClient struct {
|
|
fileContents map[string]string // key: "owner/repo/ref/path"
|
|
fileContentsErr map[string]error // key same as above → error to return
|
|
dirContents map[string][]review.ContentEntry
|
|
dirContentsErr map[string]error
|
|
allFiles map[string]map[string]string // key: "owner/repo/path"
|
|
allFilesErr map[string]error
|
|
}
|
|
|
|
func (m *mockVCSClient) key(owner, repo, extra string) string {
|
|
return owner + "/" + repo + "/" + extra
|
|
}
|
|
|
|
func (m *mockVCSClient) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcsPullRequest, error) {
|
|
panic("GetPullRequest not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
|
|
panic("GetPullRequestDiff not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcsChangedFile, error) {
|
|
panic("GetPullRequestFiles not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcsCommitStatus, error) {
|
|
panic("GetCommitStatuses not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
|
|
panic("GetFileContent not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) GetFileContentRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
|
|
k := m.key(owner, repo, ref+"/"+path)
|
|
if err, ok := m.fileContentsErr[k]; ok {
|
|
return "", err
|
|
}
|
|
if content, ok := m.fileContents[k]; ok {
|
|
return content, nil
|
|
}
|
|
return "", fmt.Errorf("HTTP 404: not found")
|
|
}
|
|
|
|
func (m *mockVCSClient) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
|
|
k := m.key(owner, repo, path)
|
|
if err, ok := m.dirContentsErr[k]; ok {
|
|
return nil, err
|
|
}
|
|
if entries, ok := m.dirContents[k]; ok {
|
|
return entries, nil
|
|
}
|
|
return nil, fmt.Errorf("HTTP 404: not found")
|
|
}
|
|
|
|
func (m *mockVCSClient) GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) {
|
|
k := m.key(owner, repo, path)
|
|
if err, ok := m.allFilesErr[k]; ok {
|
|
return nil, err
|
|
}
|
|
if files, ok := m.allFiles[k]; ok {
|
|
return files, nil
|
|
}
|
|
return nil, fmt.Errorf("HTTP 404: not found")
|
|
}
|
|
|
|
func (m *mockVCSClient) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []vcsReviewComment) (*vcsReview, error) {
|
|
panic("PostReview not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcsReview, error) {
|
|
panic("ListReviews not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
|
|
panic("DeleteReview not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
|
panic("GetAuthenticatedUser not implemented in mockVCSClient")
|
|
}
|
|
|
|
func (m *mockVCSClient) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error {
|
|
panic("RequestReviewer not implemented in mockVCSClient")
|
|
}
|
|
|
|
// ============================================================
|
|
// fetchFileContext tests
|
|
// ============================================================
|
|
|
|
func TestFetchFileContext_NoFiles(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{}
|
|
got := fetchFileContext(ctx, client, "owner", "repo", "main", nil)
|
|
if got != "" {
|
|
t.Errorf("expected empty string for no files, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchFileContext_SkipsRemovedFiles(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{}
|
|
files := []vcsChangedFile{
|
|
{Filename: "gone.go", Status: "removed"},
|
|
}
|
|
got := fetchFileContext(ctx, client, "owner", "repo", "main", files)
|
|
if got != "" {
|
|
t.Errorf("expected empty string for removed file, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchFileContext_FetchesModifiedFiles(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{
|
|
fileContents: map[string]string{
|
|
"owner/repo/main/foo.go": "package main\n\nfunc main() {}\n",
|
|
},
|
|
}
|
|
files := []vcsChangedFile{
|
|
{Filename: "foo.go", Status: "modified"},
|
|
}
|
|
got := fetchFileContext(ctx, client, "owner", "repo", "main", files)
|
|
if !strings.Contains(got, "--- foo.go ---") {
|
|
t.Errorf("expected file header in output, got: %q", got)
|
|
}
|
|
if !strings.Contains(got, "package main") {
|
|
t.Errorf("expected file content in output, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchFileContext_ContinuesOnError(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{
|
|
fileContents: map[string]string{
|
|
"owner/repo/main/good.go": "package good\n",
|
|
},
|
|
fileContentsErr: map[string]error{
|
|
"owner/repo/main/bad.go": fmt.Errorf("network error"),
|
|
},
|
|
}
|
|
files := []vcsChangedFile{
|
|
{Filename: "bad.go", Status: "modified"},
|
|
{Filename: "good.go", Status: "modified"},
|
|
}
|
|
got := fetchFileContext(ctx, client, "owner", "repo", "main", files)
|
|
// bad.go fails, good.go should still be included
|
|
if strings.Contains(got, "bad.go") {
|
|
t.Errorf("should not include failed file, got: %q", got)
|
|
}
|
|
if !strings.Contains(got, "good.go") {
|
|
t.Errorf("should include successful file, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchFileContext_RespectsContextCancellation(t *testing.T) {
|
|
ctx, cancel := context.WithCancel(context.Background())
|
|
cancel() // Cancel immediately
|
|
|
|
client := &mockVCSClient{
|
|
fileContents: map[string]string{
|
|
"owner/repo/main/foo.go": "package foo\n",
|
|
},
|
|
}
|
|
files := []vcsChangedFile{
|
|
{Filename: "foo.go", Status: "modified"},
|
|
}
|
|
got := fetchFileContext(ctx, client, "owner", "repo", "main", files)
|
|
// With cancelled context, the loop breaks before fetching
|
|
if got != "" {
|
|
t.Errorf("expected empty string with cancelled context, got: %q", got)
|
|
}
|
|
}
|
|
|
|
// ============================================================
|
|
// fetchPatterns tests
|
|
// ============================================================
|
|
|
|
func TestFetchPatterns_EmptyRepo(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{}
|
|
got := fetchPatterns(ctx, client, "", "")
|
|
if got != "" {
|
|
t.Errorf("expected empty string for empty patternsRepo, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchPatterns_SingleRepoAllFiles(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{
|
|
allFiles: map[string]map[string]string{
|
|
"rodin/patterns/": {
|
|
"patterns/go.md": "# Go patterns\n\nUse interfaces.",
|
|
"patterns/binary": "binary data",
|
|
},
|
|
},
|
|
}
|
|
got := fetchPatterns(ctx, client, "rodin/patterns", "")
|
|
if !strings.Contains(got, "# Go patterns") {
|
|
t.Errorf("expected markdown content, got: %q", got)
|
|
}
|
|
// Binary file should be excluded
|
|
if strings.Contains(got, "binary data") {
|
|
t.Errorf("binary file should be excluded, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchPatterns_SpecificFiles(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{
|
|
allFiles: map[string]map[string]string{
|
|
"rodin/patterns/go.md": {
|
|
"go.md": "# Go idioms\n",
|
|
},
|
|
},
|
|
}
|
|
got := fetchPatterns(ctx, client, "rodin/patterns", "go.md")
|
|
if !strings.Contains(got, "# Go idioms") {
|
|
t.Errorf("expected go idioms content, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchPatterns_SkipsInvalidRepo(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{}
|
|
// "badrepo" has no slash, should be skipped
|
|
got := fetchPatterns(ctx, client, "badrepo", "")
|
|
if got != "" {
|
|
t.Errorf("expected empty string for invalid repo format, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchPatterns_ContinuesOnFetchError(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{
|
|
allFilesErr: map[string]error{
|
|
"owner/repo/": fmt.Errorf("server error"),
|
|
},
|
|
}
|
|
// Should not panic; should return empty string
|
|
got := fetchPatterns(ctx, client, "owner/repo", "")
|
|
if got != "" {
|
|
t.Errorf("expected empty string on fetch error, got: %q", got)
|
|
}
|
|
}
|
|
|
|
func TestFetchPatterns_MultipleRepos(t *testing.T) {
|
|
ctx := context.Background()
|
|
client := &mockVCSClient{
|
|
allFiles: map[string]map[string]string{
|
|
"org/go-patterns/": {
|
|
"idioms.md": "# Go idioms\n",
|
|
},
|
|
"org/elixir-patterns/": {
|
|
"pipes.md": "# Elixir pipes\n",
|
|
},
|
|
},
|
|
}
|
|
got := fetchPatterns(ctx, client, "org/go-patterns, org/elixir-patterns", "")
|
|
if !strings.Contains(got, "# Go idioms") {
|
|
t.Errorf("expected Go idioms content, got: %q", got)
|
|
}
|
|
if !strings.Contains(got, "# Elixir pipes") {
|
|
t.Errorf("expected Elixir pipes content, got: %q", got)
|
|
}
|
|
}
|
|
|
|
// TestMainSubprocess_MissingLLMBaseURL confirms that --llm-base-url is required
|
|
// when provider=openai (the default).
|
|
func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) {
|
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
|
// Note: cannot use baseSubprocessArgs() here because --llm-base-url and
|
|
// --llm-api-key are intentionally omitted to test the missing-URL error.
|
|
os.Args = []string{"review-bot",
|
|
"--vcs-url", "https://gitea.example.com",
|
|
"--repo", "owner/repo",
|
|
"--pr", "1",
|
|
"--reviewer-token", "tok",
|
|
"--llm-model", "gpt-4",
|
|
}
|
|
main()
|
|
return
|
|
}
|
|
|
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_MissingLLMBaseURL")
|
|
cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
|
|
out, err := cmd.CombinedOutput()
|
|
if err == nil {
|
|
t.Fatal("expected non-zero exit when llm-base-url is missing")
|
|
}
|
|
if !strings.Contains(string(out), "llm-base-url") {
|
|
t.Errorf("expected error mentioning llm-base-url, got: %s", out)
|
|
}
|
|
}
|
|
|
|
// TestMainSubprocess_MissingAICoreCredentials confirms that aicore-specific credentials
|
|
// are required when provider=aicore.
|
|
func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) {
|
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
|
// Note: cannot use baseSubprocessArgs() here because aicore provider
|
|
// does not require --llm-base-url / --llm-api-key; those are omitted.
|
|
os.Args = []string{"review-bot",
|
|
"--vcs-url", "https://gitea.example.com",
|
|
"--repo", "owner/repo",
|
|
"--pr", "1",
|
|
"--reviewer-token", "tok",
|
|
"--llm-model", "gpt-4",
|
|
"--llm-provider", "aicore",
|
|
// aicore-client-id, aicore-client-secret, aicore-auth-url, aicore-api-url omitted
|
|
}
|
|
main()
|
|
return
|
|
}
|
|
|
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_MissingAICoreCredentials")
|
|
cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
|
|
out, err := cmd.CombinedOutput()
|
|
if err == nil {
|
|
t.Fatal("expected non-zero exit when aicore credentials are missing")
|
|
}
|
|
if !strings.Contains(string(out), "AI Core credentials") {
|
|
t.Errorf("expected error about AI Core credentials, got: %s", out)
|
|
}
|
|
}
|
|
|
|
// TestMainSubprocess_ConflictingPersonaFlags confirms that --persona and --persona-file
|
|
// cannot be used together.
|
|
func TestMainSubprocess_ConflictingPersonaFlags(t *testing.T) {
|
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
|
os.Args = append(baseSubprocessArgs(),
|
|
"--persona", "security",
|
|
"--persona-file", "custom.json",
|
|
)
|
|
main()
|
|
return
|
|
}
|
|
|
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_ConflictingPersonaFlags")
|
|
cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
|
|
out, err := cmd.CombinedOutput()
|
|
if err == nil {
|
|
t.Fatal("expected non-zero exit with both --persona and --persona-file set")
|
|
}
|
|
if !strings.Contains(string(out), "mutually exclusive") {
|
|
t.Errorf("expected error about mutually exclusive flags, got: %s", out)
|
|
}
|
|
}
|
|
|
|
// TestMainSubprocess_DeprecatedGiteaURLEnv confirms that GITEA_URL env var still works
|
|
// as a deprecated fallback for VCS_URL.
|
|
func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {
|
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
|
// Note: cannot use baseSubprocessArgs() here because --vcs-url must be
|
|
// omitted — this test verifies that GITEA_URL env var is picked up as a
|
|
// deprecated fallback when --vcs-url is absent.
|
|
os.Args = []string{"review-bot",
|
|
// No --vcs-url: should fall back to GITEA_URL env var
|
|
"--repo", "owner/repo",
|
|
"--pr", "1",
|
|
"--reviewer-token", "tok",
|
|
"--llm-base-url", "https://api.example.com",
|
|
"--llm-api-key", "key",
|
|
"--llm-model", "gpt-4",
|
|
}
|
|
main()
|
|
return
|
|
}
|
|
|
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_DeprecatedGiteaURLEnv")
|
|
// Inject GITEA_URL but NOT VCS_URL.
|
|
env := append(cleanEnv(),
|
|
"TEST_SUBPROCESS_MAIN=1",
|
|
"GITEA_URL=https://gitea.example.com",
|
|
)
|
|
cmd.Env = env
|
|
out, _ := cmd.CombinedOutput()
|
|
// The process will fail (no real server), but the deprecation warning must appear.
|
|
if !strings.Contains(string(out), "deprecated") {
|
|
t.Errorf("expected deprecation warning for GITEA_URL, got: %s", out)
|
|
}
|
|
}
|
|
|
|
// TestMainSubprocess_InvalidDocMapPath confirms that --doc-map with a path traversal
|
|
// attempt is rejected before any network I/O.
|
|
func TestMainSubprocess_InvalidDocMapPath(t *testing.T) {
|
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
|
os.Args = []string{"review-bot",
|
|
"--vcs-url", "https://gitea.example.com",
|
|
"--repo", "owner/repo",
|
|
"--pr", "1",
|
|
"--reviewer-token", "tok",
|
|
"--llm-base-url", "https://api.example.com",
|
|
"--llm-api-key", "key",
|
|
"--llm-model", "gpt-4",
|
|
"--doc-map", "../../../etc/passwd",
|
|
}
|
|
main()
|
|
return
|
|
}
|
|
|
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapPath")
|
|
// t.TempDir() is evaluated here in the outer process, producing a real directory
|
|
// that is passed as the GITHUB_WORKSPACE env var string to the subprocess.
|
|
cmd.Env = append(cleanEnv(),
|
|
"TEST_SUBPROCESS_MAIN=1",
|
|
"GITHUB_WORKSPACE="+t.TempDir(),
|
|
)
|
|
out, err := cmd.CombinedOutput()
|
|
if err == nil {
|
|
t.Fatal("expected non-zero exit with path traversal doc-map, got success")
|
|
}
|
|
output := string(out)
|
|
if !strings.Contains(output, "doc-map") {
|
|
t.Errorf("expected error mentioning doc-map, got: %s", output)
|
|
}
|
|
if !strings.Contains(output, "resolves outside workspace") {
|
|
t.Errorf("expected error about path traversal, got: %s", output)
|
|
}
|
|
}
|
|
|
|
// TestMainSubprocess_InvalidDocMapFile confirms that --doc-map with a nonexistent file
|
|
// is rejected before any network I/O.
|
|
func TestMainSubprocess_InvalidDocMapFile(t *testing.T) {
|
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
|
os.Args = []string{"review-bot",
|
|
"--vcs-url", "https://gitea.example.com",
|
|
"--repo", "owner/repo",
|
|
"--pr", "1",
|
|
"--reviewer-token", "tok",
|
|
"--llm-base-url", "https://api.example.com",
|
|
"--llm-api-key", "key",
|
|
"--llm-model", "gpt-4",
|
|
"--doc-map", "nonexistent.yml",
|
|
}
|
|
main()
|
|
return
|
|
}
|
|
|
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapFile")
|
|
// t.TempDir() is evaluated here in the outer process, producing a real directory
|
|
// that is passed as the GITHUB_WORKSPACE env var string to the subprocess.
|
|
cmd.Env = append(cleanEnv(),
|
|
"TEST_SUBPROCESS_MAIN=1",
|
|
"GITHUB_WORKSPACE="+t.TempDir(),
|
|
)
|
|
out, err := cmd.CombinedOutput()
|
|
if err == nil {
|
|
t.Fatal("expected non-zero exit with nonexistent doc-map file, got success")
|
|
}
|
|
output := string(out)
|
|
if !strings.Contains(output, "doc-map") {
|
|
t.Errorf("expected error mentioning doc-map, got: %s", output)
|
|
}
|
|
if !strings.Contains(output, "failed to resolve") {
|
|
t.Errorf("expected error about failed resolution, got: %s", output)
|
|
}
|
|
}
|
|
|
|
// TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation confirms that
|
|
// --doc-map-trusted-ref bypasses local filesystem validation for --doc-map.
|
|
// When the trusted-ref flag is set, the doc-map value is used as a VCS API
|
|
// path; a nonexistent local file must not cause an early exit before network I/O.
|
|
func TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation(t *testing.T) {
|
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
|
os.Args = []string{"review-bot",
|
|
"--vcs-url", "https://gitea.example.com",
|
|
"--repo", "owner/repo",
|
|
"--pr", "1",
|
|
"--reviewer-token", "tok",
|
|
"--llm-base-url", "https://api.example.com",
|
|
"--llm-api-key", "key",
|
|
"--llm-model", "gpt-4",
|
|
"--doc-map", "nonexistent-local.yml",
|
|
"--doc-map-trusted-ref", "main",
|
|
}
|
|
main()
|
|
return
|
|
}
|
|
|
|
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation")
|
|
cmd.Env = append(cleanEnv(),
|
|
"TEST_SUBPROCESS_MAIN=1",
|
|
"GITHUB_WORKSPACE="+t.TempDir(),
|
|
)
|
|
out, err := cmd.CombinedOutput()
|
|
output := string(out)
|
|
|
|
// The test must fail (network I/O or VCS API failure) but must NOT
|
|
// fail with the local filesystem validation error.
|
|
// "failed to resolve" would indicate the early validateWorkspacePath ran —
|
|
// that would be the bug this test is catching.
|
|
if strings.Contains(output, "failed to resolve") {
|
|
t.Errorf("--doc-map-trusted-ref should skip local path validation, but got filesystem error: %s", output)
|
|
}
|
|
|
|
// It must still exit non-zero (real VCS call to example.com will fail).
|
|
if err == nil {
|
|
t.Fatal("expected non-zero exit when VCS API is unreachable, got success")
|
|
}
|
|
}
|