[gold] /json/triage RPC: If set, use image matching algorithm name as change author.
Bug: skia:9527
Change-Id: I63ad7715cb44efa1e0fc81b095ac056a3dcad055
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/289632
Commit-Queue: Leandro Lovisolo <lovisolo@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
diff --git a/gold-client/go/goldclient/goldclient.go b/gold-client/go/goldclient/goldclient.go
index cedec00..65036d6 100644
--- a/gold-client/go/goldclient/goldclient.go
+++ b/gold-client/go/goldclient/goldclient.go
@@ -791,8 +791,9 @@
func (c *CloudClient) TriageAsPositive(testName types.TestName, digest types.Digest, changeListId string) error {
// Build TriageRequest struct and encode it into JSON.
triageRequest := &frontend.TriageRequest{
- TestDigestStatus: map[types.TestName]map[types.Digest]string{testName: {digest: expectations.Positive.String()}},
- ChangeListID: changeListId,
+ TestDigestStatus: map[types.TestName]map[types.Digest]string{testName: {digest: expectations.Positive.String()}},
+ ChangeListID: changeListId,
+ ImageMatchingAlgorithm: "", // TODO(lovisolo): Pipe through the image matching algorithm name.
}
jsonTriageRequest, err := json.Marshal(triageRequest)
if err != nil {
diff --git a/gold-client/go/goldclient/goldclient_test.go b/gold-client/go/goldclient/goldclient_test.go
index b7a2d85..e29819a 100644
--- a/gold-client/go/goldclient/goldclient_test.go
+++ b/gold-client/go/goldclient/goldclient_test.go
@@ -2004,7 +2004,7 @@
url := "https://testing-gold.skia.org/json/triage"
contentType := "application/json"
- body := bytes.NewReader([]byte(`{"testDigestStatus":{"MyTest":{"deadbeefcafefe771d61bf0ed3d84bc2":"positive"}},"issue":"123456"}`))
+ body := bytes.NewReader([]byte(`{"testDigestStatus":{"MyTest":{"deadbeefcafefe771d61bf0ed3d84bc2":"positive"}},"issue":"123456","imageMatchingAlgorithm":""}`))
httpClient.On("Post", url, contentType, body).Return(httpResponse([]byte{}, "200 OK", http.StatusOK), nil)
err = goldClient.TriageAsPositive("MyTest", "deadbeefcafefe771d61bf0ed3d84bc2", "123456")
@@ -2033,7 +2033,7 @@
url := "https://testing-gold.skia.org/json/triage"
contentType := "application/json"
- body := bytes.NewReader([]byte(`{"testDigestStatus":{"MyTest":{"deadbeefcafefe771d61bf0ed3d84bc2":"positive"}},"issue":"123456"}`))
+ body := bytes.NewReader([]byte(`{"testDigestStatus":{"MyTest":{"deadbeefcafefe771d61bf0ed3d84bc2":"positive"}},"issue":"123456","imageMatchingAlgorithm":""}`))
httpClient.On("Post", url, contentType, body).Return(httpResponse([]byte{}, "500 Internal Server Error", http.StatusInternalServerError), nil)
err = goldClient.TriageAsPositive("MyTest", "deadbeefcafefe771d61bf0ed3d84bc2", "123456")
diff --git a/golden/go/web/frontend/types.go b/golden/go/web/frontend/types.go
index 8d75f60..36bb6a9 100644
--- a/golden/go/web/frontend/types.go
+++ b/golden/go/web/frontend/types.go
@@ -95,6 +95,15 @@
// ChangeListID is the id of the ChangeList for which we want to change the expectations.
// "issue" is the JSON field for backwards compatibility.
ChangeListID string `json:"issue"`
+
+ // ImageMatchingAlgorithm is the name of the non-exact image matching algorithm requesting the
+ // triage (see http://go/gold-non-exact-matching). If set, the algorithm name will be used as
+ // the author of the triage action.
+ //
+ // An empty image matching algorithm indicates this is a manual triage operation, in which case
+ // the username that initiated the triage operation via Gold's UI will be used as the author of
+ // the operation.
+ ImageMatchingAlgorithm string `json:"imageMatchingAlgorithm"`
}
// TriageDelta represents one changed digest and the label that was
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 920d980..e5d41d0 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -934,6 +934,11 @@
expStore = wh.ExpectationsStore.ForChangeList(req.ChangeListID, wh.ChangeListStore.System())
}
+ // If set, use the image matching algorithm's name as the author of this change.
+ if req.ImageMatchingAlgorithm != "" {
+ user = req.ImageMatchingAlgorithm
+ }
+
// Add the change.
if err := expStore.AddChange(ctx, tc, user); err != nil {
return skerr.Wrapf(err, "Failed to store the updated expectations.")
diff --git a/golden/go/web/web_test.go b/golden/go/web/web_test.go
index c87472c..200b719 100644
--- a/golden/go/web/web_test.go
+++ b/golden/go/web/web_test.go
@@ -550,15 +550,15 @@
}
}
-// TestTriage_SingleDigestOnMaster_SunnyDay_Success tests a common case of a developer triaging a
-// single test on the master branch.
-func TestTriage_SingleDigestOnMaster_SunnyDay_Success(t *testing.T) {
+// TestTriage_SingleDigestOnMaster_Success tests a common case of a developer triaging a single
+// test on the master branch.
+func TestTriage_SingleDigestOnMaster_Success(t *testing.T) {
unittest.SmallTest(t)
mes := &mock_expectations.Store{}
defer mes.AssertExpectations(t)
- user := "user@example.com"
+ const user = "user@example.com"
mes.On("AddChange", testutils.AnyContext, []expectations.Delta{
{
@@ -587,9 +587,46 @@
assert.NoError(t, err)
}
-// TestTriage_SingleDigestOnCL_SunnyDay_Success tests a common case of a developer triaging a single
-// test on a ChangeList.
-func TestTriage_SingleDigestOnCL_SunnyDay_Success(t *testing.T) {
+func TestTriage_SingleDigestOnMaster_ImageMatchingAlgorithmSet_UsesAlgorithmNameAsAuthor(t *testing.T) {
+ unittest.SmallTest(t)
+
+ mes := &mock_expectations.Store{}
+ defer mes.AssertExpectations(t)
+
+ const user = "user@example.com"
+ const algorithmName = "fuzzy"
+
+ mes.On("AddChange", testutils.AnyContext, []expectations.Delta{
+ {
+ Grouping: bug_revert.TestOne,
+ Digest: bug_revert.BravoUntriagedDigest,
+ Label: expectations.Negative,
+ },
+ }, algorithmName).Return(nil)
+
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ ExpectationsStore: mes,
+ },
+ }
+
+ tr := frontend.TriageRequest{
+ ChangeListID: "",
+ TestDigestStatus: map[types.TestName]map[types.Digest]string{
+ bug_revert.TestOne: {
+ bug_revert.BravoUntriagedDigest: expectations.Negative.String(),
+ },
+ },
+ ImageMatchingAlgorithm: algorithmName,
+ }
+
+ err := wh.triage(context.Background(), user, tr)
+ assert.NoError(t, err)
+}
+
+// TestTriage_SingleDigestOnCL_Success tests a common case of a developer triaging a single test on
+// a ChangeList.
+func TestTriage_SingleDigestOnCL_Success(t *testing.T) {
unittest.SmallTest(t)
mes := &mock_expectations.Store{}
@@ -599,9 +636,9 @@
defer clExp.AssertExpectations(t)
defer mcs.AssertExpectations(t)
- clID := "12345"
- crs := "github"
- user := "user@example.com"
+ const clID = "12345"
+ const crs = "github"
+ const user = "user@example.com"
mes.On("ForChangeList", clID, crs).Return(clExp)
@@ -635,6 +672,54 @@
assert.NoError(t, err)
}
+func TestTriage_SingleDigestOnCL_ImageMatchingAlgorithmSet_UsesAlgorithmNameAsAuthor(t *testing.T) {
+ unittest.SmallTest(t)
+
+ mes := &mock_expectations.Store{}
+ clExp := &mock_expectations.Store{}
+ mcs := &mock_clstore.Store{}
+ defer mes.AssertExpectations(t)
+ defer clExp.AssertExpectations(t)
+ defer mcs.AssertExpectations(t)
+
+ const clID = "12345"
+ const crs = "github"
+ const user = "user@example.com"
+ const algorithmName = "fuzzy"
+
+ mes.On("ForChangeList", clID, crs).Return(clExp)
+
+ clExp.On("AddChange", testutils.AnyContext, []expectations.Delta{
+ {
+ Grouping: bug_revert.TestOne,
+ Digest: bug_revert.BravoUntriagedDigest,
+ Label: expectations.Negative,
+ },
+ }, algorithmName).Return(nil)
+
+ mcs.On("System").Return(crs)
+
+ wh := Handlers{
+ HandlersConfig: HandlersConfig{
+ ExpectationsStore: mes,
+ ChangeListStore: mcs,
+ },
+ }
+
+ tr := frontend.TriageRequest{
+ ChangeListID: clID,
+ TestDigestStatus: map[types.TestName]map[types.Digest]string{
+ bug_revert.TestOne: {
+ bug_revert.BravoUntriagedDigest: expectations.Negative.String(),
+ },
+ },
+ ImageMatchingAlgorithm: algorithmName,
+ }
+
+ err := wh.triage(context.Background(), user, tr)
+ assert.NoError(t, err)
+}
+
// TestTriage_BulkTriageOnMaster_SunnyDay_Success tests the case of a developer triaging multiple
// tests at once (via bulk triage).
func TestTriage_BulkTriageOnMaster_SunnyDay_Success(t *testing.T) {
@@ -643,7 +728,7 @@
mes := &mock_expectations.Store{}
defer mes.AssertExpectations(t)
- user := "user@example.com"
+ const user = "user@example.com"
matcher := mock.MatchedBy(func(delta []expectations.Delta) bool {
assert.Contains(t, delta, expectations.Delta{
@@ -703,7 +788,7 @@
mes := &mock_expectations.Store{}
defer mes.AssertExpectations(t)
- user := "user@example.com"
+ const user = "user@example.com"
mes.On("AddChange", testutils.AnyContext, []expectations.Delta{
{
