[gold] Add tests for baseline
I'm preparing to re-work several things in an effort to get the BT tracestore in.
I would like to have tests to back up those changes.
The change of master branch to be -1 is to make it the same
(expstore.MasterBranch is -1)
Bug: skia:9096
Change-Id: Ia6580832176459b0895aec11dc2c8df3051a03db
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/219781
Commit-Queue: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Ben Wagner aka dogben <benjaminwagner@google.com>
diff --git a/gold-client/cmd/goldctl/cmd_imgtest.go b/gold-client/cmd/goldctl/cmd_imgtest.go
index 7f4a997..e62a040 100644
--- a/gold-client/cmd/goldctl/cmd_imgtest.go
+++ b/gold-client/cmd/goldctl/cmd_imgtest.go
@@ -166,7 +166,7 @@
ifErrLogExit(cmd, err)
validation := shared.Validation{}
- issueID := validation.Int64Value("issue", i.flagIssueID, 0)
+ issueID := validation.Int64Value("issue", i.flagIssueID, types.MasterBranch)
patchsetID := validation.Int64Value("patchset", i.flagPatchsetID, 0)
jobID := validation.Int64Value("jobid", i.flagJobID, 0)
ifErrLogExit(cmd, validation.Errors())
diff --git a/gold-client/go/goldclient/goldclient.go b/gold-client/go/goldclient/goldclient.go
index 9d37c0c..3b18405 100644
--- a/gold-client/go/goldclient/goldclient.go
+++ b/gold-client/go/goldclient/goldclient.go
@@ -610,7 +610,7 @@
// loadExpectations fetches the expectations from Gold to compare to tests.
func (r *resultState) loadExpectations(httpClient HTTPClient) error {
urlPath := strings.Replace(shared.EXPECTATIONS_ROUTE, "{commit_hash}", r.SharedConfig.GitHash, 1)
- if r.SharedConfig.Issue > 0 {
+ if !types.IsMasterBranch(r.SharedConfig.Issue) {
urlPath = fmt.Sprintf("%s?issue=%d", urlPath, r.SharedConfig.Issue)
}
url := fmt.Sprintf("%s/%s", r.GoldURL, strings.TrimLeft(urlPath, "/"))
@@ -665,7 +665,7 @@
fileName}
path := fmt.Sprintf("%s/%04d/%02d/%02d/%02d/%s/%d/%d/%s", segments...)
- if r.SharedConfig.Issue > 0 {
+ if !types.IsMasterBranch(r.SharedConfig.Issue) {
path = "trybot/" + path
}
return fmt.Sprintf("%s/%s", r.Bucket, path)
diff --git a/gold-client/go/goldclient/goldclient_test.go b/gold-client/go/goldclient/goldclient_test.go
index a815016..1e181fa 100644
--- a/gold-client/go/goldclient/goldclient_test.go
+++ b/gold-client/go/goldclient/goldclient_test.go
@@ -55,7 +55,8 @@
assert.NotContains(t, knownHashes, "notInThere")
}
-// Test data processing of the baseline input
+// TestLoadBaseline loads a baseline for an issue (testSharedConfig defaults to being
+// an configured for a tryjob).
func TestLoadBaseline(t *testing.T) {
unittest.SmallTest(t)
@@ -79,13 +80,59 @@
assert.NoError(t, err)
// Check that the baseline was loaded correctly
- baseline := goldClient.resultState.Expectations
- assert.Len(t, baseline, 1, "only one test")
- digests := baseline["ThisIsTheOnlyTest"]
+ bl := goldClient.resultState.Expectations
+ assert.Len(t, bl, 1, "only one test")
+ digests := bl["ThisIsTheOnlyTest"]
assert.Len(t, digests, 2, "two previously seen images")
assert.Equal(t, types.NEGATIVE, digests["badbadbad1325855590527db196112e0"])
assert.Equal(t, types.POSITIVE, digests["beef00d3a1527db19619ec12a4e0df68"])
+ assert.Equal(t, testIssueID, goldClient.resultState.SharedConfig.Issue)
+
+ knownHashes := goldClient.resultState.KnownHashes
+ assert.Empty(t, knownHashes, "No hashes loaded")
+}
+
+// TestLoadBaselineMaster loads the baseline for the master branch.
+func TestLoadBaselineMaster(t *testing.T) {
+ unittest.SmallTest(t)
+
+ wd, cleanup := testutils.TempDir(t)
+ defer cleanup()
+
+ auth, httpClient, uploader := makeMocks()
+ defer auth.AssertExpectations(t)
+ defer httpClient.AssertExpectations(t)
+ defer uploader.AssertExpectations(t)
+
+ hashesResp := httpResponse([]byte("none"), "200 OK", http.StatusOK)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/hashes").Return(hashesResp, nil)
+
+ expectations := httpResponse([]byte(mockBaselineJSON), "200 OK", http.StatusOK)
+ httpClient.On("Get", "https://testing-gold.skia.org/json/expectations/commit/abcd1234").Return(expectations, nil)
+
+ goldClient, err := makeGoldClient(auth, false /*=passFail*/, false /*=uploadOnly*/, wd)
+ assert.NoError(t, err)
+ err = goldClient.SetSharedConfig(jsonio.GoldResults{
+ GitHash: "abcd1234",
+ Key: map[string]string{
+ "os": "WinTest",
+ "gpu": "GPUTest",
+ },
+ Issue: types.MasterBranch,
+ })
+ assert.NoError(t, err)
+
+ // Check that the baseline was loaded correctly
+ bl := goldClient.resultState.Expectations
+ assert.Len(t, bl, 1, "only one test")
+ digests := bl["ThisIsTheOnlyTest"]
+ assert.Len(t, digests, 2, "two previously seen images")
+ assert.Equal(t, types.NEGATIVE, digests["badbadbad1325855590527db196112e0"])
+ assert.Equal(t, types.POSITIVE, digests["beef00d3a1527db19619ec12a4e0df68"])
+
+ assert.Equal(t, types.MasterBranch, goldClient.resultState.SharedConfig.Issue)
+
knownHashes := goldClient.resultState.KnownHashes
assert.Empty(t, knownHashes, "No hashes loaded")
}
diff --git a/golden/go/baseline/baseline.go b/golden/go/baseline/baseline.go
index f03aca8..ac4dde1 100644
--- a/golden/go/baseline/baseline.go
+++ b/golden/go/baseline/baseline.go
@@ -6,7 +6,6 @@
"fmt"
"go.skia.org/infra/go/skerr"
- "go.skia.org/infra/go/sklog"
"go.skia.org/infra/go/tiling"
"go.skia.org/infra/go/util"
"go.skia.org/infra/golden/go/tryjobstore"
@@ -25,8 +24,6 @@
}
}
-// TODO(kjlubick): Add tests for GetBaselinesPerCommit.
-
// GetBaselinesPerCommit calculates the baselines for each commit in the tile.
// Of note, it only fills out the Positive matches - everything not seen is either untriaged
// or negative.
@@ -76,7 +73,7 @@
}
// Iterate over all commits. If the tile is sparse we substitute the expectations with the
- // expectations of the closest ancestor that has expecations. We also add the commits that
+ // expectations of the closest ancestor that has expectations. We also add the commits that
// have landed already, but are not captured in the current tile.
combined := allCommits
if len(extraCommits) > 0 {
@@ -86,7 +83,7 @@
}
ret := make(map[string]*Baseline, len(combined))
- var currBL *Baseline = nil
+ var currBL *Baseline
for _, commit := range combined {
bl, ok := denseBaselines[commit.Hash]
if ok {
@@ -102,19 +99,15 @@
Filled: len(bl),
Expectations: bl,
MD5: md5Sum,
+ Issue: types.MasterBranch,
}
currBL = ret[commit.Hash]
- } else if currBL != nil {
+ } else {
// Make a copy of the baseline of the previous commit and update the commit information.
- cpBL := *currBL
+ cpBL := currBL.Copy()
cpBL.StartCommit = commit
cpBL.EndCommit = commit
- ret[commit.Hash] = &cpBL
- } else {
- // Reaching this point means the first in the dense tile does not align with the first
- // commit of the sparse portion of the tile. This a sanity test and should only happen
- // in the presence of a programming error or data corruption.
- sklog.Errorf("Unable to get baseline for commit %s. It has not commits for immediate ancestors in tile.", commit.Hash)
+ ret[commit.Hash] = cpBL
}
}
@@ -123,7 +116,7 @@
// GetBaselineForIssue returns the baseline for the given issue. This baseline
// contains all triaged digests that are not in the master tile.
-// Note: CommitDelta, Total and Filled are not relevant for an issue baseline since
+// Note: Total and Filled are not relevant for an issue baseline since
// the concept of traces doesn't really make sense for a single commit.
func GetBaselineForIssue(issueID int64, tryjobs []*tryjobstore.Tryjob, tryjobResults [][]*tryjobstore.TryjobResult, exp types.Expectations, commits []*tiling.Commit) (*Baseline, error) {
startCommit := commits[len(commits)-1]
diff --git a/golden/go/baseline/baseline_test.go b/golden/go/baseline/baseline_test.go
new file mode 100644
index 0000000..634557f
--- /dev/null
+++ b/golden/go/baseline/baseline_test.go
@@ -0,0 +1,203 @@
+package baseline_test
+
+import (
+ "testing"
+ "time"
+
+ assert "github.com/stretchr/testify/require"
+ "go.skia.org/infra/go/testutils/unittest"
+ "go.skia.org/infra/go/tiling"
+ "go.skia.org/infra/golden/go/baseline"
+ "go.skia.org/infra/golden/go/mocks"
+ data "go.skia.org/infra/golden/go/testutils/data_three_devices"
+ "go.skia.org/infra/golden/go/tryjobstore"
+ "go.skia.org/infra/golden/go/types"
+)
+
+func TestGetBaselinesPerCommitSunnyDay(t *testing.T) {
+ unittest.SmallTest(t)
+
+ mti := &mocks.TileInfo{}
+ defer mti.AssertExpectations(t)
+
+ mti.On("AllCommits").Return(makeSparseCommits())
+ mti.On("DataCommits").Return(data.MakeTestCommits())
+ mti.On("GetTile", types.ExcludeIgnoredTraces).Return(data.MakeTestTile())
+
+ exp := data.MakeTestExpectations()
+
+ bm, err := baseline.GetBaselinesPerCommit(exp, mti, makeExtraCommits())
+ assert.NoError(t, err)
+
+ extraCommits := makeExtraCommits()
+ sparseCommits := makeSparseCommits()
+
+ assert.Equal(t, map[string]*baseline.Baseline{
+ data.FirstCommitHash: {
+ StartCommit: sparseCommits[0],
+ EndCommit: sparseCommits[0],
+ Total: 6,
+ Filled: 1,
+ Issue: types.MasterBranch,
+ Expectations: betaOnly,
+ MD5: BetaMD5,
+ },
+ EmptyCommitHash: {
+ StartCommit: sparseCommits[1],
+ EndCommit: sparseCommits[1],
+ Total: 6,
+ Filled: 1,
+ Issue: types.MasterBranch,
+ Expectations: betaOnly,
+ MD5: BetaMD5,
+ },
+ data.SecondCommitHash: {
+ StartCommit: sparseCommits[2],
+ EndCommit: sparseCommits[2],
+ Total: 6,
+ Filled: 1,
+ Issue: types.MasterBranch,
+ Expectations: betaOnly,
+ MD5: BetaMD5,
+ },
+ data.ThirdCommitHash: {
+ StartCommit: sparseCommits[3],
+ EndCommit: sparseCommits[3],
+ Total: 6,
+ Filled: 2,
+ Issue: types.MasterBranch,
+ Expectations: both,
+ MD5: BothMD5,
+ },
+ ExtraCommitHash: {
+ StartCommit: extraCommits[0],
+ EndCommit: extraCommits[0],
+ Total: 6,
+ Filled: 2,
+ Issue: types.MasterBranch,
+ Expectations: both,
+ MD5: BothMD5,
+ },
+ }, bm)
+}
+
+func TestGetBaselineForIssueSunnyDay(t *testing.T) {
+ unittest.SmallTest(t)
+
+ issue := int64(117)
+
+ // There were 2 tryjobs, one run when the CL had a parent of
+ // SecondCommitHash and the other when it was rebased onto
+ // ThirdCommitHash.
+ tryjobs := []*tryjobstore.Tryjob{
+ {
+ MasterCommit: data.SecondCommitHash,
+ },
+ {
+ MasterCommit: data.ThirdCommitHash,
+ },
+ }
+
+ tryjobResults := make([][]*tryjobstore.TryjobResult, len(tryjobs))
+ tryjobResults[0] = []*tryjobstore.TryjobResult{
+ {
+ TestName: data.AlphaTest,
+ Digest: data.AlphaBad1Digest,
+ },
+ {
+ TestName: data.BetaTest,
+ Digest: data.BetaGood1Digest,
+ },
+ }
+ tryjobResults[1] = []*tryjobstore.TryjobResult{
+ {
+ TestName: data.AlphaTest,
+ Digest: data.AlphaGood1Digest,
+ },
+ {
+ TestName: data.BetaTest,
+ Digest: data.BetaUntriaged1Digest,
+ },
+ }
+
+ b, err := baseline.GetBaselineForIssue(issue, tryjobs, tryjobResults,
+ data.MakeTestExpectations(), makeSparseCommits())
+ assert.NoError(t, err)
+
+ sparseCommits := makeSparseCommits()
+ assert.Equal(t, &baseline.Baseline{
+ StartCommit: sparseCommits[2],
+ EndCommit: sparseCommits[3],
+ Total: 0,
+ Filled: 0,
+ Issue: issue,
+ Expectations: both,
+ MD5: BothMD5,
+ }, b)
+}
+
+func makeSparseCommits() []*tiling.Commit {
+ // Four commits, with completely arbitrary data.
+ return []*tiling.Commit{
+ {
+ Hash: data.FirstCommitHash,
+ CommitTime: time.Date(2019, time.April, 26, 12, 0, 3, 0, time.UTC).Unix(),
+ Author: data.FirstCommitAuthor,
+ },
+ {
+ Hash: EmptyCommitHash,
+ CommitTime: time.Date(2019, time.April, 26, 12, 5, 5, 0, time.UTC).Unix(),
+ Author: data.FirstCommitAuthor,
+ },
+ {
+ Hash: data.SecondCommitHash,
+ CommitTime: time.Date(2019, time.April, 26, 12, 10, 18, 0, time.UTC).Unix(),
+ Author: data.SecondCommitAuthor,
+ },
+ {
+ Hash: data.ThirdCommitHash,
+ CommitTime: time.Date(2019, time.April, 26, 13, 10, 8, 0, time.UTC).Unix(),
+ Author: data.ThirdCommitAuthor,
+ },
+ }
+}
+
+func makeExtraCommits() []*tiling.Commit {
+ // One extra commit, completely arbitrary data.
+ return []*tiling.Commit{
+ {
+ Hash: ExtraCommitHash,
+ CommitTime: time.Date(2019, time.April, 27, 5, 17, 1, 0, time.UTC).Unix(),
+ Author: data.SecondCommitAuthor,
+ },
+ }
+}
+
+const (
+ EmptyCommitHash = "eea258b693f2fc53501ac341f3029860b3b57a10"
+ ExtraCommitHash = "8181bc0cb0ff1c83e5839aac0e8b13dc0157047a"
+
+ BetaMD5 = "fefa5ea9f0e646258516ed6cecff06c6"
+ BothMD5 = "821b673f7f240953aecf3c8fa32d5655"
+)
+
+var (
+ // For all but the last commit, BetaGood1Digest is the only
+ // positive digest seen.
+ betaOnly = types.Expectations{
+ data.BetaTest: {
+ data.BetaGood1Digest: types.POSITIVE,
+ },
+ }
+
+ // For the last commit, we see AlphaGood1Digest, so it is added
+ // to the Expectations.
+ both = types.Expectations{
+ data.AlphaTest: {
+ data.AlphaGood1Digest: types.POSITIVE,
+ },
+ data.BetaTest: {
+ data.BetaGood1Digest: types.POSITIVE,
+ },
+ }
+)
diff --git a/golden/go/baseline/gcs_baseliner/baseliner.go b/golden/go/baseline/gcs_baseliner/baseliner.go
index f181c9b..8ade84e 100644
--- a/golden/go/baseline/gcs_baseliner/baseliner.go
+++ b/golden/go/baseline/gcs_baseliner/baseliner.go
@@ -19,6 +19,7 @@
"go.skia.org/infra/golden/go/shared"
"go.skia.org/infra/golden/go/storage"
"go.skia.org/infra/golden/go/tryjobstore"
+ "go.skia.org/infra/golden/go/types"
"golang.org/x/sync/errgroup"
)
@@ -212,7 +213,7 @@
// FetchBaseline implements the baseline.Baseliner interface.
func (b *BaselinerImpl) FetchBaseline(commitHash string, issueID int64, issueOnly bool) (*baseline.Baseline, error) {
- isIssue := issueID > baseline.MasterBranch
+ isIssue := !types.IsMasterBranch(issueID)
var masterBaseline *baseline.Baseline
var issueBaseline *baseline.Baseline
@@ -350,7 +351,7 @@
}
// We did not find it in the cache so lets load it from GCS.
- ret, err := b.gStorageClient.ReadBaseline(commitHash, baseline.MasterBranch)
+ ret, err := b.gStorageClient.ReadBaseline(commitHash, types.MasterBranch)
if err != nil {
return nil, err
}
diff --git a/golden/go/baseline/gcs_baseliner/baseliner_test.go b/golden/go/baseline/gcs_baseliner/baseliner_test.go
index d886fa0..703740a 100644
--- a/golden/go/baseline/gcs_baseliner/baseliner_test.go
+++ b/golden/go/baseline/gcs_baseliner/baseliner_test.go
@@ -24,12 +24,12 @@
mgs := makeMockGCSStorage()
defer mgs.AssertExpectations(t)
- mgs.On("ReadBaseline", testCommitHash, baseline.MasterBranch).Return(three_devices.MakeTestBaseline(), nil).Once()
+ mgs.On("ReadBaseline", testCommitHash, types.MasterBranch).Return(three_devices.MakeTestBaseline(), nil).Once()
baseliner, err := New(mgs, nil, nil, nil)
assert.NoError(t, err)
- b, err := baseliner.FetchBaseline(testCommitHash, baseline.MasterBranch, false)
+ b, err := baseliner.FetchBaseline(testCommitHash, types.MasterBranch, false)
assert.NoError(t, err)
deepequal.AssertDeepEqual(t, three_devices.MakeTestBaseline(), b)
@@ -73,7 +73,7 @@
defer mgs.AssertExpectations(t)
// mock the master baseline
- mgs.On("ReadBaseline", testCommitHash, int64(0)).Return(three_devices.MakeTestBaseline(), nil).Once()
+ mgs.On("ReadBaseline", testCommitHash, types.MasterBranch).Return(three_devices.MakeTestBaseline(), nil).Once()
// mock the expectations that a user would have applied to their CL (that
// are not live on master yet).
mgs.On("ReadBaseline", "", testIssueID).Return(additionalTriages, nil).Once()
@@ -108,7 +108,7 @@
}, b.Expectations)
// Ensure that reading the issue branch does not impact the master branch
- b, err = baseliner.FetchBaseline(testCommitHash, baseline.MasterBranch, false)
+ b, err = baseliner.FetchBaseline(testCommitHash, types.MasterBranch, false)
assert.NoError(t, err)
assert.Equal(t, three_devices.MakeTestBaseline(), b)
}
@@ -122,13 +122,13 @@
defer mgs.AssertExpectations(t)
// ReadBaseline should only be called once despite multiple requests below
- mgs.On("ReadBaseline", testCommitHash, baseline.MasterBranch).Return(three_devices.MakeTestBaseline(), nil).Once()
+ mgs.On("ReadBaseline", testCommitHash, types.MasterBranch).Return(three_devices.MakeTestBaseline(), nil).Once()
baseliner, err := New(mgs, nil, nil, nil)
assert.NoError(t, err)
for i := 0; i < 10; i++ {
- b, err := baseliner.FetchBaseline(testCommitHash, baseline.MasterBranch, false)
+ b, err := baseliner.FetchBaseline(testCommitHash, types.MasterBranch, false)
assert.NoError(t, err)
assert.NotNil(t, b)
deepequal.AssertDeepEqual(t, three_devices.MakeTestBaseline(), b)
diff --git a/golden/go/baseline/types.go b/golden/go/baseline/types.go
index 740909b..1c000f1 100644
--- a/golden/go/baseline/types.go
+++ b/golden/go/baseline/types.go
@@ -18,10 +18,6 @@
// EndCommit is the commit for which this baseline was collected.
EndCommit *tiling.Commit `json:"endCommit"`
- // CommitDelta is the difference in index within the commits of a tile.
- // TODO(kjlubick) Appears to be unread and unwritten to.
- CommitDelta int `json:"commitDelta"`
-
// Total is the total number of traces that were iterated when generating the baseline.
Total int `json:"total"`
@@ -35,7 +31,7 @@
// with only the positive digests of the current commit.
Expectations types.Expectations `json:"master"`
- // Issue indicates the Gerrit issue of this baseline. 0 indicates the master branch.
+ // Issue indicates the Gerrit issue of this baseline. -1 indicates the master branch.
Issue int64
}
@@ -60,8 +56,6 @@
}
}
-const MasterBranch = int64(0)
-
// Baseliner is an interface wrapping the functionality to save and fetch baselines.
type Baseliner interface {
// CanWriteBaseline returns true if this instance was configured to write baseline files.
diff --git a/golden/go/expstorage/ds_expstore/ds_expstore.go b/golden/go/expstorage/ds_expstore/ds_expstore.go
index fe250d9..6f89fbe 100644
--- a/golden/go/expstorage/ds_expstore/ds_expstore.go
+++ b/golden/go/expstorage/ds_expstore/ds_expstore.go
@@ -120,7 +120,7 @@
blobStore := dsutil.NewBlobStore(client, ds.EXPECTATIONS_BLOB_ROOT, ds.EXPECTATIONS_BLOB)
store := &DSExpStore{
- issueID: expstorage.MasterBranch,
+ issueID: types.MasterBranch,
changeKind: ds.MASTER_EXP_CHANGE,
eventExpChange: expstorage.EV_EXPSTORAGE_CHANGED,
globalEvent: true,
@@ -581,7 +581,7 @@
Filter("OK =", true).
KeysOnly()
- if c.issueID > 0 {
+ if !types.IsMasterBranch(c.issueID) {
q = q.Filter("IssueID =", c.issueID)
}
diff --git a/golden/go/expstorage/fs_expstore/fs_expstore.go b/golden/go/expstorage/fs_expstore/fs_expstore.go
index ceb0773..4f967dd 100644
--- a/golden/go/expstorage/fs_expstore/fs_expstore.go
+++ b/golden/go/expstorage/fs_expstore/fs_expstore.go
@@ -131,7 +131,7 @@
eventBus: eventBus,
eventExpChange: expstorage.EV_EXPSTORAGE_CHANGED,
globalEvent: true,
- issue: expstorage.MasterBranch,
+ issue: types.MasterBranch,
mode: mode,
}
// pre-load the cache. This simplifies the mutex handling in Get().
@@ -144,7 +144,7 @@
// ForIssue implements the ExpectationsStore interface.
func (f *Store) ForIssue(id int64) expstorage.ExpectationsStore {
- if id == expstorage.MasterBranch {
+ if types.IsMasterBranch(id) {
// It is invalid to re-request the master branch
return nil
}
@@ -160,7 +160,7 @@
// Get implements the ExpectationsStore interface.
func (f *Store) Get() (types.Expectations, error) {
- if f.issue == expstorage.MasterBranch {
+ if f.issue == types.MasterBranch {
defer metrics2.NewTimer("gold_get_expectations", map[string]string{"master_branch": "true"}).Stop()
f.cacheMutex.RLock()
defer f.cacheMutex.RUnlock()
@@ -174,7 +174,7 @@
// based on the current state. It is expected the caller has taken care of any mutex grabbing.
func (f *Store) getMasterExpectations() (types.Expectations, error) {
if f.cache == nil {
- c, err := f.loadExpectationsSharded(expstorage.MasterBranch, masterShards)
+ c, err := f.loadExpectationsSharded(types.MasterBranch, masterShards)
if err != nil {
return nil, skerr.Fmt("could not load master expectations from firestore: %s", err)
}
diff --git a/golden/go/expstorage/fs_expstore/fs_expstore_test.go b/golden/go/expstorage/fs_expstore/fs_expstore_test.go
index e31e1ac..6d22a0a 100644
--- a/golden/go/expstorage/fs_expstore/fs_expstore_test.go
+++ b/golden/go/expstorage/fs_expstore/fs_expstore_test.go
@@ -466,11 +466,11 @@
meb.On("Publish", expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
TestChanges: change1,
- IssueID: expstorage.MasterBranch,
+ IssueID: types.MasterBranch,
}, /*global=*/ true).Once()
meb.On("Publish", expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
TestChanges: change2,
- IssueID: expstorage.MasterBranch,
+ IssueID: types.MasterBranch,
}, /*global=*/ true).Once()
ctx := context.Background()
@@ -547,11 +547,11 @@
meb.On("Publish", expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
TestChanges: change,
- IssueID: expstorage.MasterBranch,
+ IssueID: types.MasterBranch,
}, /*global=*/ true).Once()
meb.On("Publish", expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
TestChanges: expectedUndo,
- IssueID: expstorage.MasterBranch,
+ IssueID: types.MasterBranch,
}, /*global=*/ true).Once()
ctx := context.Background()
diff --git a/golden/go/expstorage/mem_expstore/mem_expstore.go b/golden/go/expstorage/mem_expstore/mem_expstore.go
index 1143963..06e727a 100644
--- a/golden/go/expstorage/mem_expstore/mem_expstore.go
+++ b/golden/go/expstorage/mem_expstore/mem_expstore.go
@@ -53,7 +53,7 @@
if m.eventBus != nil {
m.eventBus.Publish(expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
TestChanges: changedTests,
- IssueID: expstorage.MasterBranch,
+ IssueID: types.MasterBranch,
}, true)
}
@@ -81,7 +81,7 @@
if m.eventBus != nil {
m.eventBus.Publish(expstorage.EV_EXPSTORAGE_CHANGED, &expstorage.EventExpectationChange{
TestChanges: changedDigests,
- IssueID: expstorage.MasterBranch,
+ IssueID: types.MasterBranch,
}, true)
}
diff --git a/golden/go/expstorage/types.go b/golden/go/expstorage/types.go
index eb59aaf..b1f09bb 100644
--- a/golden/go/expstorage/types.go
+++ b/golden/go/expstorage/types.go
@@ -17,10 +17,6 @@
// EV_TRYJOB_EXP_CHANGED is the event type that is fired when the expectations
// for an issue change. It sends an instance of *TryjobExpChange.
EV_TRYJOB_EXP_CHANGED = "expstorage:tryjob-exp-change"
-
- // MasterBranch is the value used for IssueID when we dealing with the
- // master branch. Any other IssueID < 0 should be ignored.
- MasterBranch = int64(-1)
)
func init() {
diff --git a/golden/go/jsonio/jsonio.go b/golden/go/jsonio/jsonio.go
index f9d8003..6c06053 100644
--- a/golden/go/jsonio/jsonio.go
+++ b/golden/go/jsonio/jsonio.go
@@ -131,7 +131,10 @@
func (r *rawGoldResults) parseValidate() (*GoldResults, []string) {
jn := rawGoldResultsJsonMap
var errs []string
- issueValid := (r.Issue == "") || (r.Issue != "" && r.BuildBucketID != "" && r.Patchset != "")
+ mb := strconv.FormatInt(types.MasterBranch, 10)
+ lmb := strconv.FormatInt(types.LegacyMasterBranch, 10)
+ issueValid := (r.Issue == "") || (r.Issue == mb) || (r.Issue == lmb) ||
+ (r.Issue != "" && r.BuildBucketID != "" && r.Patchset != "")
addErrMessage(&errs, issueValid, "fields '%s', '%s' must not be empty if field '%s' contains a value", jn["Patchset"], jn["BuildBucketID"], jn["Issue"])
f := []string{"Issue", r.Issue, "Patchset", r.Patchset, "BuildBucketID", r.BuildBucketID}
@@ -149,10 +152,15 @@
Builder: r.Builder,
TaskID: r.TaskID,
}
- // If there was no error we can just parse the strings to int64.
- ret.Issue, _ = strconv.ParseInt(r.Issue, 10, 64)
- ret.BuildBucketID, _ = strconv.ParseInt(r.BuildBucketID, 10, 64)
- ret.Patchset, _ = strconv.ParseInt(r.Patchset, 10, 64)
+ if r.Issue == "" {
+ ret.Issue = types.MasterBranch
+ } else {
+ // If there was no error we can just parse the strings to int64.
+ ret.Issue, _ = strconv.ParseInt(r.Issue, 10, 64)
+ ret.BuildBucketID, _ = strconv.ParseInt(r.BuildBucketID, 10, 64)
+ ret.Patchset, _ = strconv.ParseInt(r.Patchset, 10, 64)
+ }
+
}
return ret, errs
}
@@ -174,8 +182,9 @@
addErrMessage(&errMsg, regExHexadecimal.MatchString(g.GitHash), "field '%s' must be hexadecimal. Received '%s'", jn["GitHash"], g.GitHash)
addErrMessage(&errMsg, len(g.Key) > 0 && hasNonEmptyKV(g.Key), "field '%s' must not be empty and must not have empty keys or values", jn["Key"])
- validIssue := g.Issue == 0 || (g.Issue > 0 && g.Patchset > 0 && g.BuildBucketID > 0)
- addErrMessage(&errMsg, validIssue, "fields '%s', '%s', '%s' must all be zero or all not be zero", jn["Issue"], jn["Patchset"], jn["BuildBucketID"])
+ validIssue := types.IsMasterBranch(g.Issue) ||
+ (!types.IsMasterBranch(g.Issue) && g.Patchset > 0 && g.BuildBucketID > 0)
+ addErrMessage(&errMsg, validIssue, "fields '%s', '%s', '%s' must all be set or all not be set", jn["Issue"], jn["Patchset"], jn["BuildBucketID"])
if !ignoreResults {
addErrMessage(&errMsg, len(g.Results) > 0, "field '%s' must not be empty.", jn["Results"])
diff --git a/golden/go/jsonio/jsonio_test.go b/golden/go/jsonio/jsonio_test.go
index b9eef88..aed685f 100644
--- a/golden/go/jsonio/jsonio_test.go
+++ b/golden/go/jsonio/jsonio_test.go
@@ -9,11 +9,126 @@
assert "github.com/stretchr/testify/require"
"go.skia.org/infra/go/testutils/unittest"
+ "go.skia.org/infra/golden/go/types"
)
-var (
- testJSON = `
- {
+func TestValidate(t *testing.T) {
+ unittest.SmallTest(t)
+
+ empty := &GoldResults{}
+ errMsgs, err := empty.Validate(false)
+ assert.Error(t, err)
+ assertErrorFields(t, errMsgs,
+ "gitHash",
+ "key",
+ "results")
+ assert.NotNil(t, errMsgs)
+
+ wrongResults := &GoldResults{
+ GitHash: "aaa27ef254ad66609606c7af0730ee062b25edf9",
+ Key: map[string]string{"param1": "value1"},
+ }
+ errMsgs, err = wrongResults.Validate(false)
+ assert.Error(t, err)
+ assertErrorFields(t, errMsgs, "results")
+
+ wrongResults.Results = []*Result{}
+ errMsgs, err = wrongResults.Validate(false)
+ assert.Error(t, err)
+ assertErrorFields(t, errMsgs, "results")
+
+ wrongResults.Results = []*Result{
+ {Key: map[string]string{}},
+ }
+ errMsgs, err = wrongResults.Validate(false)
+ assert.Error(t, err)
+ assertErrorFields(t, errMsgs, "results")
+
+ // Now ignore the results in the validation.
+ errMsgs, err = wrongResults.Validate(true)
+ assert.NoError(t, err)
+ assert.Equal(t, []string(nil), errMsgs)
+
+ // Check that the Validate accounts for both MasterBranch and
+ // LegacyMasterBranch values.
+ legacyMaster := &GoldResults{
+ GitHash: "aaa27ef254ad66609606c7af0730ee062b25edf9",
+ Key: map[string]string{"param1": "value1"},
+
+ Issue: types.LegacyMasterBranch,
+ }
+ _, err = legacyMaster.Validate(true)
+ assert.NoError(t, err)
+
+ master := &GoldResults{
+ GitHash: "aaa27ef254ad66609606c7af0730ee062b25edf9",
+ Key: map[string]string{"param1": "value1"},
+
+ Issue: types.MasterBranch,
+ }
+ _, err = master.Validate(true)
+ assert.NoError(t, err)
+}
+
+func TestParseGoldResults(t *testing.T) {
+ unittest.SmallTest(t)
+ r := testParse(t, testJSON)
+
+ // Make sure some key fields come out correctly, i.e. are converted correctly from string to int.
+ assert.Equal(t, "c4711517219f333c1116f47706eb57b51b5f8fc7", r.GitHash)
+ assert.Equal(t, "Xb0VhENPSRFGnf2elVQd", r.TaskID)
+ assert.Equal(t, int64(12345), r.Issue)
+ assert.Equal(t, int64(10), r.Patchset)
+ assert.Equal(t, int64(549340494940393), r.BuildBucketID)
+
+ r = testParse(t, legacyMasterBranchJSON)
+ assert.Equal(t, types.LegacyMasterBranch, r.Issue)
+
+ r = testParse(t, masterBranchJSON)
+ assert.Equal(t, types.MasterBranch, r.Issue)
+
+ r = testParse(t, emptyMasterBranchJSON)
+ assert.Equal(t, types.MasterBranch, r.Issue)
+}
+
+func TestGenJson(t *testing.T) {
+ unittest.SmallTest(t)
+
+ // Test parsing the test JSON.
+ goldResults := testParse(t, testJSON)
+
+ // For good measure we validate.
+ _, err := goldResults.Validate(false)
+ assert.NoError(t, err)
+
+ // Encode and decode the results.
+ var buf bytes.Buffer
+ assert.NoError(t, json.NewEncoder(&buf).Encode(goldResults))
+ newGoldResults := testParse(t, buf.String())
+ assert.Equal(t, goldResults, newGoldResults)
+}
+
+func testParse(t *testing.T, jsonStr string) *GoldResults {
+ buf := bytes.NewBuffer([]byte(jsonStr))
+
+ ret, errMsg, err := ParseGoldResults(buf)
+ assert.NoError(t, err)
+ assert.Nil(t, errMsg)
+ return ret
+}
+
+func assertErrorFields(t *testing.T, errMsgs []string, expectedFields ...string) {
+ for _, msg := range errMsgs {
+ found := false
+ for _, ef := range expectedFields {
+ found = found || strings.Contains(msg, ef)
+ }
+ assert.True(t, found, fmt.Sprintf("Could not find %v in msg: %s", expectedFields, msg))
+ }
+}
+
+const (
+ testJSON = `{
"gitHash" : "c4711517219f333c1116f47706eb57b51b5f8fc7",
"key" : {
"arch" : "arm64",
@@ -69,91 +184,29 @@
}
}
]
- }`
+}`
+
+ legacyMasterBranchJSON = `{
+ "gitHash" : "c4711517219f333c1116f47706eb57b51b5f8fc7",
+ "key" : {
+ "arch" : "arm64"
+ },
+ "issue": "0"
+ }`
+
+ masterBranchJSON = `{
+ "gitHash" : "c4711517219f333c1116f47706eb57b51b5f8fc7",
+ "key" : {
+ "arch" : "arm64"
+ },
+ "issue": "-1"
+ }`
+
+ emptyMasterBranchJSON = `{
+ "gitHash" : "c4711517219f333c1116f47706eb57b51b5f8fc7",
+ "key" : {
+ "arch" : "arm64"
+ },
+ "issue": ""
+ }`
)
-
-func TestValidate(t *testing.T) {
- unittest.SmallTest(t)
-
- empty := &GoldResults{}
- errMsgs, err := empty.Validate(false)
- assert.Error(t, err)
- assertErrorFields(t, errMsgs,
- "gitHash",
- "key",
- "results")
- assert.NotNil(t, errMsgs)
-
- wrongResults := &GoldResults{
- GitHash: "a1b2c3d4e5f6a7b8c9d0e1f2",
- Key: map[string]string{"param1": "value1"},
- }
- errMsgs, err = wrongResults.Validate(false)
- assert.Error(t, err)
- assertErrorFields(t, errMsgs, "results")
-
- wrongResults.Results = []*Result{}
- errMsgs, err = wrongResults.Validate(false)
- assert.Error(t, err)
- assertErrorFields(t, errMsgs, "results")
-
- wrongResults.Results = []*Result{
- {Key: map[string]string{}},
- }
- errMsgs, err = wrongResults.Validate(false)
- assert.Error(t, err)
- assertErrorFields(t, errMsgs, "results")
-
- // Now ignore the results in the validation.
- errMsgs, err = wrongResults.Validate(true)
- assert.NoError(t, err)
- assert.Equal(t, []string(nil), errMsgs)
-}
-
-func TestParseGoldResults(t *testing.T) {
- unittest.SmallTest(t)
- r := testParse(t, testJSON)
-
- // Make sure some key fields come out correctly, i.e. are converted correctly from string to int.
- assert.Equal(t, "c4711517219f333c1116f47706eb57b51b5f8fc7", r.GitHash)
- assert.Equal(t, "Xb0VhENPSRFGnf2elVQd", r.TaskID)
- assert.Equal(t, int64(12345), r.Issue)
- assert.Equal(t, int64(10), r.Patchset)
- assert.Equal(t, int64(549340494940393), r.BuildBucketID)
-}
-
-func TestGenJson(t *testing.T) {
- unittest.SmallTest(t)
-
- // Test parsing the test JSON.
- goldResults := testParse(t, testJSON)
-
- // For good measure we validate.
- _, err := goldResults.Validate(false)
- assert.NoError(t, err)
-
- // Encode and decode the results.
- var buf bytes.Buffer
- assert.NoError(t, json.NewEncoder(&buf).Encode(goldResults))
- newGoldResults := testParse(t, buf.String())
- assert.Equal(t, goldResults, newGoldResults)
-}
-
-func testParse(t *testing.T, jsonStr string) *GoldResults {
- buf := bytes.NewBuffer([]byte(jsonStr))
-
- ret, errMsg, err := ParseGoldResults(buf)
- assert.NoError(t, err)
- assert.Nil(t, errMsg)
- return ret
-}
-
-func assertErrorFields(t *testing.T, errMsgs []string, expectedFields ...string) {
- for _, msg := range errMsgs {
- found := false
- for _, ef := range expectedFields {
- found = found || strings.Contains(msg, ef)
- }
- assert.True(t, found, fmt.Sprintf("Could not find %v in msg: %s", expectedFields, msg))
- }
-}
diff --git a/golden/go/search/new_search.go b/golden/go/search/new_search.go
index 4cf8891..edb246c 100644
--- a/golden/go/search/new_search.go
+++ b/golden/go/search/new_search.go
@@ -104,7 +104,7 @@
// for the majority of queries.
getRefDiffs := !q.NoDiff
- isTryjobSearch := q.Issue > 0
+ isTryjobSearch := !types.IsMasterBranch(q.Issue)
// Get the expectations and the current index, which we assume constant
// for the duration of this query.
@@ -274,7 +274,7 @@
func (s *SearchAPI) getExpectationsFromQuery(q *Query) (ExpSlice, error) {
ret := make(ExpSlice, 0, 2)
- if (q != nil) && (q.Issue > 0) {
+ if q != nil && !types.IsMasterBranch(q.Issue) {
issueExpStore := s.storages.ExpectationsStore.ForIssue(q.Issue)
tjExp, err := issueExpStore.Get()
if err != nil {
diff --git a/golden/go/search/utils_test.go b/golden/go/search/utils_test.go
index e03c24c..8cf6179 100644
--- a/golden/go/search/utils_test.go
+++ b/golden/go/search/utils_test.go
@@ -40,7 +40,7 @@
// issues. This requires to refresh the set of input queries.
// Ignore queries for gerrit issues right now.
- if q.Issue > 0 {
+ if !types.IsMasterBranch(q.Issue) {
return 0
}
diff --git a/golden/go/storage/gcsclient.go b/golden/go/storage/gcsclient.go
index 4fb6b86..aa3658d 100644
--- a/golden/go/storage/gcsclient.go
+++ b/golden/go/storage/gcsclient.go
@@ -100,7 +100,7 @@
}
// We need a valid end commit or issue.
- if b.EndCommit == nil && b.Issue <= 0 {
+ if b.EndCommit == nil && types.IsMasterBranch(b.Issue) {
return "", skerr.Fmt("Received empty end commit and no issue. Cannot write baseline")
}
@@ -143,11 +143,11 @@
}
// getBaselinePath returns the baseline path in GCS for the given issueID.
-// If issueID <= 0 it returns the path for the master baseline.
+// This can also return the master baseline.
func (g *ClientImpl) getBaselinePath(commitHash string, issueID int64) string {
// Change the output file based on whether it's the master branch or a Gerrit issue.
var outPath string
- if issueID > baseline.MasterBranch {
+ if !types.IsMasterBranch(issueID) {
outPath = fmt.Sprintf("issue_%d.json", issueID)
} else if commitHash != "" {
outPath = fmt.Sprintf("master_%s.json", commitHash)
diff --git a/golden/go/tryjobstore/tryjobstore.go b/golden/go/tryjobstore/tryjobstore.go
index c84060d..b76687f 100644
--- a/golden/go/tryjobstore/tryjobstore.go
+++ b/golden/go/tryjobstore/tryjobstore.go
@@ -262,7 +262,7 @@
if tryjob == nil {
// make sure we have the necessary information if there are no data to be written directly.
if (buildBucketID == 0) || (newValFn == nil) {
- return sklog.FmtErrorf("Id and newValFn cannot be nil when no tryjob is provided. Update not possible")
+ return skerr.Fmt("ID and newValFn cannot be nil when no tryjob is provided. Update not possible")
}
tryjob = &Tryjob{}
} else {
@@ -283,7 +283,7 @@
func (c *cloudTryjobStore) GetTryjobs(issueID int64, patchsetIDs []int64, filterDup bool, loadResults bool) ([]*Tryjob, [][]*TryjobResult, error) {
flatTryjobKeys, tryjobsMap, err := c.getTryjobsForIssue(issueID, patchsetIDs, false, filterDup)
if err != nil {
- return nil, nil, err
+ return nil, nil, skerr.Fmt("could not locate tryjobs for issue %d: %s", issueID, err)
}
// Flatten the Tryjobs map and make sure the element in keys matches.
@@ -301,7 +301,7 @@
var err error
results, err = c.GetTryjobResults(tryjobs)
if err != nil {
- return nil, nil, err
+ return nil, nil, skerr.Fmt("could not get tryjob results for issue %d: %s", issueID, err)
}
}
diff --git a/golden/go/types/types.go b/golden/go/types/types.go
index 10ca302..00047f9 100644
--- a/golden/go/types/types.go
+++ b/golden/go/types/types.go
@@ -25,8 +25,19 @@
// MAXIMUM_NAME_LENGTH is the maximum length in bytes a test name can be.
MAXIMUM_NAME_LENGTH = 256
+
+ MasterBranch = int64(-1)
+ // There was a time when 0 meant MasterBranch in some places and -1 was used in others.
+ // It should all be -1 now, but we have this in place to work around some old code.
+ // TODO(kjlubick): remove this on/after Aug 2019 once the 0 has faded away from the
+ // latest and greatest.
+ LegacyMasterBranch = int64(0)
)
+func IsMasterBranch(b int64) bool {
+ return b == MasterBranch || b == LegacyMasterBranch
+}
+
// Label for classifying digests.
type Label int
diff --git a/golden/go/web/web.go b/golden/go/web/web.go
index 6cf8077..6d36da2 100644
--- a/golden/go/web/web.go
+++ b/golden/go/web/web.go
@@ -268,7 +268,7 @@
return
}
- if issueID <= 0 {
+ if types.IsMasterBranch(issueID) || issueID < 0 {
httputils.ReportError(w, r, fmt.Errorf("Issue id is <= 0"), "Valid issue ID required.")
return
}
@@ -308,7 +308,7 @@
return
}
- if (query.Issue > 0) || (query.BlameGroupID != "") {
+ if !types.IsMasterBranch(query.Issue) || (query.BlameGroupID != "") {
msg := "Search query cannot contain blame or issue information."
httputils.ReportError(w, r, errors.New(msg), msg)
return
@@ -1153,7 +1153,7 @@
func (wh *WebHandlers) JsonBaselineHandler(w http.ResponseWriter, r *http.Request) {
defer metrics2.FuncTimer().Stop()
commitHash := ""
- issueID := int64(0)
+ issueID := types.MasterBranch
issueOnly := false
var err error
@@ -1204,7 +1204,7 @@
return
}
- issueID := int64(0)
+ issueID := types.MasterBranch
var err error
issueIDStr, ok := mux.Vars(r)["id"]
if ok {
