Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Add support for specifying which browsers in /api/runs #280

Merged
merged 8 commits into from
Nov 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 2 additions & 18 deletions api_handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"fmt"
"io/ioutil"
"net/http"
"net/url"
"strconv"
"time"

Expand Down Expand Up @@ -52,7 +51,7 @@ func apiTestRunsHandler(w http.ResponseWriter, r *http.Request) {
}

var browserNames []string
if browserNames, err = GetBrowserNames(); err != nil {
if browserNames, err = ParseBrowsersParam(r); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffcarp is this special case of multiple-statements-on-one-line-with-error-checking a standard golang idiom?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, they're called 'short statements' - https://tour.golang.org/flowcontrol/6

http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
Expand Down Expand Up @@ -106,7 +105,7 @@ func apiTestRunGetHandler(w http.ResponseWriter, r *http.Request) {
}

var browserName string
browserName, err = getBrowserParam(r)
browserName, err = ParseBrowserParam(r)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
Expand Down Expand Up @@ -200,21 +199,6 @@ func apiTestRunPostHandler(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusCreated)
}

// getBrowserParam parses and validates the 'browser' param for the request.
// It returns "" by default (and in error cases).
func getBrowserParam(r *http.Request) (browser string, err error) {
browser = ""
params, err := url.ParseQuery(r.URL.RawQuery)
if err != nil {
return browser, err
}

if browser = params.Get("browser"); IsBrowserName(browser) {
return browser, nil
}
return "", nil
}

