fix(github): add pagination tests and fix truncation warning logic
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 24s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 56s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m31s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 24s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 56s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m31s
F1: Add comprehensive pagination tests for ListReviews covering: - Multi-page behaviour (2 full + 1 partial page) - Exact-multiple-of-pageSize (extra empty-page round-trip) - maxReviewPages cutoff (cap hit, results still returned) - Empty first page (PR with no reviews) F2: Fix truncation warning logic by moving it outside the loop with a 'truncated' flag. Previously, the warning fired inline at page==maxPages which could miss the case where the short-page break fires first on the cap page. Now it only fires when the loop exits because the cap was reached and the last page was full (indicating more data likely exists). Also adds SetReviewPagination to Client for test-time override of page size and max pages, following the existing SetRetryBackoff pattern.
This commit is contained in:
+24
-8
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user