Skip to content

Commit 5182066

Browse files
wmoucherewmouchere
committed
PR Review: make code more idiomatic and improve coverage
Fix the ...WithRateLimit funcs so that the happy path flows to the end of the method. Add unit tests for bareDoUntilFound and ...WithRateLimit funcs to validate behavior when receiving unexpected http status codes.
1 parent bce9d21 commit 5182066

8 files changed

+279
-24
lines changed

‎github/actions_artifacts.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ func (s *ActionsService) downloadArtifactWithoutRateLimit(ctx context.Context, u
157157
defer resp.Body.Close()
158158

159159
if resp.StatusCode != http.StatusFound {
160-
return nil, newResponse(resp), fmt.Errorf("unexpected status code: %s", resp.Status)
160+
return nil, newResponse(resp), fmt.Errorf("unexpected status code: %v", resp.Status)
161161
}
162162

163163
parsedURL, err := url.Parse(resp.Header.Get("Location"))
@@ -180,12 +180,12 @@ func (s *ActionsService) downloadArtifactWithRateLimit(ctx context.Context, u st
180180
}
181181
defer resp.Body.Close()
182182

183-
// If we received a valid Location in a 302 response
184-
if url != nil {
185-
return url, resp, nil
183+
// If we didn't receive a valid Location in a 302 response
184+
if url == nil {
185+
return nil, resp, fmt.Errorf("unexpected status code: %v", resp.Status)
186186
}
187187

188-
return nil, resp, fmt.Errorf("unexpected status code: %s", resp.Status)
188+
return url, resp, nil
189189
}
190190

191191
// DeleteArtifact deletes a workflow run artifact.

‎github/actions_artifacts_test.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"fmt"
1212
"net/http"
1313
"net/url"
14+
"strings"
1415
"testing"
1516

1617
"github.com/google/go-cmp/cmp"
@@ -479,6 +480,57 @@ func TestActionsService_DownloadArtifact_StatusMovedPermanently_followRedirects(
479480
}
480481
}
481482

483+
func TestActionsService_DownloadArtifact_unexpectedCode(t *testing.T) {
484+
t.Parallel()
485+
tcs := []struct {
486+
name string
487+
respectRateLimits bool
488+
}{
489+
{
490+
name: "withoutRateLimits",
491+
respectRateLimits: false,
492+
},
493+
{
494+
name: "withRateLimits",
495+
respectRateLimits: true,
496+
},
497+
}
498+
499+
for _, tc := range tcs {
500+
tc := tc
501+
t.Run(tc.name, func(t *testing.T) {
502+
t.Parallel()
503+
client, mux, serverURL := setup(t)
504+
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits
505+
506+
mux.HandleFunc("/repos/o/r/actions/artifacts/1/zip", func(w http.ResponseWriter, r *http.Request) {
507+
testMethod(t, r, "GET")
508+
redirectURL, _ := url.Parse(serverURL + baseURLPath + "/redirect")
509+
http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)
510+
})
511+
mux.HandleFunc("/redirect", func(w http.ResponseWriter, r *http.Request) {
512+
testMethod(t, r, "GET")
513+
w.WriteHeader(http.StatusNoContent)
514+
})
515+
516+
ctx := context.Background()
517+
url, resp, err := client.Actions.DownloadArtifact(ctx, "o", "r", 1, 1)
518+
if err == nil {
519+
t.Fatalf("Actions.DownloadArtifact should return error on unexpected code")
520+
}
521+
if !strings.Contains(err.Error(), "unexpected status code") {
522+
t.Error("Actions.DownloadArtifact should return unexpected status code")
523+
}
524+
if got, want := resp.Response.StatusCode, http.StatusNoContent; got != want {
525+
t.Errorf("Actions.DownloadArtifact return status %d, want %d", got, want)
526+
}
527+
if url != nil {
528+
t.Errorf("Actions.DownloadArtifact return %+v, want nil", url)
529+
}
530+
})
531+
}
532+
}
533+
482534
func TestActionsService_DeleteArtifact(t *testing.T) {
483535
t.Parallel()
484536
client, mux, _ := setup(t)

‎github/actions_workflow_jobs.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ func (s *ActionsService) getWorkflowJobLogsWithoutRateLimit(ctx context.Context,
165165
defer resp.Body.Close()
166166

167167
if resp.StatusCode != http.StatusFound {
168-
return nil, newResponse(resp), fmt.Errorf("unexpected status code: %s", resp.Status)
168+
return nil, newResponse(resp), fmt.Errorf("unexpected status code: %v", resp.Status)
169169
}
170170

171171
parsedURL, err := url.Parse(resp.Header.Get("Location"))
@@ -184,10 +184,10 @@ func (s *ActionsService) getWorkflowJobLogsWithRateLimit(ctx context.Context, u
184184
}
185185
defer resp.Body.Close()
186186

187-
// If we received a valid Location in a 302 response
188-
if url != nil {
189-
return url, resp, nil
187+
// If we didn't receive a valid Location in a 302 response
188+
if url == nil {
189+
return nil, resp, fmt.Errorf("unexpected status code: %v", resp.Status)
190190
}
191191

192-
return nil, resp, fmt.Errorf("unexpected status code: %s", resp.Status)
192+
return url, resp, nil
193193
}

‎github/actions_workflow_jobs_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"fmt"
1212
"net/http"
1313
"net/url"
14+
"strings"
1415
"testing"
1516
"time"
1617

@@ -333,6 +334,59 @@ func TestActionsService_GetWorkflowJobLogs_StatusMovedPermanently_followRedirect
333334
}
334335
}
335336

