From 1e695b1c8febf17aad3bfa7bf0a819ef94b98ad5 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Thu, 6 Oct 2016 14:47:20 -0700 Subject: [PATCH] oauth2: fix brittle test A change introduced in https://golang.org/cl/18692 expanded upon the errors returned by the json package to be more informative about where the error occurred. This breaks a test in oauth2 that relies on the exact form that an error takes. Fix this test by simply checking whether it passes or not. Fixes golang/go#17363 Updates golang/go#11811 Change-Id: I0062dc64fc1a8fd094b14ed1d0b21528edfbb282 Reviewed-on: https://go-review.googlesource.com/30600 Reviewed-by: Brad Fitzpatrick Run-TryBot: Brad Fitzpatrick --- oauth2_test.go | 42 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/oauth2_test.go b/oauth2_test.go index 982ea99..e98c01a 100644 --- a/oauth2_test.go +++ b/oauth2_test.go @@ -5,15 +5,12 @@ package oauth2 import ( - "encoding/json" "errors" "fmt" "io/ioutil" "net/http" "net/http/httptest" "net/url" - "reflect" - "strconv" "testing" "time" @@ -210,23 +207,22 @@ const day = 24 * time.Hour func TestExchangeRequest_JSONResponse_Expiry(t *testing.T) { seconds := int32(day.Seconds()) - jsonNumberType := reflect.TypeOf(json.Number("0")) for _, c := range []struct { expires string - expect error + want bool }{ - {fmt.Sprintf(`"expires_in": %d`, seconds), nil}, - {fmt.Sprintf(`"expires_in": "%d"`, seconds), nil}, // PayPal case - {fmt.Sprintf(`"expires": %d`, seconds), nil}, // Facebook case - {`"expires": false`, &json.UnmarshalTypeError{Value: "bool", Type: jsonNumberType}}, // wrong type - {`"expires": {}`, &json.UnmarshalTypeError{Value: "object", Type: jsonNumberType}}, // wrong type - {`"expires": "zzz"`, &strconv.NumError{Func: "ParseInt", Num: "zzz", Err: strconv.ErrSyntax}}, // wrong value + {fmt.Sprintf(`"expires_in": %d`, seconds), true}, + {fmt.Sprintf(`"expires_in": "%d"`, seconds), true}, // PayPal case + {fmt.Sprintf(`"expires": %d`, seconds), true}, // Facebook case + {`"expires": false`, false}, // wrong type + {`"expires": {}`, false}, // wrong type + {`"expires": "zzz"`, false}, // wrong value } { - testExchangeRequest_JSONResponse_expiry(t, c.expires, c.expect) + testExchangeRequest_JSONResponse_expiry(t, c.expires, c.want) } } -func testExchangeRequest_JSONResponse_expiry(t *testing.T, exp string, expect error) { +func testExchangeRequest_JSONResponse_expiry(t *testing.T, exp string, want bool) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.Write([]byte(fmt.Sprintf(`{"access_token": "90d", "scope": "user", "token_type": "bearer", %s}`, exp))) @@ -236,19 +232,15 @@ func testExchangeRequest_JSONResponse_expiry(t *testing.T, exp string, expect er t1 := time.Now().Add(day) tok, err := conf.Exchange(context.Background(), "exchange-code") t2 := time.Now().Add(day) - // Do a fmt.Sprint comparison so either side can be - // nil. fmt.Sprint just stringifies them to "", and no - // non-nil expected error ever stringifies as "", so this - // isn't terribly disgusting. We do this because Go 1.4 and - // Go 1.5 return a different deep value for - // json.UnmarshalTypeError. In Go 1.5, the - // json.UnmarshalTypeError contains a new field with a new - // non-zero value. Rather than ignore it here with reflect or - // add new files and +build tags, just look at the strings. - if fmt.Sprint(err) != fmt.Sprint(expect) { - t.Errorf("Error = %v; want %v", err, expect) + + if got := (err == nil); got != want { + if want { + t.Errorf("unexpected error: got %v", err) + } else { + t.Errorf("unexpected success") + } } - if err != nil { + if !want { return } if !tok.Valid() {