Skip to content

Commit

Permalink
Remove ListMergedPRs API
Browse files Browse the repository at this point in the history
  • Loading branch information
@azeemsgoogle
azeemsgoogle committed Feb 2, 2022
1 parent 86d8281 commit fb3e449
Show file tree
Hide file tree
Showing 14 changed files with 245 additions and 205 deletions.
12 changes: 7 additions & 5 deletions checks/ci_tests.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,19 @@ func init() {

// CITests runs CI-Tests check.
func CITests(c *checker.CheckRequest) checker.CheckResult {
prs, err := c.RepoClient.ListMergedPRs()
commits, err := c.RepoClient.ListCommits()
if err != nil {
e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err))
return checker.CreateRuntimeErrorResult(CheckCITests, e)
}

totalMerged := 0
totalTested := 0
for index := range prs {
pr := &prs[index]
if pr.MergedAt.IsZero() {
for i := range commits {
pr := &commits[i].AssociatedMergeRequest
// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
// but the PR was created in the original repo.
if pr.MergedAt.IsZero() || pr.Repository != c.Repo.String() {
continue
}
totalMerged++
Expand Down
28 changes: 17 additions & 11 deletions checks/code_review.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,15 @@ func githubCodeReview(c *checker.CheckRequest) (int, string, error) {
// Look at some merged PRs to see if they were reviewed.
totalMerged := 0
totalReviewed := 0
prs, err := c.RepoClient.ListMergedPRs()
commits, err := c.RepoClient.ListCommits()
if err != nil {
return 0, "", sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
return 0, "", sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.Commits: %v", err))
}
for _, pr := range prs {
if pr.MergedAt.IsZero() {
for _, commit := range commits {
pr := commit.AssociatedMergeRequest
// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
// but the PR was created in the original repo.
if pr.MergedAt.IsZero() || pr.Repository != c.Repo.String() {
continue
}
totalMerged++
Expand All @@ -122,11 +125,11 @@ func githubCodeReview(c *checker.CheckRequest) (int, string, error) {
// of equivalent to a review and is done several times on small prs to save
// time on clicking the approve button.
if !foundApprovedReview &&
pr.MergeCommit.Committer.Login != "" &&
pr.MergeCommit.Committer.Login != pr.Author.Login {
commit.Committer.Login != "" &&
commit.Committer.Login != pr.Author.Login {
c.Dlogger.Debug3(&checker.LogMessage{
Text: fmt.Sprintf("found PR#%d with committer (%s) different from author (%s)",
pr.Number, pr.Author.Login, pr.MergeCommit.Committer.Login),
pr.Number, pr.Author.Login, commit.Committer.Login),
})
totalReviewed++
foundApprovedReview = true
Expand All @@ -148,12 +151,15 @@ func prowCodeReview(c *checker.CheckRequest) (int, string, error) {
// Look at some merged PRs to see if they were reviewed
totalMerged := 0
totalReviewed := 0
prs, err := c.RepoClient.ListMergedPRs()
commits, err := c.RepoClient.ListCommits()
if err != nil {
sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err))
}
for _, pr := range prs {
if pr.MergedAt.IsZero() {
for _, commit := range commits {
pr := commit.AssociatedMergeRequest
// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
// but the PR was created in the original repo.
if pr.MergedAt.IsZero() || pr.Repository != c.Repo.String() {
continue
}
totalMerged++
Expand Down
169 changes: 90 additions & 79 deletions checks/code_review_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ import (
// TestCodeReview tests the code review checker.
func TestCodereview(t *testing.T) {
t.Parallel()
//fieldalignment lint issue. Ignoring it as it is not important for this test.
//nolint
const testRepo = "test-repo"

// fieldalignment lint issue. Ignoring it as it is not important for this test.
// nolint: govet, goerr113
tests := []struct {
err error
name string
commiterr error
commits []clients.Commit
prs []clients.PullRequest
expected checker.CheckResult
}{
{
Expand Down Expand Up @@ -70,132 +71,145 @@ func TestCodereview(t *testing.T) {
},
{
name: "Valid PR's and commits as not a bot",
prs: []clients.PullRequest{
{
Number: 1,
MergedAt: time.Now(),
Reviews: []clients.Review{
{
State: "APPROVED",
},
},
Labels: []clients.Label{
{
Name: "lgtm",
},
},
},
},
commits: []clients.Commit{
{
SHA: "sha",
Committer: clients.User{
Login: "user",
},
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
Number: 1,
MergedAt: time.Now(),
Reviews: []clients.Review{
{
State: "APPROVED",
},
},
Labels: []clients.Label{
{
Name: "lgtm",
},
},
},
},
},

expected: checker.CheckResult{
Score: 10,
Pass: true,
},
},

{
name: "Valid PR's and commits as bot",
prs: []clients.PullRequest{
{
Number: 1,
MergedAt: time.Now(),
Reviews: []clients.Review{
{
State: "APPROVED",
},
},
Labels: []clients.Label{
{
Name: "lgtm",
},
},
},
},
commits: []clients.Commit{
{
SHA: "sha",
Committer: clients.User{
Login: "bot",
},
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
Number: 1,
MergedAt: time.Now(),
Reviews: []clients.Review{
{
State: "APPROVED",
},
},
Labels: []clients.Label{
{
Name: "lgtm",
},
},
},
},
},

expected: checker.CheckResult{
Score: 10,
Pass: true,
},
},

{
name: "Valid PR's and commits not yet merged",
prs: []clients.PullRequest{
{
Number: 1,
Reviews: []clients.Review{
{
State: "APPROVED",
},
},
Labels: []clients.Label{
{
Name: "lgtm",
},
},
},
},
commits: []clients.Commit{
{
SHA: "sha",
Committer: clients.User{
Login: "bot",
},
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
Number: 1,
Reviews: []clients.Review{
{
State: "APPROVED",
},
},
Labels: []clients.Label{
{
Name: "lgtm",
},
},
},
},
},

expected: checker.CheckResult{
Score: -1,
},
},
{
name: "Valid PR's and commits with merged by someone else",
prs: []clients.PullRequest{
commits: []clients.Commit{
{
Number: 1,
Reviews: []clients.Review{
{
State: "APPROVED",
},
SHA: "sha",
Committer: clients.User{
Login: "bot",
},
Labels: []clients.Label{
{
Name: "lgtm",
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
Number: 1,
Reviews: []clients.Review{
{
State: "APPROVED",
},
},
},
MergeCommit: clients.Commit{
SHA: "sha",
Committer: clients.User{
Login: "bob",
Labels: []clients.Label{
{
Name: "lgtm",
},
},
},
},
},
expected: checker.CheckResult{
Score: -1,
},
},
{
name: "Merged PR in a different repo",
commits: []clients.Commit{
{
SHA: "sha",
Committer: clients.User{
Login: "bot",
},
AssociatedMergeRequest: clients.PullRequest{
Repository: "does-not-exist",
MergedAt: time.Now(),
Number: 1,
Reviews: []clients.Review{
{
State: "APPROVED",
},
},
Labels: []clients.Label{
{
Name: "lgtm",
},
},
},
},
},

expected: checker.CheckResult{
Score: -1,
},
Expand All @@ -207,22 +221,19 @@ func TestCodereview(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
ctrl := gomock.NewController(t)
mockRepo := mockrepo.NewMockRepoClient(ctrl)
mockRepo.EXPECT().ListMergedPRs().DoAndReturn(func() ([]clients.PullRequest, error) {
if tt.err != nil {
return tt.prs, tt.err
}
return tt.prs, tt.err
}).AnyTimes()
mockRepo.EXPECT().ListCommits().DoAndReturn(func() ([]clients.Commit, error) {
mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
mockRepoClient.EXPECT().ListCommits().DoAndReturn(func() ([]clients.Commit, error) {
if tt.commiterr != nil {
return tt.commits, tt.commiterr
}
return tt.commits, tt.err
}).AnyTimes()
mockRepo := mockrepo.NewMockRepo(ctrl)
mockRepo.EXPECT().String().Return(testRepo).AnyTimes()

req := checker.CheckRequest{
RepoClient: mockRepo,
RepoClient: mockRepoClient,
Repo: mockRepo,
}
req.Dlogger = &scut.TestDetailLogger{}
res := DoesCodeReview(&req)
Expand Down
4 changes: 2 additions & 2 deletions checks/maintained.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ func IsMaintained(c *checker.CheckRequest) checker.CheckResult {
return checker.CreateRuntimeErrorResult(CheckMaintained, e)
}
commitsWithinThreshold := 0
for _, commit := range commits {
if commit.CommittedDate.After(threshold) {
for i := range commits {
if commits[i].CommittedDate.After(threshold) {
commitsWithinThreshold++
}
}
Expand Down
11 changes: 7 additions & 4 deletions checks/sast.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,20 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {

// nolint
func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
prs, err := c.RepoClient.ListMergedPRs()
commits, err := c.RepoClient.ListCommits()
if err != nil {
//nolint
return checker.InconclusiveResultScore,
sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListMergedPRs: %v", err))
sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err))
}

totalMerged := 0
totalTested := 0
for _, pr := range prs {
if pr.MergedAt.IsZero() {
for _, commit := range commits {
pr := commit.AssociatedMergeRequest
// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
// but the PR was created in the original repo.
if pr.MergedAt.IsZero() || pr.Repository != c.Repo.String() {
continue
}
totalMerged++
Expand Down
Loading

0 comments on commit fb3e449

Please sign in to comment.