Checks: Plumb channel and improve robustness of diff urls #889
Conversation
|
Staging instance deployed by Travis CI! |
|
Staging instance deployed by Travis CI! |
lukebjerring
force-pushed
the
diff-url
branch
from
ddd71c0 to
87a5328
Compare
|
Reverted to keep including the labels, and explicitly clear the SHA instead. |
shared/run_diff.go
Outdated
| filter.Products = make(ProductSpecs, 2) | ||
| filter.Products[0].BrowserName = before.BrowserName | ||
| filter.Products[0].Revision = before.Revision | ||
| filter.Products[0].Labels = mapset.NewSet(before.Channel()) |
There was a problem hiding this comment.
What about including all the labels?
There was a problem hiding this comment.
Sigh. Let's land #906 and then it doesn't matter at all.
lukebjerring
changed the title
shared/models.go
Outdated
| case StableLabel, | ||
| ExperimentalLabel, | ||
| DevLabel, | ||
| BetaLabel: |
There was a problem hiding this comment.
This is a bit problematic: you're mixing the wpt.fyi "channels" with each browser's own channels. e.g. A Chrome run can have both "experimental" and "dev" labels: the latter is the browser channel extracted from the report while the former is the wpt.fyi channels inferred from the browser channel by the processor. This loop will nondeterministically return "dev" or "experimental" in this example.
There was a problem hiding this comment.
Yeah.. ok . Fixed.
A) #538 needs to be concluded
B) the query builders are wrong for a different, similar reason.
api/checks/update.go
Outdated
| @@ -172,8 +172,12 @@ func getDiffSummary(aeAPI shared.AppEngineAPI, diffAPI shared.DiffAPI, baseRun, | |||
| checkState := summaries.CheckState{ | |||
| TestRun: &headRun, | |||
| Product: shared.ProductSpec{ | |||
| ProductAtRevision: headRun.ProductAtRevision, | |||
| Labels: labels, | |||
| // [browser]@[sha] is plenty specific, and avoids bad version strings. | |||
There was a problem hiding this comment.
This is probably not specific enough in some cases.
Example: when headRun is a PR run, we would only pass browser[channel]@revision to GetMasterDiffURL, which will match to both pr_base and pr_head runs, and the users would always expect the pr_head run.
There was a problem hiding this comment.
This is deliberately the case. This only needs to identify any runs precisely enough for a given check; i.e. the CheckState's product is specific enough to identify chrome[experimental], and what the means as a check depends on what's been run (e.g. master vs an earlier master, pr_base vs pr_head, etc.)
As for below, let's rejig the API to take a run to diff against master, and then we can be less ambiguous :)
|
Double-checking behaviour @ https://github.com/lukebjerring/wpt/pull/8/checks?sha=6d14f4b50f4e9121bd55504a6cc33b471569eb2d ( |
Description
See web-platform-tests/wpt#14465
This avoids version strings in a couple of cases.