diff --git a/github/client.go b/github/client.go index db55266..c131d3a 100644 --- a/github/client.go +++ b/github/client.go @@ -89,6 +89,10 @@ func asAPIError(err error) (*APIError, bool) { // SetHTTPClient and SetRetryBackoff are intended for test setup only and must // be called before any goroutines issue requests; they have no synchronization. type Client struct { + // TODO: baseURL is populated by NewClient but not yet consumed by doRequest/doGet. + // Higher-level exported methods (GetPullRequest, etc.) will use it to + // construct request URLs; remove this field if those methods end up + // accepting full URLs instead. baseURL string token string http *http.Client @@ -126,6 +130,11 @@ func (c *Client) SetHTTPClient(hc *http.Client) { // SetRetryBackoff sets the delays between retry attempts. // This is intended for testing to speed up retry tests. +// +// Note: if an empty non-nil slice is provided, Retry-After delays parsed from +// server responses will be computed and capped but not applied (because +// attempt < len(backoff) is always false). This is acceptable for the +// test-only use case but callers should be aware of this edge case. func (c *Client) SetRetryBackoff(backoff []time.Duration) { c.retryBackoff = backoff } diff --git a/github/client_test.go b/github/client_test.go index e679704..8a5e4a3 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -48,7 +48,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { attempts++ if attempts == 1 { - w.Header().Set("Retry-After", "1") + w.Header().Set("Retry-After", "0") w.WriteHeader(http.StatusTooManyRequests) w.Write([]byte("rate limited")) return @@ -242,18 +242,36 @@ func TestDoRequest_401_NoRetry(t *testing.T) { } func TestDoRequest_ContextCanceled(t *testing.T) { + // This test exercises the timer-cancel path in the retry select: + // select { case <-timer.C; case <-ctx.Done() } + // The server returns 429 with a long Retry-After, and we cancel the + // context shortly after the first response so that cancellation races + // against the timer rather than preventing the initial HTTP round-trip. + requestReceived := make(chan struct{}, 1) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case requestReceived <- struct{}{}: + default: + } w.Header().Set("Retry-After", "10") w.WriteHeader(http.StatusTooManyRequests) })) defer srv.Close() c := NewClient("tok", srv.URL) - c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}) + c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second}) ctx, cancel := context.WithCancel(context.Background()) - // Cancel immediately so the retry timer is interrupted. - cancel() + defer cancel() + + // Cancel the context after the first request completes, while the + // client is blocked in the retry timer select. + go func() { + <-requestReceived + // Small delay to ensure we're inside the timer select. + time.Sleep(50 * time.Millisecond) + cancel() + }() _, err := c.doGet(ctx, srv.URL+"/test") if err == nil {