337+
func TestActionsService_GetWorkflowJobLogs_unexpectedCode(t *testing.T) {
338+
t.Parallel()
339+
tcs := []struct {
340+
name string
341+
respectRateLimits bool
342+
}{
343+
{
344+
name: "withoutRateLimits",
345+
respectRateLimits: false,
346+
},
347+
{
348+
name: "withRateLimits",
349+
respectRateLimits: true,
350+
},
351+
}
352+
353+
for _, tc := range tcs {
354+
tc := tc
355+
t.Run(tc.name, func(t *testing.T) {
356+
t.Parallel()
357+
client, mux, serverURL := setup(t)
358+
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits
359+
360+
// Mock a redirect link, which leads to an archive link
361+
mux.HandleFunc("/repos/o/r/actions/jobs/399444496/logs", func(w http.ResponseWriter, r *http.Request) {
362+
testMethod(t, r, "GET")
363+
redirectURL, _ := url.Parse(serverURL + baseURLPath + "/redirect")
364+
http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)
365+
})
366+
367+
mux.HandleFunc("/redirect", func(w http.ResponseWriter, r *http.Request) {
368+
testMethod(t, r, "GET")
369+
w.WriteHeader(http.StatusNoContent)
370+
})
371+
372+
ctx := context.Background()
373+
url, resp, err := client.Actions.GetWorkflowJobLogs(ctx, "o", "r", 399444496, 1)
374+
if err == nil {
375+
t.Fatalf("Actions.GetWorkflowJobLogs should return error on unexpected code")
376+
}
377+
if !strings.Contains(err.Error(), "unexpected status code") {
378+
t.Error("Actions.GetWorkflowJobLogs should return unexpected status code")
379+
}
380+
if got, want := resp.Response.StatusCode, http.StatusNoContent; got != want {
381+
t.Errorf("Actions.GetWorkflowJobLogs return status %d, want %d", got, want)
382+
}
383+
if url != nil {
384+
t.Errorf("Actions.GetWorkflowJobLogs return %+v, want nil", url)
385+
}
386+
})
387+
}
388+
}
389+
336390
func TestTaskStep_Marshal(t *testing.T) {
337391
t.Parallel()
338392
testJSONMarshal(t, &TaskStep{}, "{}")

‎github/actions_workflow_runs.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ func (s *ActionsService) getWorkflowRunAttemptLogsWithoutRateLimit(ctx context.C
274274
defer resp.Body.Close()
275275

276276
if resp.StatusCode != http.StatusFound {
277-
return nil, newResponse(resp), fmt.Errorf("unexpected status code: %s", resp.Status)
277+
return nil, newResponse(resp), fmt.Errorf("unexpected status code: %v", resp.Status)
278278
}
279279

280280
parsedURL, err := url.Parse(resp.Header.Get("Location"))
@@ -293,12 +293,12 @@ func (s *ActionsService) getWorkflowRunAttemptLogsWithRateLimit(ctx context.Cont
293293
}
294294
defer resp.Body.Close()
295295

296-
// If we received a valid Location in a 302 response
297-
if url != nil {
298-
return url, resp, nil
296+
// If we didn't receive a valid Location in a 302 response
297+
if url == nil {
298+
return nil, resp, fmt.Errorf("unexpected status code: %v", resp.Status)
299299
}
300300

301-
return nil, resp, fmt.Errorf("unexpected status code: %s", resp.Status)
301+
return url, resp, nil
302302
}
303303

304304
// RerunWorkflowByID re-runs a workflow by ID.
@@ -407,12 +407,12 @@ func (s *ActionsService) getWorkflowRunLogsWithRateLimit(ctx context.Context, u
407407
}
408408
defer resp.Body.Close()
409409

410-
// If we received a valid Location in a 302 response
411-
if url != nil {
412-
return url, resp, nil
410+
// If we didn't receive a valid Location in a 302 response
411+
if url == nil {
412+
return nil, resp, fmt.Errorf("unexpected status code: %v", resp.Status)
413413
}
414414

415-
return nil, resp, fmt.Errorf("unexpected status code: %s", resp.Status)
415+
return url, resp, nil
416416
}
417417

418418
// DeleteWorkflowRun deletes a workflow run by ID.

‎github/actions_workflow_runs_test.go‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"fmt"
1212
"net/http"
1313
"net/url"
14+
"strings"
1415
"testing"
1516
"time"
1617

@@ -334,6 +335,59 @@ func TestActionsService_GetWorkflowRunAttemptLogs_StatusMovedPermanently_followR
334335
}
335336
}
336337

