diff --git a/apps/backend/internal/github/client_helpers.go b/apps/backend/internal/github/client_helpers.go index fa791838c3..724b05caf3 100644 --- a/apps/backend/internal/github/client_helpers.go +++ b/apps/backend/internal/github/client_helpers.go @@ -35,17 +35,45 @@ func getPRFeedback(ctx context.Context, c Client, owner, repo string, number int if err != nil { return nil, err } - reviews, err := c.ListPRReviews(ctx, owner, repo, number) - if err != nil { - return nil, err + fetchCtx, cancel := context.WithCancel(ctx) + defer cancel() + type feedbackResult struct { + reviews []PRReview + comments []PRComment + checks []CheckRun + err error } - comments, err := c.ListPRComments(ctx, owner, repo, number, nil) - if err != nil { - return nil, err - } - checks, err := c.ListCheckRuns(ctx, owner, repo, pr.HeadSHA) - if err != nil { - return nil, err + results := make(chan feedbackResult, 3) + go func() { + value, callErr := c.ListPRReviews(fetchCtx, owner, repo, number) + results <- feedbackResult{reviews: value, err: callErr} + }() + go func() { + value, callErr := c.ListPRComments(fetchCtx, owner, repo, number, nil) + results <- feedbackResult{comments: value, err: callErr} + }() + go func() { + value, callErr := c.ListCheckRuns(fetchCtx, owner, repo, pr.HeadSHA) + results <- feedbackResult{checks: value, err: callErr} + }() + + var reviews []PRReview + var comments []PRComment + var checks []CheckRun + for range 3 { + result := <-results + if result.err != nil { + return nil, result.err + } + if result.reviews != nil { + reviews = result.reviews + } + if result.comments != nil { + comments = result.comments + } + if result.checks != nil { + checks = result.checks + } } // Ensure non-nil slices so JSON serialization produces [] instead of null. if reviews == nil { diff --git a/apps/backend/internal/github/client_helpers_test.go b/apps/backend/internal/github/client_helpers_test.go index 4e84c0c20b..5b7a5690bb 100644 --- a/apps/backend/internal/github/client_helpers_test.go +++ b/apps/backend/internal/github/client_helpers_test.go @@ -1,10 +1,233 @@ package github import ( + "context" + "errors" "testing" "time" ) +func TestGetPRFeedbackStartsIndependentRequestsConcurrently(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + started := make(chan string, 3) + release := make(chan struct{}) + pr := &PR{Number: 1, HeadSHA: "head-sha"} + reviews := []PRReview{{ID: 1, State: reviewStateChangesRequested}} + comments := []PRComment{{ID: 2, Body: "please fix"}} + checks := []CheckRun{{Name: "lint", Status: checkStatusCompleted, Conclusion: checkConclusionFail}} + client := feedbackConcurrencyClient{ + started: started, + release: release, + pr: pr, + reviews: reviews, + comments: comments, + checks: checks, + } + result := make(chan struct { + feedback *PRFeedback + err error + }, 1) + go func() { + feedback, err := getPRFeedback(ctx, &client, "owner", "repo", 1) + result <- struct { + feedback *PRFeedback + err error + }{feedback: feedback, err: err} + }() + + assertStartedRequests(t, ctx, started) + close(release) + + got := <-result + if got.err != nil { + t.Fatalf("getPRFeedback() error = %v", got.err) + } + if got.feedback.PR != pr { + t.Error("getPRFeedback() did not preserve the PR") + } + if len(got.feedback.Reviews) != 1 || got.feedback.Reviews[0] != reviews[0] { + t.Errorf("getPRFeedback() reviews = %#v, want %#v", got.feedback.Reviews, reviews) + } + if len(got.feedback.Comments) != 1 || got.feedback.Comments[0] != comments[0] { + t.Errorf("getPRFeedback() comments = %#v, want %#v", got.feedback.Comments, comments) + } + if len(got.feedback.Checks) != 1 || got.feedback.Checks[0] != checks[0] { + t.Errorf("getPRFeedback() checks = %#v, want %#v", got.feedback.Checks, checks) + } + if !got.feedback.HasIssues { + t.Error("getPRFeedback() HasIssues = false, want true") + } +} + +func TestGetPRFeedbackNormalizesNilResponses(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + started := make(chan string, 3) + release := make(chan struct{}) + client := feedbackConcurrencyClient{ + started: started, + release: release, + pr: &PR{HeadSHA: "head-sha"}, + } + result := make(chan struct { + feedback *PRFeedback + err error + }, 1) + go func() { + feedback, err := getPRFeedback(ctx, &client, "owner", "repo", 1) + result <- struct { + feedback *PRFeedback + err error + }{feedback: feedback, err: err} + }() + + assertStartedRequests(t, ctx, started) + close(release) + got := <-result + if got.err != nil { + t.Fatalf("getPRFeedback() error = %v", got.err) + } + if got.feedback.Reviews == nil || got.feedback.Comments == nil || got.feedback.Checks == nil { + t.Errorf("getPRFeedback() slices = %#v, want non-nil empty slices", got.feedback) + } + if got.feedback.HasIssues { + t.Error("getPRFeedback() HasIssues = true, want false") + } +} + +func TestGetPRFeedbackReturnsWorkerErrorWithoutWaitingForBlockedSibling(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + failure := errors.New("checks failed") + started := make(chan string, 3) + release := make(chan struct{}) + cancelled := make(chan string, 2) + client := feedbackConcurrencyClient{ + started: started, + release: release, + cancelled: cancelled, + pr: &PR{HeadSHA: "head-sha"}, + fail: "checks", + failure: failure, + } + result := make(chan error, 1) + go func() { + _, err := getPRFeedback(ctx, &client, "owner", "repo", 1) + result <- err + }() + + assertStartedRequests(t, ctx, started) + close(release) + + select { + case err := <-result: + if !errors.Is(err, failure) { + t.Fatalf("getPRFeedback() error = %v, want %v", err, failure) + } + case <-ctx.Done(): + t.Fatal("getPRFeedback() waited for blocked sibling") + } + assertCancelledRequests(t, ctx, cancelled) +} + +func assertStartedRequests(t *testing.T, ctx context.Context, started <-chan string) { + t.Helper() + want := map[string]bool{"reviews": true, "comments": true, "checks": true} + for range 3 { + select { + case request := <-started: + if !want[request] { + t.Fatalf("unexpected or duplicate request start %q", request) + } + delete(want, request) + case <-ctx.Done(): + t.Fatalf("requests that did not start: %v", want) + } + } + if len(want) != 0 { + t.Fatalf("requests that did not start: %v", want) + } +} + +func assertCancelledRequests(t *testing.T, ctx context.Context, cancelled <-chan string) { + t.Helper() + want := map[string]bool{"reviews": true, "comments": true} + for range 2 { + select { + case request := <-cancelled: + if !want[request] { + t.Fatalf("unexpected or duplicate request cancellation %q", request) + } + delete(want, request) + case <-ctx.Done(): + t.Fatalf("requests that were not cancelled: %v", want) + } + } + if len(want) != 0 { + t.Fatalf("requests that were not cancelled: %v", want) + } +} + +type feedbackConcurrencyClient struct { + Client + started chan<- string + release <-chan struct{} + cancelled chan<- string + pr *PR + reviews []PRReview + comments []PRComment + checks []CheckRun + fail string + failure error +} + +func (c *feedbackConcurrencyClient) GetPR(context.Context, string, string, int) (*PR, error) { + return c.pr, nil +} + +func (c *feedbackConcurrencyClient) ListPRReviews(ctx context.Context, _ string, _ string, _ int) ([]PRReview, error) { + if err := c.wait(ctx, "reviews"); err != nil { + return nil, err + } + return c.reviews, nil +} + +func (c *feedbackConcurrencyClient) ListPRComments(ctx context.Context, _ string, _ string, _ int, _ *time.Time) ([]PRComment, error) { + if err := c.wait(ctx, "comments"); err != nil { + return nil, err + } + return c.comments, nil +} + +func (c *feedbackConcurrencyClient) ListCheckRuns(ctx context.Context, _ string, _ string, _ string) ([]CheckRun, error) { + if err := c.wait(ctx, "checks"); err != nil { + return nil, err + } + return c.checks, nil +} + +func (c *feedbackConcurrencyClient) wait(ctx context.Context, request string) error { + c.started <- request + if c.fail != "" && c.fail != request { + <-ctx.Done() + c.cancelled <- request + return ctx.Err() + } + select { + case <-c.release: + if c.fail == request { + return c.failure + } + return nil + case <-ctx.Done(): + return ctx.Err() + } +} + func TestBuildReviewSearchQuery(t *testing.T) { tests := []struct { name string