diff --git a/api/checks/update.go b/api/checks/update.go index c9a779e7e9f..9c46acb6f8c 100644 --- a/api/checks/update.go +++ b/api/checks/update.go @@ -11,7 +11,7 @@ import ( "sort" "time" - "github.com/deckarep/golang-set" + mapset "github.com/deckarep/golang-set" "github.com/gorilla/mux" "github.com/web-platform-tests/wpt.fyi/api/checks/summaries" @@ -51,7 +51,7 @@ func updateCheckHandler(w http.ResponseWriter, r *http.Request) { return } - if len(filter.Products) < 1 { + if len(filter.Products) != 1 { msg := "product param is missing" log.Warningf(msg) http.Error(w, msg, http.StatusBadRequest) @@ -63,6 +63,7 @@ func updateCheckHandler(w http.ResponseWriter, r *http.Request) { msg := "Could not find runs to compare" if err != nil { msg = fmt.Sprintf("%s: %s", msg, err.Error()) + log.Errorf(msg) } http.Error(w, msg, http.StatusNotFound) return @@ -95,39 +96,65 @@ func updateCheckHandler(w http.ResponseWriter, r *http.Request) { } } -func loadRunsToCompare(ctx context.Context, filter shared.TestRunFilter) (prRun, masterRun *shared.TestRun, err error) { - log := shared.GetLogger(ctx) +func loadRunsToCompare(ctx context.Context, filter shared.TestRunFilter) (headRun, baseRun *shared.TestRun, err error) { one := 1 runs, err := shared.LoadTestRuns(ctx, filter.Products, filter.Labels, filter.SHA, filter.From, filter.To, &one, nil) - prRun = runs.First() if err != nil { - log.Errorf("Failed to load test runs: %s", err.Error()) - return nil, nil, err - } else if prRun == nil { - log.Debugf("No runs found for %s @ %s", filter.Products[0].String(), filter.SHA[:7]) return nil, nil, err - } else if len(runs.AllRuns()) > 1 { - log.Errorf("Found more that one test run") - return nil, nil, fmt.Errorf("Expected exactly 1 run, but found %v", len(runs)) } + run := runs.First() + if run == nil { + return nil, nil, fmt.Errorf("no test run found for %s @ %s", filter.Products[0].String(), filter.SHA[:7]) + } + + labels := run.LabelsSet() + if labels.Contains(shared.MasterLabel) { + headRun = run + baseRun, err = loadMasterRunBefore(ctx, filter, headRun) + } else if labels.Contains(shared.PRBaseLabel) { + baseRun = run + headRun, err = loadPRRun(ctx, filter, shared.PRHeadLabel) + } else if labels.Contains(shared.PRHeadLabel) { + headRun = run + baseRun, err = loadPRRun(ctx, filter, shared.PRBaseLabel) + } else { + return nil, nil, fmt.Errorf("test run %d doesn't have pr_base, pr_head or master label", run.ID) + } + + return headRun, baseRun, err +} + +func loadPRRun(ctx context.Context, filter shared.TestRunFilter, extraLabel string) (*shared.TestRun, error) { + // Find the corresponding pr_base run. + one := 1 + labels := mapset.NewSetWith(extraLabel) + runs, err := shared.LoadTestRuns(ctx, filter.Products, labels, filter.SHA, nil, nil, &one, nil) + run := runs.First() + if err != nil { + return nil, err + } + if run == nil { + err = fmt.Errorf("no test run found for %s @ %s with label %s", + filter.Products[0].String(), filter.SHA, extraLabel) + } + return run, err +} +func loadMasterRunBefore(ctx context.Context, filter shared.TestRunFilter, headRun *shared.TestRun) (*shared.TestRun, error) { // Get the most recent, but still earlier, master run to compare. - labels := filter.Labels - if labels == nil { - labels = mapset.NewSet() - } - labels.Add("master") - to := prRun.TimeStart.Add(-time.Millisecond) - masterRuns, err := shared.LoadTestRuns(ctx, filter.Products, labels, "", nil, &to, &one, nil) - masterRun = masterRuns.First() + one := 1 + to := headRun.TimeStart.Add(-time.Millisecond) + labels := mapset.NewSetWith(shared.MasterLabel) + runs, err := shared.LoadTestRuns(ctx, filter.Products, labels, "", nil, &to, &one, nil) + baseRun := runs.First() if err != nil { - log.Errorf("Failed to load master run: %s", err.Error()) - return prRun, nil, err - } else if masterRun == nil { - log.Debugf("No masters runs found before %s @ %s", filter.Products[0].String(), filter.SHA[:7]) - return prRun, masterRun, fmt.Errorf("No master run found to compare differences") + return nil, err + } + if baseRun == nil { + err = fmt.Errorf("no master run found for %s before %s", + filter.Products[0].String(), filter.SHA) } - return prRun, masterRun, nil + return baseRun, err } func getDiffSummary(aeAPI shared.AppEngineAPI, diffAPI shared.DiffAPI, masterRun, prRun shared.TestRun) (summaries.Summary, error) { diff --git a/api/checks/update_medium_test.go b/api/checks/update_medium_test.go index 7ebf756861a..a2a796c5dba 100644 --- a/api/checks/update_medium_test.go +++ b/api/checks/update_medium_test.go @@ -17,7 +17,7 @@ import ( "google.golang.org/appengine/datastore" ) -func TestLoadRunsToCompare(t *testing.T) { +func TestLoadRunsToCompare_master(t *testing.T) { ctx, done, err := sharedtest.NewAEContext(true) assert.Nil(t, err) defer done() @@ -25,9 +25,7 @@ func TestLoadRunsToCompare(t *testing.T) { testRun := shared.TestRun{ ProductAtRevision: shared.ProductAtRevision{ Product: shared.Product{ - BrowserName: "chrome", - BrowserVersion: "63.0", - OSName: "linux", + BrowserName: "chrome", }, }, Labels: []string{"master"}, @@ -42,13 +40,86 @@ func TestLoadRunsToCompare(t *testing.T) { chrome, _ := shared.ParseProductSpec("chrome") filter := shared.TestRunFilter{ - SHA: strings.Repeat("1", 10), + SHA: "1111111111", Products: shared.ProductSpecs{chrome}, } - prRun, masterRun, err := loadRunsToCompare(ctx, filter) + headRun, baseRun, err := loadRunsToCompare(ctx, filter) + + assert.Nil(t, err) + assert.NotNil(t, headRun) + assert.NotNil(t, baseRun) + assert.Equal(t, "0000000000", baseRun.Revision) + assert.Equal(t, "1111111111", headRun.Revision) +} + +func TestLoadRunsToCompare_pr_base_first(t *testing.T) { + ctx, done, err := sharedtest.NewAEContext(true) + assert.Nil(t, err) + defer done() + + labelsForRuns := [][]string{{"pr_base"}, {"pr_head"}} + yesterday := time.Now().AddDate(0, 0, -1) + for i := 0; i < 2; i++ { + testRun := shared.TestRun{ + ProductAtRevision: shared.ProductAtRevision{ + Product: shared.Product{ + BrowserName: "chrome", + }, + Revision: "1234567890", + }, + TimeStart: yesterday.Add(time.Duration(i) * time.Hour), + Labels: labelsForRuns[i], + } + key := datastore.NewIncompleteKey(ctx, "TestRun", nil) + key, _ = datastore.Put(ctx, key, &testRun) + } + + chrome, _ := shared.ParseProductSpec("chrome") + filter := shared.TestRunFilter{ + SHA: "1234567890", + Products: shared.ProductSpecs{chrome}, + } + headRun, baseRun, err := loadRunsToCompare(ctx, filter) + + assert.Nil(t, err) + assert.NotNil(t, headRun) + assert.NotNil(t, baseRun) + assert.Equal(t, []string{"pr_base"}, baseRun.Labels) + assert.Equal(t, []string{"pr_head"}, headRun.Labels) +} + +func TestLoadRunsToCompare_pr_head_first(t *testing.T) { + ctx, done, err := sharedtest.NewAEContext(true) assert.Nil(t, err) - if prRun == nil || masterRun == nil { - assert.FailNow(t, "Nil run(s) returned") + defer done() + + labelsForRuns := [][]string{{"pr_head"}, {"pr_base"}} + yesterday := time.Now().AddDate(0, 0, -1) + for i := 0; i < 2; i++ { + testRun := shared.TestRun{ + ProductAtRevision: shared.ProductAtRevision{ + Product: shared.Product{ + BrowserName: "chrome", + }, + Revision: "1234567890", + }, + TimeStart: yesterday.Add(time.Duration(i) * time.Hour), + Labels: labelsForRuns[i], + } + key := datastore.NewIncompleteKey(ctx, "TestRun", nil) + key, _ = datastore.Put(ctx, key, &testRun) } - assert.NotEqual(t, prRun.Revision, masterRun.Revision) + + chrome, _ := shared.ParseProductSpec("chrome") + filter := shared.TestRunFilter{ + SHA: "1234567890", + Products: shared.ProductSpecs{chrome}, + } + headRun, baseRun, err := loadRunsToCompare(ctx, filter) + + assert.Nil(t, err) + assert.NotNil(t, headRun) + assert.NotNil(t, baseRun) + assert.Equal(t, []string{"pr_base"}, baseRun.Labels) + assert.Equal(t, []string{"pr_head"}, headRun.Labels) } diff --git a/api/receiver/create_run.go b/api/receiver/create_run.go index b173d9a53df..da4608be338 100644 --- a/api/receiver/create_run.go +++ b/api/receiver/create_run.go @@ -65,9 +65,12 @@ func HandleResultsCreate(a AppEngineAPI, s checks.API, w http.ResponseWriter, r return } - spec := shared.ProductSpec{} - spec.BrowserName = testRun.BrowserName - s.ScheduleResultsProcessing(testRun.FullRevisionHash, spec) + // Do not schedule on pr_base to avoid redundancy with pr_head. + if !testRun.LabelsSet().Contains(shared.PRBaseLabel) { + spec := shared.ProductSpec{} + spec.BrowserName = testRun.BrowserName + s.ScheduleResultsProcessing(testRun.FullRevisionHash, spec) + } // Copy int64 representation of key into TestRun.ID so that clients can // inspect/use key value.