338+
func TestActionsService_GetWorkflowRunAttemptLogs_unexpectedCode(t *testing.T) {
339+
t.Parallel()
340+
tcs := []struct {
341+
name string
342+
respectRateLimits bool
343+
}{
344+
{
345+
name: "withoutRateLimits",
346+
respectRateLimits: false,
347+
},
348+
{
349+
name: "withRateLimits",
350+
respectRateLimits: true,
351+
},
352+
}
353+
354+
for _, tc := range tcs {
355+
tc := tc
356+
t.Run(tc.name, func(t *testing.T) {
357+
t.Parallel()
358+
client, mux, serverURL := setup(t)
359+
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits
360+
361+
// Mock a redirect link, which leads to an archive link
362+
mux.HandleFunc("/repos/o/r/actions/runs/399444496/attempts/2/logs", func(w http.ResponseWriter, r *http.Request) {
363+
testMethod(t, r, "GET")
364+
redirectURL, _ := url.Parse(serverURL + baseURLPath + "/redirect")
365+
http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)
366+
})
367+
368+
mux.HandleFunc("/redirect", func(w http.ResponseWriter, r *http.Request) {
369+
testMethod(t, r, "GET")
370+
w.WriteHeader(http.StatusNoContent)
371+
})
372+
373+
ctx := context.Background()
374+
url, resp, err := client.Actions.GetWorkflowRunAttemptLogs(ctx, "o", "r", 399444496, 2, 1)
375+
if err == nil {
376+
t.Fatalf("Actions.GetWorkflowRunAttemptLogs should return error on unexpected code")
377+
}
378+
if !strings.Contains(err.Error(), "unexpected status code") {
379+
t.Error("Actions.GetWorkflowRunAttemptLogs should return unexpected status code")
380+
}
381+
if got, want := resp.Response.StatusCode, http.StatusNoContent; got != want {
382+
t.Errorf("Actions.GetWorkflowRunAttemptLogs return status %d, want %d", got, want)
383+
}
384+
if url != nil {
385+
t.Errorf("Actions.GetWorkflowRunAttemptLogs return %+v, want nil", url)
386+
}
387+
})
388+
}
389+
}
390+
337391
func TestActionsService_RerunWorkflowRunByID(t *testing.T) {
338392
t.Parallel()
339393
client, mux, _ := setup(t)
@@ -596,6 +650,59 @@ func TestActionsService_GetWorkflowRunLogs_StatusMovedPermanently_followRedirect
596650
}
597651
}
598652

653+
func TestActionsService_GetWorkflowRunLogs_unexpectedCode(t *testing.T) {
654+
t.Parallel()
655+
tcs := []struct {
656+
name string
657+
respectRateLimits bool
658+
}{
659+
{
660+
name: "withoutRateLimits",
661+
respectRateLimits: false,
662+
},
663+
{
664+
name: "withRateLimits",
665+
respectRateLimits: true,
666+
},
667+
}
668+
669+
for _, tc := range tcs {
670+
tc := tc
671+
t.Run(tc.name, func(t *testing.T) {
672+
t.Parallel()
673+
client, mux, serverURL := setup(t)
674+
client.RateLimitRedirectionalEndpoints = tc.respectRateLimits
675+
676+
// Mock a redirect link, which leads to an archive link
677+
mux.HandleFunc("/repos/o/r/actions/runs/399444496/logs", func(w http.ResponseWriter, r *http.Request) {
678+
testMethod(t, r, "GET")
679+
redirectURL, _ := url.Parse(serverURL + baseURLPath + "/redirect")
680+
http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)
681+
})
682+
683+
mux.HandleFunc("/redirect", func(w http.ResponseWriter, r *http.Request) {
684+
testMethod(t, r, "GET")
685+
w.WriteHeader(http.StatusNoContent)
686+
})
687+
688+
ctx := context.Background()
689+
url, resp, err := client.Actions.GetWorkflowRunLogs(ctx, "o", "r", 399444496, 1)
690+
if err == nil {
691+
t.Fatalf("Actions.GetWorkflowRunLogs should return error on unexpected code")
692+
}
693+
if !strings.Contains(err.Error(), "unexpected status code") {
694+
t.Error("Actions.GetWorkflowRunLogs should return unexpected status code")
695+
}
696+
if got, want := resp.Response.StatusCode, http.StatusNoContent; got != want {
697+
t.Errorf("Actions.GetWorkflowRunLogs return status %d, want %d", got, want)
698+
}
699+
if url != nil {
700+
t.Errorf("Actions.GetWorkflowRunLogs return %+v, want nil", url)
701+
}
702+
})
703+
}
704+
}
705+
599706
func TestActionService_ListRepositoryWorkflowRuns(t *testing.T) {
600707
t.Parallel()
601708
client, mux, _ := setup(t)

0 commit comments

Comments
 (0)