[gold] Add POC web test using httptest package.
This should allow better coverage of the web package, including making
sure we send the right JSON and response codes.
Note to reviewer
Ignore PS1 (pre-rebase).
PS2 adds/ports the AddIgnores test
PS3-6 adds/ports the Update test
Ignore PS7,8
PS 9 was supposed to be a follow up CL, but I misbranched. It adds
a bunch of error-handling checks, including some new logic to make
sure an ignore.Rule can't accidentally have, for example, an entire
book as a note.
Change-Id: Ib67b706c48608cf9847929c8c4193baa4560b1d4
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/265002
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
diff --git a/golden/go/web/frontend/types.go b/golden/go/web/frontend/types.go
index c5744a4..066951a 100644
--- a/golden/go/web/frontend/types.go
+++ b/golden/go/web/frontend/types.go
@@ -183,7 +183,8 @@
Duration string `json:"duration"`
// Filter is a url-encoded set of key-value pairs that can be used to match traces.
// For example: "config=angle_d3d9_es2&cpu_or_gpu_value=RadeonHD7770"
+ // Filter is limited to 10 KB.
Filter string `json:"filter"`
- // Note is a comment by a developer, typically a bug.
+ // Note is a short comment by a developer, typically a bug. Note is limited to 1 KB.
Note string `json:"note"`
}
diff --git a/golden/go/web/helpers.go b/golden/go/web/helpers.go
index 57f27a1..e8ec606 100644
--- a/golden/go/web/helpers.go
+++ b/golden/go/web/helpers.go
@@ -8,6 +8,17 @@
"go.skia.org/infra/go/util"
)
+const (
+ contentTypeHeader = "Content-Type"
+ jsonContentType = "application/json"
+
+ accessControlHeader = "Access-Control-Allow-Origin"
+ allowAllOrigins = "*"
+
+ contentTypeOptionsHeader = "X-Content-Type-Options"
+ noSniffContent = "nosniff"
+)
+
// ResponseEnvelope wraps all responses. Some fields might be empty depending
// on context or whether there was an error or not.
type ResponseEnvelope struct {
@@ -19,9 +30,9 @@
// setJSONHeaders sets secure headers for JSON responses.
func setJSONHeaders(w http.ResponseWriter) {
h := w.Header()
- h.Set("Access-Control-Allow-Origin", "*")
- h.Set("Content-Type", "application/json")
- h.Set("X-Content-Type-Options", "nosniff")
+ h.Set(accessControlHeader, allowAllOrigins)
+ h.Set(contentTypeHeader, jsonContentType)
+ h.Set(contentTypeOptionsHeader, noSniffContent)
}
// sendResponseWithPagination wraps the data of a successful response in a response envelope
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index a7db6cb..32776e2 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -94,6 +94,10 @@
anonymousExpensiveQuota *rate.Limiter
anonymousCheapQuota *rate.Limiter
+
+ // These can be set for unit tests to simplify the testing.
+ testingAuthAs string
+ testingNow time.Time
}
// NewHandlers returns a new instance of Handlers.
@@ -139,6 +143,7 @@
HandlersConfig: conf,
anonymousExpensiveQuota: rate.NewLimiter(maxAnonQPSExpensive, maxAnonBurstExpensive),
anonymousCheapQuota: rate.NewLimiter(maxAnonQPSCheap, maxAnonBurstCheap),
+ testingAuthAs: "", // Just to be explicit that we do *not* bypass Auth.
}, nil
}
@@ -811,7 +816,7 @@
// IgnoresUpdateHandler updates an existing ignores rule.
func (wh *Handlers) IgnoresUpdateHandler(w http.ResponseWriter, r *http.Request) {
defer metrics2.FuncTimer().Stop()
- user := login.LoggedInAs(r)
+ user := wh.loggedInAs(r)
if user == "" {
http.Error(w, "You must be logged in to update an ignore rule.", http.StatusUnauthorized)
return
@@ -826,16 +831,15 @@
httputils.ReportError(w, err, "invalid ignore rule input", http.StatusBadRequest)
return
}
-
- if err := wh.updateIgnoreRule(r.Context(), id, user, time.Now().Add(expiresInterval), irb); err != nil {
+ ignoreRule := ignore.NewRule(user, wh.now().Add(expiresInterval), irb.Filter, irb.Note)
+ ignoreRule.ID = id
+ if err := wh.IgnoreStore.Update(r.Context(), ignoreRule); err != nil {
httputils.ReportError(w, err, "Unable to update ignore rule", http.StatusInternalServerError)
return
}
sklog.Infof("Successfully updated ignore with id %s", id)
- if _, err := w.Write([]byte(`{"updated":"true"}`)); err != nil {
- sklog.Warningf("error responding success to update: %s", err)
- }
+ sendJSONResponse(w, map[string]string{"updated": "true"})
}
// getValidatedIgnoreRule parses the JSON from the given request into an IgnoreRuleBody. As a
@@ -848,6 +852,13 @@
if irb.Filter == "" {
return 0, irb, skerr.Fmt("must supply a filter")
}
+ // If a user accidentally includes a huge amount of text, we'd like to catch that here.
+ if len(irb.Filter) >= 10*1024 {
+ return 0, irb, skerr.Fmt("Filter must be < 10 KB")
+ }
+ if len(irb.Note) >= 1024 {
+ return 0, irb, skerr.Fmt("Note must be < 1 KB")
+ }
d, err := human.ParseDuration(irb.Duration)
if err != nil {
return 0, irb, skerr.Wrapf(err, "invalid duration")
@@ -855,20 +866,10 @@
return d, irb, nil
}
-// updateIgnoreRule updates a stored ignore.Rule with the values provided.
-func (wh *Handlers) updateIgnoreRule(ctx context.Context, id, updatedBy string, expires time.Time, irb frontend.IgnoreRuleBody) error {
- ignoreRule := ignore.NewRule(updatedBy, expires, irb.Filter, irb.Note)
- ignoreRule.ID = id
- if err := wh.IgnoreStore.Update(ctx, ignoreRule); err != nil {
- return skerr.Wrapf(err, "updating rule with id %s", id)
- }
- return nil
-}
-
// IgnoresDeleteHandler deletes an existing ignores rule.
func (wh *Handlers) IgnoresDeleteHandler(w http.ResponseWriter, r *http.Request) {
defer metrics2.FuncTimer().Stop()
- user := login.LoggedInAs(r)
+ user := wh.loggedInAs(r)
if user == "" {
http.Error(w, "You must be logged in to delete an ignore rule", http.StatusUnauthorized)
return
@@ -883,16 +884,14 @@
httputils.ReportError(w, err, "Unable to delete ignore rule", http.StatusInternalServerError)
return
}
- sklog.Infof("Successfully deleted ignore with id %s (or it wasn't there to begin with)", id)
- if _, err := w.Write([]byte(`{"deleted": "true"}`)); err != nil {
- sklog.Warningf("error responding success to ignore deletion: %s", err)
- }
+ sklog.Infof("Successfully deleted ignore with id %s", id)
+ sendJSONResponse(w, map[string]string{"deleted": "true"})
}
// IgnoresAddHandler is for adding a new ignore rule.
func (wh *Handlers) IgnoresAddHandler(w http.ResponseWriter, r *http.Request) {
defer metrics2.FuncTimer().Stop()
- user := login.LoggedInAs(r)
+ user := wh.loggedInAs(r)
if user == "" {
http.Error(w, "You must be logged in to add an ignore rule", http.StatusUnauthorized)
return
@@ -904,22 +903,14 @@
return
}
- if err := wh.addIgnoreRule(r.Context(), user, time.Now().Add(expiresInterval), irb); err != nil {
+ ignoreRule := ignore.NewRule(user, wh.now().Add(expiresInterval), irb.Filter, irb.Note)
+ if err := wh.IgnoreStore.Create(r.Context(), ignoreRule); err != nil {
httputils.ReportError(w, err, "Failed to create ignore rule", http.StatusInternalServerError)
+ return
}
- sklog.Infof("Successfully added ignore from %s", user)
- if _, err := w.Write([]byte(`{"added":"true"}`)); err != nil {
- sklog.Warningf("error responding success to added: %s", err)
- }
-}
-// addIgnoreRule creates and saves an ignore rule to the store.
-func (wh *Handlers) addIgnoreRule(ctx context.Context, user string, expires time.Time, irb frontend.IgnoreRuleBody) error {
- ignoreRule := ignore.NewRule(user, expires, irb.Filter, irb.Note)
- if err := wh.IgnoreStore.Create(ctx, ignoreRule); err != nil {
- return skerr.Wrap(err)
- }
- return nil
+ sklog.Infof("Successfully added ignore from %s", user)
+ sendJSONResponse(w, map[string]string{"added": "true"})
}
// TriageHandler handles a request to change the triage status of one or more
@@ -1601,3 +1592,17 @@
Digests: xd,
}
}
+
+func (wh *Handlers) now() time.Time {
+ if !wh.testingNow.IsZero() {
+ return wh.testingNow
+ }
+ return time.Now()
+}
+
+func (wh *Handlers) loggedInAs(r *http.Request) string {
+ if wh.testingAuthAs != "" {
+ return wh.testingAuthAs
+ }
+ return login.LoggedInAs(r)
+}
diff --git a/golden/go/web/web_test.go b/golden/go/web/web_test.go
index c66e108..a39d198 100644
--- a/golden/go/web/web_test.go
+++ b/golden/go/web/web_test.go
@@ -2,10 +2,17 @@
import (
"context"
+ "encoding/json"
+ "errors"
"fmt"
+ "io/ioutil"
+ "net/http"
+ "net/http/httptest"
+ "strings"
"testing"
"time"
+ "github.com/gorilla/mux"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
@@ -35,6 +42,30 @@
"go.skia.org/infra/golden/go/web/frontend"
)
+func TestStubbedNow(t *testing.T) {
+ unittest.SmallTest(t)
+ fakeNow := time.Date(2020, time.January, 2, 3, 4, 5, 0, time.UTC)
+ wh := Handlers{}
+ assert.NotEqual(t, fakeNow, wh.now())
+
+ wh.testingNow = fakeNow
+ // Now, it's always the same
+ assert.Equal(t, fakeNow, wh.now())
+ assert.Equal(t, fakeNow, wh.now())
+ assert.Equal(t, fakeNow, wh.now())
+}
+
+func TestStubbedAuthAs(t *testing.T) {
+ unittest.SmallTest(t)
+ r := httptest.NewRequest(http.MethodGet, "/does/not/matter", nil)
+ wh := Handlers{}
+ assert.Equal(t, "", wh.loggedInAs(r))
+
+ const fakeUser = "user@example.com"
+ wh.testingAuthAs = fakeUser
+ assert.Equal(t, fakeUser, wh.loggedInAs(r))
+}
+
// TestByQuerySunnyDay is a unit test of the /byquery endpoint.
// It uses some example data based on the bug_revert corpus, which
// has some untriaged images that are easy to identify blames for.
@@ -932,12 +963,55 @@
assert.Len(t, xir, 3)
}
-func TestAddIgnoreRule(t *testing.T) {
+func TestHandlersThatRequireLogin(t *testing.T) {
+ unittest.SmallTest(t)
+
+ wh := Handlers{}
+
+ test := func(name string, endpoint http.HandlerFunc) {
+ t.Run(name, func(t *testing.T) {
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodPost, requestURL, strings.NewReader("does not matter"))
+ endpoint(w, r)
+
+ resp := w.Result()
+ assert.Equal(t, http.StatusUnauthorized, resp.StatusCode)
+ })
+ }
+ test("add", wh.IgnoresAddHandler)
+ test("update", wh.IgnoresUpdateHandler)
+ test("delete", wh.IgnoresDeleteHandler)
+ // TODO(kjlubick): check all handlers that need login, not just Ignores*
+}
+
+func TestHandlersThatTakeJSON(t *testing.T) {
+ unittest.SmallTest(t)
+
+ wh := Handlers{
+ testingAuthAs: "test@google.com",
+ }
+
+ test := func(name string, endpoint http.HandlerFunc) {
+ t.Run(name, func(t *testing.T) {
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodPost, requestURL, strings.NewReader("invalid JSON"))
+ endpoint(w, r)
+
+ resp := w.Result()
+ assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+ })
+ }
+ test("add", wh.IgnoresAddHandler)
+ test("update", wh.IgnoresUpdateHandler)
+ // TODO(kjlubick): check all handlers that process JSON
+}
+
+func TestAddIgnoreRule_SunnyDay(t *testing.T) {
unittest.SmallTest(t)
const user = "test@example.com"
- const filter = "a=b&c=d"
- const note = "skbug:9744"
+ var fakeNow = time.Date(2020, time.January, 2, 3, 4, 5, 0, time.UTC)
+ var oneWeekFromNow = time.Date(2020, time.January, 9, 3, 4, 5, 0, time.UTC)
mis := &mock_ignore.Store{}
defer mis.AssertExpectations(t)
@@ -946,9 +1020,9 @@
ID: "",
CreatedBy: user,
UpdatedBy: user,
- Expires: firstRuleExpire,
- Query: filter,
- Note: note,
+ Expires: oneWeekFromNow,
+ Query: "a=b&c=d",
+ Note: "skbug:9744",
}
mis.On("Create", testutils.AnyContext, expectedRule).Return(nil)
@@ -956,22 +1030,90 @@
HandlersConfig: HandlersConfig{
IgnoreStore: mis,
},
+ testingAuthAs: user,
+ testingNow: fakeNow,
}
- err := wh.addIgnoreRule(context.Background(), user, firstRuleExpire, frontend.IgnoreRuleBody{
- Duration: "not used", // this have already been processed to compute the expire time.
- Filter: filter,
- Note: note,
- })
- require.NoError(t, err)
+ w := httptest.NewRecorder()
+ body := strings.NewReader(`{"duration": "1w", "filter": "a=b&c=d", "note": "skbug:9744"}`)
+ r := httptest.NewRequest(http.MethodPost, requestURL, body)
+ wh.IgnoresAddHandler(w, r)
+
+ assertJSONResponseWas(t, http.StatusOK, `{"added":"true"}`, w)
}
-func TestUpdateIgnoreRule(t *testing.T) {
+func TestAddIgnoreRule_StoreFailure(t *testing.T) {
+ unittest.SmallTest(t)
+
+ mis := &mock_ignore.Store{}
+ defer mis.AssertExpectations(t)
+
+ mis.On("Create", testutils.AnyContext, mock.Anything).Return(errors.New("firestore broke"))
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ IgnoreStore: mis,
+ },
+ testingAuthAs: "test@google.com",
+ }
+ w := httptest.NewRecorder()
+ body := strings.NewReader(`{"duration": "1w", "filter": "a=b&c=d", "note": "skbug:9744"}`)
+ r := httptest.NewRequest(http.MethodPost, requestURL, body)
+ r = mux.SetURLVars(r, map[string]string{"id": "12345"})
+ wh.IgnoresAddHandler(w, r)
+
+ resp := w.Result()
+ assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
+}
+
+func TestGetValidatedIgnoreRule_InvalidInput(t *testing.T) {
+ unittest.SmallTest(t)
+
+ test := func(name, errorFragment, jsonInput string) {
+ t.Run(name, func(t *testing.T) {
+ r := httptest.NewRequest(http.MethodPost, requestURL, strings.NewReader(jsonInput))
+ _, _, err := getValidatedIgnoreRule(r)
+ assert.Error(t, err)
+ assert.Contains(t, err.Error(), errorFragment)
+ })
+ }
+
+ test("invalid JSON", "request JSON", "This should not be valid JSON")
+ // There's an instagram joke here... #nofilter
+ test("no filter", "supply a filter", `{"duration": "1w", "filter": "", "note": "skbug:9744"}`)
+ test("no duration", "invalid duration", `{"duration": "", "filter": "a=b", "note": "skbug:9744"}`)
+ test("invalid duration", "invalid duration", `{"duration": "bad", "filter": "a=b", "note": "skbug:9744"}`)
+ test("filter too long", "Filter must be", string(makeJSONWithLongFilter(t)))
+ test("note too long", "Note must be", string(makeJSONWithLongNote(t)))
+}
+
+func makeJSONWithLongFilter(t *testing.T) []byte {
+ superLongFilter := frontend.IgnoreRuleBody{
+ Duration: "1w",
+ Filter: strings.Repeat("a=b&", 10000),
+ Note: "really long filter",
+ }
+ superLongFilterBytes, err := json.Marshal(superLongFilter)
+ require.NoError(t, err)
+ return superLongFilterBytes
+}
+
+func makeJSONWithLongNote(t *testing.T) []byte {
+ superLongFilter := frontend.IgnoreRuleBody{
+ Duration: "1w",
+ Filter: "a=b",
+ Note: strings.Repeat("really long note ", 1000),
+ }
+ superLongFilterBytes, err := json.Marshal(superLongFilter)
+ require.NoError(t, err)
+ return superLongFilterBytes
+}
+
+func TestUpdateIgnoreRule_SunnyDay(t *testing.T) {
unittest.SmallTest(t)
const id = "12345"
const user = "test@example.com"
- const filter = "a=b&c=d"
- const note = "skbug:9744"
+ var fakeNow = time.Date(2020, time.January, 2, 3, 4, 5, 0, time.UTC)
+ var oneWeekFromNow = time.Date(2020, time.January, 9, 3, 4, 5, 0, time.UTC)
mis := &mock_ignore.Store{}
defer mis.AssertExpectations(t)
@@ -980,9 +1122,9 @@
ID: id,
CreatedBy: user,
UpdatedBy: user,
- Expires: firstRuleExpire,
- Query: filter,
- Note: note,
+ Expires: oneWeekFromNow,
+ Query: "a=b&c=d",
+ Note: "skbug:9744",
}
mis.On("Update", testutils.AnyContext, expectedRule).Return(nil)
@@ -990,15 +1132,121 @@
HandlersConfig: HandlersConfig{
IgnoreStore: mis,
},
+ testingAuthAs: user,
+ testingNow: fakeNow,
}
- err := wh.updateIgnoreRule(context.Background(), id, user, firstRuleExpire, frontend.IgnoreRuleBody{
- Duration: "not used", // this have already been processed to compute the expire time.
- Filter: filter,
- Note: note,
- })
- require.NoError(t, err)
+ w := httptest.NewRecorder()
+ body := strings.NewReader(`{"duration": "1w", "filter": "a=b&c=d", "note": "skbug:9744"}`)
+ r := httptest.NewRequest(http.MethodPost, requestURL, body)
+ r = setID(r, id)
+ wh.IgnoresUpdateHandler(w, r)
+
+ assertJSONResponseWas(t, http.StatusOK, `{"updated":"true"}`, w)
}
+func TestUpdateIgnoreRule_NoID(t *testing.T) {
+ unittest.SmallTest(t)
+
+ wh := Handlers{
+ testingAuthAs: "test@google.com",
+ }
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodPost, requestURL, strings.NewReader("doesn't matter"))
+ wh.IgnoresUpdateHandler(w, r)
+
+ resp := w.Result()
+ assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+}
+
+func TestUpdateIgnoreRule_StoreFailure(t *testing.T) {
+ unittest.SmallTest(t)
+ mis := &mock_ignore.Store{}
+ defer mis.AssertExpectations(t)
+
+ mis.On("Update", testutils.AnyContext, mock.Anything).Return(errors.New("firestore broke"))
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ IgnoreStore: mis,
+ },
+ testingAuthAs: "test@google.com",
+ }
+ w := httptest.NewRecorder()
+ body := strings.NewReader(`{"duration": "1w", "filter": "a=b&c=d", "note": "skbug:9744"}`)
+ r := httptest.NewRequest(http.MethodPost, requestURL, body)
+ r = mux.SetURLVars(r, map[string]string{"id": "12345"})
+ wh.IgnoresUpdateHandler(w, r)
+
+ resp := w.Result()
+ assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
+}
+
+func TestDeleteIgnoreRule_SunnyDay(t *testing.T) {
+ unittest.SmallTest(t)
+
+ const id = "12345"
+
+ mis := &mock_ignore.Store{}
+ defer mis.AssertExpectations(t)
+
+ mis.On("Delete", testutils.AnyContext, id).Return(nil)
+
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ IgnoreStore: mis,
+ },
+ testingAuthAs: "test@example.com",
+ }
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodPost, requestURL, nil)
+ r = setID(r, id)
+ wh.IgnoresDeleteHandler(w, r)
+
+ assertJSONResponseWas(t, http.StatusOK, `{"deleted":"true"}`, w)
+}
+
+func TestDeleteIgnoreRule_NoID(t *testing.T) {
+ unittest.SmallTest(t)
+
+ wh := Handlers{
+ testingAuthAs: "test@google.com",
+ }
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodPost, requestURL, strings.NewReader("doesn't matter"))
+ wh.IgnoresDeleteHandler(w, r)
+
+ resp := w.Result()
+ assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+}
+
+func TestDeleteIgnoreRule_StoreError(t *testing.T) {
+ unittest.SmallTest(t)
+
+ const id = "12345"
+
+ mis := &mock_ignore.Store{}
+ defer mis.AssertExpectations(t)
+
+ mis.On("Delete", testutils.AnyContext, id).Return(errors.New("firestore broke"))
+
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ IgnoreStore: mis,
+ },
+ testingAuthAs: "test@example.com",
+ }
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest(http.MethodPost, requestURL, nil)
+ r = setID(r, id)
+ wh.IgnoresDeleteHandler(w, r)
+
+ resp := w.Result()
+ assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
+}
+
+// Because we are calling our handlers directly, the target URL doesn't matter. The target URL
+// would only matter if we were calling into the router, so it knew which handler to call.
+const requestURL = "/does/not/matter"
+
var (
// These dates are arbitrary and don't matter. The logic for determining if an alert has
// "expired" is handled on the frontend.
@@ -1043,3 +1291,26 @@
ir.ParsedQuery = nil
}
}
+
+// assertJSONResponseWasOK asserts that the given ResponseRecorder was given the appropriate JSON
+// headers and saw a status OK (200) response.
+func assertJSONResponseWas(t *testing.T, status int, body string, w *httptest.ResponseRecorder) {
+ resp := w.Result()
+ assert.Equal(t, status, resp.StatusCode)
+ assert.Equal(t, jsonContentType, resp.Header.Get(contentTypeHeader))
+ assert.Equal(t, allowAllOrigins, resp.Header.Get(accessControlHeader))
+ assert.Equal(t, noSniffContent, resp.Header.Get(contentTypeOptionsHeader))
+ respBody, err := ioutil.ReadAll(resp.Body)
+ require.NoError(t, err)
+ // The JSON encoder includes a newline "\n" at the end of the body, which is awkward to include
+ // in the literals passed in above, so we add that here
+ assert.Equal(t, body+"\n", string(respBody))
+}
+
+// setID applies the ID mux.Var to a copy of the given request. In a normal server setting, mux will
+// parse the given url with a string that indicates how to extract variables (e.g.
+// '/json/ignores/save/{id}' and store those to the request's context. However, since we just call
+// the handler directly, we need to set those variables ourselves.
+func setID(r *http.Request, id string) *http.Request {
+ return mux.SetURLVars(r, map[string]string{"id": id})
+}
