diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0b7016a..a1cd9b3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,11 +7,28 @@ Thank you for considering contributing to Codeowners Plus! We welcome contributi ### How to Contribute #### Reporting Issues + * Search existing issues to see if your concern has already been raised. * Open a new issue if you find a bug, have a suggestion, or encounter a problem. * Provide as much detail as possible, including steps to reproduce the issue and any relevant context. +#### Running the Project Locally + +> [!Note] +> Running locally still requires real Github PRs to exist that you are testing against. + +```bash +go run main.go -token -dir ../chaturbate -pr -repo multimediallc/chaturbate -v true +``` + +#### Running the CLI tool Locally + +```bash +go run tools/cli/main.go [flags] +``` + #### Submitting Changes + * Fork the repository * Create a new branch for your changes * After making code changes, run `./scripts/covbadge.sh` to update the code coverage badge (this will be enforced in GHA checks) @@ -20,15 +37,19 @@ Thank you for considering contributing to Codeowners Plus! We welcome contributi * Open a pull request against the `main` branch of this repository ### Code Style and Standards + * Follow [Go best practices](https://go.dev/doc/effective_go). +* Follow additional rules in [styleguide](styleguide/) markdown files. * Write clear, concise, and well-documented code. * Include unit tests for any new functionality. ### Reviewing and Merging Changes + * All pull requests will be reviewed by maintainers. * Address feedback promptly and communicate if additional time is needed. * Once approved, your changes will be merged by a maintainer. ### Community + * Join discussions in the issues section to help troubleshoot or brainstorm solutions. * Respectfully engage with others to maintain a friendly and constructive environment. diff --git a/README.md b/README.md index be73534..b18862a 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-60.1%25-yellow) +![Coverage](https://img.shields.io/badge/Coverage-83.0%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e568e15..478bc60 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,47 +1,194 @@ package owners import ( + "os" + "path/filepath" "testing" ) -func TestReadCodeownersConfig(t *testing.T) { - conf, err := ReadConfig("../../test_project/") - if err != nil { - t.Errorf("Error reading codeowners config: %v", err) - return - } - if conf == nil { - t.Errorf("Error reading codeowners config") - return - } - // if *conf.MaxReviews != 2 { - // t.Errorf("Expected max reviews 2, got %d", *conf.MaxReviews) - // } - if conf.MaxReviews != nil { - t.Errorf("Expected max reviews to be nil, got %d", *conf.MaxReviews) - } - if len(conf.UnskippableReviewers) != 2 { - t.Errorf("Expected 2 unskippable reviewers, got %d", len(conf.UnskippableReviewers)) - } - if conf.UnskippableReviewers[0] != "@user1" { - t.Errorf("Expected unskippable reviewer @user1, got %s", conf.UnskippableReviewers[0]) - } - if conf.UnskippableReviewers[1] != "@user2" { - t.Errorf("Expected unskippable reviewer @user2, got %s", conf.UnskippableReviewers[0]) +func TestReadConfig(t *testing.T) { + tt := []struct { + name string + configContent string + path string + expected *Config + expectedErr bool + }{ + { + name: "default config when no file exists", + path: "nonexistent/", + expected: &Config{ + MaxReviews: nil, + MinReviews: nil, + UnskippableReviewers: []string{}, + Ignore: []string{}, + Enforcement: &Enforcement{Approval: false, FailCheck: true}, + }, + expectedErr: false, + }, + { + name: "valid config with all fields", + configContent: ` +max_reviews = 2 +min_reviews = 1 +unskippable_reviewers = ["@user1", "@user2"] +ignore = ["ignored/"] +[enforcement] +approval = true +fail_check = false +`, + path: "testdata/", + expected: &Config{ + MaxReviews: intPtr(2), + MinReviews: intPtr(1), + UnskippableReviewers: []string{"@user1", "@user2"}, + Ignore: []string{"ignored/"}, + Enforcement: &Enforcement{Approval: true, FailCheck: false}, + }, + expectedErr: false, + }, + { + name: "partial config with defaults", + configContent: ` +max_reviews = 3 +unskippable_reviewers = ["@user1"] +`, + path: "testdata/", + expected: &Config{ + MaxReviews: intPtr(3), + MinReviews: nil, + UnskippableReviewers: []string{"@user1"}, + Ignore: []string{}, + Enforcement: &Enforcement{Approval: false, FailCheck: true}, + }, + expectedErr: false, + }, + { + name: "invalid toml", + configContent: ` +max_reviews = invalid +`, + path: "testdata/", + expectedErr: true, + }, } - if len(conf.Ignore) != 1 { - t.Errorf("Expected 1 ignore dir, got %d", len(conf.Ignore)) + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Create test directory + testDir := t.TempDir() + configPath := filepath.Join(testDir, tc.path) + + // Create config file if content is provided + if tc.configContent != "" { + err := os.MkdirAll(configPath, 0755) + if err != nil { + t.Fatalf("failed to create test directory: %v", err) + } + err = os.WriteFile(filepath.Join(configPath, "codeowners.toml"), []byte(tc.configContent), 0644) + if err != nil { + t.Fatalf("failed to write test config: %v", err) + } + } + + // Test with and without trailing slash + paths := []string{configPath, configPath + "/"} + for _, path := range paths { + got, err := ReadConfig(path) + if tc.expectedErr { + if err == nil { + t.Error("expected error but got none") + } + continue + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + continue + } + + if got == nil { + t.Error("got nil config") + continue + } + + // Compare fields + if tc.expected.MaxReviews != nil { + if got.MaxReviews == nil { + t.Error("expected MaxReviews to be set") + } else if *got.MaxReviews != *tc.expected.MaxReviews { + t.Errorf("MaxReviews: expected %d, got %d", *tc.expected.MaxReviews, *got.MaxReviews) + } + } else if got.MaxReviews != nil { + t.Errorf("MaxReviews: expected nil, got %d", *got.MaxReviews) + } + + if tc.expected.MinReviews != nil { + if got.MinReviews == nil { + t.Error("expected MinReviews to be set") + } else if *got.MinReviews != *tc.expected.MinReviews { + t.Errorf("MinReviews: expected %d, got %d", *tc.expected.MinReviews, *got.MinReviews) + } + } else if got.MinReviews != nil { + t.Errorf("MinReviews: expected nil, got %d", *got.MinReviews) + } + + if !sliceEqual(got.UnskippableReviewers, tc.expected.UnskippableReviewers) { + t.Errorf("UnskippableReviewers: expected %v, got %v", tc.expected.UnskippableReviewers, got.UnskippableReviewers) + } + + if !sliceEqual(got.Ignore, tc.expected.Ignore) { + t.Errorf("Ignore: expected %v, got %v", tc.expected.Ignore, got.Ignore) + } + + if tc.expected.Enforcement != nil { + if got.Enforcement == nil { + t.Error("expected Enforcement to be set") + } else { + if got.Enforcement.Approval != tc.expected.Enforcement.Approval { + t.Errorf("Enforcement.Approval: expected %v, got %v", tc.expected.Enforcement.Approval, got.Enforcement.Approval) + } + if got.Enforcement.FailCheck != tc.expected.Enforcement.FailCheck { + t.Errorf("Enforcement.FailCheck: expected %v, got %v", tc.expected.Enforcement.FailCheck, got.Enforcement.FailCheck) + } + } + } else if got.Enforcement != nil { + t.Error("Enforcement: expected nil, got non-nil") + } + } + }) } - if conf.Ignore[0] != "ignored" { - t.Errorf("Expected ignore dir ignored, got %s", conf.Ignore[0]) +} + +func TestReadConfigFileError(t *testing.T) { + // Create a directory with no read permissions + testDir := t.TempDir() + configPath := filepath.Join(testDir, "test/") + err := os.MkdirAll(configPath, 0000) + if err != nil { + t.Fatalf("failed to create test directory: %v", err) } - if conf.Enforcement == nil { - t.Errorf("Expected enforcement to be set") + + // Try to read config from directory with no permissions + _, err = ReadConfig(configPath) + if err == nil { + t.Error("expected error when reading from directory with no permissions") } - if conf.Enforcement.Approval != false { - t.Errorf("Expected Approval to be false") +} + +// Helper functions +func intPtr(i int) *int { + return &i +} + +func sliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false } - if conf.Enforcement.FailCheck != true { - t.Errorf("Expected FailCheck to be true") + for i := range a { + if a[i] != b[i] { + return false + } } + return true } diff --git a/internal/git/diff.go b/internal/git/diff.go index a7aabb0..f8fb510 100644 --- a/internal/git/diff.go +++ b/internal/git/diff.go @@ -13,6 +13,26 @@ import ( "github.com/sourcegraph/go-diff/diff" ) +// gitCommandExecutor defines the interface for executing git commands +type gitCommandExecutor interface { + execute(command string, args ...string) ([]byte, error) +} + +// realGitExecutor implements GitCommandExecutor using os/exec +type realGitExecutor struct { + dir string +} + +func newRealGitExecutor(dir string) *realGitExecutor { + return &realGitExecutor{dir: dir} +} + +func (e *realGitExecutor) execute(command string, args ...string) ([]byte, error) { + cmd := exec.Command(command, args...) + cmd.Dir = e.dir + return cmd.CombinedOutput() +} + type Diff interface { AllChanges() []codeowners.DiffFile ChangesSince(ref string) ([]codeowners.DiffFile, error) @@ -20,13 +40,19 @@ type Diff interface { } type GitDiff struct { - context DiffContext - diff []*diff.FileDiff - files []codeowners.DiffFile + context DiffContext + diff []*diff.FileDiff + files []codeowners.DiffFile + executor gitCommandExecutor } func NewDiff(context DiffContext) (Diff, error) { - gitDiff, err := getGitDiff(context) + executor := newRealGitExecutor(context.Dir) + return NewDiffWithExecutor(context, executor) +} + +func NewDiffWithExecutor(context DiffContext, executor gitCommandExecutor) (Diff, error) { + gitDiff, err := getGitDiff(context, executor) if err != nil { return nil, err } @@ -36,9 +62,10 @@ func NewDiff(context DiffContext) (Diff, error) { } return &GitDiff{ - context: context, - diff: gitDiff, - files: diffFiles, + context: context, + diff: gitDiff, + files: diffFiles, + executor: executor, }, nil } @@ -53,9 +80,9 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { Dir: gd.context.Dir, IgnoreDirs: gd.context.IgnoreDirs, } - olderDiff, err := getGitDiff(olderDiffContext) + olderDiff, err := getGitDiff(olderDiffContext, gd.executor) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get older diff: %w", err) } changesContext := changesSinceContext{ newerDiff: gd.diff, @@ -63,7 +90,7 @@ func (gd *GitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { } diffFiles, err := changesSince(changesContext) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to compute changes since: %w", err) } return diffFiles, nil } @@ -140,10 +167,8 @@ func changesSince(context changesSinceContext) ([]codeowners.DiffFile, error) { return diffFiles, nil } -func getGitDiff(data DiffContext) ([]*diff.FileDiff, error) { - cmd := exec.Command("git", "diff", "-U0", fmt.Sprintf("%s...%s", data.Base, data.Head)) - cmd.Dir = data.Dir - cmdOutput, err := cmd.CombinedOutput() +func getGitDiff(data DiffContext, executor gitCommandExecutor) ([]*diff.FileDiff, error) { + cmdOutput, err := executor.execute("git", "diff", "-U0", fmt.Sprintf("%s...%s", data.Base, data.Head)) if err != nil { return nil, fmt.Errorf("Diff Error: %s\n%s\n", err, cmdOutput) } @@ -167,6 +192,10 @@ func hunkHash(hunk *diff.Hunk) [32]byte { var lines []byte data := hunk.Body + if len(data) == 0 { + return sha256.Sum256(nil) + } + scanner := bufio.NewScanner(bytes.NewReader(data)) for scanner.Scan() { diff --git a/internal/git/diff_test.go b/internal/git/diff_test.go index 53ff826..372ac56 100644 --- a/internal/git/diff_test.go +++ b/internal/git/diff_test.go @@ -1,6 +1,7 @@ package git import ( + "errors" "io" "os" "testing" @@ -9,6 +10,26 @@ import ( "github.com/sourcegraph/go-diff/diff" ) +// mockGitExecutor implements GitCommandExecutor for testing +type mockGitExecutor struct { + output string + err error +} + +func NewMockGitExecutor(output string, err error) *mockGitExecutor { + return &mockGitExecutor{ + output: output, + err: err, + } +} + +func (e *mockGitExecutor) execute(command string, args ...string) ([]byte, error) { + if e.err != nil { + return nil, e.err + } + return []byte(e.output), nil +} + func readFile(path string) ([]byte, error) { file, err := os.Open(path) if err != nil { @@ -19,6 +40,395 @@ func readFile(path string) ([]byte, error) { return io.ReadAll(file) } +// Test fixtures +const sampleGitDiff = `diff --git a/file1.go b/file1.go +index abc..def 100644 +--- a/file1.go ++++ b/file1.go +@@ -10,0 +11 @@ func Example() { ++ fmt.Println("New line") +diff --git a/file2.go b/file2.go +index ghi..jkl 100644 +--- a/file2.go ++++ b/file2.go +@@ -20,0 +21,2 @@ func AnotherExample() { ++ fmt.Println("First new line") ++ fmt.Println("Second new line")` + +func TestNewDiff(t *testing.T) { + tt := []struct { + name string + context DiffContext + mockOutput string + mockError error + expectedErr bool + expectedFiles int + expectedHunks map[string]int // filename -> number of hunks + }{ + { + name: "successful diff", + context: DiffContext{ + Base: "main", + Head: "feature", + Dir: ".", + }, + mockOutput: sampleGitDiff, + expectedErr: false, + expectedFiles: 2, + expectedHunks: map[string]int{ + "file1.go": 1, + "file2.go": 1, + }, + }, + { + name: "git command error", + context: DiffContext{ + Base: "main", + Head: "feature", + Dir: ".", + }, + mockError: errors.New("git command failed"), + expectedErr: true, + }, + { + name: "ignore directories", + context: DiffContext{ + Base: "main", + Head: "feature", + Dir: ".", + IgnoreDirs: []string{"file1"}, + }, + mockOutput: sampleGitDiff, + expectedErr: false, + expectedFiles: 1, + expectedHunks: map[string]int{ + "file2.go": 1, + }, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Set up mock executor + executor := NewMockGitExecutor(tc.mockOutput, tc.mockError) + + // Run the test + diff, err := NewDiffWithExecutor(tc.context, executor) + + if tc.expectedErr { + if err == nil { + t.Error("expected error but got none") + } + return + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + if diff == nil { + t.Error("expected non-nil diff") + return + } + + changes := diff.AllChanges() + if len(changes) != tc.expectedFiles { + t.Errorf("expected %d files, got %d", tc.expectedFiles, len(changes)) + } + + for _, file := range changes { + expectedHunks, ok := tc.expectedHunks[file.FileName] + if !ok { + t.Errorf("unexpected file: %s", file.FileName) + continue + } + if len(file.Hunks) != expectedHunks { + t.Errorf("file %s: expected %d hunks, got %d", file.FileName, expectedHunks, len(file.Hunks)) + } + } + }) + } +} + +func TestChangesSince(t *testing.T) { + const olderDiff = `diff --git a/file1.go b/file1.go +index abc..def 100644 +--- a/file1.go ++++ b/file1.go +@@ -5,0 +6 @@ func Example() { ++ fmt.Println("Old change")` + + tt := []struct { + name string + context DiffContext + ref string + currentDiff string + olderDiff string + mockError error + expectedErr bool + expectedFiles int + expectedNewHunks map[string]int // filename -> number of new hunks + }{ + { + name: "new changes detected", + context: DiffContext{ + Base: "main", + Head: "feature", + Dir: ".", + }, + ref: "old-ref", + currentDiff: sampleGitDiff, + olderDiff: olderDiff, + expectedErr: false, + expectedFiles: 2, + expectedNewHunks: map[string]int{ + "file1.go": 1, + "file2.go": 1, + }, + }, + { + name: "error getting older diff", + context: DiffContext{ + Base: "main", + Head: "feature", + Dir: ".", + }, + ref: "old-ref", + mockError: errors.New("git command failed"), + expectedErr: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Create initial diff with current changes + executor := NewMockGitExecutor(tc.currentDiff, nil) + diff, err := NewDiffWithExecutor(tc.context, executor) + if err != nil { + t.Fatalf("failed to create initial diff: %v", err) + } + + // Set up mock executor for older diff + executor = NewMockGitExecutor(tc.olderDiff, tc.mockError) + diff.(*GitDiff).executor = executor // Update the executor in the diff + + // Run the test + changes, err := diff.ChangesSince(tc.ref) + + if tc.expectedErr { + if err == nil { + t.Error("expected error but got none") + } + return + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + + if len(changes) != tc.expectedFiles { + t.Errorf("expected %d files, got %d", tc.expectedFiles, len(changes)) + } + + for _, file := range changes { + expectedHunks, ok := tc.expectedNewHunks[file.FileName] + if !ok { + t.Errorf("unexpected file: %s", file.FileName) + continue + } + if len(file.Hunks) != expectedHunks { + t.Errorf("file %s: expected %d hunks, got %d", file.FileName, expectedHunks, len(file.Hunks)) + } + } + }) + } +} + +func TestHunkHash(t *testing.T) { + tt := []struct { + name string + hunkBody []byte + hunk2Body []byte + expectedSame bool + }{ + { + name: "same content different context", + hunkBody: []byte(`-old line ++new line + context line 1`), + hunk2Body: []byte(`-old line ++new line + different context`), + expectedSame: true, + }, + { + name: "different content", + hunkBody: []byte(`-old line ++different line + context line 1`), + hunk2Body: []byte(`-old line ++another different line + context line 1`), + expectedSame: false, + }, + { + name: "empty hunk", + hunkBody: []byte(``), + hunk2Body: []byte(``), + expectedSame: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + hunk1 := &diff.Hunk{Body: tc.hunkBody} + hunk2 := &diff.Hunk{Body: tc.hunk2Body} + + hash1 := hunkHash(hunk1) + hash2 := hunkHash(hunk2) + + if tc.expectedSame { + if hash1 != hash2 { + t.Error("hashes should be equal") + } + } else { + if hash1 == hash2 { + t.Error("hashes should be different") + } + } + }) + } +} + +func TestToDiffFiles(t *testing.T) { + tt := []struct { + name string + fileDiffs []*diff.FileDiff + expected []codeowners.DiffFile + expectedErr bool + }{ + { + name: "single file single hunk", + fileDiffs: []*diff.FileDiff{ + { + NewName: "b/file1.go", + Hunks: []*diff.Hunk{ + { + NewStartLine: 10, + NewLines: 1, + }, + }, + }, + }, + expected: []codeowners.DiffFile{ + { + FileName: "file1.go", + Hunks: []codeowners.HunkRange{ + { + Start: 10, + End: 10, + }, + }, + }, + }, + }, + { + name: "multiple files multiple hunks", + fileDiffs: []*diff.FileDiff{ + { + NewName: "b/file1.go", + Hunks: []*diff.Hunk{ + { + NewStartLine: 10, + NewLines: 2, + }, + }, + }, + { + NewName: "b/file2.go", + Hunks: []*diff.Hunk{ + { + NewStartLine: 20, + NewLines: 3, + }, + { + NewStartLine: 30, + NewLines: 1, + }, + }, + }, + }, + expected: []codeowners.DiffFile{ + { + FileName: "file1.go", + Hunks: []codeowners.HunkRange{ + { + Start: 10, + End: 11, + }, + }, + }, + { + FileName: "file2.go", + Hunks: []codeowners.HunkRange{ + { + Start: 20, + End: 22, + }, + { + Start: 30, + End: 30, + }, + }, + }, + }, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + got, err := toDiffFiles(tc.fileDiffs) + + if tc.expectedErr { + if err == nil { + t.Error("expected error but got none") + } + return + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + + if len(got) != len(tc.expected) { + t.Errorf("expected %d files, got %d", len(tc.expected), len(got)) + return + } + + for i, expectedFile := range tc.expected { + gotFile := got[i] + if gotFile.FileName != expectedFile.FileName { + t.Errorf("file %d: expected name %s, got %s", i, expectedFile.FileName, gotFile.FileName) + } + if len(gotFile.Hunks) != len(expectedFile.Hunks) { + t.Errorf("file %s: expected %d hunks, got %d", gotFile.FileName, len(expectedFile.Hunks), len(gotFile.Hunks)) + } + for j, expectedHunk := range expectedFile.Hunks { + gotHunk := gotFile.Hunks[j] + if gotHunk.Start != expectedHunk.Start { + t.Errorf("file %s, hunk %d: expected start %d, got %d", gotFile.FileName, j, expectedHunk.Start, gotHunk.Start) + } + if gotHunk.End != expectedHunk.End { + t.Errorf("file %s, hunk %d: expected end %d, got %d", gotFile.FileName, j, expectedHunk.End, gotHunk.End) + } + } + } + }) + } +} + func TestDiff(t *testing.T) { // Test case 1 diffChangesOutput, err := readFile("../../test_project/.diff_changes") diff --git a/internal/github/gh.go b/internal/github/gh.go index c4c286d..12b6c15 100644 --- a/internal/github/gh.go +++ b/internal/github/gh.go @@ -26,12 +26,35 @@ func (e UserReviewerMapNotInitError) Error() string { return "User reviewer map not initialized" } -type Client struct { +type Client interface { + SetWarningBuffer(writer io.Writer) + SetInfoBuffer(writer io.Writer) + InitPR(pr_id int) error + PR() *github.PullRequest + InitUserReviewerMap(reviewers []string) error + GetTokenUser() (string, error) + InitReviews() error + AllApprovals() ([]*CurrentApproval, error) + FindUserApproval(ghUser string) (*CurrentApproval, error) + GetCurrentReviewerApprovals() ([]*CurrentApproval, error) + GetAlreadyReviewed() ([]string, error) + GetCurrentlyRequested() ([]string, error) + DismissStaleReviews(staleApprovals []*CurrentApproval) error + RequestReviewers(reviewers []string) error + ApprovePR() error + InitComments() error + AddComment(comment string) error + IsInComments(comment string, since *time.Time) (bool, error) + IsSubstringInComments(substring string, since *time.Time) (bool, error) + CheckApprovals(fileReviewerMap map[string][]string, approvals []*CurrentApproval, originalDiff git.Diff) (approvers []string, staleApprovals []*CurrentApproval) +} + +type GHClient struct { ctx context.Context owner string repo string client *github.Client - PR *github.PullRequest + pr *github.PullRequest userReviewerMap ghUserReviewerMap comments []*github.IssueComment reviews []*github.PullRequestReview @@ -39,9 +62,9 @@ type Client struct { infoBuffer io.Writer } -func NewClient(owner, repo, token string) *Client { +func NewClient(owner, repo, token string) Client { client := github.NewClient(nil).WithAuthToken(token) - return &Client{ + return &GHClient{ context.Background(), owner, repo, @@ -55,25 +78,29 @@ func NewClient(owner, repo, token string) *Client { } } -func (gh *Client) SetWarningBuffer(writer io.Writer) { +func (gh *GHClient) PR() *github.PullRequest { + return gh.pr +} + +func (gh *GHClient) SetWarningBuffer(writer io.Writer) { gh.warningBuffer = writer } -func (gh *Client) SetInfoBuffer(writer io.Writer) { +func (gh *GHClient) SetInfoBuffer(writer io.Writer) { gh.infoBuffer = writer } -func (gh *Client) InitPR(pr_id int) error { +func (gh *GHClient) InitPR(pr_id int) error { pull, res, err := gh.client.PullRequests.Get(gh.ctx, gh.owner, gh.repo, pr_id) defer res.Body.Close() if err != nil { return err } - gh.PR = pull + gh.pr = pull return nil } -func (gh *Client) InitUserReviewerMap(reviewers []string) error { +func (gh *GHClient) InitUserReviewerMap(reviewers []string) error { teamFetch := func(org, team string) []*github.User { fmt.Fprintf(gh.infoBuffer, "Fetching team members for %s/%s\n", org, team) allUsers := make([]*github.User, 0) @@ -94,7 +121,7 @@ func (gh *Client) InitUserReviewerMap(reviewers []string) error { return nil } -func (gh *Client) GetTokenUser() (string, error) { +func (gh *GHClient) GetTokenUser() (string, error) { user, _, err := gh.client.Users.Get(gh.ctx, "") if err != nil { return "", err @@ -102,14 +129,14 @@ func (gh *Client) GetTokenUser() (string, error) { return user.GetLogin(), nil } -func (gh *Client) InitReviews() error { - if gh.PR == nil { +func (gh *GHClient) InitReviews() error { + if gh.pr == nil { return &NoPRError{} } allReviews := make([]*github.PullRequestReview, 0) listReviews := func(page int) (*github.Response, error) { listOptions := &github.ListOptions{PerPage: 100, Page: page} - reviews, res, err := gh.client.PullRequests.ListReviews(gh.ctx, gh.owner, gh.repo, gh.PR.GetNumber(), listOptions) + reviews, res, err := gh.client.PullRequests.ListReviews(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), listOptions) defer res.Body.Close() allReviews = append(allReviews, reviews...) return res, err @@ -119,7 +146,7 @@ func (gh *Client) InitReviews() error { return err } allReviews = f.Filtered(allReviews, func(review *github.PullRequestReview) bool { - return review.User.GetLogin() != gh.PR.User.GetLogin() + return review.User.GetLogin() != gh.pr.User.GetLogin() }) // use descending chronological order (default ascending) slices.Reverse(allReviews) @@ -127,7 +154,7 @@ func (gh *Client) InitReviews() error { return nil } -func (gh *Client) approvals() []*github.PullRequestReview { +func (gh *GHClient) approvals() []*github.PullRequestReview { seen := make(map[string]bool, 0) approvals := f.Filtered(gh.reviews, func(approval *github.PullRequestReview) bool { userName := approval.GetUser().GetLogin() @@ -142,8 +169,8 @@ func (gh *Client) approvals() []*github.PullRequestReview { return approvals } -func (gh *Client) AllApprovals() ([]*CurrentApproval, error) { - if gh.PR == nil { +func (gh *GHClient) AllApprovals() ([]*CurrentApproval, error) { + if gh.pr == nil { return nil, &NoPRError{} } if gh.reviews == nil { @@ -157,8 +184,8 @@ func (gh *Client) AllApprovals() ([]*CurrentApproval, error) { }), nil } -func (gh *Client) FindUserApproval(ghUser string) (*CurrentApproval, error) { - if gh.PR == nil { +func (gh *GHClient) FindUserApproval(ghUser string) (*CurrentApproval, error) { + if gh.pr == nil { return nil, &NoPRError{} } if gh.reviews == nil { @@ -177,8 +204,8 @@ func (gh *Client) FindUserApproval(ghUser string) (*CurrentApproval, error) { return &CurrentApproval{review.User.GetLogin(), review.GetID(), nil, review.GetCommitID()}, nil } -func (gh *Client) GetCurrentReviewerApprovals() ([]*CurrentApproval, error) { - if gh.PR == nil { +func (gh *GHClient) GetCurrentReviewerApprovals() ([]*CurrentApproval, error) { + if gh.pr == nil { return nil, &NoPRError{} } if gh.userReviewerMap == nil { @@ -206,8 +233,8 @@ func currentReviewerApprovalsFromReviews(approvals []*github.PullRequestReview, return filteredApprovals } -func (gh *Client) GetAlreadyReviewed() ([]string, error) { - if gh.PR == nil { +func (gh *GHClient) GetAlreadyReviewed() ([]string, error) { + if gh.pr == nil { return nil, &NoPRError{} } if gh.userReviewerMap == nil { @@ -230,21 +257,21 @@ func reviewerAlreadyReviewed(reviews []*github.PullRequestReview, userReviewerMa return slices.Collect(maps.Keys(reviewsReviewers)) } -func (gh *Client) GetCurrentlyRequested() ([]string, error) { - if gh.PR == nil { +func (gh *GHClient) GetCurrentlyRequested() ([]string, error) { + if gh.pr == nil { return nil, &NoPRError{} } if gh.userReviewerMap == nil { return nil, &UserReviewerMapNotInitError{} } - return currentlyRequested(gh.PR, gh.owner, gh.userReviewerMap), nil + return currentlyRequested(gh.pr, gh.owner, gh.userReviewerMap), nil } -func currentlyRequested(PR *github.PullRequest, owner string, userReviewerMap ghUserReviewerMap) []string { - requestedUsers := f.Map(PR.RequestedReviewers, func(user *github.User) string { +func currentlyRequested(pr *github.PullRequest, owner string, userReviewerMap ghUserReviewerMap) []string { + requestedUsers := f.Map(pr.RequestedReviewers, func(user *github.User) string { return user.GetLogin() }) - requestedTeams := f.Map(PR.RequestedTeams, func(team *github.Team) string { + requestedTeams := f.Map(pr.RequestedTeams, func(team *github.Team) string { return fmt.Sprintf("%s/%s", owner, team.GetSlug()) }) requested := slices.Concat(requestedUsers, requestedTeams) @@ -257,14 +284,14 @@ func currentlyRequested(PR *github.PullRequest, owner string, userReviewerMap gh return f.RemoveDuplicates(reviewers) } -func (gh *Client) DismissStaleReviews(staleApprovals []*CurrentApproval) error { - if gh.PR == nil { +func (gh *GHClient) DismissStaleReviews(staleApprovals []*CurrentApproval) error { + if gh.pr == nil { return &NoPRError{} } staleMessage := "Changes in owned files since last approval" for _, approval := range staleApprovals { dismissRequest := &github.PullRequestReviewDismissalRequest{Message: &staleMessage} - _, res, err := gh.client.PullRequests.DismissReview(gh.ctx, gh.owner, gh.repo, gh.PR.GetNumber(), approval.ReviewID, dismissRequest) + _, res, err := gh.client.PullRequests.DismissReview(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), approval.ReviewID, dismissRequest) defer res.Body.Close() if err != nil { return err @@ -273,8 +300,8 @@ func (gh *Client) DismissStaleReviews(staleApprovals []*CurrentApproval) error { return nil } -func (gh *Client) RequestReviewers(reviewers []string) error { - if gh.PR == nil { +func (gh *GHClient) RequestReviewers(reviewers []string) error { + if gh.pr == nil { return &NoPRError{} } if len(reviewers) == 0 { @@ -282,7 +309,7 @@ func (gh *Client) RequestReviewers(reviewers []string) error { } indvidualReviewers, teamReviewers := splitReviewers(reviewers) reviewersRequest := github.ReviewersRequest{Reviewers: indvidualReviewers, TeamReviewers: teamReviewers} - _, res, err := gh.client.PullRequests.RequestReviewers(gh.ctx, gh.owner, gh.repo, gh.PR.GetNumber(), reviewersRequest) + _, res, err := gh.client.PullRequests.RequestReviewers(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), reviewersRequest) defer res.Body.Close() return err } @@ -302,27 +329,27 @@ func splitReviewers(reviewers []string) ([]string, []string) { return indvidualReviewers, teamReviewers } -func (gh *Client) ApprovePR() error { - if gh.PR == nil { +func (gh *GHClient) ApprovePR() error { + if gh.pr == nil { return &NoPRError{} } createReviewOptions := &github.PullRequestReviewRequest{ Event: github.String("APPROVE"), Body: github.String("Codeowners reviews satisfied"), } - _, res, err := gh.client.PullRequests.CreateReview(gh.ctx, gh.owner, gh.repo, gh.PR.GetNumber(), createReviewOptions) + _, res, err := gh.client.PullRequests.CreateReview(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), createReviewOptions) defer res.Body.Close() return err } -func (gh *Client) InitComments() error { - if gh.PR == nil { +func (gh *GHClient) InitComments() error { + if gh.pr == nil { return &NoPRError{} } allComments := make([]*github.IssueComment, 0) listReviews := func(page int) (*github.Response, error) { listOptions := &github.IssueListCommentsOptions{ListOptions: github.ListOptions{PerPage: 100, Page: page}} - comments, res, err := gh.client.Issues.ListComments(gh.ctx, gh.owner, gh.repo, gh.PR.GetNumber(), listOptions) + comments, res, err := gh.client.Issues.ListComments(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), listOptions) defer res.Body.Close() allComments = append(allComments, comments...) return res, err @@ -335,20 +362,20 @@ func (gh *Client) InitComments() error { return nil } -func (gh *Client) AddComment(comment string) error { - if gh.PR == nil { +func (gh *GHClient) AddComment(comment string) error { + if gh.pr == nil { return &NoPRError{} } createCommentOptions := &github.IssueComment{ Body: &comment, } - _, res, err := gh.client.Issues.CreateComment(gh.ctx, gh.owner, gh.repo, gh.PR.GetNumber(), createCommentOptions) + _, res, err := gh.client.Issues.CreateComment(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), createCommentOptions) defer res.Body.Close() return err } -func (gh *Client) IsInComments(comment string, since *time.Time) (bool, error) { - if gh.PR == nil { +func (gh *GHClient) IsInComments(comment string, since *time.Time) (bool, error) { + if gh.pr == nil { return false, &NoPRError{} } if gh.comments == nil { @@ -367,8 +394,8 @@ func (gh *Client) IsInComments(comment string, since *time.Time) (bool, error) { return false, nil } -func (gh *Client) IsSubstringInComments(substring string, since *time.Time) (bool, error) { - if gh.PR == nil { +func (gh *GHClient) IsSubstringInComments(substring string, since *time.Time) (bool, error) { + if gh.pr == nil { return false, &NoPRError{} } if gh.comments == nil { @@ -388,7 +415,7 @@ func (gh *Client) IsSubstringInComments(substring string, since *time.Time) (boo } // Apply approver satisfaction to the owners map, and return the approvals which should be invalidated -func (gh *Client) CheckApprovals( +func (gh *GHClient) CheckApprovals( fileReviewerMap map[string][]string, approvals []*CurrentApproval, originalDiff git.Diff, diff --git a/internal/github/gh_test.go b/internal/github/gh_test.go index 6dbd47a..d1e4831 100644 --- a/internal/github/gh_test.go +++ b/internal/github/gh_test.go @@ -15,7 +15,7 @@ import ( "github.com/multimediallc/codeowners-plus/pkg/functional" ) -func setupReviews() *Client { +func setupReviews() *GHClient { reviews := []*github.PullRequestReview{ {User: &github.User{Login: github.String("reviewer1")}, State: github.String("APPROVED"), ID: github.Int64(1), CommitID: github.String("commit1")}, {User: &github.User{Login: github.String("reviewer2")}, State: github.String("REQUEST_CHANGES"), ID: github.Int64(2), CommitID: github.String("commit2")}, @@ -27,10 +27,10 @@ func setupReviews() *Client { "reviewer2": []string{"@c"}, "reviewer4": []string{"@e"}, } - gh := &Client{ + gh := &GHClient{ reviews: reviews, userReviewerMap: userReviewerMap, - PR: &github.PullRequest{Number: github.Int(1)}, + pr: &github.PullRequest{Number: github.Int(1)}, } return gh } @@ -174,8 +174,8 @@ func TestCurrentlyRequested(t *testing.T) { }, } - gh := &Client{ - PR: pr, + gh := &GHClient{ + pr: pr, owner: "org", userReviewerMap: userReviewerMap, } @@ -237,8 +237,8 @@ func TestMakeGHUserReviewerMap(t *testing.T) { } func TestIsInComments(t *testing.T) { - gh := &Client{ - PR: &github.PullRequest{Number: github.Int(1)}, + gh := &GHClient{ + pr: &github.PullRequest{Number: github.Int(1)}, comments: []*github.IssueComment{ {Body: github.String("comment1"), CreatedAt: &github.Timestamp{Time: time.Now().AddDate(0, 0, -2)}}, {Body: github.String("comment2"), CreatedAt: &github.Timestamp{Time: time.Now().AddDate(0, 0, -1)}}, @@ -247,33 +247,36 @@ func TestIsInComments(t *testing.T) { } tt := []struct { + name string string string since *time.Time found bool }{ - {"comment1", nil, true}, - {"comment2", nil, true}, - {"comment3", nil, true}, - {"comment4", nil, false}, - {"comment1", &gh.comments[1].CreatedAt.Time, false}, - {"comment2", &gh.comments[1].CreatedAt.Time, true}, - {"comment3", &gh.comments[1].CreatedAt.Time, true}, - } - - for i, tc := range tt { - found, err := gh.IsInComments(tc.string, tc.since) - if err != nil { - t.Errorf("Case %d: Unexpected error: %v", i, err) - } - if found != tc.found { - t.Errorf("Case %d: Expected %t, got %t", i, tc.found, found) - } + {name: "find comment1 with no time filter", string: "comment1", since: nil, found: true}, + {name: "find comment2 with no time filter", string: "comment2", since: nil, found: true}, + {name: "find comment3 with no time filter", string: "comment3", since: nil, found: true}, + {name: "non-existent comment", string: "comment4", since: nil, found: false}, + {name: "comment1 filtered by time", string: "comment1", since: &gh.comments[1].CreatedAt.Time, found: false}, + {name: "comment2 filtered by time", string: "comment2", since: &gh.comments[1].CreatedAt.Time, found: true}, + {name: "comment3 filtered by time", string: "comment3", since: &gh.comments[1].CreatedAt.Time, found: true}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + found, err := gh.IsInComments(tc.string, tc.since) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if found != tc.found { + t.Errorf("Expected found to be %t, got %t", tc.found, found) + } + }) } } func TestIsSubstringInComments(t *testing.T) { - gh := &Client{ - PR: &github.PullRequest{Number: github.Int(1)}, + gh := &GHClient{ + pr: &github.PullRequest{Number: github.Int(1)}, comments: []*github.IssueComment{ {Body: github.String("part1 part4"), CreatedAt: &github.Timestamp{Time: time.Now().AddDate(0, 0, -2)}}, {Body: github.String("part2 part5"), CreatedAt: &github.Timestamp{Time: time.Now().AddDate(0, 0, -1)}}, @@ -282,38 +285,44 @@ func TestIsSubstringInComments(t *testing.T) { } tt := []struct { + name string string string since *time.Time found bool }{ - {"part1", nil, true}, - {"part2", nil, true}, - {"part3", nil, true}, - {"part4", nil, true}, - {"part5", nil, true}, - {"part6", nil, true}, - {"part7", nil, false}, - {"part1", &gh.comments[1].CreatedAt.Time, false}, - {"part4", &gh.comments[1].CreatedAt.Time, false}, - {"part2", &gh.comments[1].CreatedAt.Time, true}, - {"part5", &gh.comments[1].CreatedAt.Time, true}, - {"part3", &gh.comments[1].CreatedAt.Time, true}, - {"part6", &gh.comments[1].CreatedAt.Time, true}, - } - - for i, tc := range tt { - found, err := gh.IsSubstringInComments(tc.string, tc.since) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - if found != tc.found { - t.Errorf("Case %d: Expected found to be %t, got %t", i, tc.found, found) - } + {name: "find part1 with no time filter", string: "part1", since: nil, found: true}, + {name: "find part2 with no time filter", string: "part2", since: nil, found: true}, + {name: "find part3 with no time filter", string: "part3", since: nil, found: true}, + {name: "find part4 with no time filter", string: "part4", since: nil, found: true}, + {name: "find part5 with no time filter", string: "part5", since: nil, found: true}, + {name: "find part6 with no time filter", string: "part6", since: nil, found: true}, + {name: "non-existent part", string: "part7", since: nil, found: false}, + {name: "part1 filtered by time", string: "part1", since: &gh.comments[1].CreatedAt.Time, found: false}, + {name: "part4 filtered by time", string: "part4", since: &gh.comments[1].CreatedAt.Time, found: false}, + {name: "part2 filtered by time", string: "part2", since: &gh.comments[1].CreatedAt.Time, found: true}, + {name: "part5 filtered by time", string: "part5", since: &gh.comments[1].CreatedAt.Time, found: true}, + {name: "part3 filtered by time", string: "part3", since: &gh.comments[1].CreatedAt.Time, found: true}, + {name: "part6 filtered by time", string: "part6", since: &gh.comments[1].CreatedAt.Time, found: true}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + found, err := gh.IsSubstringInComments(tc.string, tc.since) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if found != tc.found { + t.Errorf("Expected found to be %t, got %t", tc.found, found) + } + }) } } func TestNewGithubClient(t *testing.T) { - client := NewClient("owner", "repo", "token") + client, ok := NewClient("owner", "repo", "token").(*GHClient) + if !ok { + t.Fatalf("Expected client to be of type *GHClient, got %T", client) + } if client.owner != "owner" { t.Errorf("Expected owner to be owner, got %s", client.owner) } @@ -323,7 +332,7 @@ func TestNewGithubClient(t *testing.T) { if client.client == nil { t.Error("Expected client to be non-nil") } - if client.PR != nil { + if client.pr != nil { t.Error("Expected PR to be nil") } if client.userReviewerMap != nil { @@ -332,90 +341,136 @@ func TestNewGithubClient(t *testing.T) { } func TestNilPRErr(t *testing.T) { - gh := &Client{} - tt := []func() (string, error){ - func() (string, error) { - _, err := gh.GetCurrentReviewerApprovals() - return "GetCurrentApprovals", err + gh := &GHClient{} + tt := []struct { + name string + testFn func() error + }{ + { + name: "GetCurrentApprovals", + testFn: func() error { + _, err := gh.GetCurrentReviewerApprovals() + return err + }, }, - func() (string, error) { - _, err := gh.AllApprovals() - return "IsSubstringInComments", err + { + name: "AllApprovals", + testFn: func() error { + _, err := gh.AllApprovals() + return err + }, }, - func() (string, error) { - err := gh.ApprovePR() - return "ApprovePR", err + { + name: "ApprovePR", + testFn: func() error { + return gh.ApprovePR() + }, }, - func() (string, error) { - _, err := gh.FindUserApproval("user") - return "FindUserApproval", err + { + name: "FindUserApproval", + testFn: func() error { + _, err := gh.FindUserApproval("user") + return err + }, }, - func() (string, error) { - _, err := gh.GetCurrentlyRequested() - return "GetCurrentlyRequested", err + { + name: "GetCurrentlyRequested", + testFn: func() error { + _, err := gh.GetCurrentlyRequested() + return err + }, }, - func() (string, error) { - err := gh.InitReviews() - return "InitReviews", err + { + name: "InitReviews", + testFn: func() error { + return gh.InitReviews() + }, }, - func() (string, error) { - err := gh.InitComments() - return "InitComments", err + { + name: "InitComments", + testFn: func() error { + return gh.InitComments() + }, }, - func() (string, error) { - err := gh.DismissStaleReviews([]*CurrentApproval{}) - return "DismissStaleReviews", err + { + name: "DismissStaleReviews", + testFn: func() error { + return gh.DismissStaleReviews([]*CurrentApproval{}) + }, }, - func() (string, error) { - err := gh.RequestReviewers([]string{}) - return "RequestReviewers", err + { + name: "RequestReviewers", + testFn: func() error { + return gh.RequestReviewers([]string{}) + }, }, - func() (string, error) { - err := gh.AddComment("comment") - return "AddComment", err + { + name: "AddComment", + testFn: func() error { + return gh.AddComment("comment") + }, }, - func() (string, error) { - _, err := gh.IsInComments("comment", nil) - return "IsInComments", err + { + name: "IsInComments", + testFn: func() error { + _, err := gh.IsInComments("comment", nil) + return err + }, }, - func() (string, error) { - _, err := gh.IsSubstringInComments("comment", nil) - return "IsSubstringInComments", err + { + name: "IsSubstringInComments", + testFn: func() error { + _, err := gh.IsSubstringInComments("comment", nil) + return err + }, }, } for _, tc := range tt { - s, err := tc() - if err == nil { - t.Errorf("%s: Expected error for nil user reviewer map", s) - } - if _, ok := err.(*NoPRError); !ok { - t.Errorf("%s: Expected NoPRError, got %T", s, err) - } + t.Run(tc.name, func(t *testing.T) { + err := tc.testFn() + if err == nil { + t.Error("Expected error for nil PR") + } + if _, ok := err.(*NoPRError); !ok { + t.Errorf("Expected NoPRError, got %T", err) + } + }) } } func TestNilUserReviewerMapErr(t *testing.T) { - gh := &Client{ - PR: &github.PullRequest{Number: github.Int(1)}, + gh := &GHClient{ + pr: &github.PullRequest{Number: github.Int(1)}, } - tt := []func() (string, error){ - func() (string, error) { - _, err := gh.GetCurrentReviewerApprovals() - return "GetCurrentApprovals", err + tt := []struct { + name string + testFn func() error + }{ + { + name: "GetCurrentApprovals", + testFn: func() error { + _, err := gh.GetCurrentReviewerApprovals() + return err + }, }, - func() (string, error) { - _, err := gh.GetCurrentlyRequested() - return "GetCurrentlyRequested", err + { + name: "GetCurrentlyRequested", + testFn: func() error { + _, err := gh.GetCurrentlyRequested() + return err + }, }, } for _, tc := range tt { - s, err := tc() - if err == nil { - t.Errorf("%s: Expected error for nil user reviewer map", s) - } - if _, ok := err.(*UserReviewerMapNotInitError); !ok { - t.Errorf("%s: Expected NoUserReviewerMapError, got %T", s, err) - } + t.Run(tc.name, func(t *testing.T) { + err := tc.testFn() + if err == nil { + t.Error("Expected error for nil user reviewer map") + } + if _, ok := err.(*UserReviewerMapNotInitError); !ok { + t.Errorf("Expected NoUserReviewerMapError, got %T", err) + } + }) } } @@ -423,36 +478,50 @@ func TestNilReviewsErr(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(1)} + gh.pr = &github.PullRequest{Number: github.Int(1)} gh.userReviewerMap = make(ghUserReviewerMap) mux.HandleFunc("/repos/test-owner/test-repo/pulls/123/reviews", func(w http.ResponseWriter, r *http.Request) { http.Error(w, "Unauthorized", http.StatusUnauthorized) }) - tt := []func() (string, error){ - func() (string, error) { - _, err := gh.AllApprovals() - return "AllApprovals", err + tt := []struct { + name string + testFn func() error + }{ + { + name: "AllApprovals", + testFn: func() error { + _, err := gh.AllApprovals() + return err + }, }, - func() (string, error) { - _, err := gh.GetCurrentReviewerApprovals() - return "GetCurrentReviewerApprovals", err + { + name: "GetCurrentReviewerApprovals", + testFn: func() error { + _, err := gh.GetCurrentReviewerApprovals() + return err + }, }, - func() (string, error) { - _, err := gh.FindUserApproval("user") - return "FindUserApproval", err + { + name: "FindUserApproval", + testFn: func() error { + _, err := gh.FindUserApproval("user") + return err + }, }, } for _, tc := range tt { - gh.reviews = nil - s, err := tc() - if err == nil { - t.Errorf("%s: Expected error for nil reviews", s) - } - if _, ok := err.(*github.ErrorResponse); !ok { - t.Errorf("%s: Expected ErrorResponse, got %T", s, err) - } + t.Run(tc.name, func(t *testing.T) { + gh.reviews = nil + err := tc.testFn() + if err == nil { + t.Error("Expected error for nil reviews") + } + if _, ok := err.(*github.ErrorResponse); !ok { + t.Errorf("Expected ErrorResponse, got %T", err) + } + }) } } @@ -460,36 +529,46 @@ func TestNilCommentsErr(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(1)} + gh.pr = &github.PullRequest{Number: github.Int(1)} gh.userReviewerMap = make(ghUserReviewerMap) mux.HandleFunc("/repos/test-owner/test-repo/issues/123/comments", func(w http.ResponseWriter, r *http.Request) { http.Error(w, "Unauthorized", http.StatusUnauthorized) }) - tt := []func() (string, bool, error){ - func() (string, bool, error) { - found, err := gh.IsInComments("", nil) - return "IsInComments", found, err + tt := []struct { + name string + testFn func() (bool, error) + }{ + { + name: "IsInComments", + testFn: func() (bool, error) { + return gh.IsInComments("", nil) + }, }, - func() (string, bool, error) { - found, err := gh.IsSubstringInComments("", nil) - return "IsSubstringInComments", found, err + { + name: "IsSubstringInComments", + testFn: func() (bool, error) { + return gh.IsSubstringInComments("", nil) + }, }, } + for _, tc := range tt { - gh.comments = nil - s, exists, err := tc() - if err != nil { - t.Errorf("%s: Expected error for nil comments", s) - } - if exists { - t.Errorf("%s: Expected no comment found", s) - } + t.Run(tc.name, func(t *testing.T) { + gh.comments = nil + exists, err := tc.testFn() + if err != nil { + t.Errorf("Expected no error for nil comments, got: %v", err) + } + if exists { + t.Error("Expected no comment found") + } + }) } } -func mockServerAndClient(t *testing.T) (*http.ServeMux, *httptest.Server, *Client) { +func mockServerAndClient(t *testing.T) (*http.ServeMux, *httptest.Server, *GHClient) { mux := http.NewServeMux() server := httptest.NewServer(mux) client := github.NewClient(nil) @@ -498,7 +577,7 @@ func mockServerAndClient(t *testing.T) (*http.ServeMux, *httptest.Server, *Clien t.Fatalf("unexpected error: %v", err) } client.BaseURL = baseURL - gh := &Client{ + gh := &GHClient{ ctx: context.Background(), owner: "test-owner", repo: "test-repo", @@ -530,10 +609,10 @@ func TestInitPRSuccess(t *testing.T) { if err != nil { t.Errorf("unexpected error: %v", err) } - if gh.PR == nil { + if gh.pr == nil { t.Error("expected PR to be initialized, got nil") - } else if gh.PR.GetNumber() != prID { - t.Errorf("expected PR number %d, got %d", prID, gh.PR.GetNumber()) + } else if gh.pr.GetNumber() != prID { + t.Errorf("expected PR number %d, got %d", prID, gh.pr.GetNumber()) } } @@ -541,7 +620,7 @@ func TestInitPRFailure(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = nil // Reset PR + gh.pr = nil // Reset PR prID := 999 @@ -553,8 +632,8 @@ func TestInitPRFailure(t *testing.T) { if err == nil { t.Error("expected an error, got nil") } - if gh.PR != nil { - t.Errorf("expected PR to be nil, got %+v", gh.PR) + if gh.pr != nil { + t.Errorf("expected PR to be nil, got %+v", gh.pr) } } @@ -602,7 +681,7 @@ func TestInitReviewsSuccess(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} mockReviews := []*github.PullRequestReview{ {User: &github.User{Login: github.String("test")}, ID: github.Int64(1)}, {User: &github.User{Login: github.String("test")}, ID: github.Int64(2)}, @@ -629,7 +708,7 @@ func TestInitReviewsFailure(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} mux.HandleFunc("/repos/test-owner/test-repo/pulls/123/reviews", func(w http.ResponseWriter, r *http.Request) { http.Error(w, "Unauthorized", http.StatusUnauthorized) @@ -648,7 +727,7 @@ func TestDismissStaleReviewsSuccess(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} staleApprovals := []*CurrentApproval{ {ReviewID: 1}, @@ -679,7 +758,7 @@ func TestDismissStaleReviewsFailure(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} staleApprovals := []*CurrentApproval{ {ReviewID: 1}, } @@ -699,7 +778,7 @@ func TestRequestReviewersSuccess(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} reviewers := []string{"@reviewer1", "@org/team1"} @@ -734,7 +813,7 @@ func TestRequestReviewersFailure(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} reviewers := []string{"@reviewer1", "@org/team1"} @@ -753,7 +832,7 @@ func TestApprovePRSuccess(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} // Mock the GitHub API endpoint mux.HandleFunc("/repos/test-owner/test-repo/pulls/123/reviews", func(w http.ResponseWriter, r *http.Request) { @@ -786,7 +865,7 @@ func TestApprovePRFailure(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} // Mock the GitHub API to simulate an error mux.HandleFunc("/repos/test-owner/test-repo/pulls/123/reviews", func(w http.ResponseWriter, r *http.Request) { @@ -803,7 +882,7 @@ func TestInitCommentsSuccess(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} mockComments := []*github.IssueComment{ {ID: github.Int64(1), Body: github.String("Comment 1")}, @@ -831,7 +910,7 @@ func TestInitCommentsFailure(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} mux.HandleFunc("/repos/test-owner/test-repo/issues/123/comments", func(w http.ResponseWriter, r *http.Request) { http.Error(w, "Internal Server Error", http.StatusInternalServerError) @@ -847,7 +926,7 @@ func TestAddCommentSuccess(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} // Mock the GitHub API endpoint mux.HandleFunc("/repos/test-owner/test-repo/issues/123/comments", func(w http.ResponseWriter, r *http.Request) { @@ -877,7 +956,7 @@ func TestAddCommentFailure(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} // Mock the GitHub API to simulate an error mux.HandleFunc("/repos/test-owner/test-repo/issues/123/comments", func(w http.ResponseWriter, r *http.Request) { @@ -894,7 +973,7 @@ func TestInitUserReviewerMap(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() - gh.PR = &github.PullRequest{Number: github.Int(123)} + gh.pr = &github.PullRequest{Number: github.Int(123)} // Mock the GitHub API endpoint reviewers := []string{"@org1/team1", "@user1", "@org2/team2"} diff --git a/main.go b/main.go index 9cbc8a8..9db5a7d 100644 --- a/main.go +++ b/main.go @@ -8,6 +8,7 @@ import ( "slices" "strconv" "strings" + "testing" "time" "github.com/multimediallc/codeowners-plus/internal/config" @@ -17,102 +18,108 @@ import ( "github.com/multimediallc/codeowners-plus/pkg/functional" ) -func getEnv(key, fallback string) string { - if value, ok := os.LookupEnv(key); ok { - return value - } - return fallback +// AppConfig holds the application configuration +type AppConfig struct { + Token string + RepoDir string + PR int + Repo string + Verbose bool } -func ignoreError[V any, E error](res V, _ E) V { - return res +// App represents the application with its dependencies +type App struct { + config AppConfig + client gh.Client + codeowners codeowners.CodeOwners + gitDiff git.Diff + conf *owners.Config +} + +// Flags holds the command line flags +type Flags struct { + Token *string + RepoDir *string + PR *int + Repo *string + Verbose *bool } var ( + flags = &Flags{ + Token: flag.String("token", getEnv("INPUT_GITHUB-TOKEN", ""), "GitHub authentication token"), + RepoDir: flag.String("dir", getEnv("GITHUB_WORKSPACE", "/"), "Path to local Git repo"), + PR: flag.Int("pr", ignoreError(strconv.Atoi(getEnv("INPUT_PR", ""))), "Pull Request number"), + Repo: flag.String("repo", getEnv("INPUT_REPOSITORY", ""), "GitHub repo name"), + Verbose: flag.Bool("v", ignoreError(strconv.ParseBool(getEnv("INPUT_VERBOSE", "0"))), "Verbose output"), + } WarningBuffer = bytes.NewBuffer([]byte{}) InfoBuffer = bytes.NewBuffer([]byte{}) ) -var ( - gh_token = flag.String("token", getEnv("INPUT_GITHUB-TOKEN", ""), "GitHub authentication token") - repo_dir = flag.String("dir", getEnv("GITHUB_WORKSPACE", "/"), "Path to local Git repo") - pr = flag.Int("pr", ignoreError(strconv.Atoi(getEnv("INPUT_PR", ""))), "Pull Request number") - gh_repo = flag.String("repo", getEnv("INPUT_REPOSITORY", ""), "GitHub repo name") - verbose = flag.Bool("v", ignoreError(strconv.ParseBool(getEnv("INPUT_VERBOSE", "0"))), "Verbose output") -) - -// shouldFail should always be true for errors that are not recoverable -func errorAndExit(shouldFail bool, format string, args ...interface{}) { - _, err := WarningBuffer.WriteTo(os.Stderr) - if err != nil { - fmt.Fprintf(os.Stderr, "Error writing warning buffer: %v\n", err) - } - if *verbose { - _, err := InfoBuffer.WriteTo(os.Stderr) - if err != nil { - fmt.Fprintf(os.Stderr, "Error writing info buffer: %v\n", err) - } +// initFlags initializes and parses command line flags +func initFlags(flags *Flags) error { + // Only parse flags if we're not testing + if !testing.Testing() { + flag.Parse() } - fmt.Fprintf(os.Stderr, format, args...) - if shouldFail { - os.Exit(1) - } else { - os.Exit(0) - } -} -func printDebug(format string, args ...interface{}) { - if *verbose { - fmt.Fprintf(InfoBuffer, format, args...) - } -} - -func printWarning(format string, args ...interface{}) { - fmt.Fprintf(WarningBuffer, format, args...) -} - -func init() { - flag.Parse() + // Validate required flags badFlags := make([]string, 0, 4) - if *gh_token == "" { + if *flags.Token == "" { badFlags = append(badFlags, "token") } - if *pr == 0 { + if *flags.PR == 0 { badFlags = append(badFlags, "pr") } - if *gh_repo == "" { + if *flags.Repo == "" { badFlags = append(badFlags, "repo") } if len(badFlags) > 0 { - errorAndExit(true, "Required flags or environment variables not set: %s\n", badFlags) + return fmt.Errorf("Required flags or environment variables not set: %s", badFlags) } + + return nil } -func main() { - repoSplit := strings.Split(*gh_repo, "/") +// NewApp creates a new App instance with the given configuration +func NewApp(cfg AppConfig) (*App, error) { + repoSplit := strings.Split(cfg.Repo, "/") if len(repoSplit) != 2 { - errorAndExit(true, "Invalid repo name: %s\n", *gh_repo) + return nil, fmt.Errorf("invalid repo name: %s", cfg.Repo) } owner := repoSplit[0] repo := repoSplit[1] - client := gh.NewClient(owner, repo, *gh_token) + client := gh.NewClient(owner, repo, cfg.Token) + app := &App{ + config: cfg, + client: client, + } - err := client.InitPR(*pr) - if err != nil { - errorAndExit(true, "InitPR Error: %v\n", err) + return app, nil +} + +// Run executes the application logic +func (a *App) Run() (bool, string, error) { + // Initialize PR + if err := a.client.InitPR(a.config.PR); err != nil { + return false, "", fmt.Errorf("InitPR Error: %v", err) } - printDebug("PR: %d\n", client.PR.GetNumber()) + printDebug("PR: %d\n", a.client.PR().GetNumber()) - conf, err := owners.ReadConfig(*repo_dir) + // Read config + conf, err := owners.ReadConfig(a.config.RepoDir) if err != nil { printWarning("Error reading codeowners.toml - using default config\n") } + a.conf = conf + // Setup diff context diffContext := git.DiffContext{ - Base: client.PR.Base.GetSHA(), - Head: client.PR.Head.GetSHA(), - Dir: *repo_dir, + Base: a.client.PR().Base.GetSHA(), + Head: a.client.PR().Head.GetSHA(), + Dir: a.config.RepoDir, IgnoreDirs: conf.Ignore, } @@ -120,122 +127,85 @@ func main() { printDebug("Getting diff for %s...%s\n", diffContext.Base, diffContext.Head) gitDiff, err := git.NewDiff(diffContext) if err != nil { - errorAndExit(true, "NewGitDiff Error: %v\n", err) + return false, "", fmt.Errorf("NewGitDiff Error: %v", err) } + a.gitDiff = gitDiff - // Based on the diff, get the codeowner teams by traversing the directory tree upwards and map against Github users/teams - codeOwners, err := codeowners.New(*repo_dir, gitDiff.AllChanges(), WarningBuffer) + // Initialize codeowners + codeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), WarningBuffer) if err != nil { - errorAndExit(true, "NewCodeOwners Error: %v\n", err) + return false, "", fmt.Errorf("NewCodeOwners Error: %v", err) } + a.codeowners = codeOwners + + // Set author + author := fmt.Sprintf("@%s", a.client.PR().User.GetLogin()) + codeOwners.SetAuthor(author) + + // Warn about unowned files for _, uFile := range codeOwners.UnownedFiles() { printWarning("WARNING: Unowned File: %s\n", uFile) } - author := fmt.Sprintf("@%s", client.PR.User.GetLogin()) - codeOwners.SetAuthor(author) - - // Print the file owners - if *verbose { - fileRequired := codeOwners.FileRequired() - printDebug("File Reviewers:\n") - for file, reviewers := range fileRequired { - printDebug("- %s: %+v\n", file, reviewers.Flatten()) - } - fileOptional := codeOwners.FileOptional() - printDebug("File Optional:\n") - for file, reviewers := range fileOptional { - printDebug("- %s: %+v\n", file, reviewers.Flatten()) - } + // Print file owners if verbose + if a.config.Verbose { + printFileOwners(codeOwners) } - // Get all required owners - before filtering by approvals - allRequiredOwners := codeOwners.AllRequired() + // Process approvals and reviewers + return a.processApprovalsAndReviewers() +} + +func (a *App) processApprovalsAndReviewers() (bool, string, error) { + message := "" + // Get all required owners before filtering + allRequiredOwners := a.codeowners.AllRequired() allRequiredOwnerNames := allRequiredOwners.Flatten() printDebug("All Required Owners: %s\n", allRequiredOwnerNames) - // Get all optional reviewers - filter out required owners as its redundant - allOptionalReviewerNames := codeOwners.AllOptional().Flatten() + // Get optional reviewers + allOptionalReviewerNames := a.codeowners.AllOptional().Flatten() allOptionalReviewerNames = f.Filtered(allOptionalReviewerNames, func(name string) bool { return !slices.Contains(allRequiredOwnerNames, name) }) printDebug("All Optional Reviewers: %s\n", allOptionalReviewerNames) - err = client.InitUserReviewerMap(allRequiredOwnerNames) - if err != nil { - errorAndExit(true, "InitUserReviewerMap Error: %v\n", err) + // Initialize user reviewer map + if err := a.client.InitUserReviewerMap(allRequiredOwnerNames); err != nil { + return false, message, fmt.Errorf("InitUserReviewerMap Error: %v", err) } - // Get approvals from the PR and the diffs since each approval - ghApprovals, err := client.GetCurrentReviewerApprovals() + // Get current approvals + ghApprovals, err := a.client.GetCurrentReviewerApprovals() if err != nil { - errorAndExit(true, "GetCurrentApprovals Error: %v\n", err) + return false, message, fmt.Errorf("GetCurrentApprovals Error: %v", err) } printDebug("Current Approvals: %+v\n", ghApprovals) + // Process token owner approval if enabled var tokenOwnerApproval *gh.CurrentApproval - if conf.Enforcement.Approval { - // Get the token owner login - tokenOwner, err := client.GetTokenUser() + if a.conf.Enforcement.Approval { + tokenOwnerApproval, err = a.processTokenOwnerApproval() if err != nil { - printWarning("WARNING: You might be trying to use a bot as an Enforcement.Approval user," + - " but this will not work due to GitHub CODEOWNERS not allowing bots as code owners." + - " To use the Enforcement.Approval feature, the token must belong to a GitHub user account") - - conf.Enforcement.Approval = false - } else { - // Find the approval for the token owner - tokenOwnerApproval, _ = client.FindUserApproval(tokenOwner) + return false, message, err } } - // Mark reviewers as approved if there have been no changes in owned files since the approval. Otherwise, dismiss as stale. - fileReviewers := f.MapMap(codeOwners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string { return reviewers.Flatten() }) - approvers, approvalsToDismiss := client.CheckApprovals(fileReviewers, ghApprovals, gitDiff) - codeOwners.ApplyApprovals(approvers) - if len(approvalsToDismiss) > 0 { - printDebug("Dismissing Stale Approvals: %+v\n", approvalsToDismiss) - err = client.DismissStaleReviews(approvalsToDismiss) - if err != nil { - errorAndExit(true, "DismissStaleReviews Error: %v\n", err) - } - } - validApprovalCount := len(ghApprovals) - len(approvalsToDismiss) - - // Get all required owners - after filtering by approvals - unapprovedOwners := codeOwners.AllRequired() - unapprovedOwnerNames := unapprovedOwners.Flatten() - printDebug("Remaining Required Owners: %s\n", unapprovedOwnerNames) - - // Get currently requested owners - currentlyRequestedOwners, err := client.GetCurrentlyRequested() + // Process approvals and dismiss stale ones + validApprovalCount, err := a.processApprovals(ghApprovals) if err != nil { - errorAndExit(true, "GetCurrentlyRequested Error: %v\n", err) + return false, message, err } - printDebug("Currently Requested Owners: %s\n", currentlyRequestedOwners) - // Get previously requested (review submitted) owners - previousReviewers, err := client.GetAlreadyReviewed() + // Request reviews from required owners + unapprovedOwners, err := a.requestReviews() if err != nil { - errorAndExit(true, "GetAlreadyReviewed Error: %v\n", err) - } - printDebug("Already Reviewed Owners: %s\n", previousReviewers) - - // Request reviews from the required owners not already requested - filteredOwners := unapprovedOwners.FilterOut(currentlyRequestedOwners...) - filteredOwners = filteredOwners.FilterOut(previousReviewers...) - filteredOwnerNames := filteredOwners.Flatten() - if len(filteredOwners) > 0 { - printDebug("Requesting Reviews from: %s\n", filteredOwnerNames) - err = client.RequestReviewers(filteredOwnerNames) - if err != nil { - errorAndExit(true, "RequestReviewers Error: %v\n", err) - } + return false, message, err } maxReviewsMet := false - if conf.MaxReviews != nil && *conf.MaxReviews > 0 { - if validApprovalCount >= *conf.MaxReviews && len(f.Intersection(unapprovedOwners.Flatten(), conf.UnskippableReviewers)) == 0 { + if a.conf.MaxReviews != nil && *a.conf.MaxReviews > 0 { + if validApprovalCount >= *a.conf.MaxReviews && len(f.Intersection(unapprovedOwners.Flatten(), a.conf.UnskippableReviewers)) == 0 { maxReviewsMet = true } } @@ -249,31 +219,35 @@ func main() { comment += "The PR has received the max number of required reviews. No further action is required." } fiveDaysAgo := time.Now().AddDate(0, 0, -5) - found, err := client.IsInComments(comment, &fiveDaysAgo) + found, err := a.client.IsInComments(comment, &fiveDaysAgo) if err != nil { - errorAndExit(true, "IsInComments Error: %v\n", err) + return false, message, fmt.Errorf("IsInComments Error: %v\n", err) } if !found { - err = client.AddComment(comment) + err = a.client.AddComment(comment) if err != nil { - errorAndExit(true, "AddComment Error: %v\n", err) + return false, message, fmt.Errorf("AddComment Error: %v\n", err) } } } if len(allOptionalReviewerNames) > 0 { + var isInCommentsError error = nil // Add CC comment to the PR with the optional reviewers that have not already been mentioned in the PR comments viewersToPing := f.Filtered(allOptionalReviewerNames, func(name string) bool { - found, err := client.IsSubstringInComments(name, nil) + found, err := a.client.IsSubstringInComments(name, nil) if err != nil { - errorAndExit(true, "IsInComments Error: %v\n", err) + isInCommentsError = err } return !found }) + if isInCommentsError != nil { + return false, message, fmt.Errorf("IsInComments Error: %v\n", err) + } if len(viewersToPing) > 0 { comment := fmt.Sprintf("cc %s", strings.Join(viewersToPing, " ")) - err = client.AddComment(comment) + err = a.client.AddComment(comment) if err != nil { - errorAndExit(true, "AddComment Error: %v\n", err) + return false, message, fmt.Errorf("AddComment Error: %v\n", err) } } } @@ -284,42 +258,193 @@ func main() { unapprovedCommentStrings := f.Map(unapprovedOwners, func(s *codeowners.ReviewerGroup) string { return s.ToCommentString() }) - if conf.Enforcement.Approval && tokenOwnerApproval != nil { - _ = client.DismissStaleReviews([]*gh.CurrentApproval{tokenOwnerApproval}) + if a.conf.Enforcement.Approval && tokenOwnerApproval != nil { + _ = a.client.DismissStaleReviews([]*gh.CurrentApproval{tokenOwnerApproval}) } - errorAndExit( - conf.Enforcement.FailCheck, - "FAIL: Codeowners reviews not satisfied\nStill required:\n - %s\n", + message = fmt.Sprintf( + "FAIL: Codeowners reviews not satisfied\nStill required:\n - %s", strings.Join(unapprovedCommentStrings, "\n - "), ) + return false, message, nil } // Exit if there are not enough reviews - if conf.MinReviews != nil && *conf.MinReviews > 0 { - allApprovals, _ := client.AllApprovals() - approvalCount := len(allApprovals) - len(approvalsToDismiss) - if approvalCount < *conf.MinReviews { - errorAndExit(conf.Enforcement.FailCheck, "FAIL: Min Reviews not satisfied. Need %d, found %d\n", *conf.MinReviews, approvalCount) + if a.conf.MinReviews != nil && *a.conf.MinReviews > 0 { + if validApprovalCount < *a.conf.MinReviews { + message = fmt.Sprintf("FAIL: Min Reviews not satisfied. Need %d, found %d", *a.conf.MinReviews, validApprovalCount) + return false, message, nil } } - if conf.Enforcement.Approval && tokenOwnerApproval == nil { + message = "Codeowners reviews satisfied" + if a.conf.Enforcement.Approval && tokenOwnerApproval == nil { // Approve the PR since all codeowner teams have approved - err = client.ApprovePR() + err = a.client.ApprovePR() if err != nil { - errorAndExit(true, "ApprovePR Error: %v\n", err) + return true, message, fmt.Errorf("ApprovePR Error: %v\n", err) } } + return true, message, nil +} +func (a *App) processTokenOwnerApproval() (*gh.CurrentApproval, error) { + tokenOwner, err := a.client.GetTokenUser() + if err != nil { + printWarning("WARNING: You might be trying to use a bot as an Enforcement.Approval user," + + " but this will not work due to GitHub CODEOWNERS not allowing bots as code owners." + + " To use the Enforcement.Approval feature, the token must belong to a GitHub user account") + + a.conf.Enforcement.Approval = false + return nil, nil + } + + tokenOwnerApproval, _ := a.client.FindUserApproval(tokenOwner) + return tokenOwnerApproval, nil +} + +func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { + fileReviewers := f.MapMap(a.codeowners.FileRequired(), func(reviewers codeowners.ReviewerGroups) []string { return reviewers.Flatten() }) + approvers, approvalsToDismiss := a.client.CheckApprovals(fileReviewers, ghApprovals, a.gitDiff) + a.codeowners.ApplyApprovals(approvers) + + if len(approvalsToDismiss) > 0 { + printDebug("Dismissing Stale Approvals: %+v\n", approvalsToDismiss) + if err := a.client.DismissStaleReviews(approvalsToDismiss); err != nil { + return 0, fmt.Errorf("DismissStaleReviews Error: %v", err) + } + } + + return len(ghApprovals) - len(approvalsToDismiss), nil +} + +func (a *App) requestReviews() (codeowners.ReviewerGroups, error) { + unapprovedOwners := a.codeowners.AllRequired() + unapprovedOwnerNames := unapprovedOwners.Flatten() + printDebug("Remaining Required Owners: %s\n", unapprovedOwnerNames) + + currentlyRequestedOwners, err := a.client.GetCurrentlyRequested() + if err != nil { + return nil, fmt.Errorf("GetCurrentlyRequested Error: %v", err) + } + printDebug("Currently Requested Owners: %s\n", currentlyRequestedOwners) + + previousReviewers, err := a.client.GetAlreadyReviewed() + if err != nil { + return nil, fmt.Errorf("GetAlreadyReviewed Error: %v", err) + } + printDebug("Already Reviewed Owners: %s\n", previousReviewers) + + filteredOwners := unapprovedOwners.FilterOut(currentlyRequestedOwners...) + filteredOwners = filteredOwners.FilterOut(previousReviewers...) + filteredOwnerNames := filteredOwners.Flatten() + + if len(filteredOwners) > 0 { + printDebug("Requesting Reviews from: %s\n", filteredOwnerNames) + if err := a.client.RequestReviewers(filteredOwnerNames); err != nil { + return nil, fmt.Errorf("RequestReviewers Error: %v", err) + } + } + + return unapprovedOwners, nil +} + +func printFileOwners(codeOwners codeowners.CodeOwners) { + fileRequired := codeOwners.FileRequired() + printDebug("File Reviewers:\n") + for file, reviewers := range fileRequired { + printDebug("- %s: %+v\n", file, reviewers.Flatten()) + } + fileOptional := codeOwners.FileOptional() + printDebug("File Optional:\n") + for file, reviewers := range fileOptional { + printDebug("- %s: %+v\n", file, reviewers.Flatten()) + } +} + +// Helper functions +func getEnv(key, fallback string) string { + if value, ok := os.LookupEnv(key); ok { + return value + } + return fallback +} + +func ignoreError[V any, E error](res V, _ E) V { + return res +} + +func errorAndExit(shouldFail bool, format string, args ...interface{}) { + _, err := WarningBuffer.WriteTo(os.Stderr) + if err != nil { + fmt.Fprintf(os.Stderr, "Error writing warning buffer: %v\n", err) + } + if *flags.Verbose { + _, err := InfoBuffer.WriteTo(os.Stderr) + if err != nil { + fmt.Fprintf(os.Stderr, "Error writing info buffer: %v\n", err) + } + } + + fmt.Fprintf(os.Stderr, format, args...) + if testing.Testing() { + return + } + if shouldFail { + os.Exit(1) + } else { + os.Exit(0) + } +} + +func printDebug(format string, args ...interface{}) { + if *flags.Verbose { + fmt.Fprintf(InfoBuffer, format, args...) + } +} + +func printWarning(format string, args ...interface{}) { + fmt.Fprintf(WarningBuffer, format, args...) +} + +func main() { + err := initFlags(flags) + if err != nil { + errorAndExit(true, "%v\n", err) + } + + cfg := AppConfig{ + Token: *flags.Token, + RepoDir: *flags.RepoDir, + PR: *flags.PR, + Repo: *flags.Repo, + Verbose: *flags.Verbose, + } + + app, err := NewApp(cfg) + if err != nil { + errorAndExit(true, "Failed to initialize app: %v\n", err) + } + + success, message, err := app.Run() + if err != nil { + errorAndExit(true, "%v\n", err) + } _, err = WarningBuffer.WriteTo(os.Stderr) if err != nil { fmt.Fprintf(os.Stderr, "Error writing warning buffer: %v\n", err) } - if *verbose { + if *flags.Verbose { _, err = InfoBuffer.WriteTo(os.Stdout) if err != nil { fmt.Fprintf(os.Stderr, "Error writing info buffer: %v\n", err) } } - fmt.Println("Codeowners reviews satisfied") + if success { + fmt.Fprintln(os.Stdout, message) + } else { + fmt.Fprintln(os.Stderr, message) + } + if !success && app.conf.Enforcement.FailCheck { + os.Exit(1) + } } diff --git a/main_test.go b/main_test.go new file mode 100644 index 0000000..b713d64 --- /dev/null +++ b/main_test.go @@ -0,0 +1,915 @@ +package main + +import ( + "fmt" + "io" + "os" + "strings" + "testing" + "time" + + "github.com/google/go-github/v63/github" + "github.com/multimediallc/codeowners-plus/internal/config" + "github.com/multimediallc/codeowners-plus/internal/git" + "github.com/multimediallc/codeowners-plus/internal/github" + "github.com/multimediallc/codeowners-plus/pkg/codeowners" + f "github.com/multimediallc/codeowners-plus/pkg/functional" +) + +// Mock implementations +type mockGitDiff struct { + changes []string + context git.DiffContext + changesSinceError error +} + +func (m mockGitDiff) AllChanges() []codeowners.DiffFile { + files := make([]codeowners.DiffFile, 0, len(m.changes)) + for _, change := range m.changes { + files = append(files, codeowners.DiffFile{ + FileName: change, + Hunks: []codeowners.HunkRange{ + {Start: 1, End: 1}, // Mock hunk for testing + }, + }) + } + return files +} + +func (m mockGitDiff) ChangesSince(ref string) ([]codeowners.DiffFile, error) { + if m.changesSinceError != nil { + return nil, m.changesSinceError + } + return m.AllChanges(), nil +} + +func (m mockGitDiff) Context() git.DiffContext { + return m.context +} + +type mockCodeOwners struct { + requiredOwners codeowners.ReviewerGroups + optionalOwners codeowners.ReviewerGroups + fileRequiredMap map[string]codeowners.ReviewerGroups + fileOptionalMap map[string]codeowners.ReviewerGroups + appliedApprovals []string + author string + unownedFiles []string +} + +func (m *mockCodeOwners) AllRequired() codeowners.ReviewerGroups { + return m.requiredOwners.FilterOut(m.appliedApprovals...) +} + +func (m *mockCodeOwners) AllOptional() codeowners.ReviewerGroups { + return m.optionalOwners +} + +func (m *mockCodeOwners) FileRequired() map[string]codeowners.ReviewerGroups { + return m.fileRequiredMap +} + +func (m *mockCodeOwners) FileOptional() map[string]codeowners.ReviewerGroups { + return m.fileOptionalMap +} + +func (m *mockCodeOwners) ApplyApprovals(approvers []string) { + m.appliedApprovals = approvers +} + +func (m *mockCodeOwners) SetAuthor(author string) { + m.author = author + // Remove author from reviewers + for _, reviewers := range m.requiredOwners { + for i, name := range reviewers.Names { + if name == author { + reviewers.Names = append(reviewers.Names[:i], reviewers.Names[i+1:]...) + if len(reviewers.Names) == 0 { + reviewers.Approved = true + } + break + } + } + } + for _, reviewers := range m.optionalOwners { + for i, name := range reviewers.Names { + if name == author { + reviewers.Names = append(reviewers.Names[:i], reviewers.Names[i+1:]...) + if len(reviewers.Names) == 0 { + reviewers.Approved = true + } + break + } + } + } +} + +func (m *mockCodeOwners) UnownedFiles() []string { + return m.unownedFiles +} + +type mockGitHubClient struct { + pr *github.PullRequest + userReviewerMapError error + currentApprovals []*gh.CurrentApproval + currentApprovalsError error + tokenUser string + tokenUserError error + currentlyRequested []string + currentlyRequestedError error + alreadyReviewed []string + alreadyReviewedError error + dismissError error + requestReviewersError error + warningBuffer io.Writer + infoBuffer io.Writer + comments []*github.IssueComment + initPRError error + initReviewsError error + initCommentsError error + addCommentError error + approvePRError error +} + +func (m *mockGitHubClient) PR() *github.PullRequest { + return m.pr +} + +func (m *mockGitHubClient) InitUserReviewerMap(owners []string) error { + return m.userReviewerMapError +} + +func (m *mockGitHubClient) GetCurrentReviewerApprovals() ([]*gh.CurrentApproval, error) { + return m.currentApprovals, m.currentApprovalsError +} + +func (m *mockGitHubClient) GetTokenUser() (string, error) { + return m.tokenUser, m.tokenUserError +} + +func (m *mockGitHubClient) FindUserApproval(user string) (*gh.CurrentApproval, error) { + for _, approval := range m.currentApprovals { + if approval.GHLogin == user { + return approval, nil + } + } + return nil, fmt.Errorf("Not found") +} + +func (m *mockGitHubClient) GetCurrentlyRequested() ([]string, error) { + return m.currentlyRequested, m.currentlyRequestedError +} + +func (m *mockGitHubClient) GetAlreadyReviewed() ([]string, error) { + return m.alreadyReviewed, m.alreadyReviewedError +} + +func (m *mockGitHubClient) DismissStaleReviews(approvals []*gh.CurrentApproval) error { + return m.dismissError +} + +func (m *mockGitHubClient) RequestReviewers(reviewers []string) error { + return m.requestReviewersError +} + +func (m *mockGitHubClient) CheckApprovals(fileReviewers map[string][]string, approvals []*gh.CurrentApproval, diff git.Diff) ([]string, []*gh.CurrentApproval) { + // Simple mock implementation - approve all reviewers + var approvers []string + for _, reviewers := range fileReviewers { + approvers = append(approvers, reviewers...) + } + return approvers, nil +} + +func (m *mockGitHubClient) SetWarningBuffer(writer io.Writer) { + m.warningBuffer = writer +} + +func (m *mockGitHubClient) SetInfoBuffer(writer io.Writer) { + m.infoBuffer = writer +} + +func (m *mockGitHubClient) InitPR(pr_id int) error { + if m.initPRError != nil { + return m.initPRError + } + if m.pr == nil { + m.pr = &github.PullRequest{Number: github.Int(pr_id)} + } + return nil +} + +func (m *mockGitHubClient) InitReviews() error { + if m.initReviewsError != nil { + return m.initReviewsError + } + return nil +} + +func (m *mockGitHubClient) AllApprovals() ([]*gh.CurrentApproval, error) { + return m.currentApprovals, m.currentApprovalsError +} + +func (m *mockGitHubClient) InitComments() error { + if m.initCommentsError != nil { + return m.initCommentsError + } + return nil +} + +func (m *mockGitHubClient) AddComment(comment string) error { + if m.addCommentError != nil { + return m.addCommentError + } + if m.comments == nil { + m.comments = make([]*github.IssueComment, 0) + } + m.comments = append(m.comments, &github.IssueComment{Body: github.String(comment)}) + return nil +} + +func (m *mockGitHubClient) ApprovePR() error { + return m.approvePRError +} + +func (m *mockGitHubClient) IsInComments(comment string, since *time.Time) (bool, error) { + if m.comments == nil { + return false, nil + } + for _, c := range m.comments { + if since != nil && c.GetCreatedAt().Before(*since) { + continue + } + if c.GetBody() == comment { + return true, nil + } + } + return false, nil +} + +func (m *mockGitHubClient) IsSubstringInComments(substring string, since *time.Time) (bool, error) { + if m.comments == nil { + return false, nil + } + for _, c := range m.comments { + if since != nil && c.GetCreatedAt().Before(*since) { + continue + } + if strings.Contains(c.GetBody(), substring) { + return true, nil + } + } + return false, nil +} + +func init() { + // Initialize test flags with default values + flags = &Flags{ + Token: new(string), + RepoDir: new(string), + PR: new(int), + Repo: new(string), + Verbose: new(bool), + } + *flags.Token = "test-token" + *flags.RepoDir = "/test/dir" + *flags.PR = 123 + *flags.Repo = "owner/repo" + *flags.Verbose = false +} + +func TestGetEnv(t *testing.T) { + tt := []struct { + name string + key string + fallback string + setEnv bool + envValue string + expected string + }{ + { + name: "environment variable set", + key: "TEST_ENV", + fallback: "fallback", + setEnv: true, + envValue: "test_value", + expected: "test_value", + }, + { + name: "environment variable not set", + key: "TEST_ENV", + fallback: "fallback", + setEnv: false, + expected: "fallback", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + if tc.setEnv { + os.Setenv(tc.key, tc.envValue) + defer os.Unsetenv(tc.key) + } + + got := getEnv(tc.key, tc.fallback) + if got != tc.expected { + t.Errorf("expected %s, got %s", tc.expected, got) + } + }) + } +} + +func TestIgnoreError(t *testing.T) { + tt := []struct { + name string + value int + err error + expected int + }{ + { + name: "error is nil", + value: 42, + err: nil, + expected: 42, + }, + { + name: "error is not nil", + value: 42, + err: os.ErrNotExist, + expected: 42, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + got := ignoreError(tc.value, tc.err) + if got != tc.expected { + t.Errorf("expected %d, got %d", tc.expected, got) + } + }) + } +} + +func TestNewApp(t *testing.T) { + tt := []struct { + name string + config AppConfig + expectError bool + }{ + { + name: "valid config", + config: AppConfig{ + Token: "test-token", + RepoDir: "/test/dir", + PR: 123, + Repo: "owner/repo", + Verbose: true, + }, + expectError: false, + }, + { + name: "invalid repo name", + config: AppConfig{ + Token: "test-token", + RepoDir: "/test/dir", + PR: 123, + Repo: "invalid-repo", + Verbose: true, + }, + expectError: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + app, err := NewApp(tc.config) + if tc.expectError { + if err == nil { + t.Error("expected error but got none") + } + return + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + + if app == nil { + t.Error("expected app to be non-nil") + return + } + + if app.config.Token != tc.config.Token { + t.Errorf("expected token %s, got %s", tc.config.Token, app.config.Token) + } + if app.config.RepoDir != tc.config.RepoDir { + t.Errorf("expected repo dir %s, got %s", tc.config.RepoDir, app.config.RepoDir) + } + if app.config.PR != tc.config.PR { + t.Errorf("expected PR %d, got %d", tc.config.PR, app.config.PR) + } + if app.config.Repo != tc.config.Repo { + t.Errorf("expected repo %s, got %s", tc.config.Repo, app.config.Repo) + } + if app.config.Verbose != tc.config.Verbose { + t.Errorf("expected verbose %v, got %v", tc.config.Verbose, app.config.Verbose) + } + }) + } +} + +func TestPrintDebug(t *testing.T) { + tt := []struct { + name string + verbose bool + format string + args []interface{} + expected string + }{ + { + name: "verbose enabled", + verbose: true, + format: "test %s %d", + args: []interface{}{"message", 42}, + expected: "test message 42", + }, + { + name: "verbose disabled", + verbose: false, + format: "test %s %d", + args: []interface{}{"message", 42}, + expected: "", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Reset buffer + InfoBuffer.Reset() + // Set verbose flag + *flags.Verbose = tc.verbose + + printDebug(tc.format, tc.args...) + + got := InfoBuffer.String() + if got != tc.expected { + t.Errorf("expected %q, got %q", tc.expected, got) + } + }) + } +} + +func TestPrintWarning(t *testing.T) { + tt := []struct { + name string + format string + args []interface{} + expected string + }{ + { + name: "simple warning", + format: "test %s %d", + args: []interface{}{"message", 42}, + expected: "test message 42", + }, + { + name: "no args", + format: "test message", + args: []interface{}{}, + expected: "test message", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Reset buffer + WarningBuffer.Reset() + + printWarning(tc.format, tc.args...) + + got := WarningBuffer.String() + if got != tc.expected { + t.Errorf("expected %q, got %q", tc.expected, got) + } + }) + } +} + +func TestErrorAndExit(t *testing.T) { + // Note: This test can't actually verify the exit behavior + // It only verifies that the buffers are written correctly + tt := []struct { + name string + shouldFail bool + format string + args []interface{} + verbose bool + warnings string + info string + }{ + { + name: "with warnings and info", + shouldFail: true, + format: "test %s %d", + args: []interface{}{"message", 42}, + verbose: true, + warnings: "warning message\n", + info: "info message\n", + }, + { + name: "with warnings only", + shouldFail: false, + format: "test %s %d", + args: []interface{}{"message", 42}, + verbose: false, + warnings: "warning message\n", + info: "", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Reset buffers + WarningBuffer.Reset() + InfoBuffer.Reset() + + // Set up test data + WarningBuffer.WriteString(tc.warnings) + InfoBuffer.WriteString(tc.info) + *flags.Verbose = tc.verbose + + errorAndExit(tc.shouldFail, tc.format, tc.args...) + }) + } +} + +func TestInitFlags(t *testing.T) { + tokenStr := "test-token" + prInt := 123 + repoStr := "owner/repo" + emptyStr := "" + zeroInt := 0 + tt := []struct { + name string + flags *Flags + expectError bool + }{ + { + name: "all required flags set", + flags: &Flags{ + Token: &tokenStr, + PR: &prInt, + Repo: &repoStr, + }, + expectError: false, + }, + { + name: "missing token", + flags: &Flags{ + Token: &emptyStr, + PR: &prInt, + Repo: &repoStr, + }, + expectError: true, + }, + { + name: "missing PR", + flags: &Flags{ + Token: &tokenStr, + PR: &zeroInt, + Repo: &repoStr, + }, + expectError: true, + }, + { + name: "missing repo", + flags: &Flags{ + Token: &tokenStr, + PR: &prInt, + Repo: &emptyStr, + }, + expectError: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + err := initFlags(tc.flags) + if tc.expectError { + if err == nil { + t.Error("expected error but got none") + } + return + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + }) + } +} + +func TestProcessApprovalsAndReviewers(t *testing.T) { + maxReviews := 2 + minReviews := 2 + tt := []struct { + name string + requiredOwners codeowners.ReviewerGroups + optionalOwners codeowners.ReviewerGroups + fileRequiredMap map[string]codeowners.ReviewerGroups + fileOptionalMap map[string]codeowners.ReviewerGroups + currentApprovals []*gh.CurrentApproval + currentlyRequested []string + alreadyReviewed []string + tokenUser string + tokenUserError error + userReviewerMapError error + approvalsError error + requestedError error + reviewedError error + dismissError error + requestError error + enforcementApproval bool + expectedEnfApproval bool + minReviews *int + maxReviews *int + unownedFiles []string + expectError bool + expectSuccess bool + expectedApprovals []string + }{ + { + name: "successful approval process", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + }, + optionalOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + }, + fileRequiredMap: map[string]codeowners.ReviewerGroups{ + "file1.go": { + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + }, + currentApprovals: []*gh.CurrentApproval{ + {GHLogin: "@user1"}, + }, + currentlyRequested: []string{"@user2"}, + alreadyReviewed: []string{"@user1"}, + expectError: false, + expectSuccess: true, + expectedApprovals: []string{"@user1"}, + }, + { + name: "not enough approvals", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: []string{"@user2"}}, + &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + }, + currentApprovals: []*gh.CurrentApproval{ + {GHLogin: "@user1"}, + {GHLogin: "@user3"}, + }, + fileRequiredMap: map[string]codeowners.ReviewerGroups{ + "file1.go": { + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + "file3.go": { + &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + }, + }, + currentlyRequested: []string{"@user2"}, + expectError: false, + expectSuccess: false, + expectedApprovals: []string{"@user1", "@user3"}, + }, + { + name: "max reviews bypass", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + &codeowners.ReviewerGroup{Names: []string{"@user2"}}, + &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + }, + currentApprovals: []*gh.CurrentApproval{ + {GHLogin: "@user1"}, + {GHLogin: "@user3"}, + }, + fileRequiredMap: map[string]codeowners.ReviewerGroups{ + "file1.go": { + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + "file3.go": { + &codeowners.ReviewerGroup{Names: []string{"@user3"}}, + }, + }, + currentlyRequested: []string{"@user2"}, + maxReviews: &maxReviews, + expectError: false, + expectSuccess: true, + expectedApprovals: []string{"@user1", "@user3"}, + }, + { + name: "min reviews enforced", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + currentApprovals: []*gh.CurrentApproval{ + {GHLogin: "@user1"}, + }, + fileRequiredMap: map[string]codeowners.ReviewerGroups{ + "file1.go": { + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + }, + minReviews: &minReviews, + expectError: false, + expectSuccess: false, + expectedApprovals: []string{"@user1"}, + }, + { + name: "token user is reviewer", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1", "@token-user"}}, + }, + tokenUser: "@token-user", + currentApprovals: []*gh.CurrentApproval{}, + currentlyRequested: []string{"@user1"}, + expectError: false, + expectSuccess: false, + }, + { + name: "token user exists", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + expectError: false, + enforcementApproval: true, + expectedEnfApproval: true, + }, + { + name: "error getting token user", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + tokenUserError: fmt.Errorf("failed to get token user"), + expectError: false, + enforcementApproval: true, + expectedEnfApproval: false, + }, + { + name: "error initializing reviewer map", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + userReviewerMapError: fmt.Errorf("failed to init reviewer map"), + expectError: true, + }, + { + name: "multiple file reviewers", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + }, + fileRequiredMap: map[string]codeowners.ReviewerGroups{ + "file1.go": { + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + "file2.go": { + &codeowners.ReviewerGroup{Names: []string{"@user2"}}, + }, + }, + currentApprovals: []*gh.CurrentApproval{ + {GHLogin: "@user1"}, + {GHLogin: "@user2"}, + }, + expectError: false, + expectSuccess: true, + expectedApprovals: []string{"@user1", "@user2"}, + }, + { + name: "optional reviewers only", + optionalOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1", "@user2"}}, + }, + fileOptionalMap: map[string]codeowners.ReviewerGroups{ + "file1.go": { + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + }, + currentApprovals: []*gh.CurrentApproval{ + {GHLogin: "@user1"}, + }, + expectError: false, + expectSuccess: true, + expectedApprovals: []string{}, + }, + { + name: "unowned files", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + unownedFiles: []string{"unowned.go"}, + expectError: false, + }, + { + name: "error getting approvals", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + approvalsError: fmt.Errorf("failed to get approvals"), + expectError: true, + }, + { + name: "error getting currently requested", + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: []string{"@user1"}}, + }, + currentApprovals: []*gh.CurrentApproval{}, + requestedError: fmt.Errorf("failed to get requested reviewers"), + expectError: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Create mock instances + mockGH := &mockGitHubClient{ + currentApprovals: tc.currentApprovals, + currentApprovalsError: tc.approvalsError, + currentlyRequested: tc.currentlyRequested, + currentlyRequestedError: tc.requestedError, + alreadyReviewed: tc.alreadyReviewed, + alreadyReviewedError: tc.reviewedError, + dismissError: tc.dismissError, + requestReviewersError: tc.requestError, + userReviewerMapError: tc.userReviewerMapError, + tokenUser: tc.tokenUser, + tokenUserError: tc.tokenUserError, + } + + mockOwners := &mockCodeOwners{ + requiredOwners: tc.requiredOwners, + optionalOwners: tc.optionalOwners, + fileRequiredMap: tc.fileRequiredMap, + fileOptionalMap: tc.fileOptionalMap, + unownedFiles: tc.unownedFiles, + } + + app := &App{ + config: AppConfig{}, + client: mockGH, + codeowners: mockOwners, + gitDiff: mockGitDiff{ + changes: []string{"file1.go", "file2.go", "unowned.go"}, + context: git.DiffContext{ + Base: "main", + Head: "feature", + Dir: "/test/dir", + }, + }, + conf: &owners.Config{ + Enforcement: &owners.Enforcement{ + Approval: tc.enforcementApproval, + FailCheck: true, + }, + MaxReviews: tc.maxReviews, + MinReviews: tc.minReviews, + }, + } + + success, _, err := app.processApprovalsAndReviewers() + if tc.expectError { + if err == nil { + t.Error("expected error but got none") + } + return + } + if tc.expectSuccess != success { + t.Errorf("expected %t, got %t for process success", tc.expectSuccess, success) + return + } + + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + + if tc.expectedEnfApproval != app.conf.Enforcement.Approval { + t.Errorf("expected %t Enforcement.Approval, got %t", tc.expectedEnfApproval, app.conf.Enforcement.Approval) + } + + // Verify that approvals were applied correctly + if len(mockOwners.appliedApprovals) != len(tc.expectedApprovals) { + t.Log(mockOwners.appliedApprovals) + t.Errorf("expected %d approvals, got %d", len(tc.expectedApprovals), len(mockOwners.appliedApprovals)) + return + } + + if !f.SlicesItemsMatch(tc.expectedApprovals, mockOwners.appliedApprovals) { + t.Errorf("expected approvals %x, got %x", tc.expectedApprovals, mockOwners.appliedApprovals) + } + }) + } +} diff --git a/pkg/codeowners/reader_test.go b/pkg/codeowners/reader_test.go index 0ea7646..d33d4ae 100644 --- a/pkg/codeowners/reader_test.go +++ b/pkg/codeowners/reader_test.go @@ -7,6 +7,7 @@ import ( func TestRead(t *testing.T) { tt := []struct { + name string path string fallback bool fallbackName string @@ -14,34 +15,66 @@ func TestRead(t *testing.T) { additional int optional int }{ - {"../../test_project", true, "@base", 4, 2, 1}, - {"../../test_project/frontend/", true, "@frontend", 2, 1, 0}, - {"../../test_project/a.py", false, "", 0, 0, 0}, // test non-directory - {"../../test_project/empty", false, "", 0, 0, 0}, // test directory with no .codeowners file + { + name: "test project root", + path: "../../test_project", + fallback: true, + fallbackName: "@base", + owners: 4, + additional: 2, + optional: 1, + }, + { + name: "frontend directory", + path: "../../test_project/frontend/", + fallback: true, + fallbackName: "@frontend", + owners: 2, + additional: 1, + optional: 0, + }, + { + name: "non-directory file", + path: "../../test_project/a.py", + fallback: false, + owners: 0, + additional: 0, + optional: 0, + }, + { + name: "empty directory", + path: "../../test_project/empty", + fallback: false, + owners: 0, + additional: 0, + optional: 0, + }, } rgMan := NewReviewerGroupMemo() - for i, tc := range tt { - rules := Read(tc.path, rgMan, io.Discard) + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + rules := Read(tc.path, rgMan, io.Discard) - if !tc.fallback && rules.Fallback != nil { - t.Errorf("Case %d: Expected fallback to be nil, got %+v", i, rules.Fallback) - } + if !tc.fallback && rules.Fallback != nil { + t.Errorf("Expected fallback to be nil, got %+v", rules.Fallback) + } - if tc.fallback && rules.Fallback.Names[0] != tc.fallbackName { - t.Errorf("Case %d: Expected fallback to be %s, got %s", i, tc.fallbackName, rules.Fallback.Names[0]) - } + if tc.fallback && rules.Fallback.Names[0] != tc.fallbackName { + t.Errorf("Expected fallback to be %s, got %s", tc.fallbackName, rules.Fallback.Names[0]) + } - if len(rules.OwnerTests) != tc.owners { - t.Errorf("Case %d: Expected %d owner tests, got %d", i, tc.owners, len(rules.OwnerTests)) - } + if len(rules.OwnerTests) != tc.owners { + t.Errorf("Expected %d owner tests, got %d", tc.owners, len(rules.OwnerTests)) + } - if len(rules.AdditionalReviewerTests) != tc.additional { - t.Errorf("Case %d: Expected %d additional tests, got %d", i, tc.additional, len(rules.AdditionalReviewerTests)) - } + if len(rules.AdditionalReviewerTests) != tc.additional { + t.Errorf("Expected %d additional tests, got %d", tc.additional, len(rules.AdditionalReviewerTests)) + } - if len(rules.OptionalReviewerTests) != tc.optional { - t.Errorf("Case %d: Expected %d optional tests, got %d", i, tc.optional, len(rules.OptionalReviewerTests)) - } + if len(rules.OptionalReviewerTests) != tc.optional { + t.Errorf("Expected %d optional tests, got %d", tc.optional, len(rules.OptionalReviewerTests)) + } + }) } } diff --git a/pkg/codeowners/reviewers_test.go b/pkg/codeowners/reviewers_test.go index c76cc8d..909417d 100644 --- a/pkg/codeowners/reviewers_test.go +++ b/pkg/codeowners/reviewers_test.go @@ -12,32 +12,54 @@ func TestToReviewers(t *testing.T) { rgMan := NewReviewerGroupMemo() existing := rgMan.ToReviewerGroup("@a") tt := []struct { + name string input []string - output []string + expected []string checkExisting bool - failMessage string }{ - {[]string{}, []string{}, false, "Empty input should return empty output"}, - {[]string{"@a"}, []string{"@a"}, true, "Single input should return single output"}, - {[]string{"@a", "@b"}, []string{"@a", "@b"}, false, "Multiple inputs should return multiple outputs"}, - {[]string{"@b", "@a"}, []string{"@b", "@a"}, false, "Multiple inputs should maintain input order"}, + { + name: "empty input", + input: []string{}, + expected: []string{}, + checkExisting: false, + }, + { + name: "single input", + input: []string{"@a"}, + expected: []string{"@a"}, + checkExisting: true, + }, + { + name: "multiple inputs", + input: []string{"@a", "@b"}, + expected: []string{"@a", "@b"}, + checkExisting: false, + }, + { + name: "maintain order", + input: []string{"@b", "@a"}, + expected: []string{"@b", "@a"}, + checkExisting: false, + }, } - for i, tc := range tt { - r := rgMan.ToReviewerGroup(tc.input...) - if r == nil { - t.Errorf("Case %d: ToReviewers should never return nil", i) - continue - } - if r.Approved { - t.Errorf("Case %d: ToReviewers should always initialize not Approved", i) - } - if !f.SlicesItemsMatch(r.Names, tc.output) { - t.Error(tc.failMessage) - } - if tc.checkExisting && r != existing { - t.Error("ToReviewers should memoize reviewers") - } + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + r := rgMan.ToReviewerGroup(tc.input...) + if r == nil { + t.Error("ToReviewers should never return nil") + return + } + if r.Approved { + t.Error("ToReviewers should always initialize not Approved") + } + if !f.SlicesItemsMatch(r.Names, tc.expected) { + t.Error("Expected reviewer names do not match") + } + if tc.checkExisting && r != existing { + t.Error("ToReviewers should memoize reviewers") + } + }) } } diff --git a/pkg/functional/helpers_test.go b/pkg/functional/helpers_test.go index 447841e..b235b9c 100644 --- a/pkg/functional/helpers_test.go +++ b/pkg/functional/helpers_test.go @@ -7,42 +7,169 @@ import ( func TestSlicesItemsMatch(t *testing.T) { tt := []struct { - s1 []int - s2 []int - result bool - failMessage string + name string + s1 []int + s2 []int + expected bool }{ - {[]int{1, 2, 3, 4}, []int{1, 2, 3}, false, "Different size Slices should not match"}, - {[]int{1, 2, 3, 3}, []int{1, 2, 3}, false, "Different size Slices should not match even with same items"}, - {[]int{1, 2, 3}, []int{1, 2, 3}, true, "Same order same items Slices should match"}, - {[]int{1, 2, 3}, []int{2, 1, 3}, true, "Different order same items Slices should match"}, - {[]int{1, 2, 3}, []int{1, 2, 4}, false, "Different items Slices should not match"}, - {[]int{1, 2, 3}, []int{1, 1, 3}, false, "Missing items Slices should not match"}, - {[]int{1, 1, 3}, []int{1, 2, 3}, false, "Missing items Slices should not match reversed"}, + { + name: "empty slices", + s1: []int{}, + s2: []int{}, + expected: true, + }, + { + name: "nil slices", + s1: nil, + s2: nil, + expected: true, + }, + { + name: "nil and empty slice", + s1: nil, + s2: []int{}, + expected: true, + }, + { + name: "single element slices", + s1: []int{1}, + s2: []int{1}, + expected: true, + }, + { + name: "different single element slices", + s1: []int{1}, + s2: []int{2}, + expected: false, + }, + { + name: "different size slices", + s1: []int{1, 2, 3, 4}, + s2: []int{1, 2, 3}, + expected: false, + }, + { + name: "different size slices with same items", + s1: []int{1, 2, 3, 3}, + s2: []int{1, 2, 3}, + expected: false, + }, + { + name: "same order same items", + s1: []int{1, 2, 3}, + s2: []int{1, 2, 3}, + expected: true, + }, + { + name: "different order same items", + s1: []int{1, 2, 3}, + s2: []int{2, 1, 3}, + expected: true, + }, + { + name: "different items", + s1: []int{1, 2, 3}, + s2: []int{1, 2, 4}, + expected: false, + }, + { + name: "missing items", + s1: []int{1, 2, 3}, + s2: []int{1, 1, 3}, + expected: false, + }, + { + name: "missing items reversed", + s1: []int{1, 1, 3}, + s2: []int{1, 2, 3}, + expected: false, + }, } for _, tc := range tt { - if SlicesItemsMatch(tc.s1, tc.s2) != tc.result { - t.Error(tc.failMessage) - } + t.Run(tc.name, func(t *testing.T) { + if SlicesItemsMatch(tc.s1, tc.s2) != tc.expected { + if tc.expected { + t.Error("Expected match") + } else { + t.Error("Expected not to match") + } + } + }) } } func TestSet(t *testing.T) { - s := NewSet[int]() - s.Add(1) - if !s.Contains(1) { - t.Error("Set should contain Added item") - } - s.Remove(1) - if s.Contains(1) { - t.Error("Set should not contain Removed item") - } - s.Add(1) - s.Add(2) - if !SlicesItemsMatch(s.Items(), []int{1, 2}) { - t.Error("Items should return all items in the set") - } + t.Run("basic operations", func(t *testing.T) { + s := NewSet[int]() + s.Add(1) + if !s.Contains(1) { + t.Error("Set should contain Added item") + } + s.Remove(1) + if s.Contains(1) { + t.Error("Set should not contain Removed item") + } + s.Add(1) + s.Add(2) + if !SlicesItemsMatch(s.Items(), []int{1, 2}) { + t.Error("Items should return all items in the set") + } + }) + + t.Run("empty set operations", func(t *testing.T) { + s := NewSet[int]() + if len(s.Items()) != 0 { + t.Error("New set should be empty") + } + s.Remove(1) // Should not panic + if s.Contains(1) { + t.Error("Empty set should not contain any items") + } + }) + + t.Run("multiple operations", func(t *testing.T) { + s := NewSet[string]() + s.Add("a") + s.Add("b") + s.Add("a") // Duplicate add + if len(s.Items()) != 2 { + t.Error("Set should have 2 unique items") + } + s.Remove("a") + if s.Contains("a") { + t.Error("Set should not contain removed item") + } + if !s.Contains("b") { + t.Error("Set should still contain non-removed item") + } + s.Remove("b") + if len(s.Items()) != 0 { + t.Error("Set should be empty after removing all items") + } + }) + + t.Run("complex type", func(t *testing.T) { + type testStruct struct { + id int + name string + } + s := NewSet[testStruct]() + item1 := testStruct{1, "test1"} + item2 := testStruct{2, "test2"} + s.Add(item1) + s.Add(item2) + if !s.Contains(item1) { + t.Error("Set should contain added struct") + } + s.Remove(item1) + if s.Contains(item1) { + t.Error("Set should not contain removed struct") + } + if !s.Contains(item2) { + t.Error("Set should still contain non-removed struct") + } + }) } func TestMap(t *testing.T) { @@ -94,39 +221,179 @@ func TestRemoveDuplicates(t *testing.T) { func TestIntersection(t *testing.T) { tt := []struct { + name string ts1 []int ts2 []int - result []int + expected []int failMessage string }{ - {[]int{1, 2, 3}, []int{2, 3, 4}, []int{2, 3}, "Should work for simple case"}, - {[]int{1, 2, 2, 3}, []int{2, 3, 4}, []int{2, 3}, "Should not include duplicates if only in one slice"}, - {[]int{1, 2, 3}, []int{2, 2, 3, 4}, []int{2, 3}, "Should not include duplicates if only in one slice reversed"}, - {[]int{1, 2, 2, 3}, []int{2, 2, 3, 4}, []int{2, 2, 3}, "Should include duplicates if in both slices"}, + { + name: "simple case", + ts1: []int{1, 2, 3}, + ts2: []int{2, 3, 4}, + expected: []int{2, 3}, + failMessage: "Should work for simple case", + }, + { + name: "duplicates in first slice", + ts1: []int{1, 2, 2, 3}, + ts2: []int{2, 3, 4}, + expected: []int{2, 3}, + failMessage: "Should not include duplicates if only in one slice", + }, + { + name: "duplicates in second slice", + ts1: []int{1, 2, 3}, + ts2: []int{2, 2, 3, 4}, + expected: []int{2, 3}, + failMessage: "Should not include duplicates if only in one slice reversed", + }, + { + name: "duplicates in both slices", + ts1: []int{1, 2, 2, 3}, + ts2: []int{2, 2, 3, 4}, + expected: []int{2, 2, 3}, + failMessage: "Should include duplicates if in both slices", + }, } for _, tc := range tt { - if !SlicesItemsMatch(Intersection(tc.ts1, tc.ts2), tc.result) { - t.Error(tc.failMessage) - } + t.Run(tc.name, func(t *testing.T) { + if !SlicesItemsMatch(Intersection(tc.ts1, tc.ts2), tc.expected) { + t.Error(tc.failMessage) + } + }) } } func TestRemoveValue(t *testing.T) { tt := []struct { + name string + slice []int + value int + expected []int + }{ + { + name: "value in slice", + slice: []int{1, 2, 3}, + value: 2, + expected: []int{1, 3}, + }, + { + name: "value not in slice", + slice: []int{1, 2, 3}, + value: 4, + expected: []int{1, 2, 3}, + }, + { + name: "multiple occurrences", + slice: []int{1, 2, 3, 2, 2}, + value: 2, + expected: []int{1, 3}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + removeResult := RemoveValue(tc.slice, tc.value) + if !SlicesItemsMatch(removeResult, tc.expected) { + t.Errorf("Expected items %+v, got %+v", tc.expected, removeResult) + } + }) + } +} + +func TestGetZero(t *testing.T) { + t.Run("int", func(t *testing.T) { + if getZero[int]() != 0 { + t.Error("Expected zero value for int to be 0") + } + }) + + t.Run("string", func(t *testing.T) { + if getZero[string]() != "" { + t.Error("Expected zero value for string to be empty string") + } + }) + + t.Run("bool", func(t *testing.T) { + if getZero[bool]() != false { + t.Error("Expected zero value for bool to be false") + } + }) + + t.Run("struct", func(t *testing.T) { + type testStruct struct { + a int + b string + } + zero := getZero[testStruct]() + if zero.a != 0 || zero.b != "" { + t.Error("Expected zero value for struct to have zero values for all fields") + } + }) +} + +func TestFind(t *testing.T) { + tt := []struct { + name string slice []int - value int - result []int + findFunc func(int) bool + expected int + shouldFind bool failMessage string }{ - {[]int{1, 2, 3}, 2, []int{1, 3}, "Value in slice remove item"}, - {[]int{1, 2, 3}, 4, []int{1, 2, 3}, "Value not in slice should not remove any elements"}, - {[]int{1, 2, 3, 2, 2}, 2, []int{1, 3}, "Value in slice should be removed all occurrences"}, + { + name: "find existing value", + slice: []int{1, 2, 3, 4, 5}, + findFunc: func(i int) bool { + return i == 3 + }, + expected: 3, + shouldFind: true, + failMessage: "Should find value 3 in slice", + }, + { + name: "find first even number", + slice: []int{1, 2, 3, 4, 5}, + findFunc: func(i int) bool { + return i%2 == 0 + }, + expected: 2, + shouldFind: true, + failMessage: "Should find first even number (2) in slice", + }, + { + name: "value not found", + slice: []int{1, 2, 3, 4, 5}, + findFunc: func(i int) bool { + return i > 10 + }, + expected: 0, + shouldFind: false, + failMessage: "Should not find value greater than 10", + }, + { + name: "empty slice", + slice: []int{}, + findFunc: func(i int) bool { + return true + }, + expected: 0, + shouldFind: false, + failMessage: "Should not find anything in empty slice", + }, } for _, tc := range tt { - if !SlicesItemsMatch(RemoveValue(tc.slice, tc.value), tc.result) { - t.Error(tc.failMessage) - } + t.Run(tc.name, func(t *testing.T) { + found, ok := Find(tc.slice, tc.findFunc) + if ok != tc.shouldFind { + t.Errorf("%s: Expected shouldFind=%v, got %v", tc.name, tc.shouldFind, ok) + } + if found != tc.expected { + t.Errorf("%s: Expected %v, got %v", tc.name, tc.expected, found) + } + }) } } diff --git a/styleguide/packages.md b/styleguide/packages.md new file mode 100644 index 0000000..d2ac0ce --- /dev/null +++ b/styleguide/packages.md @@ -0,0 +1,5 @@ +# Packages + +Any package which is vital for use in a CLI tool to parse `.codeowners` files or work with file-reviewer mappings ought to be in `pkg`. These packages could be easily used by third party developers to build on top of Codeowners Plus. + +Packages which require external or environmental dependencies such as `git` and `github` should live in `internal`. These internal packages should never be imported in `pkg` (exposed) packages. diff --git a/styleguide/tests.md b/styleguide/tests.md new file mode 100644 index 0000000..1534e13 --- /dev/null +++ b/styleguide/tests.md @@ -0,0 +1,24 @@ +# Tests + +All unit tests should use Go standard library `testing` framework, not any testing dependencies. + +Expected results should be named `expected` or use `expected` as a prefix. + +If a function or data structure requires mocks and does not already use dependency injection, +consider adding dependency injection via interfaces to improve testability. + +For http request mocking, use standard library `httptest` to create the server. + +Use `tt` as the name for a table of tests. Use `tc` as the name for individual test cases from a `tt` test table. Use `name` as the struct field representing the test name. Example: + +```go +tt := []struct{ + name: string, + ... +} { + ... +} +for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) ...) +} +``` diff --git a/tools/cli/main_test.go b/tools/cli/main_test.go new file mode 100644 index 0000000..d885e40 --- /dev/null +++ b/tools/cli/main_test.go @@ -0,0 +1,387 @@ +package main + +import ( + "io" + "os" + "path/filepath" + "strings" + "testing" + + f "github.com/multimediallc/codeowners-plus/pkg/functional" +) + +func setupTestRepo(t *testing.T) (string, func()) { + // Create a temporary directory for the test repo + tmpDir, err := os.MkdirTemp("", "codeowners-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + + // Create .git directory + err = os.Mkdir(filepath.Join(tmpDir, ".git"), 0755) + if err != nil { + os.RemoveAll(tmpDir) + t.Fatalf("Failed to create .git dir: %v", err) + } + + // Create test files and directories + files := map[string]string{ + ".codeowners": ` +.codeowners @default-owner +**/.codeowners @default-owner + +*.go @backend-team + +# Additional reviewers +& internal/* @security-team + +# Optional reviewers +? **/*.test.js @qa-team +? **/*.test.ts @qa-team`, + "main.go": "package main", + "internal/.codeowners": ` +* @backend-team`, + "internal/util.go": "package internal", + "frontend/.codeowners": ` +* @frontend-team`, + "frontend/app.js": "// Frontend code", + "frontend/app.ts": "// TypeScript code", + "unowned/file.txt": "No owner", + "unowned/inner/file2.txt": "No owner", + "unowned2/file3.txt": "No owner", + "tests/.codeowners": ` +*.go @backend-team +*.js @frontend-team`, + "tests/test.js": "// Test file", + "tests/test.go": "// Test file", + } + + for path, content := range files { + fullPath := filepath.Join(tmpDir, path) + err := os.MkdirAll(filepath.Dir(fullPath), 0755) + if err != nil { + os.RemoveAll(tmpDir) + t.Fatalf("Failed to create directory %s: %v", filepath.Dir(fullPath), err) + } + err = os.WriteFile(fullPath, []byte(content), 0644) + if err != nil { + os.RemoveAll(tmpDir) + t.Fatalf("Failed to write file %s: %v", fullPath, err) + } + } + + cleanup := func() { + os.RemoveAll(tmpDir) + } + + return tmpDir, cleanup +} + +func TestUnownedFiles(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + tt := []struct { + name string + target string + depth int + dirsOnly bool + want []string + }{ + { + name: "all unowned files", + target: "", + depth: 0, + want: []string{"unowned/file.txt", "unowned/inner/file2.txt", "unowned2/file3.txt"}, + }, + { + name: "unowned directories", + target: "", + depth: 0, + dirsOnly: true, + want: []string{"unowned", "unowned/inner", "unowned2"}, + }, + { + name: "specific directory", + target: "unowned2", + depth: 0, + want: []string{"unowned2/file3.txt"}, + }, + { + name: "with depth limit", + target: "", + depth: 1, + want: []string{"unowned/file.txt", "unowned2/file3.txt"}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Capture stdout + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + err := unownedFiles(testRepo, tc.target, tc.depth, tc.dirsOnly) + if err != nil { + t.Errorf("unownedFiles() error = %v", err) + return + } + + // Restore stdout and get output + w.Close() + os.Stdout = oldStdout + out, _ := io.ReadAll(r) + got := strings.Split(strings.TrimSpace(string(out)), "\n") + + if len(got) != len(tc.want) { + t.Errorf("unownedFiles() got %d files, want %d", len(got), len(tc.want)) + return + } + + if !f.SlicesItemsMatch(got, tc.want) { + t.Errorf("unownedFiles() got %+v, want %+v", got, tc.want) + } + }) + } +} + +func TestFileOwner(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + tt := []struct { + name string + target string + wantErr bool + wantOwner []string + }{ + { + name: "go file", + target: "main.go", + wantErr: false, + wantOwner: []string{"@backend-team"}, + }, + { + name: "internal file", + target: "internal/util.go", + wantErr: false, + wantOwner: []string{"@backend-team", "@security-team"}, + }, + { + name: "frontend file", + target: "frontend/app.js", + wantErr: false, + wantOwner: []string{"@frontend-team"}, + }, + { + name: "test file", + target: "tests/test.js", + wantErr: false, + wantOwner: []string{"@frontend-team"}, + }, + { + name: "non-existent file", + target: "does-not-exist.go", + wantErr: true, + }, + { + name: "empty target", + target: "", + wantErr: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Capture stdout + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + err := fileOwner(testRepo, tc.target) + if (err != nil) != tc.wantErr { + t.Errorf("fileOwner() error = %v, wantErr %v", err, tc.wantErr) + return + } + + if tc.wantErr { + return + } + + // Restore stdout and get output + w.Close() + os.Stdout = oldStdout + out, _ := io.ReadAll(r) + got := strings.Split(strings.TrimSpace(string(out)), "\n") + + // Remove "Optional:" line and empty lines + got = func(lines []string) []string { + result := make([]string, 0) + for _, line := range lines { + if line != "" && line != "Optional:" { + result = append(result, line) + } + } + return result + }(got) + + if len(got) != len(tc.wantOwner) { + t.Errorf("fileOwner() got %d owners, want %d", len(got), len(tc.wantOwner)) + return + } + + if !f.SlicesItemsMatch(tc.wantOwner, got) { + t.Errorf("fileOwner() got %+v, want to contain %+v", got, tc.wantOwner) + } + }) + } +} + +func TestVerifyCodeowners(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + tt := []struct { + name string + setup func(string) error + target string + wantErr bool + errMatch string + }{ + { + name: "valid codeowners", + target: "", + wantErr: false, + }, + { + name: "invalid owner format", + setup: func(dir string) error { + return os.WriteFile(filepath.Join(dir, ".codeowners"), []byte("* invalid-owner"), 0644) + }, + target: "", + wantErr: true, + errMatch: "doesn't start with @", + }, + { + name: "non-existent directory", + target: "does-not-exist", + wantErr: true, + }, + { + name: "missing codeowners file", + setup: func(dir string) error { + return os.Remove(filepath.Join(dir, ".codeowners")) + }, + target: "", + wantErr: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + if tc.setup != nil { + err := tc.setup(testRepo) + if err != nil { + t.Fatalf("Setup failed: %v", err) + } + } + + err := verifyCodeowners(testRepo, tc.target) + if (err != nil) != tc.wantErr { + t.Errorf("verifyCodeowners() error = %v, wantErr %v", err, tc.wantErr) + return + } + + if tc.wantErr && tc.errMatch != "" && !strings.Contains(err.Error(), tc.errMatch) { + t.Errorf("verifyCodeowners() error = %v, want to contain %v", err, tc.errMatch) + } + }) + } +} + +func TestStripRoot(t *testing.T) { + tt := []struct { + name string + root string + path string + want string + }{ + { + name: "current directory", + root: ".", + path: "file.txt", + want: "file.txt", + }, + { + name: "subdirectory", + root: "/test", + path: "/test/file.txt", + want: "file.txt", + }, + { + name: "nested subdirectory", + root: "/test", + path: "/test/dir/file.txt", + want: "dir/file.txt", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + got := stripRoot(tc.root, tc.path) + if got != tc.want { + t.Errorf("stripRoot() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestDepthCheck(t *testing.T) { + tt := []struct { + name string + path string + target string + depth int + want bool + }{ + { + name: "within depth", + path: "dir/file.txt", + target: "", + depth: 1, + want: false, + }, + { + name: "exceeds depth", + path: "dir1/dir2/dir3/file.txt", + target: "", + depth: 1, + want: true, + }, + { + name: "with target within depth", + path: "target/dir/file.txt", + target: "target", + depth: 1, + want: false, + }, + { + name: "with target exceeds depth", + path: "target/dir1/dir2/file.txt", + target: "target", + depth: 1, + want: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + got := depthCheck(tc.path, tc.target, tc.depth) + if got != tc.want { + t.Errorf("depthCheck() = %v, want %v", got, tc.want) + } + }) + } +} +