[goldctl] Add extra logging surrounding empty expectations
Bug: skia:13621, angleproject:7550
Change-Id: I9cce68560b7bd7504699c385dfcd654ff6edb2df
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/565501
Reviewed-by: Joe Gregorio <jcgregorio@google.com>
diff --git a/gold-client/cmd/goldctl/cmd_imgtest_test.go b/gold-client/cmd/goldctl/cmd_imgtest_test.go
index b6a2478..77810b3 100644
--- a/gold-client/cmd/goldctl/cmd_imgtest_test.go
+++ b/gold-client/cmd/goldctl/cmd_imgtest_test.go
@@ -63,7 +63,7 @@
Known("11111111111111111111111111111111").Build()
// Call imgtest init with the following flags. We expect it to load the baseline expectations
- // and the known hashes (both empty).
+ // (with two images triaged) and the known hashes (one entry).
ctx, output, exit := testContext(nil, mh, nil, nil)
env := imgTest{
gitHash: "1234567890123456789012345678901234567890",
@@ -101,7 +101,7 @@
Known("11111111111111111111111111111111").Build()
// Call imgtest init with the following flags. We expect it to load the baseline expectations
- // and the known hashes (both empty).
+ // (with two images triaged) and the known hashes (one entry).
ctx, output, exit := testContext(nil, mh, nil, nil)
env := imgTest{
commitID: "92.103234.1.123456",
@@ -195,6 +195,71 @@
assert.Contains(t, outStr, `invalid configuration: field "gitHash", "commit_id", or "change_list_id" must be set`)
}
+func TestImgTest_Init_EmptyExpectationsReturned_EmitsWarning(t *testing.T) {
+ unittest.MediumTest(t)
+
+ workDir := t.TempDir()
+ setupAuthWithGSUtil(t, workDir)
+
+ keysFile := filepath.Join(workDir, "keys.json")
+ require.NoError(t, ioutil.WriteFile(keysFile, []byte(`{"os": "Android"}`), 0644))
+
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").
+ Known("11111111111111111111111111111111").Build()
+
+ ctx, output, exit := testContext(nil, mh, nil, nil)
+ env := imgTest{
+ gitHash: "1234567890123456789012345678901234567890",
+ corpus: "my_corpus",
+ instanceID: "my-instance",
+ keysFile: keysFile,
+ passFailStep: true,
+ workDir: workDir,
+ }
+ runUntilExit(t, func() {
+ env.Init(ctx)
+ })
+ logs := output.String()
+ exit.AssertWasCalledWithCode(t, 0, logs)
+ assert.Contains(t, logs, "warning: got empty expectations when querying https://my-instance-gold.skia.org/json/v2/expectations\n")
+}
+
+func TestImgTest_InitCheck_EmptyExpectationsReturned_ReturnsNonzeroExitCode(t *testing.T) {
+ unittest.MediumTest(t)
+
+ workDir := t.TempDir()
+ setupAuthWithGSUtil(t, workDir)
+
+ keysFile := filepath.Join(workDir, "keys.json")
+ require.NoError(t, ioutil.WriteFile(keysFile, []byte(`{"os": "Android"}`), 0644))
+
+ mh := mockRPCResponses("https://my-instance-gold.skia.org").
+ Known("11111111111111111111111111111111").Build()
+
+ ctx, output, exit := testContext(nil, mh, nil, nil)
+ env := imgTest{
+ gitHash: "1234567890123456789012345678901234567890",
+ corpus: "my_corpus",
+ instanceID: "my-instance",
+ keysFile: keysFile,
+ passFailStep: true,
+ workDir: workDir,
+ }
+ runUntilExit(t, func() {
+ env.Init(ctx)
+ })
+ exit.AssertWasCalledWithCode(t, 0, output.String())
+
+ output.buf.Reset()
+ runUntilExit(t, func() {
+ env.Check(ctx)
+ })
+ logs := output.String()
+ exit.AssertWasCalledWithCode(t, 1, logs)
+ assert.Contains(t, logs, "Expectations are empty, despite re-loading them from Gold")
+ assert.Contains(t, logs, `raw expectation response "{}"`)
+}
+
func TestImgTest_InitAdd_StreamingPassFail_DoesNotMatchExpectations_NonzeroExitCode(t *testing.T) {
unittest.MediumTest(t)
diff --git a/gold-client/cmd/goldctl/cmd_whoami_test.go b/gold-client/cmd/goldctl/cmd_whoami_test.go
index 9a51c74..8afcaed 100644
--- a/gold-client/cmd/goldctl/cmd_whoami_test.go
+++ b/gold-client/cmd/goldctl/cmd_whoami_test.go
@@ -64,10 +64,15 @@
assert.Contains(t, output.String(), `Logged in as ""`)
}
-func httpResponse(body, status string, statusCode int) *http.Response {
- return &http.Response{
- Body: ioutil.NopCloser(strings.NewReader(body)),
- Status: status,
- StatusCode: statusCode,
+// This returns a function that returns a fresh response. Returning a static response works for
+// the first mocked call to this function, but subsequent ones read nothing (because the string
+// reader has already read all its bytes).
+func httpResponse(body, status string, statusCode int) func(string) *http.Response {
+ return func(_ string) *http.Response {
+ return &http.Response{
+ Body: ioutil.NopCloser(strings.NewReader(body)),
+ Status: status,
+ StatusCode: statusCode,
+ }
}
}
diff --git a/gold-client/go/goldclient/goldclient.go b/gold-client/go/goldclient/goldclient.go
index ce01758..d754a36 100644
--- a/gold-client/go/goldclient/goldclient.go
+++ b/gold-client/go/goldclient/goldclient.go
@@ -400,6 +400,9 @@
return false, skerr.Wrapf(err, "writing the expectations to disk")
}
}
+ if len(c.resultState.Expectations) == 0 {
+ return false, skerr.Fmt("Expectations are empty, despite re-loading them from Gold")
+ }
// Load the PNG from disk and hash it.
imgBytes, imgHash, err := c.loadAndHashImage(imgFileName)
@@ -574,14 +577,14 @@
func (c *CloudClient) downloadHashesAndBaselineFromGold(ctx context.Context) error {
// What hashes have we seen already (to avoid uploading them again).
if err := c.resultState.loadKnownHashes(ctx); err != nil {
- return err
+ return skerr.Wrap(err)
}
infof(ctx, "Loaded %d known hashes\n", len(c.resultState.KnownHashes))
- // Fetch the baseline (may be empty but should not fail).
+ // Fetch the baseline
if err := c.resultState.loadExpectations(ctx); err != nil {
- return err
+ return skerr.Wrap(err)
}
infof(ctx, "Loaded %d tests from the baseline\n", len(c.resultState.Expectations))
diff --git a/gold-client/go/goldclient/resultstate.go b/gold-client/go/goldclient/resultstate.go
index abc1cfd..d185501 100644
--- a/gold-client/go/goldclient/resultstate.go
+++ b/gold-client/go/goldclient/resultstate.go
@@ -137,10 +137,14 @@
if len(jsonBytes) > 200 {
infof(ctx, `Invalid JSON: "%s..."`, string(jsonBytes[0:200]))
} else {
- infof(ctx, `Invalid JSON: "%s"`, string(jsonBytes))
+ infof(ctx, `Invalid JSON: %q`, string(jsonBytes))
}
return skerr.Wrapf(err, "parsing JSON; this sometimes means auth issues")
}
+ if len(exp.Expectations) == 0 {
+ errorf(ctx, "warning: got empty expectations when querying %s\n", u)
+ errorf(ctx, "raw expectation response %q\n", string(jsonBytes))
+ }
r.Expectations = exp.Expectations
return nil
