[goldctl] Include grouping info in links to details page.
Bug: skia:13712
Change-Id: I73bb3308605188e4647309c92ba711de27f3b1fa
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/578736
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Leandro Lovisolo <lovisolo@google.com>
diff --git a/gold-client/cmd/goldctl/cmd_imgtest_test.go b/gold-client/cmd/goldctl/cmd_imgtest_test.go
index a39ed65..547cc53 100644
--- a/gold-client/cmd/goldctl/cmd_imgtest_test.go
+++ b/gold-client/cmd/goldctl/cmd_imgtest_test.go
@@ -320,12 +320,12 @@
logs := output.String()
exit.AssertWasCalledWithCode(t, 1, logs)
mg.AssertExpectations(t)
- assert.Contains(t, logs, `Untriaged or negative image: https://my-instance-gold.skia.org/detail?test=pixel-tests&digest=00000000000000000000000000000000`)
+ assert.Contains(t, logs, `Untriaged or negative image: https://my-instance-gold.skia.org/detail?grouping=name%3Dpixel-tests%26source_type%3Dmy_corpus&digest=00000000000000000000000000000000`)
assert.Contains(t, logs, `Test: pixel-tests FAIL`)
fb, err := ioutil.ReadFile(filepath.Join(workDir, "failures.txt"))
require.NoError(t, err)
- assert.Contains(t, string(fb), "https://my-instance-gold.skia.org/detail?test=pixel-tests&digest=00000000000000000000000000000000")
+ assert.Contains(t, string(fb), "https://my-instance-gold.skia.org/detail?grouping=name%3Dpixel-tests%26source_type%3Dmy_corpus&digest=00000000000000000000000000000000")
}
func TestImgTest_InitAdd_OverwriteBucketAndURL_ProperLinks(t *testing.T) {
@@ -397,12 +397,12 @@
logs := output.String()
exit.AssertWasCalledWithCode(t, 1, logs)
mg.AssertExpectations(t)
- assert.Contains(t, logs, `Untriaged or negative image: https://my-custom-gold-url.example.com/detail?test=pixel-tests&digest=00000000000000000000000000000000`)
+ assert.Contains(t, logs, `Untriaged or negative image: https://my-custom-gold-url.example.com/detail?grouping=name%3Dpixel-tests%26source_type%3Dmy_corpus&digest=00000000000000000000000000000000`)
assert.Contains(t, logs, `Test: pixel-tests FAIL`)
fb, err := ioutil.ReadFile(filepath.Join(workDir, "failures.txt"))
require.NoError(t, err)
- assert.Contains(t, string(fb), "https://my-custom-gold-url.example.com/detail?test=pixel-tests&digest=00000000000000000000000000000000")
+ assert.Contains(t, string(fb), "https://my-custom-gold-url.example.com/detail?grouping=name%3Dpixel-tests%26source_type%3Dmy_corpus&digest=00000000000000000000000000000000")
}
func TestImgTest_InitAdd_StreamingPassFail_MatchesExpectations_ZeroExitCode(t *testing.T) {
diff --git a/gold-client/go/goldclient/goldclient.go b/gold-client/go/goldclient/goldclient.go
index a589b96..f42df04 100644
--- a/gold-client/go/goldclient/goldclient.go
+++ b/gold-client/go/goldclient/goldclient.go
@@ -308,7 +308,7 @@
}
// Add the result of this test.
- traceID := c.addResult(name, imgDigest, additionalKeys, optionalKeys)
+ traceParams, traceID := c.addResult(name, imgDigest, additionalKeys, optionalKeys)
// At this point the result should be correct for uploading.
if err := c.resultState.SharedConfig.Validate(); err != nil {
@@ -357,7 +357,18 @@
}
if !match {
- link := fmt.Sprintf("%s/detail?test=%s&digest=%s", c.resultState.GoldURL, name, imgDigest)
+ // TODO(lovisolo): Compute grouping based on what's returned by the
+ // /json/v1/groupings RPC.
+ corpus, ok := traceParams[types.CorpusField]
+ if !ok {
+ return skerr.Fmt("no corpus associated with test %q", name)
+ }
+ grouping := paramtools.Params{
+ types.CorpusField: corpus,
+ types.PrimaryKeyField: string(name),
+ }
+ link := fmt.Sprintf("%s/detail?grouping=%s&digest=%s", c.resultState.GoldURL, url.QueryEscape(urlEncode(grouping)), imgDigest)
+
if c.resultState.SharedConfig.ChangelistID != "" {
link += "&changelist_id=" + c.resultState.SharedConfig.ChangelistID
link += "&crs=" + c.resultState.SharedConfig.CodeReviewSystem
@@ -414,7 +425,7 @@
infof(ctx, "Expectation for test: %s (%s)\n", expectHash, expectLabel)
}
- _, traceID := c.makeResultKeyAndTraceId(name, keys)
+ _, _, traceID := c.makeResultKeyAndTraceParamsAndID(name, keys)
match, _, err := c.matchImageAgainstBaseline(ctx, name, traceID, imgBytes, imgHash, optionalKeys)
return match, skerr.Wrap(err)
}
@@ -513,13 +524,14 @@
return filepath.Join(c.workDir, stateFile)
}
-// addResult adds the given test to the overall results and returns the ID of the affected trace.
-func (c *CloudClient) addResult(name types.TestName, imgHash types.Digest, additionalKeys, optionalKeys map[string]string) tiling.TraceIDV2 {
- key, traceID := c.makeResultKeyAndTraceId(name, additionalKeys)
+// addResult adds the given test to the overall results and returns the params and ID of the
+// affected trace.
+func (c *CloudClient) addResult(name types.TestName, imgHash types.Digest, additionalKeys, optionalKeys map[string]string) (paramtools.Params, tiling.TraceIDV2) {
+ resultKey, traceParams, traceID := c.makeResultKeyAndTraceParamsAndID(name, additionalKeys)
newResult := jsonio.Result{
Digest: imgHash,
- Key: key,
+ Key: resultKey,
// We need to specify this is a png, otherwise the backend will refuse
// to ingest it.
@@ -531,11 +543,12 @@
c.resultState.SharedConfig.Results = append(c.resultState.SharedConfig.Results, newResult)
- return traceID
+ return traceParams, traceID
}
-// makeResultKeyAndTraceId computes the key for a jsonio.Result and its corresponding trace ID.
-func (c *CloudClient) makeResultKeyAndTraceId(name types.TestName, additionalKeys map[string]string) (map[string]string, tiling.TraceIDV2) {
+// makeResultKeyAndTraceParamsAndID computes the key for a jsonio.Result and the params and ID of
+// the affected trace.
+func (c *CloudClient) makeResultKeyAndTraceParamsAndID(name types.TestName, additionalKeys map[string]string) (map[string]string, paramtools.Params, tiling.TraceIDV2) {
// Retrieve the shared keys given the via the "imgtest init" command with the --keys-file flag.
// If said command was not previously invoked (e.g. when calling "imgtest check") the shared
// config will be nil, in which case we initialize the shared keys as an empty map.
@@ -561,7 +574,7 @@
traceParams := paramtools.Params{}
traceParams.Add(sharedKeys, resultKey)
- return resultKey, createTraceIDV2(traceParams)
+ return resultKey, traceParams, createTraceIDV2(traceParams)
}
// createTraceIDV2 encodes the params to JSON and then hex encodes it to create a traceIDV2.
diff --git a/gold-client/go/goldclient/goldclient_test.go b/gold-client/go/goldclient/goldclient_test.go
index 27feece..1a73294 100644
--- a/gold-client/go/goldclient/goldclient_test.go
+++ b/gold-client/go/goldclient/goldclient_test.go
@@ -260,7 +260,13 @@
},
}
- traceId := goldClient.addResult("my_test", "9d0568469d206c1aedf1b71f12f474bc", map[string]string{"gamma": "delta"}, map[string]string{"epsilon": "zeta"})
+ traceParams, traceID := goldClient.addResult("my_test", "9d0568469d206c1aedf1b71f12f474bc", map[string]string{"gamma": "delta"}, map[string]string{"epsilon": "zeta"})
+ assert.Equal(t, paramtools.Params{
+ types.CorpusField: "my_corpus",
+ "name": "my_test",
+ "alpha": "beta",
+ "gamma": "delta",
+ }, traceParams)
assert.Equal(t, []jsonio.Result{
{
Digest: "9d0568469d206c1aedf1b71f12f474bc",
@@ -274,7 +280,7 @@
},
},
}, goldClient.resultState.SharedConfig.Results)
- assert.Equal(t, tiling.TraceIDV2(expectedTraceID), traceId)
+ assert.Equal(t, tiling.TraceIDV2(expectedTraceID), traceID)
}
func TestAddResult_NoCorpusSpecified_UsesInstanceIdAsCorpus_Success(t *testing.T) {
@@ -295,7 +301,13 @@
},
}
- traceId := goldClient.addResult("my_test", "9d0568469d206c1aedf1b71f12f474bc", map[string]string{"gamma": "delta"}, map[string]string{"epsilon": "zeta"})
+ traceParams, traceID := goldClient.addResult("my_test", "9d0568469d206c1aedf1b71f12f474bc", map[string]string{"gamma": "delta"}, map[string]string{"epsilon": "zeta"})
+ assert.Equal(t, paramtools.Params{
+ types.CorpusField: "my_instance",
+ "name": "my_test",
+ "alpha": "beta",
+ "gamma": "delta",
+ }, traceParams)
assert.Equal(t, []jsonio.Result{
{
Digest: "9d0568469d206c1aedf1b71f12f474bc",
@@ -310,7 +322,7 @@
},
},
}, goldClient.resultState.SharedConfig.Results)
- assert.Equal(t, tiling.TraceIDV2(expectedTraceID), traceId)
+ assert.Equal(t, tiling.TraceIDV2(expectedTraceID), traceID)
}
// Report an image that does not match any previous digests.
@@ -716,7 +728,7 @@
b, err := ioutil.ReadFile(filepath.Join(wd, failureLog))
assert.NoError(t, err)
- assert.Equal(t, "https://testing-gold.skia.org/detail?test=TestNotSeenBefore&digest=9d0568469d206c1aedf1b71f12f474bc&changelist_id=867&crs=gerrit\n", string(b))
+ assert.Equal(t, "https://testing-gold.skia.org/detail?grouping=name%3DTestNotSeenBefore%26source_type%3Dtesting&digest=9d0568469d206c1aedf1b71f12f474bc&changelist_id=867&crs=gerrit\n", string(b))
}
// TestReportPassFailPassWithCorpus test that when we set the corpus via the initial config
@@ -1054,8 +1066,8 @@
b, err := ioutil.ReadFile(filepath.Join(wd, failureLog))
assert.NoError(t, err)
- assert.Equal(t, `https://testing-gold.skia.org/detail?test=ThisIsTheOnlyTest&digest=badbadbad1325855590527db196112e0&changelist_id=867&crs=gerrit
-https://testing-gold.skia.org/detail?test=ThisIsTheOnlyTest&digest=badbadbad1325855590527db196112e0&changelist_id=867&crs=gerrit
+ assert.Equal(t, `https://testing-gold.skia.org/detail?grouping=name%3DThisIsTheOnlyTest%26source_type%3Dtesting&digest=badbadbad1325855590527db196112e0&changelist_id=867&crs=gerrit
+https://testing-gold.skia.org/detail?grouping=name%3DThisIsTheOnlyTest%26source_type%3Dtesting&digest=badbadbad1325855590527db196112e0&changelist_id=867&crs=gerrit
`, string(b))
}
@@ -1511,7 +1523,7 @@
}),
}
- resultKey, traceID := goldClient.makeResultKeyAndTraceId(testName, tc.additionalKeys)
+ resultKey, _, traceID := goldClient.makeResultKeyAndTraceParamsAndID(testName, tc.additionalKeys)
assert.Equal(t, tc.expectedResultKey, resultKey)
_, tb := sql.SerializeMap(tc.expectedTrace)
expectedTraceID := tiling.TraceIDV2(hex.EncodeToString(tb))
