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 9037444 commit 7053e63
Show file tree
Hide file tree
Showing 12 changed files with 139 additions and 127 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
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
109 changes: 84 additions & 25 deletions checks/sast_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ import (

func TestSAST(t *testing.T) {
t.Parallel()
//nolint

const testRepo = "test-repo"
// nolint: govet, goerr113
tests := []struct {
name string
prs []clients.PullRequest
commits []clients.Commit
err error
searchresult clients.SearchResponse
checkRuns []clients.CheckRun
Expand All @@ -42,23 +44,26 @@ func TestSAST(t *testing.T) {
}{
{
name: "SAST checker should return failed status when no PRs are found",
prs: []clients.PullRequest{},
commits: []clients.Commit{},
searchresult: clients.SearchResponse{},
checkRuns: []clients.CheckRun{},
},
{
name: "SAST checker should return failed status when no PRs are found",
err: errors.New("error"),
prs: []clients.PullRequest{},
commits: []clients.Commit{},
searchresult: clients.SearchResponse{},
checkRuns: []clients.CheckRun{},
expected: checker.CheckResult{Score: -1, Pass: false},
},
{
name: "Successful SAST checker should return success status",
prs: []clients.PullRequest{
commits: []clients.Commit{
{
MergedAt: time.Now().Add(time.Hour - 1),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 1),
},
},
},
searchresult: clients.SearchResponse{},
Expand All @@ -78,18 +83,30 @@ func TestSAST(t *testing.T) {
},
{
name: "Failed SAST checker should return success status",
prs: []clients.PullRequest{
commits: []clients.Commit{
{
MergedAt: time.Now().Add(time.Hour - 1),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 1),
},
},
{
MergedAt: time.Now().Add(time.Hour - 10),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 10),
},
},
{
MergedAt: time.Now().Add(time.Hour - 20),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 20),
},
},
{
MergedAt: time.Now().Add(time.Hour - 30),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 30),
},
},
},
searchresult: clients.SearchResponse{Hits: 1, Results: []clients.SearchResult{{
Expand All @@ -110,18 +127,30 @@ func TestSAST(t *testing.T) {
},
{
name: "Failed SAST checker with checkRuns not completed",
prs: []clients.PullRequest{
commits: []clients.Commit{
{
MergedAt: time.Now().Add(time.Hour - 1),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 1),
},
},
{
MergedAt: time.Now().Add(time.Hour - 10),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 10),
},
},
{
MergedAt: time.Now().Add(time.Hour - 20),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 20),
},
},
{
MergedAt: time.Now().Add(time.Hour - 30),
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
MergedAt: time.Now().Add(time.Hour - 30),
},
},
},
searchresult: clients.SearchResponse{},
Expand All @@ -139,9 +168,36 @@ func TestSAST(t *testing.T) {
},
{
name: "Failed SAST with PullRequest not merged",
prs: []clients.PullRequest{
commits: []clients.Commit{
{
Number: 1,
AssociatedMergeRequest: clients.PullRequest{
Repository: testRepo,
Number: 1,
},
},
},
searchresult: clients.SearchResponse{},
checkRuns: []clients.CheckRun{
{
App: clients.CheckRunApp{
Slug: "lgtm-com",
},
},
},
expected: checker.CheckResult{
Score: 0,
Pass: false,
},
},
{
name: "Merged PullRequest in a different repo",
commits: []clients.Commit{
{
AssociatedMergeRequest: clients.PullRequest{
Repository: "does-not-exist",
MergedAt: time.Now(),
Number: 1,
},
},
},
searchresult: clients.SearchResponse{},
Expand All @@ -167,20 +223,23 @@ func TestSAST(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) {
mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
mockRepoClient.EXPECT().ListCommits().DoAndReturn(func() ([]clients.Commit, error) {
if tt.err != nil {
return nil, tt.err
}
return tt.prs, tt.err
return tt.commits, tt.err
})
mockRepo.EXPECT().ListCheckRunsForRef("").Return(tt.checkRuns, nil).AnyTimes()
mockRepo.EXPECT().Search(searchRequest).Return(tt.searchresult, nil).AnyTimes()
mockRepoClient.EXPECT().ListCheckRunsForRef("").Return(tt.checkRuns, nil).AnyTimes()
mockRepoClient.EXPECT().Search(searchRequest).Return(tt.searchresult, nil).AnyTimes()

mockRepo := mockrepo.NewMockRepo(ctrl)
mockRepo.EXPECT().String().Return(testRepo).AnyTimes()

dl := scut.TestDetailLogger{}
req := checker.CheckRequest{
RepoClient: mockRepo,
RepoClient: mockRepoClient,
Repo: mockRepo,
Ctx: context.TODO(),
Dlogger: &dl,
}
Expand Down
9 changes: 5 additions & 4 deletions clients/commit.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ import "time"

// Commit represents a Git commit.
type Commit struct {
CommittedDate time.Time
Message string
SHA string
Committer User
CommittedDate time.Time
Message string
SHA string
Committer User
AssociatedMergeRequest PullRequest
}
5 changes: 0 additions & 5 deletions clients/githubrepo/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,6 @@ func (client *Client) GetFileContent(filename string) ([]byte, error) {
return client.tarball.getFileContent(filename)
}

// ListMergedPRs implements RepoClient.ListMergedPRs.
func (client *Client) ListMergedPRs() ([]clients.PullRequest, error) {
return client.graphClient.getMergedPRs()
}

// ListCommits implements RepoClient.ListCommits.
func (client *Client) ListCommits() ([]clients.Commit, error) {
return client.graphClient.getCommits()
Expand Down
Loading

0 comments on commit 7053e63

Please sign in to comment.