From 11c467c3bc5ddd23215ab140849ee4cb15830d58 Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Tue, 11 Dec 2018 11:33:37 -0500 Subject: [PATCH 1/9] Improve diff urls --- api/checks/update.go | 8 ++++++-- shared/run_diff.go | 9 +++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/api/checks/update.go b/api/checks/update.go index fdcc697459c..68b0a4aaacc 100644 --- a/api/checks/update.go +++ b/api/checks/update.go @@ -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. + ProductAtRevision: shared.ProductAtRevision{ + Product: shared.Product{BrowserName: headRun.BrowserName}, + Revision: headRun.Revision, + }, + Labels: labels, }, HeadSHA: headRun.FullRevisionHash, DetailsURL: diffURL, diff --git a/shared/run_diff.go b/shared/run_diff.go index ffdd078a333..e432a932523 100644 --- a/shared/run_diff.go +++ b/shared/run_diff.go @@ -41,13 +41,10 @@ func NewDiffAPI(ctx context.Context) DiffAPI { } func (d diffAPIImpl) GetDiffURL(before, after TestRun, diffFilter *DiffFilterParam) *url.URL { - filter := TestRunFilter{} - filter.Products = ProductSpecs{ - ProductSpec{ProductAtRevision: before.ProductAtRevision}, - ProductSpec{ProductAtRevision: after.ProductAtRevision}, - } - detailsURL := d.aeAPI.GetResultsURL(filter) + detailsURL, _ := url.Parse(fmt.Sprintf("https://%s/results/", d.aeAPI.GetHostname())) query := detailsURL.Query() + query.Add("run_id", fmt.Sprintf("%v", before.ID)) + query.Add("run_id", fmt.Sprintf("%v", after.ID)) query.Set("diff", "") if diffFilter != nil { query.Set("filter", diffFilter.String()) From 038de5e3b270452f4610667f56924f5b770785f5 Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Tue, 11 Dec 2018 12:43:24 -0500 Subject: [PATCH 2/9] Fix master diff URL --- api/checks/api.go | 2 +- api/checks/update.go | 2 +- shared/run_diff.go | 10 ++++++---- shared/sharedtest/run_diff_mock.go | 7 ++++--- shared/sharedtest/util.go | 6 +++++- 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/api/checks/api.go b/api/checks/api.go index 741ec03757a..aec161c6f9f 100644 --- a/api/checks/api.go +++ b/api/checks/api.go @@ -71,7 +71,7 @@ func (s checksAPIImpl) PendingCheckRun(suite shared.CheckSuite, product shared.P TestRun: nil, // It's pending, no run exists yet. Product: product, HeadSHA: suite.SHA, - DetailsURL: shared.NewDiffAPI(s.ctx).GetMasterDiffURL(suite.SHA, product), + DetailsURL: shared.NewDiffAPI(s.ctx).GetMasterDiffURL(suite.SHA, product.Product), Status: "in_progress", }, HostName: host, diff --git a/api/checks/update.go b/api/checks/update.go index 68b0a4aaacc..acc27c1f156 100644 --- a/api/checks/update.go +++ b/api/checks/update.go @@ -198,7 +198,7 @@ func getDiffSummary(aeAPI shared.AppEngineAPI, diffAPI shared.DiffAPI, baseRun, HostName: host, HostURL: fmt.Sprintf("https://%s/", host), DiffURL: diffURL.String(), - MasterDiffURL: diffAPI.GetMasterDiffURL(checkState.HeadSHA, checkState.Product).String(), + MasterDiffURL: diffAPI.GetMasterDiffURL(checkState.HeadSHA, checkState.Product.Product).String(), } hasRegressions := regressions.Cardinality() > 0 diff --git a/shared/run_diff.go b/shared/run_diff.go index e432a932523..fb0e1e6701e 100644 --- a/shared/run_diff.go +++ b/shared/run_diff.go @@ -24,7 +24,7 @@ import ( type DiffAPI interface { GetRunsDiff(before, after TestRun, filter DiffFilterParam, paths mapset.Set) (RunDiff, error) GetDiffURL(before, after TestRun, diffFilter *DiffFilterParam) *url.URL - GetMasterDiffURL(sha string, product ProductSpec) *url.URL + GetMasterDiffURL(sha string, product Product) *url.URL } type diffAPIImpl struct { @@ -53,11 +53,13 @@ func (d diffAPIImpl) GetDiffURL(before, after TestRun, diffFilter *DiffFilterPar return detailsURL } -func (d diffAPIImpl) GetMasterDiffURL(sha string, product ProductSpec) *url.URL { +func (d diffAPIImpl) GetMasterDiffURL(sha string, product Product) *url.URL { filter := TestRunFilter{} - filter.Products = ProductSpecs{product, product} + filter.Products = make(ProductSpecs, 2) + filter.Products[0].Product = product filter.Products[0].Labels = mapset.NewSet("master") - filter.Products[1].Revision = sha + filter.Products[1].Product = product + filter.Products[1].Revision = sha // Specific SHA for product. detailsURL := d.aeAPI.GetResultsURL(filter) query := detailsURL.Query() query.Set("diff", "") diff --git a/shared/sharedtest/run_diff_mock.go b/shared/sharedtest/run_diff_mock.go index 0bc80a4b077..cb5d7d7936b 100644 --- a/shared/sharedtest/run_diff_mock.go +++ b/shared/sharedtest/run_diff_mock.go @@ -5,11 +5,12 @@ package sharedtest import ( + url "net/url" + reflect "reflect" + golang_set "github.com/deckarep/golang-set" gomock "github.com/golang/mock/gomock" shared "github.com/web-platform-tests/wpt.fyi/shared" - url "net/url" - reflect "reflect" ) // MockDiffAPI is a mock of DiffAPI interface @@ -48,7 +49,7 @@ func (mr *MockDiffAPIMockRecorder) GetDiffURL(arg0, arg1, arg2 interface{}) *gom } // GetMasterDiffURL mocks base method -func (m *MockDiffAPI) GetMasterDiffURL(arg0 string, arg1 shared.ProductSpec) *url.URL { +func (m *MockDiffAPI) GetMasterDiffURL(arg0 string, arg1 shared.Product) *url.URL { ret := m.ctrl.Call(m, "GetMasterDiffURL", arg0, arg1) ret0, _ := ret[0].(*url.URL) return ret0 diff --git a/shared/sharedtest/util.go b/shared/sharedtest/util.go index 5272c1e1dac..31336379ab7 100644 --- a/shared/sharedtest/util.go +++ b/shared/sharedtest/util.go @@ -53,8 +53,12 @@ type sameProductSpec struct { spec string } +type stringifiable interface { + String() string +} + func (s sameProductSpec) Matches(x interface{}) bool { - if p, ok := x.(shared.ProductSpec); ok && p.String() == s.spec { + if p, ok := x.(stringifiable); ok && p.String() == s.spec { return true } else if str, ok := x.(string); ok && str == s.spec { return true From 87a5328b77507d2cffad957b93dc969f36264cd5 Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Tue, 11 Dec 2018 12:50:47 -0500 Subject: [PATCH 3/9] Revert run_ids, use less specificity --- shared/run_diff.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/shared/run_diff.go b/shared/run_diff.go index fb0e1e6701e..a3b9cb30b0b 100644 --- a/shared/run_diff.go +++ b/shared/run_diff.go @@ -41,10 +41,14 @@ func NewDiffAPI(ctx context.Context) DiffAPI { } func (d diffAPIImpl) GetDiffURL(before, after TestRun, diffFilter *DiffFilterParam) *url.URL { - detailsURL, _ := url.Parse(fmt.Sprintf("https://%s/results/", d.aeAPI.GetHostname())) + filter := TestRunFilter{} + filter.Products = make(ProductSpecs, 2) + filter.Products[0].BrowserName = before.BrowserName + filter.Products[0].Revision = before.Revision + filter.Products[1].BrowserName = after.BrowserName + filter.Products[1].Revision = after.Revision + detailsURL := d.aeAPI.GetResultsURL(filter) query := detailsURL.Query() - query.Add("run_id", fmt.Sprintf("%v", before.ID)) - query.Add("run_id", fmt.Sprintf("%v", after.ID)) query.Set("diff", "") if diffFilter != nil { query.Set("filter", diffFilter.String()) From 9688cdfa9b11f0e8042c8b328ced946a65a87b49 Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Wed, 12 Dec 2018 08:25:11 -0500 Subject: [PATCH 4/9] Use ProductSpec (need labels) --- api/checks/api.go | 2 +- api/checks/update.go | 2 +- api/checks/update_test.go | 4 ++-- shared/run_diff.go | 26 ++++++++++++++------------ shared/sharedtest/run_diff_mock.go | 13 ++++++------- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/api/checks/api.go b/api/checks/api.go index aec161c6f9f..3d6681d0867 100644 --- a/api/checks/api.go +++ b/api/checks/api.go @@ -71,7 +71,7 @@ func (s checksAPIImpl) PendingCheckRun(suite shared.CheckSuite, product shared.P TestRun: nil, // It's pending, no run exists yet. Product: product, HeadSHA: suite.SHA, - DetailsURL: shared.NewDiffAPI(s.ctx).GetMasterDiffURL(suite.SHA, product.Product), + DetailsURL: shared.NewDiffAPI(s.ctx).GetMasterDiffURL(suite.SHA, product, nil), Status: "in_progress", }, HostName: host, diff --git a/api/checks/update.go b/api/checks/update.go index acc27c1f156..21736136a80 100644 --- a/api/checks/update.go +++ b/api/checks/update.go @@ -198,7 +198,7 @@ func getDiffSummary(aeAPI shared.AppEngineAPI, diffAPI shared.DiffAPI, baseRun, HostName: host, HostURL: fmt.Sprintf("https://%s/", host), DiffURL: diffURL.String(), - MasterDiffURL: diffAPI.GetMasterDiffURL(checkState.HeadSHA, checkState.Product.Product).String(), + MasterDiffURL: diffAPI.GetMasterDiffURL(checkState.HeadSHA, checkState.Product, nil).String(), } hasRegressions := regressions.Cardinality() > 0 diff --git a/api/checks/update_test.go b/api/checks/update_test.go index fe94813cf29..4c7b1c2abd9 100644 --- a/api/checks/update_test.go +++ b/api/checks/update_test.go @@ -32,7 +32,7 @@ func TestGetDiffSummary_Regressed(t *testing.T) { diffAPI.EXPECT().GetRunsDiff(before, after, gomock.Any(), gomock.Any()).Return(runDiff, nil) diffURL, _ := url.Parse("https://wpt.fyi/results?diff") diffAPI.EXPECT().GetDiffURL(before, after, gomock.Any()).Return(diffURL) - diffAPI.EXPECT().GetMasterDiffURL(after.FullRevisionHash, sharedtest.SameProductSpec(before.BrowserName)).Return(diffURL) + diffAPI.EXPECT().GetMasterDiffURL(after.FullRevisionHash, sharedtest.SameProductSpec(before.BrowserName), nil).Return(diffURL) summary, err := getDiffSummary(aeAPI, diffAPI, before, after) assert.Nil(t, err) @@ -57,7 +57,7 @@ func TestGetDiffSummary_Completed(t *testing.T) { diffAPI.EXPECT().GetRunsDiff(before, after, gomock.Any(), gomock.Any()).Return(runDiff, nil) diffURL, _ := url.Parse("https://wpt.fyi/results?diff") diffAPI.EXPECT().GetDiffURL(before, after, gomock.Any()).Return(diffURL) - diffAPI.EXPECT().GetMasterDiffURL(after.FullRevisionHash, sharedtest.SameProductSpec(before.BrowserName)).Return(diffURL) + diffAPI.EXPECT().GetMasterDiffURL(after.FullRevisionHash, sharedtest.SameProductSpec(before.BrowserName), nil).Return(diffURL) summary, err := getDiffSummary(aeAPI, diffAPI, before, after) assert.Nil(t, err) diff --git a/shared/run_diff.go b/shared/run_diff.go index a3b9cb30b0b..c97cc77eb52 100644 --- a/shared/run_diff.go +++ b/shared/run_diff.go @@ -24,7 +24,7 @@ import ( type DiffAPI interface { GetRunsDiff(before, after TestRun, filter DiffFilterParam, paths mapset.Set) (RunDiff, error) GetDiffURL(before, after TestRun, diffFilter *DiffFilterParam) *url.URL - GetMasterDiffURL(sha string, product Product) *url.URL + GetMasterDiffURL(sha string, product ProductSpec, diffFilter *DiffFilterParam) *url.URL } type diffAPIImpl struct { @@ -57,21 +57,23 @@ func (d diffAPIImpl) GetDiffURL(before, after TestRun, diffFilter *DiffFilterPar return detailsURL } -func (d diffAPIImpl) GetMasterDiffURL(sha string, product Product) *url.URL { +func (d diffAPIImpl) GetMasterDiffURL(sha string, product ProductSpec, diffFilter *DiffFilterParam) *url.URL { filter := TestRunFilter{} - filter.Products = make(ProductSpecs, 2) - filter.Products[0].Product = product - filter.Products[0].Labels = mapset.NewSet("master") - filter.Products[1].Product = product - filter.Products[1].Revision = sha // Specific SHA for product. + filter.Products = ProductSpecs{product, product} + filter.Products[0].Revision = "" // No specific SHA for base (master). + if product.Labels == nil { + filter.Products[0].Labels = mapset.NewSet() + } + filter.Products[0].Labels.Add(MasterLabel) + + filter.Products[1].Revision = sha // Specific SHA for head. + detailsURL := d.aeAPI.GetResultsURL(filter) query := detailsURL.Query() query.Set("diff", "") - query.Set("filter", DiffFilterParam{ - Added: true, - Changed: true, - Unchanged: true, - }.String()) + if diffFilter != nil { + query.Set("filter", diffFilter.String()) + } detailsURL.RawQuery = query.Encode() return detailsURL } diff --git a/shared/sharedtest/run_diff_mock.go b/shared/sharedtest/run_diff_mock.go index cb5d7d7936b..179c7a5f72d 100644 --- a/shared/sharedtest/run_diff_mock.go +++ b/shared/sharedtest/run_diff_mock.go @@ -5,12 +5,11 @@ package sharedtest import ( - url "net/url" - reflect "reflect" - golang_set "github.com/deckarep/golang-set" gomock "github.com/golang/mock/gomock" shared "github.com/web-platform-tests/wpt.fyi/shared" + url "net/url" + reflect "reflect" ) // MockDiffAPI is a mock of DiffAPI interface @@ -49,15 +48,15 @@ func (mr *MockDiffAPIMockRecorder) GetDiffURL(arg0, arg1, arg2 interface{}) *gom } // GetMasterDiffURL mocks base method -func (m *MockDiffAPI) GetMasterDiffURL(arg0 string, arg1 shared.Product) *url.URL { - ret := m.ctrl.Call(m, "GetMasterDiffURL", arg0, arg1) +func (m *MockDiffAPI) GetMasterDiffURL(arg0 string, arg1 shared.ProductSpec, arg2 *shared.DiffFilterParam) *url.URL { + ret := m.ctrl.Call(m, "GetMasterDiffURL", arg0, arg1, arg2) ret0, _ := ret[0].(*url.URL) return ret0 } // GetMasterDiffURL indicates an expected call of GetMasterDiffURL -func (mr *MockDiffAPIMockRecorder) GetMasterDiffURL(arg0, arg1 interface{}) *gomock.Call { - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMasterDiffURL", reflect.TypeOf((*MockDiffAPI)(nil).GetMasterDiffURL), arg0, arg1) +func (mr *MockDiffAPIMockRecorder) GetMasterDiffURL(arg0, arg1, arg2 interface{}) *gomock.Call { + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMasterDiffURL", reflect.TypeOf((*MockDiffAPI)(nil).GetMasterDiffURL), arg0, arg1, arg2) } // GetRunsDiff mocks base method From 69d2ccf5d1b67903c1c8ce63aff68b3c5e2c0a54 Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Wed, 12 Dec 2018 11:36:41 -0500 Subject: [PATCH 5/9] Include channel --- shared/models.go | 14 ++++++++++++++ shared/params.go | 4 +++- shared/run_diff.go | 2 ++ shared/util.go | 3 +++ 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/shared/models.go b/shared/models.go index d69b97b6f02..8badac8f9b1 100644 --- a/shared/models.go +++ b/shared/models.go @@ -119,6 +119,20 @@ func (r TestRun) IsExperimental() bool { return len(r.Labels) > 0 && r.LabelsSet().Contains(ExperimentalLabel) } +// Channel return the channel label, if any, for the given run. +func (r TestRun) Channel() string { + for _, label := range r.Labels { + switch label { + case StableLabel, + ExperimentalLabel, + DevLabel, + BetaLabel: + return label + } + } + return "" +} + // TestRunStatus is an enum for PendingTestRun statuses. type TestRunStatus int64 diff --git a/shared/params.go b/shared/params.go index 35689e3ab14..04c0b21ba72 100644 --- a/shared/params.go +++ b/shared/params.go @@ -116,7 +116,9 @@ func (p ProductSpec) String() string { if p.Labels != nil && p.Labels.Cardinality() > 0 { labels := make([]string, 0, p.Labels.Cardinality()) for l := range p.Labels.Iter() { - labels = append(labels, l.(string)) + if l != "" { + labels = append(labels, l.(string)) + } } sort.Strings(labels) // Deterministic String() output. s += "[" + strings.Join(labels, ",") + "]" diff --git a/shared/run_diff.go b/shared/run_diff.go index c97cc77eb52..33f6397e0cb 100644 --- a/shared/run_diff.go +++ b/shared/run_diff.go @@ -45,8 +45,10 @@ func (d diffAPIImpl) GetDiffURL(before, after TestRun, diffFilter *DiffFilterPar 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()) filter.Products[1].BrowserName = after.BrowserName filter.Products[1].Revision = after.Revision + filter.Products[1].Labels = mapset.NewSet(after.Channel()) detailsURL := d.aeAPI.GetResultsURL(filter) query := detailsURL.Query() query.Set("diff", "") diff --git a/shared/util.go b/shared/util.go index c54bfdbe6ea..083e77d7231 100644 --- a/shared/util.go +++ b/shared/util.go @@ -31,6 +31,9 @@ const StableLabel = "stable" // BetaLabel is the implicit label present for runs marked 'beta'. const BetaLabel = "beta" +// DevLabel is the implicit label present for runs marked 'dev'. +const DevLabel = "dev" + // MasterLabel is the implicit label present for runs marked 'master', // i.e. run from the master branch. const MasterLabel = "master" From 20d2bc12f2603255c412a19686f43cfb3037c92c Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Wed, 12 Dec 2018 13:07:21 -0500 Subject: [PATCH 6/9] Use run ID --- shared/run_diff.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/shared/run_diff.go b/shared/run_diff.go index 33f6397e0cb..97bd010c6f2 100644 --- a/shared/run_diff.go +++ b/shared/run_diff.go @@ -41,16 +41,10 @@ func NewDiffAPI(ctx context.Context) DiffAPI { } func (d diffAPIImpl) GetDiffURL(before, after TestRun, diffFilter *DiffFilterParam) *url.URL { - filter := TestRunFilter{} - 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()) - filter.Products[1].BrowserName = after.BrowserName - filter.Products[1].Revision = after.Revision - filter.Products[1].Labels = mapset.NewSet(after.Channel()) - detailsURL := d.aeAPI.GetResultsURL(filter) + detailsURL, _ := url.Parse(fmt.Sprintf("https://%s/results/", d.aeAPI.GetHostname())) query := detailsURL.Query() + query.Add("run_id", fmt.Sprintf("%v", before.ID)) + query.Add("run_id", fmt.Sprintf("%v", after.ID)) query.Set("diff", "") if diffFilter != nil { query.Set("filter", diffFilter.String()) From b69bd8ed3da3f6db4591d10212cca00217622a94 Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Wed, 12 Dec 2018 13:14:09 -0500 Subject: [PATCH 7/9] Pass channel to checks-processor --- api/checks/update.go | 2 +- api/receiver/create_run.go | 2 ++ shared/datastore.go | 1 + shared/params.go | 17 ++++++++++------- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/api/checks/update.go b/api/checks/update.go index c5c8eff0812..d69957b44b6 100644 --- a/api/checks/update.go +++ b/api/checks/update.go @@ -144,7 +144,7 @@ func loadMasterRunBefore(ctx context.Context, filter shared.TestRunFilter, headR // Get the most recent, but still earlier, master run to compare. one := 1 to := headRun.TimeStart.Add(-time.Millisecond) - labels := mapset.NewSetWith(shared.MasterLabel) + labels := mapset.NewSetWith(headRun.Channel(), shared.MasterLabel) runs, err := shared.LoadTestRuns(ctx, filter.Products, labels, shared.LatestSHA, nil, &to, &one, nil) baseRun := runs.First() if err != nil { diff --git a/api/receiver/create_run.go b/api/receiver/create_run.go index da4608be338..c2ca7959f63 100644 --- a/api/receiver/create_run.go +++ b/api/receiver/create_run.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/deckarep/golang-set" "github.com/web-platform-tests/wpt.fyi/api/checks" "github.com/web-platform-tests/wpt.fyi/shared" ) @@ -69,6 +70,7 @@ func HandleResultsCreate(a AppEngineAPI, s checks.API, w http.ResponseWriter, r if !testRun.LabelsSet().Contains(shared.PRBaseLabel) { spec := shared.ProductSpec{} spec.BrowserName = testRun.BrowserName + spec.Labels = mapset.NewSet(testRun.Channel()) s.ScheduleResultsProcessing(testRun.FullRevisionHash, spec) } diff --git a/shared/datastore.go b/shared/datastore.go index 734db841569..764741714cb 100644 --- a/shared/datastore.go +++ b/shared/datastore.go @@ -63,6 +63,7 @@ func LoadTestRunKeys( baseQuery = baseQuery.Offset(*offset) } if labels != nil { + labels.Remove("") // Ensure the empty string isn't present. for i := range labels.Iter() { baseQuery = baseQuery.Filter("Labels =", i.(string)) } diff --git a/shared/params.go b/shared/params.go index 04c0b21ba72..0def032f55b 100644 --- a/shared/params.go +++ b/shared/params.go @@ -113,15 +113,18 @@ func (p ProductSpecs) Strings() []string { func (p ProductSpec) String() string { s := p.Product.String() - if p.Labels != nil && p.Labels.Cardinality() > 0 { - labels := make([]string, 0, p.Labels.Cardinality()) - for l := range p.Labels.Iter() { - if l != "" { - labels = append(labels, l.(string)) + if p.Labels != nil { + p.Labels.Remove("") // Remove the empty label, if present. + if p.Labels.Cardinality() > 0 { + labels := make([]string, 0, p.Labels.Cardinality()) + for l := range p.Labels.Iter() { + if l != "" { + labels = append(labels, l.(string)) + } } + sort.Strings(labels) // Deterministic String() output. + s += "[" + strings.Join(labels, ",") + "]" } - sort.Strings(labels) // Deterministic String() output. - s += "[" + strings.Join(labels, ",") + "]" } if !IsLatest(p.Revision) { s += "@" + p.Revision From eaa1dcf69d4e0f4ae231a02c523f333de6f8dbcd Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Wed, 12 Dec 2018 13:31:49 -0500 Subject: [PATCH 8/9] Dev is not a channel --- shared/models.go | 5 ++--- shared/util.go | 3 --- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/shared/models.go b/shared/models.go index 8badac8f9b1..4b7be6bc6e0 100644 --- a/shared/models.go +++ b/shared/models.go @@ -124,9 +124,8 @@ func (r TestRun) Channel() string { for _, label := range r.Labels { switch label { case StableLabel, - ExperimentalLabel, - DevLabel, - BetaLabel: + BetaLabel, + ExperimentalLabel: return label } } diff --git a/shared/util.go b/shared/util.go index 083e77d7231..c54bfdbe6ea 100644 --- a/shared/util.go +++ b/shared/util.go @@ -31,9 +31,6 @@ const StableLabel = "stable" // BetaLabel is the implicit label present for runs marked 'beta'. const BetaLabel = "beta" -// DevLabel is the implicit label present for runs marked 'dev'. -const DevLabel = "dev" - // MasterLabel is the implicit label present for runs marked 'master', // i.e. run from the master branch. const MasterLabel = "master" From 95eea8fe7a294e54b71f94745125b1a99823d4b2 Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Wed, 12 Dec 2018 16:32:24 -0500 Subject: [PATCH 9/9] Only do master diff for pr_head --- api/checks/api.go | 10 ++++--- api/checks/summaries/compile_test.go | 19 ++++++++++++- api/checks/summaries/completed.md | 2 ++ api/checks/summaries/regressed.md | 2 ++ api/checks/update.go | 40 +++++++++++++++------------- api/checks/update_test.go | 6 +++-- api/receiver/create_run.go | 2 +- shared/params.go | 4 +-- shared/run_diff.go | 31 +++++++++++---------- shared/sharedtest/run_diff_mock.go | 8 +++--- shared/sharedtest/util.go | 15 ++++++++--- 11 files changed, 88 insertions(+), 51 deletions(-) diff --git a/api/checks/api.go b/api/checks/api.go index 3d6681d0867..2d255ad50bf 100644 --- a/api/checks/api.go +++ b/api/checks/api.go @@ -65,17 +65,21 @@ func (s checksAPIImpl) ScheduleResultsProcessing(sha string, product shared.Prod // PendingCheckRun posts an in_progress check run for the given CheckSuite/Product. // Returns true if any check_runs were created (i.e. the create succeeded). func (s checksAPIImpl) PendingCheckRun(suite shared.CheckSuite, product shared.ProductSpec) (bool, error) { - host := shared.NewAppEngineAPI(s.ctx).GetHostname() + aeAPI := shared.NewAppEngineAPI(s.ctx) + host := aeAPI.GetHostname() + filter := shared.TestRunFilter{SHA: suite.SHA[:10]} + runsURL := aeAPI.GetRunsURL(filter) + pending := summaries.Pending{ CheckState: summaries.CheckState{ TestRun: nil, // It's pending, no run exists yet. Product: product, HeadSHA: suite.SHA, - DetailsURL: shared.NewDiffAPI(s.ctx).GetMasterDiffURL(suite.SHA, product, nil), + DetailsURL: runsURL, Status: "in_progress", }, HostName: host, - RunsURL: fmt.Sprintf("https://%s/runs", host), + RunsURL: runsURL.String(), } return updateCheckRunSummary(s.ctx, pending, suite) } diff --git a/api/checks/summaries/compile_test.go b/api/checks/summaries/compile_test.go index e96f66b24b3..af9f7c997a7 100644 --- a/api/checks/summaries/compile_test.go +++ b/api/checks/summaries/compile_test.go @@ -51,6 +51,15 @@ func TestGetSummary_Completed(t *testing.T) { assert.Contains(t, s, "2 / 3") assert.Contains(t, s, "And 1 others...") assert.Contains(t, s, foo.FileIssueURL().String()) + + // And with MasterDiffURL + foo.MasterDiffURL = "https://foo.com/?products=chrome[master],chrome@0123456789&diff" + s, err = foo.GetSummary() + printOutput(s) + if err != nil { + assert.FailNow(t, err.Error()) + } + assert.Contains(t, s, foo.MasterDiffURL) } func TestGetSummary_Pending(t *testing.T) { @@ -83,7 +92,6 @@ func TestGetSummary_Regressed(t *testing.T) { foo.HostName = "foo.com" foo.HostURL = "https://foo.com/" foo.DiffURL = "https://foo.com/?products=chrome@0000000000,chrome@0123456789&diff" - foo.MasterDiffURL = "https://foo.com/?products=chrome[master],chrome@0123456789&diff" foo.Regressions = map[string]BeforeAndAfter{ "/foo.html": BeforeAndAfter{ PassingBefore: 1, @@ -108,6 +116,15 @@ func TestGetSummary_Regressed(t *testing.T) { assert.Contains(t, s, "1 / 1") assert.Contains(t, s, "And 1 others...") assert.Contains(t, s, foo.FileIssueURL().String()) + + // And with MasterDiffURL + foo.MasterDiffURL = "https://foo.com/?products=chrome[master],chrome@0123456789&diff" + s, err = foo.GetSummary() + printOutput(s) + if err != nil { + assert.FailNow(t, err.Error()) + } + assert.Contains(t, s, foo.MasterDiffURL) } func printOutput(s string) { diff --git a/api/checks/summaries/completed.md b/api/checks/summaries/completed.md index f995292b200..9d8426916a5 100644 --- a/api/checks/summaries/completed.md +++ b/api/checks/summaries/completed.md @@ -19,7 +19,9 @@ And {{ .More }} others... Other views that might be useful: - [`{{ printf "%.7s" .HeadRun.FullRevisionHash }}` vs its merge base]({{ .DiffURL }}) +{{- if .MasterDiffURL }} - [`{{ printf "%.7s" .HeadRun.FullRevisionHash }}` vs latest master]({{ .MasterDiffURL }}) +{{- end }} - [Latest results for `{{ printf "%.7s" .HeadRun.FullRevisionHash }}`]({{.HostURL}}?sha={{.HeadRun.Revision}}) {{ template "_file_an_issue.md" . }} diff --git a/api/checks/summaries/regressed.md b/api/checks/summaries/regressed.md index 5aab1a6c0dc..a6e2d504e4b 100644 --- a/api/checks/summaries/regressed.md +++ b/api/checks/summaries/regressed.md @@ -20,7 +20,9 @@ And {{ .More }} others... Other views that might be useful: - [`{{ printf "%.7s" .HeadRun.FullRevisionHash }}` vs its merge base]({{ .DiffURL }}) +{{- if .MasterDiffURL }} - [`{{ printf "%.7s" .HeadRun.FullRevisionHash }}` vs latest master]({{ .MasterDiffURL }}) +{{- end }} - [Latest results for `{{ printf "%.7s" .HeadRun.FullRevisionHash }}`]({{.HostURL}}?sha={{.HeadRun.Revision}}) {{ template "_file_an_issue.md" . }} diff --git a/api/checks/update.go b/api/checks/update.go index d69957b44b6..1004612daef 100644 --- a/api/checks/update.go +++ b/api/checks/update.go @@ -164,21 +164,19 @@ func getDiffSummary(aeAPI shared.AppEngineAPI, diffAPI shared.DiffAPI, baseRun, return nil, err } - diffURL := diffAPI.GetDiffURL(baseRun, headRun, &diffFilter) - var labels mapset.Set - if headRun.IsExperimental() { - labels = mapset.NewSet(shared.ExperimentalLabel) + checkProduct := shared.ProductSpec{ + // [browser]@[sha] is plenty specific, and avoids bad version strings. + ProductAtRevision: shared.ProductAtRevision{ + Product: shared.Product{BrowserName: headRun.BrowserName}, + Revision: headRun.Revision, + }, + Labels: mapset.NewSetWith(baseRun.Channel()), } + + diffURL := diffAPI.GetDiffURL(baseRun, headRun, &diffFilter) checkState := summaries.CheckState{ - TestRun: &headRun, - Product: shared.ProductSpec{ - // [browser]@[sha] is plenty specific, and avoids bad version strings. - ProductAtRevision: shared.ProductAtRevision{ - Product: shared.Product{BrowserName: headRun.BrowserName}, - Revision: headRun.Revision, - }, - Labels: labels, - }, + TestRun: &headRun, + Product: checkProduct, HeadSHA: headRun.FullRevisionHash, DetailsURL: diffURL, Status: "completed", @@ -193,12 +191,16 @@ func getDiffSummary(aeAPI shared.AppEngineAPI, diffAPI shared.DiffAPI, baseRun, host := aeAPI.GetHostname() resultsComparison := summaries.ResultsComparison{ - BaseRun: baseRun, - HeadRun: headRun, - HostName: host, - HostURL: fmt.Sprintf("https://%s/", host), - DiffURL: diffURL.String(), - MasterDiffURL: diffAPI.GetMasterDiffURL(checkState.HeadSHA, checkState.Product, nil).String(), + BaseRun: baseRun, + HeadRun: headRun, + HostName: host, + HostURL: fmt.Sprintf("https://%s/", host), + DiffURL: diffURL.String(), + } + if headRun.LabelsSet().Contains(shared.PRHeadLabel) { + // Deletions are meaningless and abundant comparing to master; ignore them. + masterDiffFilter := shared.DiffFilterParam{Added: true, Changed: true, Unchanged: true} + resultsComparison.MasterDiffURL = diffAPI.GetMasterDiffURL(headRun, &masterDiffFilter).String() } hasRegressions := regressions.Cardinality() > 0 diff --git a/api/checks/update_test.go b/api/checks/update_test.go index 4c7b1c2abd9..b2f179ae5a5 100644 --- a/api/checks/update_test.go +++ b/api/checks/update_test.go @@ -32,7 +32,7 @@ func TestGetDiffSummary_Regressed(t *testing.T) { diffAPI.EXPECT().GetRunsDiff(before, after, gomock.Any(), gomock.Any()).Return(runDiff, nil) diffURL, _ := url.Parse("https://wpt.fyi/results?diff") diffAPI.EXPECT().GetDiffURL(before, after, gomock.Any()).Return(diffURL) - diffAPI.EXPECT().GetMasterDiffURL(after.FullRevisionHash, sharedtest.SameProductSpec(before.BrowserName), nil).Return(diffURL) + diffAPI.EXPECT().GetMasterDiffURL(after, sharedtest.SameDiffFilter("ACU")).Return(diffURL) summary, err := getDiffSummary(aeAPI, diffAPI, before, after) assert.Nil(t, err) @@ -57,7 +57,7 @@ func TestGetDiffSummary_Completed(t *testing.T) { diffAPI.EXPECT().GetRunsDiff(before, after, gomock.Any(), gomock.Any()).Return(runDiff, nil) diffURL, _ := url.Parse("https://wpt.fyi/results?diff") diffAPI.EXPECT().GetDiffURL(before, after, gomock.Any()).Return(diffURL) - diffAPI.EXPECT().GetMasterDiffURL(after.FullRevisionHash, sharedtest.SameProductSpec(before.BrowserName), nil).Return(diffURL) + diffAPI.EXPECT().GetMasterDiffURL(after, sharedtest.SameDiffFilter("ACU")).Return(diffURL) summary, err := getDiffSummary(aeAPI, diffAPI, before, after) assert.Nil(t, err) @@ -68,7 +68,9 @@ func TestGetDiffSummary_Completed(t *testing.T) { func getBeforeAndAfterRuns() (before, after shared.TestRun) { before.FullRevisionHash = strings.Repeat("0", 40) before.BrowserName = "chrome" + before.Labels = []string{shared.PRBaseLabel} after.FullRevisionHash = strings.Repeat("1", 40) after.BrowserName = "chrome" + after.Labels = []string{shared.PRHeadLabel} return before, after } diff --git a/api/receiver/create_run.go b/api/receiver/create_run.go index c2ca7959f63..dc1ea38e817 100644 --- a/api/receiver/create_run.go +++ b/api/receiver/create_run.go @@ -12,7 +12,7 @@ import ( "strings" "time" - "github.com/deckarep/golang-set" + mapset "github.com/deckarep/golang-set" "github.com/web-platform-tests/wpt.fyi/api/checks" "github.com/web-platform-tests/wpt.fyi/shared" ) diff --git a/shared/params.go b/shared/params.go index 0def032f55b..d3e5b16081e 100644 --- a/shared/params.go +++ b/shared/params.go @@ -118,9 +118,7 @@ func (p ProductSpec) String() string { if p.Labels.Cardinality() > 0 { labels := make([]string, 0, p.Labels.Cardinality()) for l := range p.Labels.Iter() { - if l != "" { - labels = append(labels, l.(string)) - } + labels = append(labels, l.(string)) } sort.Strings(labels) // Deterministic String() output. s += "[" + strings.Join(labels, ",") + "]" diff --git a/shared/run_diff.go b/shared/run_diff.go index 97bd010c6f2..9c371168aa6 100644 --- a/shared/run_diff.go +++ b/shared/run_diff.go @@ -24,7 +24,7 @@ import ( type DiffAPI interface { GetRunsDiff(before, after TestRun, filter DiffFilterParam, paths mapset.Set) (RunDiff, error) GetDiffURL(before, after TestRun, diffFilter *DiffFilterParam) *url.URL - GetMasterDiffURL(sha string, product ProductSpec, diffFilter *DiffFilterParam) *url.URL + GetMasterDiffURL(testRun TestRun, diffFilter *DiffFilterParam) *url.URL } type diffAPIImpl struct { @@ -53,25 +53,28 @@ func (d diffAPIImpl) GetDiffURL(before, after TestRun, diffFilter *DiffFilterPar return detailsURL } -func (d diffAPIImpl) GetMasterDiffURL(sha string, product ProductSpec, diffFilter *DiffFilterParam) *url.URL { - filter := TestRunFilter{} - filter.Products = ProductSpecs{product, product} - filter.Products[0].Revision = "" // No specific SHA for base (master). - if product.Labels == nil { - filter.Products[0].Labels = mapset.NewSet() - } - filter.Products[0].Labels.Add(MasterLabel) +// GetMasterDiffURL returns the diff url for comparing a pr_head run against the most recent +// master run for the same product channel. +func (d diffAPIImpl) GetMasterDiffURL(testRun TestRun, diffFilter *DiffFilterParam) *url.URL { + runSpec := ProductSpec{} + runSpec.ProductAtRevision = testRun.ProductAtRevision + runSpec.Labels = mapset.NewSetWith(PRHeadLabel) - filter.Products[1].Revision = sha // Specific SHA for head. + masterSpec := ProductSpec{} + masterSpec.BrowserName = testRun.BrowserName + masterSpec.Labels = mapset.NewSetWith(testRun.Channel(), MasterLabel) - detailsURL := d.aeAPI.GetResultsURL(filter) - query := detailsURL.Query() + filter := TestRunFilter{ + Products: ProductSpecs{runSpec, masterSpec}, + } + diffURL := d.aeAPI.GetResultsURL(filter) + query := diffURL.Query() query.Set("diff", "") if diffFilter != nil { query.Set("filter", diffFilter.String()) } - detailsURL.RawQuery = query.Encode() - return detailsURL + diffURL.RawQuery = query.Encode() + return diffURL } // RunDiff represents a summary of the differences between 2 runs. diff --git a/shared/sharedtest/run_diff_mock.go b/shared/sharedtest/run_diff_mock.go index 179c7a5f72d..3d095fb0e39 100644 --- a/shared/sharedtest/run_diff_mock.go +++ b/shared/sharedtest/run_diff_mock.go @@ -48,15 +48,15 @@ func (mr *MockDiffAPIMockRecorder) GetDiffURL(arg0, arg1, arg2 interface{}) *gom } // GetMasterDiffURL mocks base method -func (m *MockDiffAPI) GetMasterDiffURL(arg0 string, arg1 shared.ProductSpec, arg2 *shared.DiffFilterParam) *url.URL { - ret := m.ctrl.Call(m, "GetMasterDiffURL", arg0, arg1, arg2) +func (m *MockDiffAPI) GetMasterDiffURL(arg0 shared.TestRun, arg1 *shared.DiffFilterParam) *url.URL { + ret := m.ctrl.Call(m, "GetMasterDiffURL", arg0, arg1) ret0, _ := ret[0].(*url.URL) return ret0 } // GetMasterDiffURL indicates an expected call of GetMasterDiffURL -func (mr *MockDiffAPIMockRecorder) GetMasterDiffURL(arg0, arg1, arg2 interface{}) *gomock.Call { - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMasterDiffURL", reflect.TypeOf((*MockDiffAPI)(nil).GetMasterDiffURL), arg0, arg1, arg2) +func (mr *MockDiffAPIMockRecorder) GetMasterDiffURL(arg0, arg1 interface{}) *gomock.Call { + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMasterDiffURL", reflect.TypeOf((*MockDiffAPI)(nil).GetMasterDiffURL), arg0, arg1) } // GetRunsDiff mocks base method diff --git a/shared/sharedtest/util.go b/shared/sharedtest/util.go index 31336379ab7..f37b41a8849 100644 --- a/shared/sharedtest/util.go +++ b/shared/sharedtest/util.go @@ -49,7 +49,7 @@ func NewTestContext() context.Context { return ctx } -type sameProductSpec struct { +type sameStringSpec struct { spec string } @@ -57,7 +57,7 @@ type stringifiable interface { String() string } -func (s sameProductSpec) Matches(x interface{}) bool { +func (s sameStringSpec) Matches(x interface{}) bool { if p, ok := x.(stringifiable); ok && p.String() == s.spec { return true } else if str, ok := x.(string); ok && str == s.spec { @@ -65,13 +65,20 @@ func (s sameProductSpec) Matches(x interface{}) bool { } return false } -func (s sameProductSpec) String() string { +func (s sameStringSpec) String() string { return s.spec } // SameProductSpec returns a gomock matcher for a product spec. func SameProductSpec(spec string) gomock.Matcher { - return sameProductSpec{ + return sameStringSpec{ spec: spec, } } + +// SameDiffFilter returns a gomock matcher for a diff filter. +func SameDiffFilter(filter string) gomock.Matcher { + return sameStringSpec{ + spec: filter, + } +}