[gold] Add JSON endpoint to query untriaged digests for a CL.
This is for https://github.com/flutter/flutter/issues/48744
Change-Id: I5bde2607deb46d0f49535a5fdc86f3ec8febfb28
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/264571
Commit-Queue: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/golden/cmd/skiacorrectness/main.go b/golden/cmd/skiacorrectness/main.go
index f38d4e6..42bfc38 100644
--- a/golden/cmd/skiacorrectness/main.go
+++ b/golden/cmd/skiacorrectness/main.go
@@ -521,6 +521,7 @@
jsonRouter.HandleFunc(trim("/json/triagelog/undo"), handlers.TriageUndoHandler).Methods("POST")
jsonRouter.HandleFunc(trim("/json/changelists"), handlers.ChangeListsHandler).Methods("GET")
jsonRouter.HandleFunc(trim("/json/changelist/{system}/{id}"), handlers.ChangeListSummaryHandler).Methods("GET")
+ jsonRouter.HandleFunc(trim("/json/changelist/{system}/{id}/{patchset}/untriaged"), handlers.ChangeListUntriagedHandler).Methods("GET")
jsonRouter.HandleFunc(trim("/json/digests"), handlers.DigestListHandler).Methods("GET")
// Retrieving that baseline for master and an Gerrit issue are handled the same way
diff --git a/golden/go/search/frontend/types.go b/golden/go/search/frontend/types.go
index 101dfc1..ce5d004 100644
--- a/golden/go/search/frontend/types.go
+++ b/golden/go/search/frontend/types.go
@@ -86,3 +86,8 @@
Left *SRDigest `json:"left"` // The left hand digest and its params.
Right *SRDiffDigest `json:"right"` // The right hand digest, its params and the diff result.
}
+
+// DigestList represents multiple returned digests.
+type DigestList struct {
+ Digests []types.Digest `json:"digests"`
+}
diff --git a/golden/go/search/search.go b/golden/go/search/search.go
index 78f015c..21724df 100644
--- a/golden/go/search/search.go
+++ b/golden/go/search/search.go
@@ -685,5 +685,54 @@
return -1
}
+// UntriagedUnignoredTryJobExclusiveDigests implements the SearchAPI interface. It uses the cached
+// TryJobResults, so as to improve performance.
+// TODO(kjlubick) when we have indexes for changelist results, use those.
+func (s *SearchImpl) UntriagedUnignoredTryJobExclusiveDigests(ctx context.Context, psID tjstore.CombinedPSID) (*frontend.DigestList, error) {
+ xtr, err := s.getTryJobResults(ctx, psID)
+ if err != nil {
+ return nil, skerr.Wrapf(err, "getting tryjob results for %v", psID)
+ }
+ exp, err := s.getExpectations(ctx, psID.CL, psID.CRS)
+ if err != nil {
+ return nil, skerr.Wrap(err)
+ }
+
+ idx := s.indexSource.GetIndex()
+ ignoreMatcher := idx.GetIgnoreMatcher()
+ knownDigestsForTest := idx.DigestCountsByTest(types.IncludeIgnoredTraces)
+
+ var returnDigests []types.Digest
+
+ for _, tr := range xtr {
+ if err := ctx.Err(); err != nil {
+ return nil, skerr.Wrap(err)
+ }
+ tn := types.TestName(tr.ResultParams[types.PRIMARY_KEY_FIELD])
+ if exp.Classification(tn, tr.Digest) != expectations.Untriaged {
+ // It's been triaged already.
+ continue
+ }
+ if _, ok := knownDigestsForTest[tn][tr.Digest]; ok {
+ // It's already been seen on master
+ continue
+ }
+ p := make(paramtools.Params, len(tr.ResultParams)+len(tr.GroupParams)+len(tr.Options))
+ p.Add(tr.GroupParams)
+ p.Add(tr.Options)
+ p.Add(tr.ResultParams)
+ if ignoreMatcher.MatchAnyParams(p) {
+ // This trace matches an ignore
+ continue
+ }
+ returnDigests = append(returnDigests, tr.Digest)
+ }
+ // Sort digests alphabetically for determinism.
+ sort.Slice(returnDigests, func(i, j int) bool {
+ return returnDigests[i] < returnDigests[j]
+ })
+ return &frontend.DigestList{Digests: returnDigests}, nil
+}
+
// Make sure SearchImpl fulfills the SearchAPI interface.
var _ SearchAPI = (*SearchImpl)(nil)
diff --git a/golden/go/search/search_test.go b/golden/go/search/search_test.go
index c0f00cd..4067baf 100644
--- a/golden/go/search/search_test.go
+++ b/golden/go/search/search_test.go
@@ -1093,6 +1093,137 @@
assert.Equal(t, cd.Left.Status, expectations.Negative.String())
}
+// TestUntriagedUnignoredTryJobExclusiveDigestsSunnyDay models the case where a set of TryJobs has
+// produced five digests that were "untriaged on master" (and one good digest). We are testing that
+// we can properly deduce which are untriaged, "newly seen" and unignored. One of these untriaged
+// digests was already seen on master (data.AlphaUntriaged1Digest), one was already triaged negative
+// for this CL (gammaNegativeTryJobDigest), and one trace matched an ignore rule (deltaIgnoredTryJobDigest). Thus,
+// We only expect tjUntriagedAlpha and tjUntriagedBeta to be reported to us.
+func TestUntriagedUnignoredTryJobExclusiveDigestsSunnyDay(t *testing.T) {
+ unittest.SmallTest(t)
+
+ const clID = "44474"
+ const crs = "github"
+ expectedID := tjstore.CombinedPSID{
+ CL: clID,
+ CRS: crs,
+ PS: "abcdef",
+ }
+
+ const alphaUntriagedTryJobDigest = types.Digest("aaaa65e567de97c8a62918401731c7ec")
+ const betaUntriagedTryJobDigest = types.Digest("bbbb34f7c915a1ac3a5ba524c741946c")
+ const gammaNegativeTryJobDigest = types.Digest("cccc41bf4584e51be99e423707157277")
+ const deltaIgnoredTryJobDigest = types.Digest("dddd84e51be99e42370715727765e563")
+
+ mes := &mocks.ExpectationsStore{}
+ mi := &mock_index.IndexSource{}
+ issueStore := &mocks.ExpectationsStore{}
+ mtjs := &mock_tjstore.Store{}
+ defer mes.AssertExpectations(t)
+ defer mi.AssertExpectations(t)
+ defer issueStore.AssertExpectations(t)
+ defer mtjs.AssertExpectations(t)
+
+ cpxTile := types.NewComplexTile(data.MakeTestTile())
+ reduced := data.MakeTestTile()
+ delete(reduced.Traces, data.BullheadBetaTraceID)
+ // The following rule exclusively matches BullheadBetaTraceID, for which the tryjob produced
+ // deltaIgnoredTryJobDigest
+ cpxTile.SetIgnoreRules(reduced, paramtools.ParamMatcher{
+ {
+ "device": []string{data.BullheadDevice},
+ types.PRIMARY_KEY_FIELD: []string{string(data.BetaTest)},
+ },
+ })
+ dc := digest_counter.New(data.MakeTestTile())
+ fis, err := indexer.SearchIndexForTesting(cpxTile, [2]digest_counter.DigestCounter{dc, dc}, [2]paramsets.ParamSummary{}, mes, nil)
+ require.NoError(t, err)
+ mi.On("GetIndex").Return(fis)
+
+ // Set up the expectations such that for this CL, we have one extra expectation - marking
+ // gammaNegativeTryJobDigest negative (it would be untriaged on master).
+ mes.On("ForChangeList", clID, crs).Return(issueStore, nil)
+ var ie expectations.Expectations
+ ie.Set(data.AlphaTest, gammaNegativeTryJobDigest, expectations.Negative)
+ issueStore.On("Get", testutils.AnyContext).Return(&ie, nil)
+ mes.On("Get", testutils.AnyContext).Return(data.MakeTestExpectations(), nil)
+
+ anglerGroup := map[string]string{
+ "device": data.AnglerDevice,
+ }
+ bullheadGroup := map[string]string{
+ "device": data.BullheadDevice,
+ }
+ crosshatchGroup := map[string]string{
+ "device": data.CrosshatchDevice,
+ }
+ options := map[string]string{
+ "ext": "png",
+ }
+ mtjs.On("GetResults", testutils.AnyContext, expectedID).Return([]tjstore.TryJobResult{
+ {
+ GroupParams: anglerGroup,
+ Options: options,
+ Digest: betaUntriagedTryJobDigest,
+ ResultParams: map[string]string{
+ types.PRIMARY_KEY_FIELD: string(data.AlphaTest),
+ types.CORPUS_FIELD: "gm",
+ },
+ },
+ {
+ GroupParams: bullheadGroup,
+ Options: options,
+ Digest: data.AlphaUntriaged1Digest,
+ ResultParams: map[string]string{
+ types.PRIMARY_KEY_FIELD: string(data.AlphaTest),
+ types.CORPUS_FIELD: "gm",
+ },
+ },
+ {
+ GroupParams: anglerGroup,
+ Options: options,
+ Digest: alphaUntriagedTryJobDigest,
+ ResultParams: map[string]string{
+ types.PRIMARY_KEY_FIELD: string(data.BetaTest),
+ types.CORPUS_FIELD: "gm",
+ },
+ },
+ {
+ GroupParams: bullheadGroup,
+ Options: options,
+ Digest: deltaIgnoredTryJobDigest,
+ ResultParams: map[string]string{
+ types.PRIMARY_KEY_FIELD: string(data.BetaTest),
+ types.CORPUS_FIELD: "gm",
+ },
+ },
+ {
+ GroupParams: crosshatchGroup,
+ Options: options,
+ Digest: gammaNegativeTryJobDigest,
+ ResultParams: map[string]string{
+ types.PRIMARY_KEY_FIELD: string(data.AlphaTest),
+ types.CORPUS_FIELD: "gm",
+ },
+ },
+ {
+ GroupParams: crosshatchGroup,
+ Options: options,
+ Digest: data.BetaGood1Digest,
+ ResultParams: map[string]string{
+ types.PRIMARY_KEY_FIELD: string(data.BetaTest),
+ types.CORPUS_FIELD: "gm",
+ },
+ },
+ }, nil).Once()
+
+ s := New(nil, mes, mi, nil, mtjs, everythingPublic)
+
+ dl, err := s.UntriagedUnignoredTryJobExclusiveDigests(context.Background(), expectedID)
+ require.NoError(t, err)
+ assert.Equal(t, []types.Digest{alphaUntriagedTryJobDigest, betaUntriagedTryJobDigest}, dl.Digests)
+}
+
var everythingPublic = paramtools.ParamSet{}
// makeThreeDevicesIndex returns a search index corresponding to the three_devices_data
diff --git a/golden/go/search/types.go b/golden/go/search/types.go
index c2f539b..bcf5219 100644
--- a/golden/go/search/types.go
+++ b/golden/go/search/types.go
@@ -5,6 +5,7 @@
"go.skia.org/infra/golden/go/search/frontend"
"go.skia.org/infra/golden/go/search/query"
+ "go.skia.org/infra/golden/go/tjstore"
"go.skia.org/infra/golden/go/types"
)
@@ -25,4 +26,9 @@
// the expectations from the given CL and Code Review System (e.g. "gerrit") will be used
// instead of those at master.
DiffDigests(ctx context.Context, t types.TestName, left, right types.Digest, clID string, crs string) (*frontend.DigestComparison, error)
+
+ // UntriagedUnignoredTryJobExclusiveDigests returns a list of untriaged, non-ignored,
+ // digests that are exclusive to a given CL (i.e. they don't also show up on the master branch
+ // in the last N commits).
+ UntriagedUnignoredTryJobExclusiveDigests(context.Context, tjstore.CombinedPSID) (*frontend.DigestList, error)
}
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 16deca2..89126e0 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -479,6 +479,43 @@
}, nil
}
+// ChangeListUntriagedHandler writes out a list of untriaged digests uploaded by this CL that
+// are not on master already and are not ignored.
+func (wh *Handlers) ChangeListUntriagedHandler(w http.ResponseWriter, r *http.Request) {
+ defer metrics2.FuncTimer().Stop()
+ if err := wh.cheapLimitForAnonUsers(r); err != nil {
+ httputils.ReportError(w, err, "Try again later", http.StatusInternalServerError)
+ return
+ }
+
+ clID, ok := mux.Vars(r)["id"]
+ if !ok {
+ http.Error(w, "Must specify 'id' of ChangeList.", http.StatusBadRequest)
+ return
+ }
+ psID, ok := mux.Vars(r)["patchset"]
+ if !ok {
+ http.Error(w, "Must specify 'patchset' of ChangeList.", http.StatusBadRequest)
+ return
+ }
+ crs, ok := mux.Vars(r)["system"]
+ if !ok {
+ http.Error(w, "Must specify 'system' of ChangeList.", http.StatusBadRequest)
+ return
+ }
+
+ dl, err := wh.SearchAPI.UntriagedUnignoredTryJobExclusiveDigests(r.Context(), tjstore.CombinedPSID{
+ CL: clID,
+ CRS: crs,
+ PS: psID,
+ })
+ if err != nil {
+ httputils.ReportError(w, err, "could not retrieve untriaged digests for CL.", http.StatusInternalServerError)
+ return
+ }
+ sendJSONResponse(w, dl)
+}
+
// SearchHandler is the endpoint for all searches, including accessing
// results that belong to a tryjob. It times out after 3 minutes, to prevent outstanding requests
// from growing unbounded.
