Increase test coverage above 70%

Author: BakerNet

CONTRIBUTING.md

@@ -7,11 +7,22 @@ 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 <your_gh_token> -dir ../chaturbate -pr <pr_num> -repo multimediallc/chaturbate -v true
+```
+
 #### 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 +31,18 @@ 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).
 * 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.

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-74.3%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)
 

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
 }

internal/git/diff.go

@@ -13,20 +13,46 @@ 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)
 	Context() DiffContext
 }
 
 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,17 +80,17 @@ 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,
 		olderDiff: olderDiff,
 	}
 	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() {