Compare PR results against the results without changes #881
Conversation
|
Staging instance deployed by Travis CI! |
api/checks/update.go
Outdated
| // If the run is on master, we will have only one match; | ||
| // for PRs, we have one for pr_head and one for pr_base. | ||
| two := 2 | ||
| runs, err := shared.LoadTestRuns(ctx, filter.Products, filter.Labels, filter.SHA, filter.From, filter.To, &two, nil) |
There was a problem hiding this comment.
In the rare case that we have a master commit land with exactly the same SHA, or in the case of duplicate runs, I'm concerned that we'll 404 here when we probably shouldn't. Given that you're deliberately filtering for shared.PRHeadLabel below, why not query for exactly one run with that label? And if it's not found, we try and fetch master @ the same SHA.
So, for say, chrome:
- let head be chrome[pr_head] @ sha
- If head is nil, let head be chrome[master] @ sha
- If head is still nil, return a 404
- If head is a pr_head, let base be chrome[pr_base] @ sha
- If base is nil, let base be the latest chrome[master] before head.TimeStart
The last line means "fall back to diffing against master when pr_base can't be found."
EDIT: Sorry for tagging user sha
There was a problem hiding this comment.
That is a bit too many special conditions and fallbacks. What about making this a bit more generic?
How about we include all labels for a test run in the spec? So for master runs we will have chrome[master,taskcluster,experimental]@sha and for PR runs we have chrome[pr_head,taskcluster,experimental]@sha. Then in this function we can simply test whether labels contain "master" or "pr_head", then use different queries (either the same spec but earlier than sha, or the same spec but with "pr_head" replaced by "pr_base").
There was a problem hiding this comment.
I don't think that reduces the complexity, it just makes the original check creator responsible for being very specific about the product spec (rather than just knowing "chrome is starting", it's tightly coupled to head/base concepts).
I'd rather make the regression comparator responsible for what "checking chrome@sha for regressions" means in actuality.
Hexcles
force-pushed
the
compare-against-without-changes
branch
from
25ad123 to
e4a5af7
Compare
|
@lukebjerring PTAL again (at the first commit). I didn't exactly follow your suggestions; instead, the logic is:
(1, 2, 3 are mutually exclusive.) |
Hexcles
force-pushed
the
compare-against-without-changes
branch
from
e4a5af7 to
ccb58d3
Compare
Master runs are still compared against the most recent earlier master runs. Based on what labels the first run found has, loadRunsToCompare uses different strategies to find the run to compare against. Also add some medium tests. Drive-by changes: simplify logging in api/checks/update.go.
Hexcles
force-pushed
the
compare-against-without-changes
branch
from
da02941 to
0e29dc8
Compare
Description
The biggest changes are in
loadRunsToCompareandapi/receiver/create_run.go:loadRunsToCompare, we now need to handle the master runs and PR runs separately.api/receiver/create_run.go, we no longer schedule the results processing for runs with the label "pr_base"; otherwise, checks on PRs will be scheduled to be updated twice.Fixes #708 .
Review Information
Please review commit by commit, as the second one is largely mechanical.