test-results: package model: Add full_result.go and tests

test-results: package model: Add type FullResult and tests

* Add type FullResult and related types (for "full_results.json")

* Add documentation for exported names, mostly in aggregate_result.go
* Fix golint messages in this package: https://paste.googleplex.com/4585834795761664
* Follow consistent format in error strings
* Remove unnecessary checks and tests in AggregateResult

* Rename TestNode -> Node
* Export type Number 


BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/88d3e66393f24f96716fe8ec4d929bafb9943cc2

[nishanths, 2016-08-11 11:05:45]

Description was changed from

==========
test-results: package model: Add full_result.go and tests

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

to

==========
test-results: package model: Add type FullResult and tests

* Add type FullResult and related types (for "full_results.json")

* Add documentation for exported names, mostly in aggregate_result.go
* Remove unnecessary checks and tests in AggregateResult

* Rename TestNode -> Node
* Export type Number 


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

[nishanths, 2016-08-11 11:06:34]

Description was changed from

==========
test-results: package model: Add type FullResult and tests

* Add type FullResult and related types (for "full_results.json")

* Add documentation for exported names, mostly in aggregate_result.go
* Remove unnecessary checks and tests in AggregateResult

* Rename TestNode -> Node
* Export type Number 


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

to

==========
test-results: package model: Add type FullResult and tests

* Add type FullResult and related types (for "full_results.json")

* Add documentation for exported names, mostly in aggregate_result.go
* Follow consistent format in error strings
* Remove unnecessary checks and tests in AggregateResult

* Rename TestNode -> Node
* Export type Number 


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

[nishanths, 2016-08-11 11:07:13]

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

[nishanths, 2016-08-11 11:13:44]

PTAL, thanks!

[nishanths, 2016-08-11 18:56:42]

Description was changed from

==========
test-results: package model: Add type FullResult and tests

* Add type FullResult and related types (for "full_results.json")

* Add documentation for exported names, mostly in aggregate_result.go
* Follow consistent format in error strings
* Remove unnecessary checks and tests in AggregateResult

* Rename TestNode -> Node
* Export type Number 


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

to

==========
test-results: package model: Add type FullResult and tests

* Add type FullResult and related types (for "full_results.json")

* Add documentation for exported names, mostly in aggregate_result.go
* Fix golint messages in this package:
https://paste.googleplex.com/4585834795761664
* Follow consistent format in error strings
* Remove unnecessary checks and tests in AggregateResult

* Rename TestNode -> Node
* Export type Number 


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

[estaab, 2016-08-12 15:17:20]

lgtm

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

https://codereview.chromium.org/2234353002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/model/common.go:18: // In reality, it as
almost as weak as empty interface,
Interesting, is this a common pattern in go?

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

https://codereview.chromium.org/2234353002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/model/full_result.go:85: func (ft FullTest)
AggregateTest() (AggregateTest, error) {
won't this make a copy of ft since it's not a pointer receiver? use pointer
receiver here and on 80

[nishanths, 2016-08-12 16:45:58]

Addressed estaab@ comments.
Thanks for reviewing!

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

https://codereview.chromium.org/2234353002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/model/common.go:18: // In reality, it as
almost as weak as empty interface,
On 2016/08/12 15:17:20, estaab wrote:
> Interesting, is this a common pattern in go?

I may have seen it elsewhere, but don't remember where.

It's nice because users of the package are now only able to use the types
AggregateTest, AggregateTestLeaf, FullTest, and FullTestLeaf when they construct
a test trie by hand, hence the type safety.

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

https://codereview.chromium.org/2234353002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/model/full_result.go:85: func (ft FullTest)
AggregateTest() (AggregateTest, error) {
On 2016/08/12 15:17:20, estaab wrote:
> won't this make a copy of ft since it's not a pointer receiver? use pointer
> receiver here and on 80

FullTest's underlying type is a map, so it will be passed by reference.

[nishanths, 2016-08-12 16:46:47]

On 2016/08/12 16:45:58, nishanths wrote:
> Addressed estaab@ comments.
> Thanks for reviewing!
> 
>
https://codereview.chromium.org/2234353002/diff/1/go/src/infra/appengine/test...
> File go/src/infra/appengine/test-results/model/common.go (right):
> 
>
https://codereview.chromium.org/2234353002/diff/1/go/src/infra/appengine/test...
> go/src/infra/appengine/test-results/model/common.go:18: // In reality, it as
> almost as weak as empty interface,
> On 2016/08/12 15:17:20, estaab wrote:
> > Interesting, is this a common pattern in go?
> 
> I may have seen it elsewhere, but don't remember where.
> 
> It's nice because users of the package are now only able to use the types
> AggregateTest, AggregateTestLeaf, FullTest, and FullTestLeaf when they
construct
> a test trie by hand, hence the type safety.
> 
>
https://codereview.chromium.org/2234353002/diff/1/go/src/infra/appengine/test...
> File go/src/infra/appengine/test-results/model/full_result.go (right):
> 
>
https://codereview.chromium.org/2234353002/diff/1/go/src/infra/appengine/test...
> go/src/infra/appengine/test-results/model/full_result.go:85: func (ft
FullTest)
> AggregateTest() (AggregateTest, error) {
> On 2016/08/12 15:17:20, estaab wrote:
> > won't this make a copy of ft since it's not a pointer receiver? use pointer
> > receiver here and on 80
> 
> FullTest's underlying type is a map, so it will be passed by reference.

(for clarity, addressing comments did not need a new patch set.)

[nishanths, 2016-08-12 16:47:16]

The CQ bit was checked by nishanths@google.com

[commit-bot: I haz the power, 2016-08-12 16:47:22]

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-12 16:59:51]

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

[commit-bot: I haz the power, 2016-08-12 16:59:52]

Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/309878641a85d110)

[nishanths, 2016-08-12 18:19:17]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-08-12 18:19:17]

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/2234353002/#ps20001
(title: "Improve coverage")

[commit-bot: I haz the power, 2016-08-12 18:19:22]

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-12 18:33:12]

Description was changed from

==========
test-results: package model: Add type FullResult and tests

* Add type FullResult and related types (for "full_results.json")

* Add documentation for exported names, mostly in aggregate_result.go
* Fix golint messages in this package:
https://paste.googleplex.com/4585834795761664
* Follow consistent format in error strings
* Remove unnecessary checks and tests in AggregateResult

* Rename TestNode -> Node
* Export type Number 


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

to

==========
test-results: package model: Add type FullResult and tests

* Add type FullResult and related types (for "full_results.json")

* Add documentation for exported names, mostly in aggregate_result.go
* Fix golint messages in this package:
https://paste.googleplex.com/4585834795761664
* Follow consistent format in error strings
* Remove unnecessary checks and tests in AggregateResult

* Rename TestNode -> Node
* Export type Number 


BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/88d3e66393f24f96716fe8ec4d929...
==========

[commit-bot: I haz the power, 2016-08-12 18:33:13]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/infra/infra/+/88d3e66393f24f96716fe8ec4d929...