Skip to content

Commit

Permalink
Merge pull request kubernetes#12185 from brendandburns/queue
Browse files Browse the repository at this point in the history 
Fix a mis-undersanding about the github API, add more tests
  • Loading branch information
@alex-mohr
alex-mohr committed Aug 4, 2015
2 parents 784cef5 + f50f0b2 commit 4271f28
Show file tree
Hide file tree
Showing 2 changed files with 152 additions and 29 deletions.
29 changes: 23 additions & 6 deletions contrib/submit-queue/github/github.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,22 @@ type FilterConfig struct {
RequiredStatusContexts []string
}

func validateLGTMAfterPush(client *github.Client, user, project string, pr *github.PullRequest) (bool, error) {
func lastModifiedTime(client *github.Client, user, project string, pr *github.PullRequest) (*time.Time, error) {
list, _, err := client.PullRequests.ListCommits(user, project, *pr.Number, &github.ListOptions{})
if err != nil {
return nil, err
}
var lastModified *time.Time
for ix := range list {
item := list[ix]
if lastModified == nil || item.Commit.Committer.Date.After(*lastModified) {
lastModified = item.Commit.Committer.Date
}
}
return lastModified, nil
}

func validateLGTMAfterPush(client *github.Client, user, project string, pr *github.PullRequest, lastModifiedTime *time.Time) (bool, error) {
var lgtmTime *time.Time
events, _, err := client.Issues.ListIssueEvents(user, project, *pr.Number, &github.ListOptions{})
if err != nil {
Expand All @@ -102,10 +117,7 @@ func validateLGTMAfterPush(client *github.Client, user, project string, pr *gith
if lgtmTime == nil {
return false, fmt.Errorf("Couldn't find time for LGTM label, this shouldn't happen, skipping PR: %d", *pr.Number)
}
if pr.Head == nil || pr.Head.Repo == nil || pr.Head.Repo.PushedAt == nil {
return false, fmt.Errorf("Couldn't find push time for PR, this shouldn't happen, skipping PR: %d", *pr.Number)
}
return pr.Head.Repo.PushedAt.Before(*lgtmTime), nil
return lastModifiedTime.Before(*lgtmTime), nil
}

// For each PR in the project that matches:
Expand Down Expand Up @@ -156,7 +168,12 @@ func ForEachCandidatePRDo(client *github.Client, user, project string, fn PRFunc
continue
}

if ok, err := validateLGTMAfterPush(client, user, project, pr); err != nil {
lastModifiedTime, err := lastModifiedTime(client, user, project, pr)
if err != nil {
glog.Errorf("Failed to get last modified time, skipping PR: %d", *pr.Number)
continue
}
if ok, err := validateLGTMAfterPush(client, user, project, pr, lastModifiedTime); err != nil {
glog.Errorf("Error validating LGTM: %v, Skipping: %d", err, *pr.Number)
continue
} else if !ok {
Expand Down
152 changes: 129 additions & 23 deletions contrib/submit-queue/github/github_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,9 +394,9 @@ func TestComputeStatus(t *testing.T) {

func TestValidateLGTMAfterPush(t *testing.T) {
tests := []struct {
issueEvents []github.IssueEvent
shouldPass bool
pull github.PullRequest
issueEvents []github.IssueEvent
shouldPass bool
lastModified time.Time
}{
{
issueEvents: []github.IssueEvent{
Expand All @@ -408,15 +408,8 @@ func TestValidateLGTMAfterPush(t *testing.T) {
CreatedAt: timePtr(time.Unix(10, 0)),
},
},
pull: github.PullRequest{
Number: intPtr(1),
Head: &github.PullRequestBranch{
Repo: &github.Repository{
PushedAt: &github.Timestamp{time.Unix(9, 0)},
},
},
},
shouldPass: true,
lastModified: time.Unix(9, 0),
shouldPass: true,
},
{
issueEvents: []github.IssueEvent{
Expand All @@ -428,20 +421,13 @@ func TestValidateLGTMAfterPush(t *testing.T) {
CreatedAt: timePtr(time.Unix(10, 0)),
},
},
pull: github.PullRequest{
Number: intPtr(1),
Head: &github.PullRequestBranch{
Repo: &github.Repository{
PushedAt: &github.Timestamp{time.Unix(11, 0)},
},
},
},
shouldPass: false,
lastModified: time.Unix(11, 0),
shouldPass: false,
},
}
for _, test := range tests {
client, server, mux := initTest()
mux.HandleFunc(fmt.Sprintf("/repos/o/r/issues/%d/events", test.pull.Number), func(w http.ResponseWriter, r *http.Request) {
mux.HandleFunc(fmt.Sprintf("/repos/o/r/issues/1/events"), func(w http.ResponseWriter, r *http.Request) {
if r.Method != "GET" {
t.Errorf("Unexpected method: %s", r.Method)
}
Expand All @@ -451,7 +437,7 @@ func TestValidateLGTMAfterPush(t *testing.T) {
t.Errorf("Unexpected error: %v", err)
}
w.Write(data)
ok, err := validateLGTMAfterPush(client, "o", "r", &test.pull)
ok, err := validateLGTMAfterPush(client, "o", "r", &github.PullRequest{Number: intPtr(1)}, &test.lastModified)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
Expand All @@ -462,3 +448,123 @@ func TestValidateLGTMAfterPush(t *testing.T) {
server.Close()
}
}

func TestGetLastModified(t *testing.T) {
tests := []struct {
commits []github.RepositoryCommit
expectedTime *time.Time
}{
{
commits: []github.RepositoryCommit{
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(10, 0)),
},
},
},
},
expectedTime: timePtr(time.Unix(10, 0)),
},
{
commits: []github.RepositoryCommit{
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(10, 0)),
},
},
},
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(11, 0)),
},
},
},
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(12, 0)),
},
},
},
},
expectedTime: timePtr(time.Unix(12, 0)),
},
{
commits: []github.RepositoryCommit{
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(10, 0)),
},
},
},
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(9, 0)),
},
},
},
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(8, 0)),
},
},
},
},
expectedTime: timePtr(time.Unix(10, 0)),
},
{
commits: []github.RepositoryCommit{
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(9, 0)),
},
},
},
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(10, 0)),
},
},
},
{
Commit: &github.Commit{
Committer: &github.CommitAuthor{
Date: timePtr(time.Unix(9, 0)),
},
},
},
},
expectedTime: timePtr(time.Unix(10, 0)),
},
}
for _, test := range tests {
client, server, mux := initTest()
mux.HandleFunc(fmt.Sprintf("/repos/o/r/pulls/1/commits"), func(w http.ResponseWriter, r *http.Request) {
if r.Method != "GET" {
t.Errorf("Unexpected method: %s", r.Method)
}
w.WriteHeader(http.StatusOK)
data, err := json.Marshal(test.commits)
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
w.Write(data)
ts, err := lastModifiedTime(client, "o", "r", &github.PullRequest{Number: intPtr(1)})
if err != nil {
t.Errorf("unexpected error: %v", err)
}
if !ts.Equal(*test.expectedTime) {
t.Errorf("expected: %v, saw: %v", test.expectedTime, ts)
}
})
server.Close()
}
}

0 comments on commit 4271f28

Please sign in to comment.