diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go index 3be1bdc49..98544a54a 100644 --- 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, nil), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")}) + testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), 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, nil), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")}) + testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), 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, nil), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")}) + testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")}) testApprovers.RequireIssue = false for approver := range test.currentlyApproved { testApprovers.AddApprover(approver, "REFERENCE", false) @@ -387,23 +387,13 @@ func TestIsApproved(t *testing.T) { dApprovers := sets.NewString("David", "Dan", "Debbie") eApprovers := sets.NewString("Eve", "Erin") edcApprovers := eApprovers.Union(dApprovers).Union(cApprovers) - minApproversRoot := sets.NewString("Alice") - minApprovers2Required := sets.NewString("Alice", "Bob") FakeRepoMap := map[string]sets.String{ - "": rootApprovers, - "a": aApprovers, - "b": bApprovers, - "c": cApprovers, - "a/d": dApprovers, - "a/combo": edcApprovers, - "minReviewers": minApproversRoot, - "minReviewers/2Required": minApprovers2Required, - } - fakeMinReviewersMap := map[string]int{ - "minReviewers/2RequiredNoParents": 2, - } - fakeNoParentsOwnersMap := map[string]bool{ - "minReviewers/2Required": true, + "": rootApprovers, + "a": aApprovers, + "b": bApprovers, + "c": cApprovers, + "a/d": dApprovers, + "a/combo": edcApprovers, } tests := []struct { @@ -483,74 +473,322 @@ func TestIsApproved(t *testing.T) { currentlyApproved: sets.NewString("Anne"), isApproved: false, }, + } + + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + fakeRepo := createFakeRepo(FakeRepoMap) + testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: fakeRepo, seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")}) + for approver := range test.currentlyApproved { + testApprovers.AddApprover(approver, "REFERENCE", false) + } + calculated := testApprovers.IsApproved() + assert.Equal(t, test.isApproved, calculated, "Failed for test %v", test.testName) + }) + } +} + +func TestIsApprovedWithMinReviewers(t *testing.T) { + tests := []struct { + testName string + filenames []string + leafApprovers map[string]sets.String + minReviewersMap map[string]int + noParentOwnersMap map[string]bool + currentlyApproved sets.String + testSeed int64 + isApproved bool + }{ { - testName: "Min Reviewers Root; 1 approval", - filenames: []string{"minReviewers/test.go"}, + testName: "1 min reviewer: 1 approval", + filenames: []string{"f/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice"), + }, + minReviewersMap: map[string]int{ + "f": 1, + }, testSeed: 0, currentlyApproved: sets.NewString("Alice"), isApproved: true, }, { - testName: "Min Reviewers/2required; 1 approval", - filenames: []string{"minReviewers/2Required/test.go"}, + testName: "2 min reviewers: 1 approval", + filenames: []string{"f/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + }, + minReviewersMap: map[string]int{ + "f": 2, + }, testSeed: 0, currentlyApproved: sets.NewString("Alice"), isApproved: false, }, { - testName: "Min Reviewers/2required; 2 approvals", - filenames: []string{"minReviewers/2Required/test.go"}, + testName: "2 min reviewers: 2 approvals from OWNERS", + filenames: []string{"f/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + }, + minReviewersMap: map[string]int{ + "f": 2, + }, testSeed: 0, currentlyApproved: sets.NewString("Alice", "Bob"), isApproved: true, }, { - testName: "Min Reviewers/2required; 2 approvals but not from required approvers", - filenames: []string{"minReviewers/2Required/test.go"}, + testName: "2 min reviewers: 1 approve from OWNERS, 1 not", + filenames: []string{"f/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + }, + minReviewersMap: map[string]int{ + "f": 2, + }, testSeed: 0, - currentlyApproved: sets.NewString("Tim", "Sally"), + currentlyApproved: sets.NewString("Bob", "Tim"), isApproved: false, }, { - testName: "Min Reviewers/2required & root; 1 approval", - filenames: []string{"minReviewers/test.go", "minReviewers/2Required/test.go"}, + testName: "Multi-file: 2 min reviewers: everyone approves", + filenames: []string{"f/test.go", "g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + "g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "g": 2, + }, testSeed: 0, - currentlyApproved: sets.NewString("Alice"), + currentlyApproved: sets.NewString("Alice", "Bob", "Tom", "Harry"), + isApproved: true, + }, + { + testName: "Multi-file: 2 min reviewers: 1 approval from each file", + filenames: []string{"f/test.go", "g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + "g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "g": 2, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Tom"), isApproved: false, }, { - testName: "Min Reviewers/2required & root; 2 approval", - filenames: []string{"minReviewers/test.go", "minReviewers/2Required/test.go"}, + testName: "Multi-file: 2 min reviewers: f fully approved, g not", + filenames: []string{"f/test.go", "g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + "g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "g": 2, + }, testSeed: 0, - currentlyApproved: sets.NewString("Alice", "Bob"), + currentlyApproved: sets.NewString("Alice", "Bob", "Tom"), + isApproved: false, + }, + { + testName: "Nested-files: equal min reviewers: everyone approves", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + "f/g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "f/g": 2, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Bob", "Tom", "Harry"), + isApproved: true, + }, + { + testName: "Nested-files: equal min reviewers: 1 approval from each file", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + "f/g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "f/g": 2, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Tom"), + isApproved: false, + }, + { + testName: "Nested-files: equal min reviewers: f fully approved, f/g not", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + "f/g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "f/g": 2, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Bob", "Tom"), isApproved: true, }, { - testName: "Min Reviewers/2required & root; 1 approval, 1 not registered approver", - filenames: []string{"minReviewers/test.go", "minReviewers/2Required/test.go"}, + testName: "Nested-files: equal min reviewers: f/g fully approved, f not", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Bob"), + "f/g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "f/g": 2, + }, testSeed: 0, - currentlyApproved: sets.NewString("Alice", "Derek"), + currentlyApproved: sets.NewString("Tom", "Harry", "Alice"), isApproved: false, }, { - testName: "Min Reviewers/2required & other OWNERS; One from each OWNERS", - filenames: []string{"a/test.go", "minReviewers/2Required/test.go"}, + testName: "Nested-files: top-level less min reviewers: everyone approves", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice"), + "f/g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 1, + "f/g": 2, + }, testSeed: 0, - currentlyApproved: sets.NewString("Alice", "Anne"), + currentlyApproved: sets.NewString("Alice", "Tom", "Harry"), isApproved: true, }, + { + testName: "Nested-files: top-level less min reviewers: 1 approval from each file", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice"), + "f/g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 1, + "f/g": 2, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Tom"), + isApproved: true, + }, + { + testName: "Nested-files: top-level less min reviewers: f fully approved, f/g not", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice"), + "f/g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 1, + "f/g": 2, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Tom"), + isApproved: true, + }, + { + testName: "Nested-files: top-level less min reviewers: f/g fully approved, f not", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice"), + "f/g": sets.NewString("Tom", "Harry"), + }, + minReviewersMap: map[string]int{ + "f": 1, + "f/g": 2, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Tom", "Harry"), + isApproved: false, + }, + { + testName: "Nested-files: top-level more min reviewers: everyone approves", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Harry"), + "f/g": sets.NewString("Tom"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "f/g": 1, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Tom", "Harry"), + isApproved: true, + }, + { + testName: "Nested-files: top-level more min reviewers: 1 approval from each file", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Harry"), + "f/g": sets.NewString("Tom"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "f/g": 1, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Tom"), + isApproved: false, + }, + { + testName: "Nested-files: top-level more min reviewers: f fully approved, f/g not", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Harry"), + "f/g": sets.NewString("Tom"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "f/g": 1, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Alice", "Harry"), + isApproved: true, + }, + { + testName: "Nested-files: top-level more min reviewers: f/g fully approved, f not", + filenames: []string{"f/test.go", "f/g/test.go"}, + leafApprovers: map[string]sets.String{ + "f": sets.NewString("Alice", "Harry"), + "f/g": sets.NewString("Tom"), + }, + minReviewersMap: map[string]int{ + "f": 2, + "f/g": 1, + }, + testSeed: 0, + currentlyApproved: sets.NewString("Tom", "Harry"), + isApproved: false, + }, } for _, test := range tests { t.Run(test.testName, func(t *testing.T) { - fakeRepo := createFakeRepo(FakeRepoMap, fakeMinReviewersMap) - fakeRepo.noParentOwnersMap = fakeNoParentsOwnersMap + fakeRepo := createFakeRepo(test.leafApprovers) + fakeRepo.minReviewersMap = test.minReviewersMap + fakeRepo.noParentOwnersMap = test.noParentOwnersMap testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: fakeRepo, seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")}) for approver := range test.currentlyApproved { testApprovers.AddApprover(approver, "REFERENCE", false) } calculated := testApprovers.IsApproved() - assert.Equal(t, test.isApproved, calculated, "Failed for test %v", test.testName) + assert.Equal(t, test.isApproved, calculated) }) } } @@ -681,7 +919,7 @@ func TestIsApprovedWithIssue(t *testing.T) { } for _, test := range tests { - testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: 0, log: logrus.WithField("plugin", "some_plugin")}) + testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: 0, log: logrus.WithField("plugin", "some_plugin")}) testApprovers.RequireIssue = true testApprovers.AssociatedIssue = test.associatedIssue for approver, noissue := range test.currentlyApproved { @@ -760,7 +998,7 @@ func TestGetFilesApprovers(t *testing.T) { } for _, test := range tests { - testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(test.owners, nil), log: logrus.WithField("plugin", "some_plugin")}) + testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(test.owners), log: logrus.WithField("plugin", "some_plugin")}) for _, approver := range test.approvers { testApprovers.AddApprover(approver, "REFERENCE", false) } @@ -778,7 +1016,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"), }, ) @@ -819,7 +1057,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"), }, ) @@ -860,7 +1098,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"), }, ) @@ -901,7 +1139,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"), }, ) @@ -942,7 +1180,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"), }, ) @@ -984,7 +1222,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"), }, ) @@ -1024,7 +1262,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"), }, ) @@ -1068,7 +1306,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"), }, ) @@ -1112,7 +1350,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"), }, ) @@ -1153,7 +1391,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_test.go b/pkg/plugins/approve/approvers/owners_test.go index fe055a404..63a040de3 100644 --- a/pkg/plugins/approve/approvers/owners_test.go +++ b/pkg/plugins/approve/approvers/owners_test.go @@ -73,7 +73,7 @@ func canonicalize(path string) string { return strings.TrimSuffix(path, "/") } -func createFakeRepo(la map[string]sets.String, minReviewers map[string]int) FakeRepo { +func createFakeRepo(la map[string]sets.String) FakeRepo { // github doesn't use / at the root a := map[string]sets.String{} for dir, approvers := range la { @@ -90,7 +90,7 @@ func createFakeRepo(la map[string]sets.String, minReviewers map[string]int) Fake } } } - return FakeRepo{approversMap: a, leafApproversMap: la, minReviewersMap: minReviewers} + return FakeRepo{approversMap: a, leafApproversMap: la} } func setToLower(s sets.String) sets.String { @@ -115,7 +115,7 @@ func TestCreateFakeRepo(t *testing.T) { "c": cApprovers, "a/combo": edcApprovers, } - fakeRepo := createFakeRepo(FakeRepoMap, nil) + fakeRepo := createFakeRepo(FakeRepoMap) tests := []struct { testName string @@ -223,7 +223,7 @@ func TestGetLeafApprovers(t *testing.T) { for _, test := range tests { testOwners := Owners{ filenames: test.filenames, - repo: createFakeRepo(FakeRepoMap, nil), + repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin"), } @@ -286,7 +286,7 @@ func TestGetOwnersSet(t *testing.T) { for _, test := range tests { testOwners := Owners{ filenames: test.filenames, - repo: createFakeRepo(FakeRepoMap, nil), + repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin"), } @@ -362,7 +362,7 @@ func TestGetSuggestedApprovers(t *testing.T) { for _, test := range tests { testOwners := Owners{ filenames: test.filenames, - repo: createFakeRepo(FakeRepoMap, nil), + repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin"), } @@ -448,7 +448,7 @@ func TestGetAllPotentialApprovers(t *testing.T) { for _, test := range tests { testOwners := Owners{ filenames: test.filenames, - repo: createFakeRepo(FakeRepoMap, nil), + repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin"), } @@ -529,7 +529,7 @@ func TestFindMostCoveringApprover(t *testing.T) { for _, test := range tests { testOwners := Owners{ filenames: test.filenames, - repo: createFakeRepo(FakeRepoMap, nil), + repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin"), } @@ -590,7 +590,7 @@ func TestGetReverseMap(t *testing.T) { for _, test := range tests { testOwners := Owners{ filenames: test.filenames, - repo: createFakeRepo(FakeRepoMap, nil), + repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin"), } @@ -667,7 +667,7 @@ func TestGetShuffledApprovers(t *testing.T) { for _, test := range tests { testOwners := Owners{ filenames: test.filenames, - repo: createFakeRepo(FakeRepoMap, nil), + repo: createFakeRepo(FakeRepoMap), seed: test.seed, log: logrus.WithField("plugin", "some_plugin"), }