[gold] Clarify IssueExpectations should be the delta
Bug: skia:9090
Change-Id: I0d5d02c073d36a7fe186b241e47cd6d5351baeb8
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/219380
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
diff --git a/golden/go/expstorage/fs_expstore/FIRESTORE.md b/golden/go/expstorage/fs_expstore/FIRESTORE.md
index 7ca66c8..48756f7 100644
--- a/golden/go/expstorage/fs_expstore/FIRESTORE.md
+++ b/golden/go/expstorage/fs_expstore/FIRESTORE.md
@@ -13,6 +13,7 @@
There is the idea of the MasterExpectations, which is the Expectations belonging to the
git branch "master". Additionally, there can be smaller IssueExpectations that belong
to a ChangeList (CL) and stay separate from the MasterExpectations until the CL lands.
+These IssueExpectations are the "delta" compared to the MasterExpectations.
We'd like to be able to do the following:
diff --git a/golden/go/expstorage/fs_expstore/fs_expstore.go b/golden/go/expstorage/fs_expstore/fs_expstore.go
index 80a0b56..da7b6bf 100644
--- a/golden/go/expstorage/fs_expstore/fs_expstore.go
+++ b/golden/go/expstorage/fs_expstore/fs_expstore.go
@@ -152,44 +152,35 @@
f.cacheMutex.RLock()
defer f.cacheMutex.RUnlock()
}
- return f.getMasterExpectations(true)
+ return f.getMasterExpectations()
}
defer metrics2.NewTimer("gold_get_expectations", map[string]string{"master_branch": "false"}).Stop()
return f.getIssueExpectations()
}
// getMasterExpectations returns an Expectations object which is safe to mutate
-// based on the current state. In cases where caching is fine (e.g. the MasterBranch).
-// If caching it is used, it is expected the caller has taken care of any mutex grabbing.
-func (f *Store) getMasterExpectations(useCache bool) (types.Expectations, error) {
- if f.cache == nil || !useCache {
+// 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.loadExpectations(expstorage.MasterBranch)
if err != nil {
return nil, skerr.Fmt("could not load master expectations from firestore: %s", err)
}
- if !useCache {
- return c, nil
- }
f.cache = c
}
return f.cache.DeepCopy(), nil
}
// getIssueExpectations returns an Expectations object which is safe to mutate
-// that has all issue-specific Expectations applied to the master Expectations.
+// that has all issue-specific Expectations.
// It fetches everything from firestore every time, as there could be multiple
// readers and writers and thus caching isn't safe.
func (f *Store) getIssueExpectations() (types.Expectations, error) {
- masterExp, err := f.getMasterExpectations(false)
- if err != nil {
- return nil, skerr.Fmt("could not load master expectations: %s", err)
- }
issueExp, err := f.loadExpectations(f.issue)
if err != nil {
return nil, skerr.Fmt("could not load expectations delta for issue %d from firestore: %s", f.issue, err)
}
- masterExp.MergeExpectations(issueExp)
- return masterExp, nil
+ return issueExp, nil
}
// loadExpectations returns an Expectations object from the expectationsCollection,
diff --git a/golden/go/expstorage/fs_expstore/fs_expstore_test.go b/golden/go/expstorage/fs_expstore/fs_expstore_test.go
index 61c07d5..7aa169b 100644
--- a/golden/go/expstorage/fs_expstore/fs_expstore_test.go
+++ b/golden/go/expstorage/fs_expstore/fs_expstore_test.go
@@ -550,8 +550,8 @@
// TestIssueExpectationsAddGet tests the separation of the MasterExpectations
// and the IssueExpectations. It starts with a shared history, then
// adds some expectations to both, before asserting that they are properly dealt
-// with. Specifically, the IssueExpectations should be applied as a delta to
-// the MasterExpectations.
+// with. Specifically, the IssueExpectations should be treated as a delta to
+// the MasterExpectations (but doesn't actually contain MasterExpectations).
func TestIssueExpectationsAddGet(t *testing.T) {
unittest.ManualTest(t)
unittest.RequiresFirestoreEmulator(t)
@@ -568,16 +568,15 @@
ib := mb.ForIssue(117) // arbitrary issue id
- masterE, err := mb.Get()
- assert.NoError(t, err)
+ // Check that it starts out blank.
issueE, err := ib.Get()
assert.NoError(t, err)
- assert.Equal(t, masterE, issueE)
+ assert.Equal(t, types.Expectations{}, issueE)
// Add to the IssueExpectations
assert.NoError(t, ib.AddChange(ctx, types.Expectations{
data.AlphaTest: {
- data.AlphaGood1Digest: types.POSITIVE, // overwrites previous
+ data.AlphaGood1Digest: types.POSITIVE,
},
data.BetaTest: {
data.BetaGood1Digest: types.POSITIVE,
@@ -591,7 +590,7 @@
},
}, userOne))
- masterE, err = mb.Get()
+ masterE, err := mb.Get()
assert.NoError(t, err)
issueE, err = ib.Get()
assert.NoError(t, err)
@@ -604,12 +603,10 @@
},
}, masterE)
- // Make sure the IssueExpectations are applied on top of the updated
- // MasterExpectations.
+ // Make sure the IssueExpectations are separate from the MasterExpectations.
assert.Equal(t, types.Expectations{
data.AlphaTest: {
data.AlphaGood1Digest: types.POSITIVE,
- data.AlphaBad1Digest: types.NEGATIVE,
},
data.BetaTest: {
data.BetaGood1Digest: types.POSITIVE,
diff --git a/golden/go/expstorage/types.go b/golden/go/expstorage/types.go
index 7060133..c659de2 100644
--- a/golden/go/expstorage/types.go
+++ b/golden/go/expstorage/types.go
@@ -55,8 +55,9 @@
UndoChange(ctx context.Context, changeID, userID string) (types.Expectations, error)
// Returns a new ExpectationStore that will deal with the Expectations for an issue
- // with the given id (aka an IssueExpectations). Any expectations sent to the returned
- // ExpectationStore will be kept separate from the master branch.
+ // with the given id (aka an IssueExpectations). Any Expectations added to the returned
+ // ExpectationStore will be kept separate from the master branch. Any Expectations
+ // returned should be treated as the delta between the MasterBranch and the given issue.
// This issue id is the Gerrit id or GitHub id.
ForIssue(id int64) ExpectationsStore
}
diff --git a/golden/go/storage/storage.go b/golden/go/storage/storage.go
index 6d27d30..4db4404 100644
--- a/golden/go/storage/storage.go
+++ b/golden/go/storage/storage.go
@@ -436,6 +436,8 @@
// checkCommitableIssues checks all commits of the current tile whether
// the associated expectations have been added to the baseline of the master.
+// TODO(kjlubick): This should not be here, but likely in tryjobMonitor, named
+// something like "CatchUpIssues" or something.
func (s *Storage) checkCommitableIssues(cpxTile types.ComplexTile) {
go func() {
var egroup errgroup.Group
@@ -443,6 +445,8 @@
for _, commit := range cpxTile.AllCommits() {
func(commit *tiling.Commit) {
egroup.Go(func() error {
+ // TODO(kjlubick): We probably don't need to run this individually, we could
+ // use DetailsMulti instead.
longCommit, err := s.VCS.Details(context.Background(), commit.Hash, false)
if err != nil {
return sklog.FmtErrorf("Error retrieving details for commit %s. Got error: %s", commit.Hash, err)
