|
|
|
net/http/httptest: add examples
Patch Set 1 #Patch Set 2 : diff -r 8d71734a0cb0 https://code.google.com/p/go/ #Patch Set 3 : diff -r 8d71734a0cb0 https://code.google.com/p/go/ #
Total comments: 10
Patch Set 4 : diff -r 052cab218361 https://code.google.com/p/go/ #
Total comments: 1
Patch Set 5 : diff -r 052cab218361 https://code.google.com/p/go/ #Total messages: 9
|
kisielk
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
|
12 years, 11 months ago (2013年02月06日 05:38:24 UTC) #1 |
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Nice examples https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... File src/pkg/net/http/httptest/example_test.go (right): https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:17: http.Error(w, req.URL.Path+": something went horribly wrong.", http.StatusInternalServerError) remove the period from the message https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:29: //Output: 500 - /foo: something went horribly wrong. "// Output:" add the space. https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:50: //Output: Hello, client ditto
https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... File src/pkg/net/http/httptest/example_test.go (right): https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:36: server := httptest.NewServer(http.HandlerFunc(handler)) in all the net/http tests, we never make a variable for "handler" even... just: server := httptest.NewServer(http.HandlerFunc(w http.ResponseWriter, req *http.Request) { .... }) defer server.Close() We also normally say "ts", but "server" is fine. https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:41: log.Fatal("server did not respond") how do you know? maybe there was no DNS and you never even reached the server. don't throw out err. it tries to say something useful.
Thanks for the feedback, PTAL. https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... File src/pkg/net/http/httptest/example_test.go (right): https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:17: http.Error(w, req.URL.Path+": something went horribly wrong.", http.StatusInternalServerError) On 2013年02月07日 22:21:43, adg wrote: > remove the period from the message Done https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:29: //Output: 500 - /foo: something went horribly wrong. On 2013年02月07日 22:21:43, adg wrote: > "// Output:" > > add the space. Done https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:36: server := httptest.NewServer(http.HandlerFunc(handler)) On 2013年02月07日 22:31:14, bradfitz wrote: > in all the net/http tests, we never make a variable for "handler" even... just: > > server := httptest.NewServer(http.HandlerFunc(w http.ResponseWriter, req > *http.Request) { > .... > }) > defer server.Close() > > We also normally say "ts", but "server" is fine. I took a look at how this was used in net/http and reworked it in to a similar style. https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:41: log.Fatal("server did not respond") On 2013年02月07日 22:31:14, bradfitz wrote: > how do you know? maybe there was no DNS and you never even reached the server. > don't throw out err. it tries to say something useful. Totally, not sure why I ate the error here but not in the other spot. Fixed to just print err. https://codereview.appspot.com/7314046/diff/4001/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:50: //Output: Hello, client On 2013年02月07日 22:21:43, adg wrote: > ditto Done
https://codereview.appspot.com/7314046/diff/4002/src/pkg/net/http/httptest/ex... File src/pkg/net/http/httptest/example_test.go (right): https://codereview.appspot.com/7314046/diff/4002/src/pkg/net/http/httptest/ex... src/pkg/net/http/httptest/example_test.go:17: http.Error(w, r.URL.Path+": something went horribly wrong", http.StatusInternalServerError) this is an XSS vector, so makes for a bad example. http.Error doesn't escape anything. Just say "something failed", or use html.EscapeString.
Hello golang-dev@googlegroups.com, adg@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
LGTM. I'll reorder one line before I submit. On Thu, Feb 7, 2013 at 8:14 PM, <kamil.kisiel@gmail.com> wrote: > Hello golang-dev@googlegroups.com, adg@golang.org, bradfitz@golang.org > (cc: golang-dev@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.**com/7314046/<https://codereview.appspot.com/7314... >
*** Submitted as https://code.google.com/p/go/source/detail?r=bad13530d9b3 *** net/http/httptest: add examples R=golang-dev, adg, bradfitz CC=golang-dev https://codereview.appspot.com/7314046 Committer: Brad Fitzpatrick <bradfitz@golang.org>