test-results: Add merge and trim methods

go/src/infra/appengine/test-results/model/merge.go

+package model

[martiniss, 2016/08/12 00:14:24]

I don't see the need for a merge.go file; why don't these live in the files
where these structs are defined? If you're calling a common function in here,
that would make more sense, but it looks like there's no reason these functions
need to be in this file.

[nishanths, 2016/08/12 02:37:39]

As discussed offline, will move the contents of merge.go and trim.go to
aggregate_result.go after https://codereview.chromium.org/2234353002/ lands.

[nishanths, 2016/08/16 01:01:53]

Done in patch set 7.

+
+import "errors"
+
+var (
+        // ErrBuildNumberConflict is returned when the build numbers
+        // are the same when merging.
+        ErrBuildNumberConflict = errors.New("build number conflict")

[martiniss, 2016/08/12 00:14:24]

It's not obvious to me from reading this file why this can't be the case. Maybe
expand more in the function documentation?

[nishanths, 2016/08/12 02:37:39]

Done.

+
+        // ErrBuilderNameConflict is returned when the builder names
+        // do not match when merging.
+        ErrBuilderNameConflict = errors.New("builder name conflict")
+)
+
+// Merge merges x into ar. After merging, ar's fields may reference the same
+// objects referenced by x.
+func (ar *AggregateResult) Merge(x *AggregateResult) error {

[martiniss, 2016/08/12 00:14:24]

nit: "other" is a bit more descriptive than "x".

[nishanths, 2016/08/12 02:37:39]

Done, thanks.

+        if ar == nil {
+                return errors.New("model: Merge: nil *AggregateResult")
+        }
+        if ar.Builder != x.Builder {
+                return ErrBuilderNameConflict
+        }
+        if ar.BuilderInfo == nil {
+                ar.BuilderInfo = &BuilderInfo{}
+        }
+        ar.Version = ResultsVersion
+        return ar.BuilderInfo.Merge(x.BuilderInfo)
+}
+
+// Merge merges x into info. After merging, info's fields may reference the same

[martiniss, 2016/08/12 00:14:24]

Does the reference comment matter? Would that ever be confusing for callers?

[nishanths, 2016/08/12 02:37:39]

Removed, thanks.

