test-results: Add merge and trim methods

test-results: Add merge and trim methods

* Add merge and trim: implementation + tests
* Begin adding end-to-end tests

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/972ccfc6ce9389ff6edb7c620eb2742f1e273485

[nishanths, 2016-08-11 11:17:54]

nishanths@google.com changed reviewers:
+ estaab@chromium.org, martiniss@chromium.org, vadimsh@chromium.org

[nishanths, 2016-08-11 11:17:55]

Most of the lines in this CL is tests and testdata.
`merge_test.go` contains tests ported from the Python version:

https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

Only 272 lines of the total 1978 lines is implementation.

Thanks!

[nishanths, 2016-08-11 11:18:53]

On 2016/08/11 11:17:55, nishanths wrote:
> Most of the lines in this CL is tests and testdata.
> `merge_test.go` contains tests ported from the Python version:
> 
>
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
> 
> Only 272 lines of the total 1978 lines is implementation.
> 
> Thanks!

Depends on CL: https://codereview.chromium.org/2234353002/

[martiniss, 2016-08-12 00:14:25]

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/end_to_end_test.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/end_to_end_test.go:47: "audio":
AggregateTest{
Just to confirm, the W3C video src tests in the test file aren't here because
they're not failing? Maybe add a comment.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/end_to_end_test.go:107: var x
FullResult
nit: "var result FullResult"?

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/merge.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:1: package model
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.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:8: ErrBuildNumberConflict =
errors.New("build number conflict")
It's not obvious to me from reading this file why this can't be the case. Maybe
expand more in the function documentation?

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:17: func (ar
*AggregateResult) Merge(x *AggregateResult) error {
nit: "other" is a bit more descriptive than "x".

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:31: // Merge merges x into
info. After merging, info's fields may reference the same
Does the reference comment matter? Would that ever be confusing for callers?

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:33: func (info *BuilderInfo)
Merge(x *BuilderInfo) error {
nit: "x" variable name.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:34: if info == nil {
Why would info ever be nil?

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:50: if info.FailuresByType ==
nil {
Move outside for loop.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:80: xcopy :=
make(AggregateTest, len(x))
shouldn't this be make([]AggregateTest, len(x)) ? Or does this work to create a
slice

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:140: if leaf == nil {
Same comment; seems unnecessary

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/merge_test.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge_test.go:149:
Convey("AggregateTest in at, *AggregateTestLeaf in x", func() {
"at" and "x" are implementation details. Could just say "Merge AggregateTestLeaf
into aggregateTest"

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge_test.go:286:
Convey("AggregateResult", func() {
"AggregateResult" merge? What is this testing

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge_test.go:365: // The test names
here correspond to the names in the original file
My opinion, but I think you should remove the underscores and make the test
names just sentences, e.g. "empty aggregated results"

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/trim.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/trim.go:64: func (leaf
*AggregateTestLeaf) trim(size int) {
why is size an int here, but threshold is declared as a float?

[nishanths, 2016-08-12 02:37:40]

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/end_to_end_test.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/end_to_end_test.go:47: "audio":
AggregateTest{
On 2016/08/12 00:14:24, martiniss wrote:
> Just to confirm, the W3C video src tests in the test file aren't here because
> they're not failing? Maybe add a comment.

Yes, added comment. Also added comment that is a port of an existing python
test.

https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/end_to_end_test.go:107: var x
FullResult
On 2016/08/12 00:14:24, martiniss wrote:
> nit: "var result FullResult"?

Done.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/merge.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:1: package model
On 2016/08/12 00:14:24, martiniss wrote:
> 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.

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

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:8: ErrBuildNumberConflict =
errors.New("build number conflict")
On 2016/08/12 00:14:24, martiniss wrote:
> It's not obvious to me from reading this file why this can't be the case.
Maybe
> expand more in the function documentation?

Done.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:17: func (ar
*AggregateResult) Merge(x *AggregateResult) error {
On 2016/08/12 00:14:24, martiniss wrote:
> nit: "other" is a bit more descriptive than "x".

Done, thanks.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:31: // Merge merges x into
info. After merging, info's fields may reference the same
On 2016/08/12 00:14:24, martiniss wrote:
> Does the reference comment matter? Would that ever be confusing for callers?

Removed, thanks.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:33: func (info *BuilderInfo)
Merge(x *BuilderInfo) error {
On 2016/08/12 00:14:24, martiniss wrote:
> nit: "x" variable name.

Done: used "other".

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:34: if info == nil {
On 2016/08/12 00:14:24, martiniss wrote:
> Why would info ever be nil?

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.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:50: if info.FailuresByType ==
nil {
On 2016/08/12 00:14:24, martiniss wrote:
> Move outside for loop.

Done.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:80: xcopy :=
make(AggregateTest, len(x))
On 2016/08/12 00:14:24, martiniss wrote:
> shouldn't this be make([]AggregateTest, len(x)) ? Or does this work to create
a
> slice

AggregateTest is a map[string]Node.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:140: if leaf == nil {
On 2016/08/12 00:14:24, martiniss wrote:
> Same comment; seems unnecessary

Done, removed check.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/merge_test.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge_test.go:149:
Convey("AggregateTest in at, *AggregateTestLeaf in x", func() {
On 2016/08/12 00:14:24, martiniss wrote:
> "at" and "x" are implementation details. Could just say "Merge
AggregateTestLeaf
> into aggregateTest"

Done, thanks!

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge_test.go:286:
Convey("AggregateResult", func() {
On 2016/08/12 00:14:24, martiniss wrote:
> "AggregateResult" merge? What is this testing

Done.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge_test.go:365: // The test names
here correspond to the names in the original file
On 2016/08/12 00:14:24, martiniss wrote:
> My opinion, but I think you should remove the underscores and make the test
> names just sentences, e.g. "empty aggregated results"

Done.

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/trim.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/trim.go:64: func (leaf
*AggregateTestLeaf) trim(size int) {
On 2016/08/12 00:14:24, martiniss wrote:
> why is size an int here, but threshold is declared as a float?

threshold is a float because it is used to compare against the Runtimes field in
an AggregateTestLeaf.

Runtimes are defined as times in seconds represented as floats.
https://www.chromium.org/developers/the-json-test-results-format (see "time"
field).

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

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/model/merge.go (right):

https://codereview.chromium.org/2231393002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/model/merge.go:1: package model
On 2016/08/12 02:37:39, nishanths wrote:
> On 2016/08/12 00:14:24, martiniss wrote:
> > 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.
> 
> As discussed offline, will move the contents of merge.go and trim.go to
> aggregate_result.go after https://codereview.chromium.org/2234353002/ lands.

Done in patch set 7.

[estaab, 2016-08-16 15:42:54]

lgtm

[nishanths, 2016-08-16 16:15:57]

The CQ bit was checked by nishanths@google.com

[commit-bot: I haz the power, 2016-08-16 16:16:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nishanths, 2016-08-16 16:39:30]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-08-16 16:39:31]

The patchset sent to the CQ was uploaded after l-g-t-m from estaab@chromium.org
Link to the patchset: https://codereview.chromium.org/2231393002/#ps160001
(title: "tests: remove unecessary Trim calls")

[commit-bot: I haz the power, 2016-08-16 16:39:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 16:53:14]

Description was changed from

==========
test-results: Add merge and trim methods

* Add merge and trim: implementation + tests
* Begin adding end-to-end tests

BUG=
==========

to

==========
test-results: Add merge and trim methods

* Add merge and trim: implementation + tests
* Begin adding end-to-end tests

BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/972ccfc6ce9389ff6edb7c620eb27...
==========

[commit-bot: I haz the power, 2016-08-16 16:53:15]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/infra/infra/+/972ccfc6ce9389ff6edb7c620eb27...