Compare PR runs against the base runs

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.
web-platform-tests · Dec 10, 2018 · 1f58ff6 · 1f58ff6
1 parent 8ac9cef
commit 1f58ff6
Show file tree

Hide file tree

Showing 3 changed files with 139 additions and 38 deletions.
diff --git 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
@@ -17,17 +17,15 @@ 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()
 
 	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
@@ -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.