From: Dave Day Date: Fri, 9 Sep 2016 07:59:43 +0000 (+1000) Subject: net/url: modernise parse and unit tests X-Git-Tag: go1.8beta1~1379 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=9e876861017991fe1478421b85a83964ea7abc6d;p=gostls13.git net/url: modernise parse and unit tests Remove the naked returns and goto statements from parse. Make tests more consistent in the got/want ordering, and clean up some unnecessary helper functions. Change-Id: Iaa244cb8c00dd6b42836d95448bf02caa72bfabd Reviewed-on: https://go-review.googlesource.com/28890 Reviewed-by: Ian Lance Taylor --- diff --git a/src/net/url/url.go b/src/net/url/url.go index a0a2931c9e..fb70dbac0d 100644 --- a/src/net/url/url.go +++ b/src/net/url/url.go @@ -420,7 +420,7 @@ func Parse(rawurl string) (*URL, error) { u, frag := split(rawurl, "#", true) url, err := parse(u, false) if err != nil { - return nil, err + return nil, &Error{"parse", u, err} } if frag == "" { return url, nil @@ -437,31 +437,35 @@ func Parse(rawurl string) (*URL, error) { // The string rawurl is assumed not to have a #fragment suffix. // (Web browsers strip #fragment before sending the URL to a web server.) func ParseRequestURI(rawurl string) (*URL, error) { - return parse(rawurl, true) + url, err := parse(rawurl, true) + if err != nil { + return nil, &Error{"parse", rawurl, err} + } + return url, nil } // parse parses a URL from a string in one of two contexts. If // viaRequest is true, the URL is assumed to have arrived via an HTTP request, // in which case only absolute URLs or path-absolute relative URLs are allowed. // If viaRequest is false, all forms of relative URLs are allowed. -func parse(rawurl string, viaRequest bool) (url *URL, err error) { +func parse(rawurl string, viaRequest bool) (*URL, error) { var rest string + var err error if rawurl == "" && viaRequest { - err = errors.New("empty url") - goto Error + return nil, errors.New("empty url") } - url = new(URL) + url := new(URL) if rawurl == "*" { url.Path = "*" - return + return url, nil } // Split off possible leading "http:", "mailto:", etc. // Cannot contain escaped characters. if url.Scheme, rest, err = getscheme(rawurl); err != nil { - goto Error + return nil, err } url.Scheme = strings.ToLower(url.Scheme) @@ -479,8 +483,7 @@ func parse(rawurl string, viaRequest bool) (url *URL, err error) { return url, nil } if viaRequest { - err = errors.New("invalid URI for request") - goto Error + return nil, errors.New("invalid URI for request") } } @@ -489,7 +492,7 @@ func parse(rawurl string, viaRequest bool) (url *URL, err error) { authority, rest = split(rest[2:], "/", false) url.User, url.Host, err = parseAuthority(authority) if err != nil { - goto Error + return nil, err } } // Set Path and, optionally, RawPath. @@ -497,12 +500,9 @@ func parse(rawurl string, viaRequest bool) (url *URL, err error) { // the default escaping of Path is equivalent, to help make sure that people // don't rely on it in general. if err := url.setPath(rest); err != nil { - goto Error + return nil, err } return url, nil - -Error: - return nil, &Error{"parse", rawurl, err} } func parseAuthority(authority string) (user *Userinfo, host string, err error) { diff --git a/src/net/url/url_test.go b/src/net/url/url_test.go index 73f5699152..a48da73e4a 100644 --- a/src/net/url/url_test.go +++ b/src/net/url/url_test.go @@ -579,20 +579,6 @@ func ufmt(u *URL) string { u.Opaque, u.Scheme, user, pass, u.Host, u.Path, u.RawPath, u.RawQuery, u.Fragment, u.ForceQuery) } -func DoTest(t *testing.T, parse func(string) (*URL, error), name string, tests []URLTest) { - for _, tt := range tests { - u, err := parse(tt.in) - if err != nil { - t.Errorf("%s(%q) returned error %s", name, tt.in, err) - continue - } - if !reflect.DeepEqual(u, tt.out) { - t.Errorf("%s(%q):\n\thave %v\n\twant %v\n", - name, tt.in, ufmt(u), ufmt(tt.out)) - } - } -} - func BenchmarkString(b *testing.B) { b.StopTimer() b.ReportAllocs() @@ -618,7 +604,16 @@ func BenchmarkString(b *testing.B) { } func TestParse(t *testing.T) { - DoTest(t, Parse, "Parse", urltests) + for _, tt := range urltests { + u, err := Parse(tt.in) + if err != nil { + t.Errorf("Parse(%q) returned error %v", tt.in, err) + continue + } + if !reflect.DeepEqual(u, tt.out) { + t.Errorf("Parse(%q):\n\tgot %v\n\twant %v\n", tt.in, ufmt(u), ufmt(tt.out)) + } + } } const pathThatLooksSchemeRelative = "//not.a.user@not.a.host/just/a/path" @@ -665,9 +660,10 @@ var parseRequestURLTests = []struct { func TestParseRequestURI(t *testing.T) { for _, test := range parseRequestURLTests { _, err := ParseRequestURI(test.url) - valid := err == nil - if valid != test.expectedValid { - t.Errorf("Expected valid=%v for %q; got %v", test.expectedValid, test.url, valid) + if test.expectedValid && err != nil { + t.Errorf("ParseRequestURI(%q) gave err %v; want no error", test.url, err) + } else if !test.expectedValid && err == nil { + t.Errorf("ParseRequestURI(%q) gave nil error; want some error", test.url) } } @@ -676,45 +672,36 @@ func TestParseRequestURI(t *testing.T) { t.Fatalf("Unexpected error %v", err) } if url.Path != pathThatLooksSchemeRelative { - t.Errorf("Expected path %q; got %q", pathThatLooksSchemeRelative, url.Path) + t.Errorf("ParseRequestURI path:\ngot %q\nwant %q", url.Path, pathThatLooksSchemeRelative) } } -func DoTestString(t *testing.T, parse func(string) (*URL, error), name string, tests []URLTest) { - for _, tt := range tests { - u, err := parse(tt.in) +func TestURLString(t *testing.T) { + for _, tt := range urltests { + u, err := Parse(tt.in) if err != nil { - t.Errorf("%s(%q) returned error %s", name, tt.in, err) + t.Errorf("Parse(%q) returned error %s", tt.in, err) continue } expected := tt.in - if len(tt.roundtrip) > 0 { + if tt.roundtrip != "" { expected = tt.roundtrip } s := u.String() if s != expected { - t.Errorf("%s(%q).String() == %q (expected %q)", name, tt.in, s, expected) + t.Errorf("Parse(%q).String() == %q (expected %q)", tt.in, s, expected) } } -} -func TestURLString(t *testing.T) { - DoTestString(t, Parse, "Parse", urltests) - - // no leading slash on path should prepend + // No leading slash on path should prepend // slash on String() call - noslash := URLTest{ - "http://www.google.com/search", - &URL{ - Scheme: "http", - Host: "www.google.com", - Path: "search", - }, - "", + noslash := URL{ + Scheme: "http", + Host: "www.google.com", + Path: "search", } - s := noslash.out.String() - if s != noslash.in { - t.Errorf("Expected %s; go %s", noslash.in, s) + if got, want := noslash.String(), "http://www.google.com/search"; got != want { + t.Errorf("No slash\ngot %q\nwant %q", got, want) } } @@ -1013,7 +1000,7 @@ func TestResolveReference(t *testing.T) { mustParse := func(url string) *URL { u, err := Parse(url) if err != nil { - t.Fatalf("Expected URL to parse: %q, got error: %v", url, err) + t.Fatalf("Parse(%q) got err %v", url, err) } return u } @@ -1042,14 +1029,14 @@ func TestResolveReference(t *testing.T) { // Ensure Opaque resets the URL. url = base.ResolveReference(opaque) if *url != *opaque { - t.Errorf("ResolveReference failed to resolve opaque URL: want %#v, got %#v", url, opaque) + t.Errorf("ResolveReference failed to resolve opaque URL:\ngot %#v\nwant %#v", url, opaque) } // Test the convenience wrapper with an opaque URL too. url, err = base.Parse("scheme:opaque") if err != nil { t.Errorf(`URL(%q).Parse("scheme:opaque") failed: %v`, test.base, err) } else if *url != *opaque { - t.Errorf("Parse failed to resolve opaque URL: want %#v, got %#v", url, opaque) + t.Errorf("Parse failed to resolve opaque URL:\ngot %#v\nwant %#v", opaque, url) } else if base == url { // Ensure that new instances are returned, again. t.Errorf("Expected URL.Parse to return new URL instance.") @@ -1471,11 +1458,11 @@ func TestURLErrorImplementsNetError(t *testing.T) { continue } if err.Timeout() != tt.timeout { - t.Errorf("%d: err.Timeout(): want %v, have %v", i+1, tt.timeout, err.Timeout()) + t.Errorf("%d: err.Timeout(): got %v, want %v", i+1, err.Timeout(), tt.timeout) continue } if err.Temporary() != tt.temporary { - t.Errorf("%d: err.Temporary(): want %v, have %v", i+1, tt.temporary, err.Temporary()) + t.Errorf("%d: err.Temporary(): got %v, want %v", i+1, err.Temporary(), tt.temporary) } } }