7c83365fc4
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m48s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m0s
- Create vcs/util.go with GetAllFilesInPath and BuildLineToPositionMap - Create vcs/util_test.go with comprehensive tests for both functions - Remove review.ContentEntry type, replace with vcs.ContentEntry - Remove review.GiteaClient interface, replace with vcs.FileReader - Update review/repo_persona.go to use vcs.FileReader - Update review/repo_persona_test.go to use vcs.ContentEntry - Update cmd/review-bot/main.go adapter to implement vcs.FileReader - Add Number and Base fields to vcs.PullRequest - Add CommitStatus type to vcs/types.go - Add GetFileContentAtRef to vcs.PRReader interface - Add GetCommitStatuses to vcs.PRReader interface - Add DismissReview to vcs.Reviewer interface - Add stub implementations on gitea.Client for new interface methods Closes #84, Closes #85, Closes #86
413 lines
12 KiB
Go
413 lines
12 KiB
Go
package review
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"strings"
|
|
"testing"
|
|
|
|
"gitea.weiker.me/rodin/review-bot/vcs"
|
|
)
|
|
|
|
func TestParsePersonaBytes(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
data string
|
|
source string
|
|
wantName string
|
|
wantErr string
|
|
}{
|
|
{
|
|
name: "valid yaml",
|
|
data: "name: test\nidentity: test identity\nfocus:\n - testing\n",
|
|
source: "test.yaml",
|
|
wantName: "test",
|
|
},
|
|
{
|
|
name: "missing name",
|
|
data: "identity: test\n",
|
|
source: "test.yaml",
|
|
wantErr: "name is required",
|
|
},
|
|
{
|
|
name: "invalid yaml",
|
|
data: "not: valid:\n yaml: [broken",
|
|
source: "test.yaml",
|
|
wantErr: "parse",
|
|
},
|
|
{
|
|
name: "json format by extension",
|
|
data: `{"name": "jsontest", "identity": "json identity"}`,
|
|
source: "test.json",
|
|
wantName: "jsontest",
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
p, err := ParsePersonaBytes([]byte(tt.data), tt.source)
|
|
if tt.wantErr != "" {
|
|
if err == nil {
|
|
t.Fatalf("expected error containing %q, got nil", tt.wantErr)
|
|
}
|
|
if !strings.Contains(err.Error(), tt.wantErr) {
|
|
t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
|
|
}
|
|
return
|
|
}
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if p.Name != tt.wantName {
|
|
t.Errorf("Name = %q, want %q", p.Name, tt.wantName)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// mockGiteaClient implements vcs.FileReader for testing.
|
|
type mockGiteaClient struct {
|
|
contents map[string][]vcs.ContentEntry // path -> entries
|
|
files map[string]string // path -> content
|
|
listErr error
|
|
fileErr map[string]error // path -> error
|
|
}
|
|
|
|
func (m *mockGiteaClient) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
|
|
if m.listErr != nil {
|
|
return nil, m.listErr
|
|
}
|
|
entries, ok := m.contents[path]
|
|
if !ok {
|
|
return nil, errors.New("list contents .review-bot/personas: HTTP 404: not found")
|
|
}
|
|
return entries, nil
|
|
}
|
|
|
|
func (m *mockGiteaClient) GetFileContent(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
|
|
if m.fileErr != nil {
|
|
if err, ok := m.fileErr[filepath]; ok {
|
|
return "", err
|
|
}
|
|
}
|
|
content, ok := m.files[filepath]
|
|
if !ok {
|
|
return "", errors.New("HTTP 404: file not found")
|
|
}
|
|
return content, nil
|
|
}
|
|
|
|
func TestLoadRepoPersonas(t *testing.T) {
|
|
ctx := context.Background()
|
|
|
|
t.Run("directory not found returns empty map", func(t *testing.T) {
|
|
client := &mockGiteaClient{} // No contents configured -> 404
|
|
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if personas == nil {
|
|
t.Error("expected empty map, got nil")
|
|
}
|
|
if len(personas) != 0 {
|
|
t.Errorf("expected 0 personas, got %d", len(personas))
|
|
}
|
|
})
|
|
|
|
t.Run("empty directory returns empty map", func(t *testing.T) {
|
|
client := &mockGiteaClient{
|
|
contents: map[string][]vcs.ContentEntry{
|
|
RepoPersonaPath: {},
|
|
},
|
|
}
|
|
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if len(personas) != 0 {
|
|
t.Errorf("expected 0 personas, got %d", len(personas))
|
|
}
|
|
})
|
|
|
|
t.Run("loads valid personas", func(t *testing.T) {
|
|
client := &mockGiteaClient{
|
|
contents: map[string][]vcs.ContentEntry{
|
|
RepoPersonaPath: {
|
|
{Name: "trading.yaml", Path: ".review-bot/personas/trading.yaml", Type: "file"},
|
|
{Name: "crypto.yaml", Path: ".review-bot/personas/crypto.yaml", Type: "file"},
|
|
},
|
|
},
|
|
files: map[string]string{
|
|
".review-bot/personas/trading.yaml": "name: trading\ndisplay_name: Trading Expert\nidentity: You are a trading expert.\nfocus:\n - order handling\n - risk management\n",
|
|
".review-bot/personas/crypto.yaml": "name: crypto\ndisplay_name: Crypto Expert\nidentity: You are a cryptography expert.\nfocus:\n - key management\n - encryption\n",
|
|
},
|
|
}
|
|
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if len(personas) != 2 {
|
|
t.Fatalf("expected 2 personas, got %d", len(personas))
|
|
}
|
|
if personas["trading"] == nil {
|
|
t.Error("expected trading persona")
|
|
}
|
|
if personas["crypto"] == nil {
|
|
t.Error("expected crypto persona")
|
|
}
|
|
if personas["trading"].DisplayName != "Trading Expert" {
|
|
t.Errorf("trading display name = %q, want %q", personas["trading"].DisplayName, "Trading Expert")
|
|
}
|
|
})
|
|
|
|
t.Run("skips invalid persona files", func(t *testing.T) {
|
|
client := &mockGiteaClient{
|
|
contents: map[string][]vcs.ContentEntry{
|
|
RepoPersonaPath: {
|
|
{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
|
|
{Name: "invalid.yaml", Path: ".review-bot/personas/invalid.yaml", Type: "file"},
|
|
},
|
|
},
|
|
files: map[string]string{
|
|
".review-bot/personas/valid.yaml": "name: valid\nidentity: Valid persona\n",
|
|
".review-bot/personas/invalid.yaml": "not valid yaml: [broken",
|
|
},
|
|
}
|
|
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if len(personas) != 1 {
|
|
t.Fatalf("expected 1 persona (skipped invalid), got %d", len(personas))
|
|
}
|
|
if personas["valid"] == nil {
|
|
t.Error("expected valid persona")
|
|
}
|
|
})
|
|
|
|
t.Run("skips non-yaml files", func(t *testing.T) {
|
|
client := &mockGiteaClient{
|
|
contents: map[string][]vcs.ContentEntry{
|
|
RepoPersonaPath: {
|
|
{Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"},
|
|
{Name: "README.md", Path: ".review-bot/personas/README.md", Type: "file"},
|
|
{Name: "notes.txt", Path: ".review-bot/personas/notes.txt", Type: "file"},
|
|
},
|
|
},
|
|
files: map[string]string{
|
|
".review-bot/personas/persona.yaml": "name: test\nidentity: Test persona\n",
|
|
".review-bot/personas/README.md": "# Personas\n\nPut your personas here.",
|
|
},
|
|
}
|
|
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if len(personas) != 1 {
|
|
t.Fatalf("expected 1 persona (yaml only), got %d", len(personas))
|
|
}
|
|
})
|
|
|
|
t.Run("skips subdirectories", func(t *testing.T) {
|
|
client := &mockGiteaClient{
|
|
contents: map[string][]vcs.ContentEntry{
|
|
RepoPersonaPath: {
|
|
{Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"},
|
|
{Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"},
|
|
},
|
|
},
|
|
files: map[string]string{
|
|
".review-bot/personas/persona.yaml": "name: test\nidentity: Test persona\n",
|
|
},
|
|
}
|
|
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if len(personas) != 1 {
|
|
t.Fatalf("expected 1 persona (files only), got %d", len(personas))
|
|
}
|
|
})
|
|
|
|
t.Run("propagates auth errors", func(t *testing.T) {
|
|
client := &mockGiteaClient{
|
|
listErr: errors.New("HTTP 401: unauthorized"),
|
|
}
|
|
_, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err == nil {
|
|
t.Fatal("expected error for auth failure")
|
|
}
|
|
if !strings.Contains(err.Error(), "401") {
|
|
t.Errorf("error = %q, want containing '401'", err.Error())
|
|
}
|
|
})
|
|
|
|
t.Run("skips files that fail to fetch", func(t *testing.T) {
|
|
client := &mockGiteaClient{
|
|
contents: map[string][]vcs.ContentEntry{
|
|
RepoPersonaPath: {
|
|
{Name: "good.yaml", Path: ".review-bot/personas/good.yaml", Type: "file"},
|
|
{Name: "bad.yaml", Path: ".review-bot/personas/bad.yaml", Type: "file"},
|
|
},
|
|
},
|
|
files: map[string]string{
|
|
".review-bot/personas/good.yaml": "name: good\nidentity: Good persona\n",
|
|
},
|
|
fileErr: map[string]error{
|
|
".review-bot/personas/bad.yaml": errors.New("HTTP 500: internal server error"),
|
|
},
|
|
}
|
|
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if len(personas) != 1 {
|
|
t.Fatalf("expected 1 persona (skipped failed fetch), got %d", len(personas))
|
|
}
|
|
})
|
|
|
|
t.Run("skips oversized files", func(t *testing.T) {
|
|
oversizedContent := strings.Repeat("a", MaxPersonaFileSize+1)
|
|
client := &mockGiteaClient{
|
|
contents: map[string][]vcs.ContentEntry{
|
|
RepoPersonaPath: {
|
|
{Name: "normal.yaml", Path: ".review-bot/personas/normal.yaml", Type: "file"},
|
|
{Name: "huge.yaml", Path: ".review-bot/personas/huge.yaml", Type: "file"},
|
|
},
|
|
},
|
|
files: map[string]string{
|
|
".review-bot/personas/normal.yaml": "name: normal\nidentity: Normal sized persona\n",
|
|
".review-bot/personas/huge.yaml": oversizedContent,
|
|
},
|
|
}
|
|
personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
|
|
if err != nil {
|
|
t.Fatalf("unexpected error: %v", err)
|
|
}
|
|
if len(personas) != 1 {
|
|
t.Fatalf("expected 1 persona (skipped oversized), got %d", len(personas))
|
|
}
|
|
if personas["normal"] == nil {
|
|
t.Error("expected normal persona")
|
|
}
|
|
})
|
|
}
|
|
|
|
func TestMergePersonas(t *testing.T) {
|
|
builtin := map[string]*Persona{
|
|
"security": {Name: "security", Identity: "Built-in security"},
|
|
"docs": {Name: "docs", Identity: "Built-in docs"},
|
|
}
|
|
repo := map[string]*Persona{
|
|
"security": {Name: "security", Identity: "Repo security override"},
|
|
"trading": {Name: "trading", Identity: "Repo trading"},
|
|
}
|
|
|
|
merged := MergePersonas(builtin, repo)
|
|
|
|
t.Run("repo overrides builtin on collision", func(t *testing.T) {
|
|
if merged["security"].Identity != "Repo security override" {
|
|
t.Errorf("security identity = %q, want repo override", merged["security"].Identity)
|
|
}
|
|
})
|
|
|
|
t.Run("builtin preserved when no collision", func(t *testing.T) {
|
|
if merged["docs"].Identity != "Built-in docs" {
|
|
t.Errorf("docs identity = %q, want built-in", merged["docs"].Identity)
|
|
}
|
|
})
|
|
|
|
t.Run("repo-only persona added", func(t *testing.T) {
|
|
if merged["trading"] == nil {
|
|
t.Error("expected trading persona from repo")
|
|
}
|
|
if merged["trading"].Identity != "Repo trading" {
|
|
t.Errorf("trading identity = %q, want repo", merged["trading"].Identity)
|
|
}
|
|
})
|
|
|
|
t.Run("original maps not modified", func(t *testing.T) {
|
|
if builtin["trading"] != nil {
|
|
t.Error("builtin map was modified")
|
|
}
|
|
if len(repo) != 2 {
|
|
t.Error("repo map was modified")
|
|
}
|
|
})
|
|
}
|
|
|
|
func TestGetBuiltinPersonasMap(t *testing.T) {
|
|
personas := GetBuiltinPersonasMap()
|
|
|
|
if len(personas) == 0 {
|
|
t.Fatal("expected at least one built-in persona")
|
|
}
|
|
|
|
expected := []string{"security", "architect", "docs"}
|
|
for _, name := range expected {
|
|
if personas[name] == nil {
|
|
t.Errorf("expected built-in persona %q", name)
|
|
}
|
|
}
|
|
|
|
for name, p := range personas {
|
|
if p.Name != name {
|
|
t.Errorf("persona %q has mismatched name %q", name, p.Name)
|
|
}
|
|
if p.Identity == "" {
|
|
t.Errorf("persona %q has empty identity", name)
|
|
}
|
|
}
|
|
}
|
|
|
|
func TestIsYAMLFile(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
want bool
|
|
}{
|
|
{"test.yaml", true},
|
|
{"test.yml", true},
|
|
{"test.YAML", true},
|
|
{"test.YML", true},
|
|
{"test.json", false},
|
|
{"test.md", false},
|
|
{"test.txt", false},
|
|
{"yaml", false},
|
|
{"yaml.md", false},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
if got := isYAMLFile(tt.name); got != tt.want {
|
|
t.Errorf("isYAMLFile(%q) = %v, want %v", tt.name, got, tt.want)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestIsNotFoundError(t *testing.T) {
|
|
tests := []struct {
|
|
err error
|
|
want bool
|
|
}{
|
|
{nil, false},
|
|
{errors.New("HTTP 404: not found"), true},
|
|
{errors.New("HTTP 404"), true},
|
|
{errors.New("something not found"), false},
|
|
{errors.New("HTTP 401: unauthorized"), false},
|
|
{errors.New("connection refused"), false},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
name := "nil"
|
|
if tt.err != nil {
|
|
name = tt.err.Error()
|
|
}
|
|
t.Run(name, func(t *testing.T) {
|
|
if got := isNotFoundError(tt.err); got != tt.want {
|
|
t.Errorf("isNotFoundError(%v) = %v, want %v", tt.err, got, tt.want)
|
|
}
|
|
})
|
|
}
|
|
}
|