FIXIT: refactor http testing functions to builder pattern. Fixes #426 #440
Conversation
packager/testing/testing.go
Outdated
| func GetHH(t *testing.T, handler http.Handler, target string, host string, headers http.Header) *http.Response { | ||
| return GetBHH(t, handler, target, host, nil, headers) | ||
| // NewBody sets body related information for the request. | ||
| func (r *Request) NewBody(body io.Reader) *Request { |
There was a problem hiding this comment.
Ditto; rename SetBody or Body or AddBody.
packager/testing/testing.go
Outdated
| func GetH(t *testing.T, handler http.Handler, target string, headers http.Header) *http.Response { | ||
| return GetBHH(t, handler, target, "", nil, headers) | ||
| // NewHeader sets header related information for the request. | ||
| func (r *Request) NewHeader(host string, header http.Header) *Request { |
There was a problem hiding this comment.
I would rename SetHeaders or Headers. New makes me think it returns the headers. And plural because it is all the headers; not meant to be called incrementally for each one.
packager/signer/signer_test.go
Outdated
| @@ -90,17 +90,17 @@ func (this *SignerSuite) new(urlSets []util.URLSet) http.Handler { | |||
| } | |||
|
|
|||
| func (this *SignerSuite) get(t *testing.T, handler http.Handler, target string) *http.Response { | |||
| return pkgt.GetH(t, handler, target, http.Header{ | |||
| "AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion}}) | |||
| header := http.Header{"AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion}} | |||
There was a problem hiding this comment.
Is it possible to get rid of these three cryptic get functions, too? Just make this header value a top-level var inside signer_test.
packager/testing/testing.go
Outdated
| } | ||
|
|
||
| func GetHH(t *testing.T, handler http.Handler, target string, host string, headers http.Header) *http.Response { | ||
| return GetBHH(t, handler, target, host, nil, headers) | ||
| // NewBody sets body related information for the request. |
There was a problem hiding this comment.
"NewBody sets the body for the request. May be nil for an empty body."
packager/testing/testing.go
Outdated
| } | ||
|
|
||
| func GetH(t *testing.T, handler http.Handler, target string, headers http.Header) *http.Response { | ||
| return GetBHH(t, handler, target, "", nil, headers) | ||
| // NewHeader sets header related information for the request. |
There was a problem hiding this comment.
"NewHeader sets the headers for the request. Host may be empty for the default."
packager/testing/testing.go
Outdated
| } | ||
|
|
||
| func GetBHH(t *testing.T, handler http.Handler, target string, host string, body io.Reader, headers http.Header) *http.Response { | ||
| // Get returns the completed test request object. | ||
| func (r *Request) Get() *http.Response { |
There was a problem hiding this comment.
Rename this "Do", since it may do a post instead of a get.
packager/testing/testing.go
Outdated
| method = "POST" | ||
| headers.Set("Content-Type", "application/x-www-form-urlencoded") | ||
| r.NewHeader(r.Host, headers) |
There was a problem hiding this comment.
I think this'll overwrite any preexisting headers. e.g. if I call:
pkgt.NewRequest(t, handler, "/foo").
NewHeader("", {{"foo": "bar"}}).
NewBody(strings.NewReader("blah")).
Get()It won't send the foo header. I think you just want r.Header.Set(...) in the line above, and then you can get rid of the local headers variable and this NewHeader call.
banaag
force-pushed
the
fixit-test-refactor
branch
from
088a564
to
d68fca4
Compare
packager/signer/signer_test.go
Outdated
| func (this *SignerSuite) getB(t *testing.T, handler http.Handler, target string, body string) *http.Response { | ||
| return pkgt.GetBHH(t, handler, target, "", strings.NewReader(body), http.Header{ | ||
| "AMP-Cache-Transform": {"google"}, "Accept": {"application/signed-exchange;v=" + accept.AcceptedSxgVersion}}) | ||
| func (this *SignerSuite) getX(t *testing.T, handler http.Handler, target string) *http.Response { |
There was a problem hiding this comment.
Oops, I think you accidentally left this unused getX behind.
There was a problem hiding this comment.
Good catch, removed.
banaag
force-pushed
the
fixit-test-refactor
branch
from
d68fca4
to
bc6283b
Compare
Refactor http testing functions