Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(httpext): return error on unsupported 101 response #1172

Merged
merged 2 commits into from
Sep 27, 2019
Merged

fix(httpext): return error on unsupported 101 response #1172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12bd929
fix(httpext): return error on unsupported 101 response
Sep 13, 2019
b3d9fd5
chore: disable godox in CI
Sep 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,5 @@ linters:
enable-all: true
disable:
- gochecknoinits
- godox
fast: false
10 changes: 10 additions & 0 deletions lib/netext/httpext/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ package httpext
import (
"bytes"
"context"
"fmt"
"io"
"io/ioutil"
"net"
Expand Down Expand Up @@ -314,6 +315,15 @@ func MakeRequest(ctx context.Context, preq *ParsedHTTPRequest) (*Response, error
mreq := preq.Req.WithContext(ctx)
res, resErr := client.Do(mreq)

// TODO(imiric): It would be safer to check for a writeable
na-- marked this conversation as resolved.
Show resolved Hide resolved
// response body here instead of status code, but those are
// wrapped in a read-only body when using client timeouts and are
// unusable until https://github.com/golang/go/issues/31391 is fixed.
if res != nil && res.StatusCode == http.StatusSwitchingProtocols {
imiric marked this conversation as resolved.
Show resolved Hide resolved
_ = res.Body.Close()
return nil, fmt.Errorf("unsupported response status: %s", res.Status)
}

resp.Body, resErr = readResponseBody(state, preq.ResponseType, res, resErr)
finishedReq := tracerTransport.processLastSavedRequest(wrapDecompressionError(resErr))
if finishedReq != nil {
Expand Down
27 changes: 27 additions & 0 deletions lib/netext/httpext/request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@ import (
"io"
"io/ioutil"
"net/http"
"net/http/httptest"
"net/url"
"testing"

"github.com/loadimpact/k6/lib"
"github.com/loadimpact/k6/stats"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -82,6 +86,29 @@ func TestMakeRequestError(t *testing.T) {
require.Error(t, err)
require.Equal(t, err.Error(), "unknown compressionType CompressionType(13)")
})

t.Run("invalid upgrade response", func(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Add("Connection", "Upgrade")
w.Header().Add("Upgrade", "h2c")
w.WriteHeader(http.StatusSwitchingProtocols)
}))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not using HTTPMultiBin here to avoid a circular reference.

Sidenote: I know creating test servers at runtime like this is fairly inexpensive and often encouraged in Go testing, but this feels like a needlessly heavy end-to-end test, when a simple unit test would've been enough if MakeRequest worked with interfaces instead of concrete http APIs. Tools like moq can help with generating test structs/mocks. Let me know if this can be avoided, and your thoughts on the interface approach.

Copy link
Collaborator

@mstoykov mstoykov Sep 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not using HTTPMultiBin here to avoid a circular reference.

This should be fixed. As we will need it at some point or another
In this particular part above you are basically doing what httpmultibin is doing ... just in a smaller scale. While I have no problems in the current situation and as a quick solution for the fact httpmultibin is using netext(if I remember correctly), for something that is probably not needed - I am fine with it. And if this would be the only place where this is needed - it's fine. But if we add more tests here (more accurately move them from the http js module here) we would probably need to use httpmultibin either way ;).

About the sidenote: My problem with mocking is that ... it tests your assumptions .. it doesn't test whether they are true. In the particular case, here it's probably fine, but in general I would say "No". Also if you go through most of the tests using httpmultibin .. if you need to set mocks for half of them it's probably going to be more work than the httpmultibin :(
I would say that definitely there are places that we can optimize and there are use cases for mocks (database connections mainly ;)) but in general if the price is not high I prefer to use the real thing ;)

Copy link
Contributor Author

@imiric imiric Sep 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My problem with mocking is that ... it tests your assumptions .. it doesn't test whether they are true.

Agreed. Though typically mocking would be reserved for unit tests only. We should also have extensive integration and end-to-end tests, which we currently do. I suppose this distinction of the traditional test pyramid is not that important in Go where the end-to-end approach in this case is very cheap. Still, having a clear separation of test type would be helpful in other ways. For a quick sanity check I'd rather run the full unit test suite in seconds, than a suite with tests of all type in minutes. I can run the full suite once before the last push, or wait for CI. But again, I'm not sure how important this is in Go, which has good test caching, from what I've seen.

Anyways, we can continue this discussion at another time/place. :)

You're right about fixing the circular import. I'll refactor it if it needs to be used again here.

defer srv.Close()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
state := &lib.State{
Options: lib.Options{RunTags: &stats.SampleTags{}},
Transport: srv.Client().Transport,
}
ctx = lib.WithState(ctx, state)
req, _ := http.NewRequest("GET", srv.URL, nil)
var preq = &ParsedHTTPRequest{Req: req, URL: &URL{u: req.URL}, Body: new(bytes.Buffer)}

res, err := MakeRequest(ctx, preq)

assert.Nil(t, res)
assert.EqualError(t, err, "unsupported response status: 101 Switching Protocols")
})
}

func TestURL(t *testing.T) {
Expand Down