test-results: package model: Add type TestList

test-results: package model: Add type TestList

* Automatically extract builder name when unmarshaling
  AggregateResult.
* Fix test for above.
* Fix comment in constructTree.
* Fix trim method

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/aaeb6090b2ad88b283313cc3c9ee5c393e109749

[nishanths, 2016-08-16 05:04:51]

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

[nishanths, 2016-08-16 06:21:36]

Description was changed from

==========
test-results: package model: Add type TestList

* Automatically extract builder name when unmarshaling
  AggregateResult.
* Fix test for above.
* Fix comment in constructTree.

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

to

==========
test-results: package model: Add type TestList

* Automatically extract builder name when unmarshaling
  AggregateResult.
* Fix test for above.
* Fix comment in constructTree.
* Fix trim method

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

[nishanths, 2016-08-16 06:35:01]

nishanths@google.com changed reviewers:
+ vadimsh@chromium.org
- vadim@chromium.org

[Vadim Sh., 2016-08-16 18:36:28]

lgtm with question

https://codereview.chromium.org/2252623002/diff/1/go/src/infra/appengine/test...
File go/src/infra/appengine/test-results/model/aggregate_result.go (right):

https://codereview.chromium.org/2252623002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/model/aggregate_result.go:111:
tl.Tests.WalkLeaves(func(_ string, leaf *AggregateTestLeaf) {
is it ok that function that looks like a getter (TestList()) actually mutates
the underlying data (*AggregateTestLeaf that live somewhere inside ag.Tests)?
Also, why is this needed?

[nishanths, 2016-08-16 20:02:13]

On 2016/08/16 18:36:28, Vadim Sh. wrote:
> lgtm with question
> 
>
https://codereview.chromium.org/2252623002/diff/1/go/src/infra/appengine/test...
> File go/src/infra/appengine/test-results/model/aggregate_result.go (right):
> 
>
https://codereview.chromium.org/2252623002/diff/1/go/src/infra/appengine/test...
> go/src/infra/appengine/test-results/model/aggregate_result.go:111:
> tl.Tests.WalkLeaves(func(_ string, leaf *AggregateTestLeaf) {
> is it ok that function that looks like a getter (TestList()) actually mutates
> the underlying data (*AggregateTestLeaf that live somewhere inside ag.Tests)?
> Also, why is this needed?

yes, that does not look good.
I've renamed the method to "ToTestList" and documented that the Tests field of
the receiver will be mutated.
(the alternative is to do a deep copy, but that is overkill for now?)

It is used when the query string parameter "testlistjson" is present (see line
219 on the right): 
https://codereview.chromium.org/2251623002/diff/40001/go/src/infra/appengine/...

[nishanths, 2016-08-16 20:35:50]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-08-16 20:35:50]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2252623002/#ps20001
(title: "Address Vadim's comment")

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

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 20:44:06]

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  Infra Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30ade301e3350810)
  Infra Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/30ade30178fc4610)

[nishanths, 2016-08-16 20:48:27]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-08-16 20:48:28]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2252623002/#ps40001
(title: "Fix renamed function call")

[commit-bot: I haz the power, 2016-08-16 20:48:36]

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

[nishanths, 2016-08-16 20:49:40]

On 2016/08/16 20:44:06, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   Infra Mac Tester on luci.infra.try (JOB_FAILED,
> https://luci-milo.appspot.com/swarming/task/30ade301e3350810)
>   Infra Win Tester on luci.infra.try (JOB_FAILED,
> https://luci-milo.appspot.com/swarming/task/30ade30178fc4610)

Failed because the function call "TestList"/"ToTestList" had not been renamed at
usage sites.

[commit-bot: I haz the power, 2016-08-16 22:36:20]

Description was changed from

==========
test-results: package model: Add type TestList

* Automatically extract builder name when unmarshaling
  AggregateResult.
* Fix test for above.
* Fix comment in constructTree.
* Fix trim method

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

to

==========
test-results: package model: Add type TestList

* Automatically extract builder name when unmarshaling
  AggregateResult.
* Fix test for above.
* Fix comment in constructTree.
* Fix trim method

BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/aaeb6090b2ad88b283313cc3c9ee5...
==========

[commit-bot: I haz the power, 2016-08-16 22:36:21]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/aaeb6090b2ad88b283313cc3c9ee5...