Skip to content

Commit

Permalink
Use httptest instead of shortcutting roundtrippers for HTTP testing
Browse files Browse the repository at this point in the history 
Using roundtrippers to modify HTTP requests for testing is pretty confusing, especially when there's a great testing tool right there for us to use!

This also lets us completely remove the factory's `HttpClient` member
  • Loading branch information
@moskyb
moskyb committed Aug 18, 2024
1 parent 74530db commit 5205cb7
Show file tree
Hide file tree
Showing 7 changed files with 158 additions and 108 deletions.
60 changes: 39 additions & 21 deletions internal/build/resolver/current_user_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"io"
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
Expand All @@ -30,20 +31,21 @@ func TestResolveBuildForCurrentUser(t *testing.T) {
t.Run("Errors if user cannot be found", func(t *testing.T) {
t.Parallel()

// mock a failed repsonse
transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
return &http.Response{
StatusCode: http.StatusNotFound,
}, nil
})
client := &http.Client{Transport: transport}
f := &factory.Factory{
RestAPIClient: buildkite.NewClient(client),
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNotFound)
}))
t.Cleanup(s.Close)

apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
if err != nil {
t.Fatal(err)
}

r := resolver.ResolveBuildForCurrentUser("main", pipelineResolver, f)
_, err := r(context.Background())
// mock a failed repsonse
f := &factory.Factory{RestAPIClient: apiClient}

r := resolver.ResolveBuildForCurrentUser("main", pipelineResolver, f)
_, err = r(context.Background())
if err == nil {
t.Fatal("Resolver should return error if user not found")
}
Expand Down Expand Up @@ -72,16 +74,24 @@ func TestResolveBuildForCurrentUser(t *testing.T) {
Body: io.NopCloser(bytes.NewReader(in)),
},
}

// mock a failed repsonse
transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resp := responses[callIndex]
callIndex++
return &resp, nil
})
client := &http.Client{Transport: transport}
w.WriteHeader(resp.StatusCode)
io.Copy(w, resp.Body)
}))
t.Cleanup(s.Close)

apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
if err != nil {
t.Fatal(err)
}

fs := afero.NewMemMapFs()
f := &factory.Factory{
RestAPIClient: buildkite.NewClient(client),
RestAPIClient: apiClient,
Config: config.New(fs, nil),
}

Expand Down Expand Up @@ -118,16 +128,24 @@ func TestResolveBuildForCurrentUser(t *testing.T) {
Body: io.NopCloser(strings.NewReader("[]")),
},
}

// mock a failed repsonse
transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resp := responses[callIndex]
callIndex++
return &resp, nil
})
client := &http.Client{Transport: transport}
w.WriteHeader(resp.StatusCode)
io.Copy(w, resp.Body)
}))
t.Cleanup(s.Close)

apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
if err != nil {
t.Fatal(err)
}

fs := afero.NewMemMapFs()
f := &factory.Factory{
RestAPIClient: buildkite.NewClient(client),
RestAPIClient: apiClient,
Config: config.New(fs, nil),
}

Expand Down
28 changes: 12 additions & 16 deletions internal/build/resolver/user_builds_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ package resolver_test
import (
"context"
"errors"
"io"
"net/http"
"strings"
"net/http/httptest"
"testing"

"github.com/buildkite/cli/v3/internal/build/resolver"
Expand All @@ -24,15 +23,18 @@ func TestResolveBuildFromUserId(t *testing.T) {
return nil, errors.New("")
}

transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
return &http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(strings.NewReader(``)),
}, nil
})
client := &http.Client{Transport: transport}
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
}))
t.Cleanup(s.Close)

apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
if err != nil {
t.Fatal(err)
}

f := &factory.Factory{
RestAPIClient: buildkite.NewClient(client),
RestAPIClient: apiClient,
}

