Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fake-support the label=experimental param #82

Merged
merged 3 commits into from
Apr 25, 2018
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
14 changes: 14 additions & 0 deletions webapp/api_handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"net/http"
"net/url"
"strconv"
"strings"
"time"

models "github.com/web-platform-tests/wpt.fyi/shared"
Expand All @@ -19,6 +20,8 @@ import (
"google.golang.org/appengine/datastore"
)

const experimentalLabel = `experimental`

// apiTestRunsHandler is responsible for emitting test-run JSON for all the runs at a given SHA.
//
// URL Params:
Expand Down Expand Up @@ -48,6 +51,9 @@ func apiTestRunsHandler(w http.ResponseWriter, r *http.Request) {
return
}

labels := ParseLabelsParam(r)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have comments somewhere discussing the binding between unstable in one context and -experimental in another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jugglinmike - can you weigh in on your preference or stable or experimental as the label? You chose -experimental as the browser suffix, so perhaps you prefer that? I'm not fussed - unstable does, as @foolip points out, kinda sound test-related.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I agree with @foolip's advice, and since he made the improvement to WPT, using the same convention here will promote consistency in terminology.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - unstable => experimental

experimentalBrowsers := labels != nil && labels.Contains(experimentalLabel)

var testRuns []models.TestRun
var limit int
if limit, err = ParseMaxCountParam(r); err != nil {
Expand All @@ -61,6 +67,9 @@ func apiTestRunsHandler(w http.ResponseWriter, r *http.Request) {

for _, browserName := range browserNames {
var testRunResults []models.TestRun
if experimentalBrowsers {
browserName = browserName + "-" + experimentalLabel
}
query := baseQuery.Filter("BrowserName =", browserName)
if runSHA != "" && runSHA != "latest" {
query = query.Filter("Revision =", runSHA)
Expand All @@ -69,6 +78,11 @@ func apiTestRunsHandler(w http.ResponseWriter, r *http.Request) {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
if experimentalBrowsers {
for i := range testRunResults {
testRunResults[i].BrowserName = strings.Replace(testRunResults[i].BrowserName, "-"+experimentalLabel, "", 1)
}
}
testRuns = append(testRuns, testRunResults...)
}

Expand Down
44 changes: 29 additions & 15 deletions webapp/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,29 +164,43 @@ func ParseDiffFilterParams(r *http.Request) (param DiffFilterParam, err error) {
return param, nil
}

// ParsePathsParam returns a set list of test paths to include, or nil if no filter is provided (and all tests should be
// included). It parses the 'paths' parameter, split on commas, and also checks for the (repeatable) 'path' params.
// ParsePathsParam returns a set list of test paths to include, or nil if no
// filter is provided (and all tests should be included). It parses the 'paths'
// parameter, split on commas, and also checks for the (repeatable) 'path' params
func ParsePathsParam(r *http.Request) (paths mapset.Set) {
pathParams := r.URL.Query()["path"]
pathsParam := r.URL.Query().Get("paths")
if len(pathParams) == 0 && pathsParam == "" {
return ParseRepeatedParam(r, "path", "paths")
}

// ParseLabelsParam returns a set list of test-run labels to include, or nil if
// no labels are provided.
func ParseLabelsParam(r *http.Request) (labels mapset.Set) {
return ParseRepeatedParam(r, "label", "labels")
}

// ParseRepeatedParam parses a param that may be a plural name, with all values
// comma-separated, or a repeated singular param.
// e.g. ?label=foo&label=bar vs ?labels=foo,bar
func ParseRepeatedParam(r *http.Request, singular string, plural string) (params mapset.Set) {
repeatedParam := r.URL.Query()[singular]
pluralParam := r.URL.Query().Get(plural)
if len(repeatedParam) == 0 && pluralParam == "" {
return nil
}

paths = mapset.NewSet()
for _, path := range pathParams {
paths.Add(path)
params = mapset.NewSet()
for _, label := range repeatedParam {
params.Add(label)
}
if pathsParam != "" {
for _, path := range strings.Split(pathsParam, ",") {
paths.Add(path)
if pluralParam != "" {
for _, label := range strings.Split(pluralParam, ",") {
params.Add(label)
}
}
if paths.Contains("") {
paths.Remove("")
if params.Contains("") {
params.Remove("")
}
if paths.Cardinality() == 0 {
if params.Cardinality() == 0 {
return nil
}
return paths
return params
}
34 changes: 34 additions & 0 deletions webapp/params_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,3 +261,37 @@ func TestParseDiffFilterParam_Invalid(t *testing.T) {
_, err := ParseDiffFilterParams(r)
assert.NotNil(t, err)
}

func TestParseLabelsParam_Missing(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs", nil)
labels := ParseLabelsParam(r)
assert.Nil(t, labels)
}

func TestParseLabelsParam_Empty(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs?label=", nil)
labels := ParseLabelsParam(r)
assert.Nil(t, labels)

r = httptest.NewRequest("GET", "http://wpt.fyi/api/runs?labels=", nil)
labels = ParseLabelsParam(r)
assert.Nil(t, labels)
}

func TestParseLabelsParam_Label_Duplicate(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs?label=experimental&label=experimental", nil)
labels := ParseLabelsParam(r)
assert.Equal(t, 1, labels.Cardinality())
}

func TestParseLabelsParam_Labels_Duplicate(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs?labels=experimental,experimental", nil)
labels := ParseLabelsParam(r)
assert.Equal(t, 1, labels.Cardinality())
}

func TestParseLabelsParam_LabelsAndLabel_Duplicate(t *testing.T) {
r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs?labels=experimental&label=experimental", nil)
labels := ParseLabelsParam(r)
assert.Equal(t, 1, labels.Cardinality())
}
8 changes: 7 additions & 1 deletion webapp/test_results_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,13 @@ func getTestRunsAndSources(r *http.Request, runSHA string) (testRunSources []str
testRunSources = append(testRunSources, fmt.Sprintf(singleRunURL, afterSpec.Revision, afterSpec.Platform))
}
} else {
const sourceURL = `/api/runs?sha=%s`
var sourceURL = `/api/runs?sha=%s`
labels := ParseLabelsParam(r)
if labels != nil {
for label := range labels.Iterator().C {
sourceURL = sourceURL + "&label=" + label.(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto (type assertion)

}
}
testRunSources = []string{fmt.Sprintf(sourceURL, runSHA)}
}

Expand Down
7 changes: 7 additions & 0 deletions webapp/test_run_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ func handleTestRunGet(w http.ResponseWriter, r *http.Request) {
}
sourceURL := fmt.Sprintf(`/api/runs?max-count=%d`, maxCount)

labels := ParseLabelsParam(r)
if labels != nil {
for label := range labels.Iterator().C {
sourceURL = sourceURL + "&label=" + label.(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is .(string) a type assertion? Could it fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's an assertion; no it can't fail unless the set items aren't strings (which, they are).

}
}

// Serialize the data + pipe through the test-runs.html template.
testRunSourcesBytes, err := json.Marshal([]string{sourceURL})
if err != nil {
Expand Down