Fake-support the label=experimental param #82
Conversation
|
Oh, turns out (for obvious reasons) forks can't access the Travis secrets, so this didn't autodeploy. Manually deployed to https://lukebjerring-unstable-dot-wptdashboard.appspot.com/results/?label=experimental EDIT: |
|
You're right; I am happy to see this :) |
| @@ -48,6 +49,9 @@ func apiTestRunsHandler(w http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
| labels := ParseLabelsParam(r) | |||
There was a problem hiding this comment.
I think we should have comments somewhere discussing the binding between unstable in one context and -experimental in another.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK - unstable => experimental
webapp/params.go
Outdated
| // 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) { | ||
| labelParams := r.URL.Query()["label"] |
There was a problem hiding this comment.
Do we do this "singular or plural" work elsewhere? Just thinking about the convenience/complexity tradeoff here, but that's moot if we have established a pattern already.
There was a problem hiding this comment.
Yes, this was a copy-paste of some code above.
I'll do you one better and refactor it.
| labels := ParseLabelsParam(r) | ||
| if labels != nil { | ||
| for label := range labels.Iterator().C { | ||
| sourceURL = sourceURL + "&label=" + label.(string) |
There was a problem hiding this comment.
Is .(string) a type assertion? Could it fail?
There was a problem hiding this comment.
Yes it's an assertion; no it can't fail unless the set items aren't strings (which, they are).
| labels := ParseLabelsParam(r) | ||
| if labels != nil { | ||
| for label := range labels.Iterator().C { | ||
| sourceURL = sourceURL + "&label=" + label.(string) |
webapp/api_handlers.go
Outdated
| @@ -48,6 +49,9 @@ func apiTestRunsHandler(w http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
| labels := ParseLabelsParam(r) | |||
| unstable := labels != nil && labels.Contains("unstable") | |||
There was a problem hiding this comment.
I'm reminded of web-platform-tests/wpt#5956 and it doesn't seem far-fetched that it might be mistaken to mean flakiness on wpt.fyi as well, or that the label name makes its way back to wpt PRs via web-platform-tests/wpt#10503 happens.
But the only counter-proposal I can come up with which accurately describes Chrome Dev/Canary, Edge Insider Preview, Firefox Nightly and Safari Technology Preview is "latest". But if you don't think that's any better, then I surrender to "unstable" :)
webapp/params.go
Outdated
| // no labels are provided. | ||
| func ParseLabelsParam(r *http.Request) (labels mapset.Set) { | ||
| labelParams := r.URL.Query()["label"] | ||
| labelsParam := r.URL.Query().Get("labels") |
There was a problem hiding this comment.
Can you exercise labels=foo,bar in tests as well? And label=foo&label=bar if that is intended to work, which it seems like it is. (Not testing labels=foo,bar&labels=baz seems fine.)
lukebjerring
changed the title
Working towards #60
Spoofs the
?label=unstablefunctionality support by querying for[browser]-experimentalduring query execution. This is to ensure that when we develop real labels, the UI doesn't need to change behaviour (just the datastore query).Notes:
?labels=A,B,Cand?label=A&label=Bforms of the param are both supported./api/runsand the/resultsendpoints (and bubbles through).CC @jugglinmike who may be happy to be able to view the experimental runs in the UI.