diff --git a/bots-common/gitea_utils.go b/bots-common/gitea_utils.go index 3aa0674..143b129 100644 --- a/bots-common/gitea_utils.go +++ b/bots-common/gitea_utils.go @@ -65,7 +65,7 @@ type GiteaPRFetcher interface { } type GiteaReviewFetcher interface { - GetPullRequestReviews(org, project string, num int64) ([]*models.PullReview, error) + GetPullRequestReviews(org, project string, PRnum int64) ([]*models.PullReview, error) } type GiteaReviewRequester interface { diff --git a/workflow-pr/pr.go b/workflow-pr/pr.go index 979fade..2a85b05 100644 --- a/workflow-pr/pr.go +++ b/workflow-pr/pr.go @@ -11,7 +11,7 @@ import ( ) type PRInfo struct { - pr *models.PullRequest + pr *models.PullRequest } type PRSet struct { @@ -96,7 +96,18 @@ func (rs *PRSet) IsConsistent() bool { return true } -func (rs *PRSet) IsReviewed() bool { - return false +func (rs *PRSet) IsReviewed(gitea common.GiteaReviewFetcher) bool { + common_reviwers := rs.config.Reviewers + is_reviewed := false + for _, pr := range rs.prs { + r, err := FetchGiteaReviews(gitea, common_reviwers, pr.pr.Base.Repo.Owner.UserName, pr.pr.Base.Repo.Name, pr.pr.Index) + if err != nil { + return false + } + is_reviewed = r.IsReviewed() + if !is_reviewed { + return false + } + } + return is_reviewed } - diff --git a/workflow-pr/pr_test.go b/workflow-pr/pr_test.go index fe45e21..4e0043d 100644 --- a/workflow-pr/pr_test.go +++ b/workflow-pr/pr_test.go @@ -29,8 +29,10 @@ func TestPR(t *testing.T) { } type prdata struct { - pr *models.PullRequest - err error + pr *models.PullRequest + pr_err error + reviews []*models.PullReview + review_error error } t.Run("TestPR", func(t *testing.T) { @@ -44,13 +46,12 @@ func TestPR(t *testing.T) { consistentSet bool prjGitPRIndex int - customMockSetup func(*mock_common.MockGiteaPRFetcher) error reviewSetFetcher func(*mock_common.MockGiteaPRFetcher) (*PRSet, error) }{ { name: "Error fetching PullRequest", data: []prdata{ - {pr: &models.PullRequest{Body: "", Index: 42, Base: &models.PRBranchInfo{Repo: &models.Repository{Name: "repo", Owner: &models.User{UserName: "test"}}}}, err: errors.New("Missing PR")}, + {pr: &models.PullRequest{Body: "", Index: 42, Base: &models.PRBranchInfo{Repo: &models.Repository{Name: "repo", Owner: &models.User{UserName: "test"}}}}, pr_err: errors.New("Missing PR")}, }, }, @@ -58,7 +59,7 @@ func TestPR(t *testing.T) { name: "Error fetching PullRequest in PrjGit", data: []prdata{ {pr: &models.PullRequest{Body: "PR: foo/barPrj#22", Index: 42, Base: &models.PRBranchInfo{Repo: &models.Repository{Name: "repo", Owner: &models.User{UserName: "test"}}}}}, - {pr: &models.PullRequest{Body: "", Index: 22, Base: &models.PRBranchInfo{Repo: &models.Repository{Name: "barPrj", Owner: &models.User{UserName: "foo"}}}}, err: errors.New("Missing PR")}, + {pr: &models.PullRequest{Body: "", Index: 22, Base: &models.PRBranchInfo{Repo: &models.Repository{Name: "barPrj", Owner: &models.User{UserName: "foo"}}}}, pr_err: errors.New("Missing PR")}, }, }, @@ -104,11 +105,18 @@ func TestPR(t *testing.T) { { name: "Review set of prjgit PR is consistent", data: []prdata{ - {pr: &models.PullRequest{Body: "", Index: 42, Base: &models.PRBranchInfo{Repo: &models.Repository{Name: "barPrj", Owner: &models.User{UserName: "foo"}}}}}, + { + pr: &models.PullRequest{Body: "", Index: 42, Base: &models.PRBranchInfo{Repo: &models.Repository{Name: "barPrj", Owner: &models.User{UserName: "foo"}}}}, + reviews: []*models.PullReview{ + {Body: "LGTM", User: &models.User{UserName: "super1"}, State: common.ReviewStateApproved}, + {Body: "LGTM", User: &models.User{UserName: "super2"}, State: common.ReviewStateApproved}, + }, + }, }, resLen: 1, prjGitPRIndex: 0, consistentSet: true, + reviewed: true, reviewSetFetcher: func(mock *mock_common.MockGiteaPRFetcher) (*PRSet, error) { return FetchPRSet(mock, "foo", "barPrj", 42, &baseConfig) }, @@ -139,29 +147,27 @@ func TestPR(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { ctl := gomock.NewController(t) - mock := mock_common.NewMockGiteaPRFetcher(ctl) + pr_mock := mock_common.NewMockGiteaPRFetcher(ctl) + review_mock := mock_common.NewMockGiteaReviewFetcher(ctl) var test_err error - if test.customMockSetup != nil { - if err := test.customMockSetup(mock); err != nil { - test_err = err - } - } else { - for _, data := range test.data { - mock.EXPECT().GetPullRequest(data.pr.Base.Repo.Owner.UserName, data.pr.Base.Repo.Name, data.pr.Index).Return(data.pr, data.err).AnyTimes() - if data.err != nil { - test_err = data.err - } + for _, data := range test.data { + pr_mock.EXPECT().GetPullRequest(data.pr.Base.Repo.Owner.UserName, data.pr.Base.Repo.Name, data.pr.Index).Return(data.pr, data.pr_err).AnyTimes() + if data.pr_err != nil { + test_err = data.pr_err } + review_mock.EXPECT().GetPullRequestReviews(data.pr.Base.Repo.Owner.UserName, data.pr.Base.Repo.Name, data.pr.Index).Return(data.reviews, data.review_error).AnyTimes() } var res *PRSet var err error + if test.reviewSetFetcher != nil { - res, err = test.reviewSetFetcher(mock) + res, err = test.reviewSetFetcher(pr_mock) } else { - res, err = FetchPRSet(mock, "test", "repo", 42, &baseConfig) + res, err = FetchPRSet(pr_mock, "test", "repo", 42, &baseConfig) } + if err == nil { if test_err != nil { t.Fatal("Expected", test_err, "but got", err) @@ -181,8 +187,6 @@ func TestPR(t *testing.T) { return } - t.Log(res) - if test.resLen != len(res.prs) { t.Error("expected result len", test.resLen, "but got", len(res.prs)) } @@ -214,8 +218,8 @@ func TestPR(t *testing.T) { t.Error("IsConsistent() returned unexpected:", test.consistentSet) } - if res.IsReviewed() != test.reviewed { - t.Error("expected reviewed to be NOT", res.IsReviewed()) + if isReviewed := res.IsReviewed(review_mock); isReviewed != test.reviewed { + t.Error("expected reviewed to be NOT", isReviewed) } }) } diff --git a/workflow-pr/reviews.go b/workflow-pr/reviews.go index 9ab1e9b..f95ee27 100644 --- a/workflow-pr/reviews.go +++ b/workflow-pr/reviews.go @@ -10,7 +10,7 @@ type PRReviews struct { reviewers []string } -func FetchReviews(rf common.GiteaReviewFetcher, reviewers []string, org, repo string, no int64) (*PRReviews, error) { +func FetchGiteaReviews(rf common.GiteaReviewFetcher, reviewers []string, org, repo string, no int64) (*PRReviews, error) { reviews, err := rf.GetPullRequestReviews(org, repo, no) if err != nil { return nil, err diff --git a/workflow-pr/reviews_test.go b/workflow-pr/reviews_test.go index 4d54dcb..7bf9fd7 100644 --- a/workflow-pr/reviews_test.go +++ b/workflow-pr/reviews_test.go @@ -1,6 +1,7 @@ package main import ( + "errors" "testing" "go.uber.org/mock/gomock" @@ -51,6 +52,26 @@ func TestReviews(t *testing.T) { reviewers: []string{"user1", "user2"}, isReviewed: true, }, + { + name: "Two reviewer approved, but fetch error", + reviews: []*models.PullReview{ + &models.PullReview{State: common.ReviewStateApproved, User: &models.User{UserName: "user1"}}, + &models.PullReview{State: common.ReviewStateApproved, User: &models.User{UserName: "user2"}}, + }, + reviewers: []string{"user1", "user2"}, + fetchErr: errors.New("System error fetching reviews."), + isReviewed: true, + }, + { + name: "Extra reviewers are ignored", + reviews: []*models.PullReview{ + &models.PullReview{State: common.ReviewStateApproved, User: &models.User{UserName: "user1"}}, + &models.PullReview{State: common.ReviewStateApproved, User: &models.User{UserName: "user4"}}, + &models.PullReview{State: common.ReviewStateApproved, User: &models.User{UserName: "user2"}}, + }, + reviewers: []string{"user1", "user2"}, + isReviewed: true, + }, } for _, test := range tests { @@ -60,10 +81,13 @@ func TestReviews(t *testing.T) { rf.EXPECT().GetPullRequestReviews("test", "pr", int64(1)).Return(test.reviews, test.fetchErr) - reviews, err := FetchReviews(rf, test.reviewers, "test", "pr", 1) + reviews, err := FetchGiteaReviews(rf, test.reviewers, "test", "pr", 1) - if err != test.fetchErr { - t.Fatal("FetchReviews() failed with unexpected error:", err) + if test.fetchErr != nil { + if err != test.fetchErr { + t.Fatal("FetchReviews() failed with unexpected error:", err) + } + return } if r := reviews.IsReviewed(); r != test.isReviewed {