t.Run("Errors if pipeline cannot be resolved", func(t *testing.T) {
Expand All @@ -55,9 +57,3 @@ func TestResolveBuildFromUserId(t *testing.T) {
}
})
}

type roundTripperFunc func(*http.Request) (*http.Response, error)

func (fn roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) {
return fn(r)
}
42 changes: 27 additions & 15 deletions internal/build/resolver/userid_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"io"
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
Expand All @@ -30,20 +31,24 @@ func TestResolveBuildFromUserUUID(t *testing.T) {
t.Run("Errors when user id is not a member of the organization", func(t *testing.T) {
t.Parallel()
// mock a failed repsonse
transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
return &http.Response{
StatusCode: http.StatusNotFound,
}, nil
})
client := &http.Client{Transport: transport}
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNotFound)
}))
t.Cleanup(s.Close)

apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
if err != nil {
t.Fatal(err)
}

fs := afero.NewMemMapFs()
f := &factory.Factory{
RestAPIClient: buildkite.NewClient(client),
RestAPIClient: apiClient,
Config: config.New(fs, nil),
}

r := resolver.ResolveBuildForUserID("1234", pipelineResolver, f)
_, err := r(context.Background())
_, err = r(context.Background())

if err == nil {
t.Fatal("Resolver should return error if user not found")
Expand All @@ -59,30 +64,37 @@ func TestResolveBuildFromUserUUID(t *testing.T) {
{
StatusCode: http.StatusOK,
Body: io.NopCloser(strings.NewReader(`[{
"id": "abc123-4567-8910-...",
"id": "abc123-4567-8910-...",
"number": 584,
"creator": {
"id": "0183c4e6-c88c-xxxx-b15e-7801077a9181",
"graphql_id": "VXNlci0tLTAxODNjNGU2LWM4OGxxxxxxxxxiMTVlLTc4MDEwNzdhOTE4MQ=="
}
}
}]`)),
},
{
StatusCode: http.StatusOK,
Body: io.NopCloser(bytes.NewReader(in)),
},
}

// mock a failed repsonse
transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resp := responses[callIndex]
callIndex++
return &resp, nil
})
w.WriteHeader(resp.StatusCode)
io.Copy(w, resp.Body)
}))
t.Cleanup(s.Close)

apiClient, err := buildkite.NewOpts(buildkite.WithBaseURL(s.URL))
if err != nil {
t.Fatal(err)
}

client := &http.Client{Transport: transport}
fs := afero.NewMemMapFs()
f := &factory.Factory{
RestAPIClient: buildkite.NewClient(client),
RestAPIClient: apiClient,
Config: config.New(fs, nil),
}

Expand Down
64 changes: 34 additions & 30 deletions internal/pipeline/resolver/repository_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package resolver

