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 <iant@golang.org>

diff --git a/src/net/url/url.go b/src/net/url/url.go
index a0a2931c9ebc829ef63e128805749e460d0400e1..fb70dbac0de795951023aa12424463e8141ba6ec 100644 (file)
--- 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 73f5699152f23c7f18f08990257b16c2d983e282..a48da73e4ab74953808aba12863cd4426aecd2da 100644 (file)
--- 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)
                }
        }
 }