// getLastCompleteRunSHA returns the SHA[0:10] for the most recent run that exists for all initially-loaded browser
// names (see GetBrowserNames).
func getLastCompleteRunSHA(ctx context.Context) (sha string, err error) {
Expand Down
87 changes: 87 additions & 0 deletions params.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright 2017 Google Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.package wptdashboard

package wptdashboard

import (
"fmt"
"net/http"
"net/url"
"regexp"
"sort"
"strings"
)

// SHARegex is a regex for SHA[0:10] slice of a git hash.
var SHARegex = regexp.MustCompile("[0-9a-fA-F]{10}")

// ParseSHAParam parses and validates the 'sha' param for the request.
// It returns "latest" by default (and in error cases).
func ParseSHAParam(r *http.Request) (runSHA string, err error) {
// Get the SHA for the run being loaded (the first part of the path.)
runSHA = "latest"
params, err := url.ParseQuery(r.URL.RawQuery)
if err != nil {
return runSHA, err
}

runParam := params.Get("sha")
if SHARegex.MatchString(runParam) {
runSHA = runParam
}
return runSHA, err
}

// ParseBrowserParam parses and validates the 'browser' param for the request.
// It returns "" by default (and in error cases).
func ParseBrowserParam(r *http.Request) (browser string, err error) {
browser = r.URL.Query().Get("browser")
if "" == browser {
return "", nil
}
if IsBrowserName(browser) {
return browser, nil
}
return "", fmt.Errorf("invalid browser param %s", browser)
}

// ParseBrowsersParam returns a sorted list of browsers to include.
// It parses the 'browsers' parameter, split on commas, and also checks for the (repeatable) 'browser' params,
// before falling back to the default set of browsers.
func ParseBrowsersParam(r *http.Request) (browsers []string, err error) {
browsers = r.URL.Query()["browser"]
if browsersParam := r.URL.Query().Get("browsers"); browsersParam != "" {
browsers = append(browsers, strings.Split(browsersParam, ",")...)
}
// If no params found, return the default.
var browserNames []string
if browserNames, err = GetBrowserNames(); err != nil {
return nil, err
}
if len(browsers) == 0 {
return browserNames, nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto (logging)

}
// Otherwise filter to valid browser names.
for i := 0; i < len(browsers); {
if !IsBrowserName(browsers[i]) {
// 'Remove' browser by switching to end and cropping.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you say so 😃 @jeffcarp can you please golang-review this bit?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed #283 to include this.

browsers[len(browsers)-1], browsers[i] = browsers[i], browsers[len(browsers)-1]
browsers = browsers[:len(browsers)-1]
continue
}
i++
}
sort.Strings(browsers)
return browsers, nil
}
142 changes: 142 additions & 0 deletions params_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
// Copyright 2017 Google Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package wptdashboard

import (
"github.com/stretchr/testify/assert"
"net/http/httptest"
"testing"
)

func TestParseSHAParam(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/", nil)
runSHA, err := ParseSHAParam(r)
assert.Nil(t, err)
assert.Equal(t, "latest", runSHA)
}

func TestParseSHAParam_2(t *testing.T) {
sha := "0123456789"
r := httptest.NewRequest("GET", "http://wpt.fyi/?sha="+sha, nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: + sha

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files are gofmt'd, that's the way they like it 😕

runSHA, err := ParseSHAParam(r)
assert.Nil(t, err)
assert.Equal(t, sha, runSHA)
}

func TestParseSHAParam_BadRequest(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?sha=%zz", nil)
runSHA, err := ParseSHAParam(r)
assert.NotNil(t, err)
assert.Equal(t, "latest", runSHA)
}

func TestParseSHAParam_NonSHA(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?sha=123", nil)
runSHA, err := ParseSHAParam(r)
assert.Nil(t, err)
assert.Equal(t, "latest", runSHA)
}

func TestParseSHAParam_NonSHA_2(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?sha=zapper0123", nil)
runSHA, err := ParseSHAParam(r)
assert.Nil(t, err)
assert.Equal(t, "latest", runSHA)
}

func TestParseBrowserParam(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/", nil)
browser, err := ParseBrowserParam(r)
assert.Nil(t, err)
assert.Equal(t, "", browser)
}

func TestParseBrowserParam_Chrome(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browser=chrome", nil)
browser, err := ParseBrowserParam(r)
assert.Nil(t, err)
assert.Equal(t, "chrome", browser)
}

func TestParseBrowserParam_Invalid(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browser=invalid", nil)
browser, err := ParseBrowserParam(r)
assert.NotNil(t, err)
assert.Equal(t, "", browser)
}

func TestParseBrowsersParam(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
defaultBrowsers, err := GetBrowserNames()
assert.Equal(t, defaultBrowsers, browsers)
}

func TestParseBrowsersParam_ChromeSafari(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browsers=chrome,safari", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
assert.Equal(t, []string{"chrome", "safari"}, browsers)
}

func TestParseBrowsersParam_ChromeInvalid(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browsers=chrome,invalid", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
assert.Equal(t, []string{"chrome"}, browsers)
}

func TestParseBrowsersParam_AllInvalid(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browsers=notabrowser,invalid", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
assert.Empty(t, browsers)
}

func TestParseBrowsersParam_EmptyCommas(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browsers=,notabrowser,,,,invalid,,", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
assert.Empty(t, browsers)
}

func TestParseBrowsersParam_SafariChrome(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browsers=safari,chrome", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
assert.Equal(t, []string{"chrome", "safari"}, browsers)
}

func TestParseBrowsersParam_MultiBrowserParam_SafariChrome(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browser=safari&browser=chrome", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
assert.Equal(t, []string{"chrome", "safari"}, browsers)
}

func TestParseBrowsersParam_MultiBrowserParam_SafariInvalid(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browser=safari&browser=invalid", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
assert.Equal(t, []string{"safari"}, browsers)
}

func TestParseBrowsersParam_MultiBrowserParam_AllInvalid(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/?browser=invalid&browser=notabrowser", nil)
browsers, err := ParseBrowsersParam(r)
assert.Nil(t, err)
assert.Empty(t, browsers)
}
20 changes: 0 additions & 20 deletions test_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import (
"encoding/json"
"fmt"
"net/http"
"net/url"
"regexp"
)

// This handler is responsible for all pages that display test results.
Expand Down Expand Up @@ -68,21 +66,3 @@ func testHandler(w http.ResponseWriter, r *http.Request) {
return
}
}

// ParseSHAParam parses and validates the 'sha' param for the request.
// It returns "latest" by default (and in error cases).
func ParseSHAParam(r *http.Request) (runSHA string, err error) {
// Get the SHA for the run being loaded (the first part of the path.)
runSHA = "latest"
params, err := url.ParseQuery(r.URL.RawQuery)
if err != nil {
return runSHA, err
}

runParam := params.Get("sha")
regex := regexp.MustCompile("[0-9a-fA-F]{10}")
if regex.MatchString(runParam) {
runSHA = runParam
}
return runSHA, err
}
57 changes: 0 additions & 57 deletions test_handler_test.go

This file was deleted.