import (
"io"
"net/http"
"strings"
"net/http/httptest"
"testing"

"github.com/buildkite/cli/v3/internal/config"
Expand All @@ -13,20 +12,16 @@ import (
"github.com/spf13/afero"
)

type roundTripperFunc func(*http.Request) (*http.Response, error)

func (fn roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) {
return fn(r)
}

func TestResolvePipelinesFromPath(t *testing.T) {
t.Parallel()

t.Run("no pipelines found", func(t *testing.T) {
t.Parallel()
// mock a response that doesn't match the current repository url
client := mockHttpClient(`[{"slug": "my-pipeline", "repository": "git@github.com:buildkite/test.git"}]`)
f := testFactory(client, "testOrg", testRepository())
s := mockHTTPServer(`[{"slug": "my-pipeline", "repository": "git@github.com:buildkite/test.git"}]`)
t.Cleanup(s.Close)

f := testFactory(t, s.URL, "testOrg", testRepository())
pipelines, err := resolveFromRepository(f)
if err != nil {
t.Errorf("Error: %s", err)
Expand All @@ -39,8 +34,10 @@ func TestResolvePipelinesFromPath(t *testing.T) {
t.Run("one pipeline", func(t *testing.T) {
t.Parallel()
// mock an http client response with a single pipeline matching the current repo url
client := mockHttpClient(`[{"slug": "my-pipeline", "repository": "git@github.com:buildkite/cli.git"}]`)
f := testFactory(client, "testOrg", testRepository())
s := mockHTTPServer(`[{"slug": "my-pipeline", "repository": "git@github.com:buildkite/cli.git"}]`)
t.Cleanup(s.Close)

f := testFactory(t, s.URL, "testOrg", testRepository())
pipelines, err := resolveFromRepository(f)
if err != nil {
t.Errorf("Error: %s", err)
Expand All @@ -53,9 +50,10 @@ func TestResolvePipelinesFromPath(t *testing.T) {
t.Run("multiple pipelines", func(t *testing.T) {
t.Parallel()
// mock an http client response with 2 pipelines matching the current repo url
client := mockHttpClient(`[{"slug": "my-pipeline", "repository": "git@github.com:buildkite/cli.git"},
{"slug": "my-pipeline-2", "repository": "git@github.com:buildkite/cli.git"}]`)
f := testFactory(client, "testOrg", testRepository())
s := mockHTTPServer(`[{"slug": "my-pipeline", "repository": "git@github.com:buildkite/cli.git"}, {"slug": "my-pipeline-2", "repository": "git@github.com:buildkite/cli.git"}]`)
t.Cleanup(s.Close)

f := testFactory(t, s.URL, "testOrg", testRepository())
pipelines, err := resolveFromRepository(f)
if err != nil {
t.Errorf("Error: %s", err)
Expand All @@ -66,8 +64,10 @@ func TestResolvePipelinesFromPath(t *testing.T) {
})

t.Run("no repository found", func(t *testing.T) {
client := mockHttpClient(`[{"slug": "", "repository": ""}]`)
f := testFactory(client, "testOrg", nil)
s := mockHTTPServer(`[{"slug": "", "repository": ""}]`)
t.Cleanup(s.Close)

f := testFactory(t, s.URL, "testOrg", nil)
pipelines, err := resolveFromRepository(f)
if pipelines != nil {
t.Errorf("Expected nil, got %v", pipelines)
Expand All @@ -78,8 +78,10 @@ func TestResolvePipelinesFromPath(t *testing.T) {
})

t.Run("no remote repository found", func(t *testing.T) {
client := mockHttpClient(`[{"slug": "", "repository": ""}]`)
f := testFactory(client, "testOrg", testRepository())
s := mockHTTPServer(`[{"slug": "", "repository": ""}]`)
t.Cleanup(s.Close)

f := testFactory(t, s.URL, "testOrg", testRepository())
pipelines, err := resolveFromRepository(f)
if pipelines != nil {
t.Errorf("Expected nil, got %v", pipelines)
Expand All @@ -95,24 +97,26 @@ func testRepository() *git.Repository {
return repo
}

func testFactory(client *http.Client, org string, repo *git.Repository) *factory.Factory {
bkClient := buildkite.NewClient(client)
func testFactory(t *testing.T, serverURL string, org string, repo *git.Repository) *factory.Factory {
t.Helper()

bkClient, err := buildkite.NewOpts(buildkite.WithBaseURL(serverURL))
if err != nil {
t.Errorf("Error creating buildkite client: %s", err)
}

conf := config.New(afero.NewMemMapFs(), nil)
conf.SelectOrganization(org)
return &factory.Factory{
Config: conf,
RestAPIClient: bkClient,
HttpClient: client,
GitRepository: repo,
}
}

func mockHttpClient(response string) *http.Client {
transport := roundTripperFunc(func(r *http.Request) (*http.Response, error) {
return &http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(strings.NewReader(response)),
}, nil
})
return &http.Client{Transport: transport}
func mockHTTPServer(response string) *httptest.Server {
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
w.Write([]byte(response))
}))
}
Loading

0 comments on commit 5205cb7

Please sign in to comment.