diff --git a/github/client.go b/github/client.go index cbaf828..695e289 100644 --- a/github/client.go +++ b/github/client.go @@ -110,6 +110,11 @@ type Client struct { // retryBackoff[i] is the delay before attempt i+1 (after attempt i fails). // If nil, defaults to {1s, 2s}. Set to shorter durations in tests via SetRetryBackoff. retryBackoff []time.Duration + + // reviewPageSize overrides reviewsPerPage for testing. Zero means use default. + reviewPageSize int + // reviewMaxPages overrides maxReviewPages for testing. Zero means use default. + reviewMaxPages int } // defaultCheckRedirect is the redirect policy used by NewClient and SetHTTPClient(nil). @@ -194,6 +199,13 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error { return nil } +// SetReviewPagination overrides the page size and max pages for ListReviews. +// Intended for testing only; must be called before any goroutines issue requests. +func (c *Client) SetReviewPagination(pageSize, maxPages int) { + c.reviewPageSize = pageSize + c.reviewMaxPages = maxPages +} + // requestOptions holds per-request configuration for doRequestCore. type requestOptions struct { // bodyFn returns a fresh io.Reader for the request body on each attempt. diff --git a/github/review_test.go b/github/review_test.go index ad14b9f..3459956 100644 --- a/github/review_test.go +++ b/github/review_test.go @@ -2,6 +2,7 @@ package github import ( "context" + "fmt" "encoding/json" "errors" "io" @@ -482,3 +483,195 @@ func TestPostReview_RequestCommitID_FallbackToComment(t *testing.T) { t.Errorf("sent commit_id = %q, want %q (fallback from comment)", gotPayload.CommitID, "comment-sha") } } + +// --- ListReviews pagination tests --- + +func TestListReviews_MultiPage(t *testing.T) { + // Test multi-page pagination: 2 full pages + 1 partial page. + // pageSize=3, so pages return [3, 3, 2] reviews = 8 total. + const pageSize = 3 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Fatalf("expected GET, got %s", r.Method) + } + callCount++ + + page := r.URL.Query().Get("page") + var reviews []map[string]interface{} + + switch page { + case "1": + for i := 1; i <= pageSize; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "APPROVED", "commit_id": "sha1", + "user": map[string]string{"login": "user1"}, + }) + } + case "2": + for i := pageSize + 1; i <= pageSize*2; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "COMMENTED", "commit_id": "sha1", + "user": map[string]string{"login": "user2"}, + }) + } + case "3": + // Partial page: only 2 reviews (less than pageSize) + for i := pageSize*2 + 1; i <= pageSize*2+2; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "CHANGES_REQUESTED", "commit_id": "sha1", + "user": map[string]string{"login": "user3"}, + }) + } + default: + t.Fatalf("unexpected page: %s", page) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, 10) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 8 { + t.Fatalf("expected 8 reviews, got %d", len(reviews)) + } + if callCount != 3 { + t.Errorf("expected 3 API calls, got %d", callCount) + } + + // Verify reviews are correctly concatenated in order + for i, r := range reviews { + expectedID := int64(i + 1) + if r.ID != expectedID { + t.Errorf("review[%d]: expected ID %d, got %d", i, expectedID, r.ID) + } + } +} + +func TestListReviews_ExactMultipleOfPageSize(t *testing.T) { + // When total reviews is an exact multiple of pageSize, an extra request + // returning 0 results terminates the loop. No truncation warning. + const pageSize = 2 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + page := r.URL.Query().Get("page") + + var reviews []map[string]interface{} + switch page { + case "1": + reviews = []map[string]interface{}{ + {"id": 1, "body": "r1", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u1"}}, + {"id": 2, "body": "r2", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u2"}}, + } + case "2": + reviews = []map[string]interface{}{ + {"id": 3, "body": "r3", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u3"}}, + {"id": 4, "body": "r4", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u4"}}, + } + case "3": + // Empty page — signals end of data + reviews = []map[string]interface{}{} + default: + t.Fatalf("unexpected page: %s", page) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, 10) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 4 { + t.Fatalf("expected 4 reviews, got %d", len(reviews)) + } + // 3 calls: page 1 (full), page 2 (full), page 3 (empty) + if callCount != 3 { + t.Errorf("expected 3 API calls, got %d", callCount) + } +} + +func TestListReviews_MaxPagesCutoff(t *testing.T) { + // When maxPages is hit and the last page is full, results are truncated + // and a warning would fire (we verify the reviews are still returned). + const pageSize = 2 + const maxPages = 2 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + page := r.URL.Query().Get("page") + + // Always return a full page (simulating more data exists) + var reviews []map[string]interface{} + var baseID int + switch page { + case "1": + baseID = 0 + case "2": + baseID = pageSize + default: + t.Fatalf("unexpected page %s (should not exceed maxPages)", page) + } + for i := 1; i <= pageSize; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": baseID + i, "body": fmt.Sprintf("r%d", baseID+i), + "state": "APPROVED", "commit_id": "sha1", + "user": map[string]string{"login": "user"}, + }) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, maxPages) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Should return all reviews fetched within the cap + expectedCount := pageSize * maxPages + if len(reviews) != expectedCount { + t.Fatalf("expected %d reviews, got %d", expectedCount, len(reviews)) + } + if callCount != maxPages { + t.Errorf("expected %d API calls, got %d", maxPages, callCount) + } + // Verify concatenation order + for i, r := range reviews { + if r.ID != int64(i+1) { + t.Errorf("review[%d]: expected ID %d, got %d", i, i+1, r.ID) + } + } +} + +func TestListReviews_EmptyFirstPage(t *testing.T) { + // PR with no reviews: first page returns empty array. + callCount := 0 + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + json.NewEncoder(w).Encode([]map[string]interface{}{}) + }) + c.SetReviewPagination(10, 5) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 0 { + t.Fatalf("expected 0 reviews, got %d", len(reviews)) + } + if callCount != 1 { + t.Errorf("expected 1 API call, got %d", callCount) + } +} diff --git a/github/reviews.go b/github/reviews.go index 6a129bd..b448d53 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -153,11 +153,21 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, // ListReviews retrieves all reviews for a pull request with pagination. // GitHub review states are translated to canonical vcs values. func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) { - var allReviews []vcs.Review + perPage := reviewsPerPage + if c.reviewPageSize > 0 { + perPage = c.reviewPageSize + } + maxPages := maxReviewPages + if c.reviewMaxPages > 0 { + maxPages = c.reviewMaxPages + } - for page := 1; page <= maxReviewPages; page++ { + var allReviews []vcs.Review + truncated := false + + for page := 1; page <= maxPages; page++ { reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewsPerPage, page) + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, perPage, page) body, err := c.doGet(ctx, reqURL) if err != nil { @@ -183,17 +193,23 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int }) } - if len(responses) < reviewsPerPage { + if len(responses) < perPage { break } - if page == maxReviewPages { - slog.Warn("ListReviews hit page limit; results may be truncated", - "owner", owner, "repo", repo, "pr", number, - "maxPages", maxReviewPages, "reviewsFetched", len(allReviews)) + // If we just fetched page maxPages and it was full (the short-page break + // above didn't fire), more reviews likely exist beyond our limit. + if page == maxPages { + truncated = true } } + if truncated { + slog.Warn("ListReviews hit page limit; results may be truncated", + "owner", owner, "repo", repo, "pr", number, + "maxPages", maxPages, "reviewsFetched", len(allReviews)) + } + return allReviews, nil }