Fix a mis-undersanding about the github API, add more tests

contrib/submit-queue/github/github.go

@@ -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 {
@@ -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 lastModifiedTime.Before(*lgtmTime), 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
 }
 
 // For each PR in the project that matches:
@@ -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 {

contrib/submit-queue/github/github_test.go

@@ -394,9 +394,9 @@ func TestComputeStatus(t *testing.T) {
 
 func TestValidateLGTMAfterPush(t *testing.T) {
 	tests := []struct {
-		issueEvents []github.IssueEvent
+		issueEvents  []github.IssueEvent
-		shouldPass  bool
+		shouldPass   bool
-		pull        github.PullRequest
+		lastModified time.Time
 	}{
 		{
 			issueEvents: []github.IssueEvent{
@@ -408,15 +408,8 @@ func TestValidateLGTMAfterPush(t *testing.T) {
 					CreatedAt: timePtr(time.Unix(10, 0)),
 				},
 			},
-			pull: github.PullRequest{
+			lastModified: time.Unix(9, 0),
-				Number: intPtr(1),
+			shouldPass:   true,
-				Head: &github.PullRequestBranch{
-					Repo: &github.Repository{
-						PushedAt: &github.Timestamp{time.Unix(9, 0)},
-					},
-				},
-			},
-			shouldPass: true,
 		},
 		{
 			issueEvents: []github.IssueEvent{
@@ -428,20 +421,13 @@ func TestValidateLGTMAfterPush(t *testing.T) {
 					CreatedAt: timePtr(time.Unix(10, 0)),
 				},
 			},
-			pull: github.PullRequest{
+			lastModified: time.Unix(11, 0),
-				Number: intPtr(1),
+			shouldPass:   false,
-				Head: &github.PullRequestBranch{
-					Repo: &github.Repository{
-						PushedAt: &github.Timestamp{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)
 			}
@@ -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)
 			}
@@ -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()
+	}
+}