[gold] Clean up TODOs in goldctl and jsonio
Chrome should have updated their calls to not be empty
Bug: skia:9040
Change-Id: I31d01e32796b7cd02099d5c0bb3c985aa72a5fcc
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/255784
Auto-Submit: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Leandro Lovisolo <lovisolo@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/gold-client/go/goldclient/goldclient.go b/gold-client/go/goldclient/goldclient.go
index 6900014..5dc3b03 100644
--- a/gold-client/go/goldclient/goldclient.go
+++ b/gold-client/go/goldclient/goldclient.go
@@ -95,7 +95,7 @@
// in debugging.
type GoldClientDebug interface {
// DumpBaseline returns a human-readable representation of the baseline as a string.
- // This is a set of test names that each have a set of image
+ // This is a set of test names that each have a set of image
// digests that each have exactly one types.Label.
DumpBaseline() (string, error)
// DumpKnownHashes returns a human-readable representation of the known image digests
@@ -230,7 +230,7 @@
now: defaultNow,
}
var err error
- ret.resultState, err = loadStateFromJson(ret.getResultStatePath())
+ ret.resultState, err = loadStateFromJSON(ret.getResultStatePath())
if err != nil {
return nil, skerr.Wrapf(err, "loading state from disk")
}
@@ -450,7 +450,7 @@
// Check if the GoldResults instance is complete once results are added.
if err := c.resultState.SharedConfig.Validate(true); err != nil {
- return skerr.Fmt("Gold results fields invalid: %s", err)
+ return skerr.Wrapf(err, "Gold results fields invalid")
}
c.ready = true
@@ -534,10 +534,6 @@
// newResultState creates a new instance of resultState
func newResultState(sharedConfig *jsonio.GoldResults, config *GoldClientConfig) *resultState {
-
- // TODO(stephana): Move deriving the URLs and the bucket to a central place in the backend
- // or get rid of the bucket entirely and expose an upload URL (requires authentication)
-
goldURL := config.OverrideGoldURL
if goldURL == "" {
goldURL = getHostURL(config.InstanceID)
@@ -556,9 +552,9 @@
return ret
}
-// loadStateFromJson loads a serialization of a resultState instance that was previously written
+// loadStateFromJSON loads a serialization of a resultState instance that was previously written
// via the save method.
-func loadStateFromJson(fileName string) (*resultState, error) {
+func loadStateFromJSON(fileName string) (*resultState, error) {
ret := &resultState{}
exists, err := loadJSONFile(fileName, ret)
if err != nil {
diff --git a/gold-client/go/goldclient/goldclient_test.go b/gold-client/go/goldclient/goldclient_test.go
index 3b41b74..37c5e47 100644
--- a/gold-client/go/goldclient/goldclient_test.go
+++ b/gold-client/go/goldclient/goldclient_test.go
@@ -168,7 +168,7 @@
outFile := filepath.Join(wd, stateFile)
assert.True(t, fileutil.FileExists(outFile))
- state, err := loadStateFromJson(outFile)
+ state, err := loadStateFromJSON(outFile)
assert.NoError(t, err)
assert.True(t, state.PerTestPassFail)
assert.False(t, state.UploadOnly)
@@ -180,7 +180,7 @@
assert.Len(t, state.Expectations["ThisIsTheOnlyTest"], 2)
assert.Equal(t, makeTestSharedConfig(), *state.SharedConfig)
- state, err = loadStateFromJson("/tmp/some-file-guaranteed-not-to-exist")
+ state, err = loadStateFromJSON("/tmp/some-file-guaranteed-not-to-exist")
assert.Error(t, err)
}
@@ -234,7 +234,7 @@
outFile := filepath.Join(wd, stateFile)
assert.True(t, fileutil.FileExists(outFile))
- state, err := loadStateFromJson(outFile)
+ state, err := loadStateFromJSON(outFile)
assert.NoError(t, err)
assert.False(t, state.PerTestPassFail)
assert.True(t, state.UploadOnly)
diff --git a/golden/go/jsonio/jsonio.go b/golden/go/jsonio/jsonio.go
index 1c42b01..c33ffb3 100644
--- a/golden/go/jsonio/jsonio.go
+++ b/golden/go/jsonio/jsonio.go
@@ -203,11 +203,10 @@
if len(g.Key) == 0 {
return skerr.Fmt("field %q must not be empty", jn["Key"])
}
- // TODO(kjlubick) chrome-gpu is currently uploading a key with an empty value
- // Re-enable this check once they are fixed.
- //if ok, err := validateParams(g.Key); !ok {
- // return skerr.Wrapf(err, "field %q must not have empty keys or values", jn["Key"])
- //}
+
+ if ok, err := validateParams(g.Key); !ok {
+ return skerr.Wrapf(err, "field %q must not have empty keys or values", jn["Key"])
+ }
if !((g.ContinuousIntegrationSystem == "" && g.CodeReviewSystem == "" && g.TryJobID == "" && g.ChangeListID == "" && g.PatchSetOrder == 0) ||
(g.ContinuousIntegrationSystem != "" && g.CodeReviewSystem != "" && g.TryJobID != "" && g.ChangeListID != "" && g.PatchSetOrder > 0)) {
diff --git a/golden/go/jsonio/jsonio_test.go b/golden/go/jsonio/jsonio_test.go
index d94782d..a9e138f 100644
--- a/golden/go/jsonio/jsonio_test.go
+++ b/golden/go/jsonio/jsonio_test.go
@@ -72,24 +72,22 @@
ignoreResults: false,
errFragment: `results" index 0: field "key" must not be empty`,
},
- // TODO(kjlubick) chrome-gpu is currently uploading a key with an empty value
- // Re-enable this check once they are fixed.
- //"emptyKeyValue": {
- // results: GoldResults{
- // GitHash: "aaa27ef254ad66609606c7af0730ee062b25edf9",
- // Key: map[string]string{"param1": ""},
- // Results: []*Result{
- // {
- // Key: map[string]string{
- // types.PRIMARY_KEY_FIELD: "foo",
- // },
- // Digest: "12345abc",
- // },
- // },
- // },
- // ignoreResults: false,
- // errFragment: `field "key" must not have empty keys`,
- //},
+ "emptyKeyValue": {
+ results: GoldResults{
+ GitHash: "aaa27ef254ad66609606c7af0730ee062b25edf9",
+ Key: map[string]string{"param1": ""},
+ Results: []*Result{
+ {
+ Key: map[string]string{
+ types.PRIMARY_KEY_FIELD: "foo",
+ },
+ Digest: "12345abc",
+ },
+ },
+ },
+ ignoreResults: false,
+ errFragment: `field "key" must not have empty keys`,
+ },
"noNameField": {
results: GoldResults{
GitHash: "aaa27ef254ad66609606c7af0730ee062b25edf9",
