test: add min reviewers to createFakeRepo

jenkins-x · Jan 5, 2025 · de51fe2 · de51fe2
1 parent 621ccab
commit de51fe2
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 36 deletions.
diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
@@ -106,7 +106,7 @@ func TestUnapprovedFiles(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = false
 		for approver := range test.currentlyApproved {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -220,7 +220,7 @@ func TestGetFiles(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
+			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
 			testApprovers.RequireIssue = false
 			for approver := range test.currentlyApproved {
 				testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -366,7 +366,7 @@ func TestGetCCs(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = false
 		for approver := range test.currentlyApproved {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -526,7 +526,7 @@ func TestIsApproved(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepoWithMinReviewers(FakeRepoMap, fakeMinReviewersMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
+			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, fakeMinReviewersMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
 			for approver := range test.currentlyApproved {
 				testApprovers.AddApprover(approver, "REFERENCE", false)
 			}
@@ -662,7 +662,7 @@ func TestIsApprovedWithIssue(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: 0, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: 0, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = true
 		testApprovers.AssociatedIssue = test.associatedIssue
 		for approver, noissue := range test.currentlyApproved {
@@ -741,7 +741,7 @@ func TestGetFilesApprovers(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(test.owners), log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(test.owners, nil), log: logrus.WithField("plugin", "some_plugin")})
 		for _, approver := range test.approvers {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
 		}
@@ -759,7 +759,7 @@ func TestGetMessage(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -800,7 +800,7 @@ func TestGetMessageBitBucketServer(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -841,7 +841,7 @@ func TestGetMessageGitLab(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -882,7 +882,7 @@ func TestGetMessageWithPrefix(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -923,7 +923,7 @@ func TestGetMessageAllApproved(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -965,7 +965,7 @@ func TestGetMessageNoneApproved(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1005,7 +1005,7 @@ func TestGetMessageApprovedIssueAssociated(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1049,7 +1049,7 @@ func TestGetMessageApprovedNoIssueByPassed(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1093,7 +1093,7 @@ func TestGetMessageMDOwners(t *testing.T) {
 				"a":           sets.NewString("Alice"),
 				"b":           sets.NewString("Bill"),
 				"b/README.md": sets.NewString("Doctor"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1134,7 +1134,7 @@ func TestGetMessageDifferentGitHubLink(t *testing.T) {
 				"a":           sets.NewString("Alice"),
 				"b":           sets.NewString("Bill"),
 				"b/README.md": sets.NewString("Doctor"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)

diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
@@ -525,8 +525,7 @@ func (ap Approvers) GetCCs() []string {
 // the PR are approved.  If this returns true, the PR may still not be fully approved depending
 // on the associated issue requirement
 func (ap Approvers) AreFilesApproved() bool {
-	rr := ap.GetRemainingRequiredApprovers()
-	return ap.UnapprovedFiles().Len() == 0 && rr <= 0
+	return ap.UnapprovedFiles().Len() == 0 && ap.GetRemainingRequiredApprovers() <= 0
 }
 
 // RequirementsMet returns a bool indicating whether the PR has met all approval requirements:

diff --git a/pkg/plugins/approve/approvers/owners_test.go b/pkg/plugins/approve/approvers/owners_test.go
@@ -42,7 +42,7 @@ func (f FakeRepo) MinimumReviewersForFile(path string) int {
 	if out, ok := f.minReviewersMap[path]; ok {
 		return out
 	}
-	return 1
+	return 0
 }
 
 func (f FakeRepo) Approvers(path string) sets.String {
@@ -73,7 +73,7 @@ func canonicalize(path string) string {
 	return strings.TrimSuffix(path, "/")
 }
 
-func createFakeRepo(la map[string]sets.String) FakeRepo {
+func createFakeRepo(la map[string]sets.String, minReviewers map[string]int) FakeRepo {
 	// github doesn't use / at the root
 	a := map[string]sets.String{}
 	for dir, approvers := range la {
@@ -90,14 +90,7 @@ func createFakeRepo(la map[string]sets.String) FakeRepo {
 			}
 		}
 	}
-
-	return FakeRepo{approversMap: a, leafApproversMap: la}
-}
-
-func createFakeRepoWithMinReviewers(la map[string]sets.String, ma map[string]int) FakeRepo {
-	fr := createFakeRepo(la)
-	fr.minReviewersMap = ma
-	return fr
+	return FakeRepo{approversMap: a, leafApproversMap: la, minReviewersMap: minReviewers}
 }
 
 func setToLower(s sets.String) sets.String {
@@ -122,7 +115,7 @@ func TestCreateFakeRepo(t *testing.T) {
 		"c":       cApprovers,
 		"a/combo": edcApprovers,
 	}
-	fakeRepo := createFakeRepo(FakeRepoMap)
+	fakeRepo := createFakeRepo(FakeRepoMap, nil)
 
 	tests := []struct {
 		testName              string
@@ -230,7 +223,7 @@ func TestGetLeafApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -293,7 +286,7 @@ func TestGetOwnersSet(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -369,7 +362,7 @@ func TestGetSuggestedApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -455,7 +448,7 @@ func TestGetAllPotentialApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -536,7 +529,7 @@ func TestFindMostCoveringApprover(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -597,7 +590,7 @@ func TestGetReverseMap(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -674,7 +667,7 @@ func TestGetShuffledApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      test.seed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}