+// objects referenced by x.
+func (info *BuilderInfo) Merge(x *BuilderInfo) error {

[martiniss, 2016/08/12 00:14:24]

nit: "x" variable name.

[nishanths, 2016/08/12 02:37:39]

Done: used "other".

+        if info == nil {

[martiniss, 2016/08/12 00:14:24]

Why would info ever be nil?

[nishanths, 2016/08/12 02:37:39]

Removed the check (also removed at similar locations).

Unlikely `info` would be nil. 
If it were, we would panic below when dereferencing. 
panic seems to be acceptable behavior in this case.

+                return errors.New("model: Merge: nil *BuilderInfo")
+        }
+
+        if len(info.BuildNumbers) > 0 && len(x.BuildNumbers) > 0 {
+                if info.BuildNumbers[0] == x.BuildNumbers[0] {
+                        return ErrBuildNumberConflict
+                }
+        }
+
+        info.SecondsEpoch = append(x.SecondsEpoch, info.SecondsEpoch...)
+        info.BlinkRevs = append(x.BlinkRevs, info.BlinkRevs...)
+        info.BuildNumbers = append(x.BuildNumbers, info.BuildNumbers...)
+        info.ChromeRevs = append(x.ChromeRevs, info.ChromeRevs...)
+
+        for k, v := range x.FailuresByType {
+                if info.FailuresByType == nil {

[martiniss, 2016/08/12 00:14:24]

Move outside for loop.

[nishanths, 2016/08/12 02:37:39]

Done.

+                        info.FailuresByType = make(map[string][]int)
+                }
+                info.FailuresByType[k] = append(v, info.FailuresByType[k]...)
+        }
+
+        info.FailureMap = FailureLongNames
+
+        if info.Tests == nil {
+                info.Tests = AggregateTest{}
+        }
+
+        info.Tests.WalkLeaves(func(_ string, leaf *AggregateTestLeaf) {
+                leaf.Expected = nil
+                leaf.Bugs = nil
+        })
+
+        return info.Tests.Merge(x.Tests)
+}
+
+// Merge merges x into at. After merging, at's fields may reference the same
+// objects referenced by x.
+func (at *AggregateTest) Merge(x AggregateTest) error {
+        if at == nil {
+                return errors.New("model: Merge: nil *AggregateTest")
+        }
+
+        // Shallow copy but OK. We take care to not modify xcopy
+        // values; instead always create new objects
+        // and assign to xcopy[key].
+        xcopy := make(AggregateTest, len(x))

[martiniss, 2016/08/12 00:14:24]

shouldn't this be make([]AggregateTest, len(x)) ? Or does this work to create a
slice

[nishanths, 2016/08/12 02:37:39]

AggregateTest is a map[string]Node.

+        for k, v := range x {
+                xcopy[k] = v
+        }
+
+        for k, v := range *at {
+                if _, ok := xcopy[k]; !ok {
+                        switch v.(type) {
+                        case *AggregateTestLeaf:
+                                l := &AggregateTestLeaf{}
+                                l.defaultFields()
+                                xcopy[k] = l
+                        case AggregateTest:
+                                xcopy[k] = AggregateTest{}
+                        }
+                }
+        }
+
+        for k, v := range xcopy {
+                // Key does not exist: assign entire subtree.
+                if _, ok := (*at)[k]; !ok {
+                        if *at == nil {
+                                *at = AggregateTest{}
+                        }
+                        (*at)[k] = v
+                        continue
+                }
+
+                // Leaf node.
+                if leaf1, ok := (*at)[k].(*AggregateTestLeaf); ok {
+                        leaf2, ok := v.(*AggregateTestLeaf)
+                        if !ok {
+                                return errors.New("model: Merge: expected *AggregateTestLeaf")
+                        }
+                        if err := leaf1.Merge(leaf2); err != nil {
+                                return err
+                        }
+                        continue
+                }
+
+                // Not leaf node: merge subtree recursively.
+                at1, ok := (*at)[k].(AggregateTest)
+                if !ok {
+                        return errors.New("model: Merge: expected AggregateTest")
+                }
+                at2, ok := v.(AggregateTest)
+                if !ok {
+                        return errors.New("model: Merge: expected AggregateTest")
+                }
+                if err := at1.Merge(at2); err != nil {
+                        return err
+                }
+        }
+
+        return nil
+}
+
+// Merge merges x into leaf. After merging, leaf's fields may reference the same
+// objects referenced by x.
+func (leaf *AggregateTestLeaf) Merge(x *AggregateTestLeaf) error {
+        if leaf == nil {

[martiniss, 2016/08/12 00:14:24]

Same comment; seems unnecessary

[nishanths, 2016/08/12 02:37:39]

Done, removed check.

+                return errors.New("model: Merge: nil *AggregateTestLeaf")
+        }
+
+        // Bugs and Expected should come from from x only.
+        leaf.Bugs = x.Bugs
+        if len(x.Expected) == 1 && x.Expected[0] != "PASS" {
+                leaf.Expected = x.Expected
+        }
+
+        for _, r := range x.Results {
+                if len(leaf.Results) > 0 && r.Type == leaf.Results[0].Type {
+                        leaf.Results[0].Count += r.Count
+                } else {
+                        leaf.Results = append([]ResultSummary{r}, leaf.Results...)
+                }
+        }
+
+        for _, r := range x.Runtimes {
+                if len(leaf.Runtimes) > 0 && r.Runtime == leaf.Runtimes[0].Runtime {
+                        leaf.Runtimes[0].Count += r.Count
+                } else {
+                        leaf.Runtimes = append([]RuntimeSummary{r}, leaf.Runtimes...)
+                }
+        }
+
+        return nil
+}