test-results: Add revisions handler

test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  * https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_module/test_results/main.py
  * https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_module/test_results/handlers/redirector.py

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/6b46ad94077b0deec2161d7799fa7b1aa905ac38

[nishanths, 2016-08-16 22:14:32]

Description was changed from

==========
test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

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

to

==========
test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

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

[nishanths, 2016-08-16 22:14:32]

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

[nishanths, 2016-08-16 22:16:11]

Description was changed from

==========
test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

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

to

==========
[no hurry to review] test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

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

[nishanths, 2016-08-16 22:45:16]

no hurry to review :)

[nishanths, 2016-08-16 22:56:36]

https://codereview.chromium.org/2255433003/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/frontend/revision.go (right):

https://codereview.chromium.org/2255433003/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/frontend/revision.go:51: ),
http.StatusMovedPermanently)
Used permanent redirect because that is the default for webapp2.RedirectHandler
used in python impl.
https://github.com/GoogleCloudPlatform/webapp2/blob/042cc5ade660e094c773cdbe0...

[estaab, 2016-08-17 18:34:51]

lgtm

https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/frontend/revision.go (right):

https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/frontend/revision.go:66: func
commitPositionToHash(c context.Context, pos string) (string, error) {
How about having a separate function that takes a http.Client parameter and the
test uses it, and a wrapper that creates its own http.Client that regular code
calls? The global pattern isn't great.

[nishanths, 2016-08-17 18:44:08]

https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/frontend/revision.go (right):

https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/frontend/revision.go:66: func
commitPositionToHash(c context.Context, pos string) (string, error) {
On 2016/08/17 18:34:50, estaab wrote:
> How about having a separate function that takes a http.Client parameter and
the
> test uses it, and a wrapper that creates its own http.Client that regular code
> calls? The global pattern isn't great.

Much better!
Should I do the same for the crRevURL global also?

[estaab, 2016-08-17 18:47:43]

lgtm

https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/frontend/revision.go (right):

https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/frontend/revision.go:66: func
commitPositionToHash(c context.Context, pos string) (string, error) {
On 2016/08/17 at 18:44:08, nishanths wrote:
> On 2016/08/17 18:34:50, estaab wrote:
> > How about having a separate function that takes a http.Client parameter and
the
> > test uses it, and a wrapper that creates its own http.Client that regular
code
> > calls? The global pattern isn't great.
> 
> Much better!
> Should I do the same for the crRevURL global also?

Is the test actually using it? I see it being set but I don't think it's used,
right? Maybe just make it const.

[nishanths, 2016-08-17 22:18:20]

On 2016/08/17 18:47:43, estaab wrote:
> lgtm
> 
>
https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
> File go/src/infra/appengine/test-results/frontend/revision.go (right):
> 
>
https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
> go/src/infra/appengine/test-results/frontend/revision.go:66: func
> commitPositionToHash(c context.Context, pos string) (string, error) {
> On 2016/08/17 at 18:44:08, nishanths wrote:
> > On 2016/08/17 18:34:50, estaab wrote:
> > > How about having a separate function that takes a http.Client parameter
and
> the
> > > test uses it, and a wrapper that creates its own http.Client that regular
> code
> > > calls? The global pattern isn't great.
> > 
> > Much better!
> > Should I do the same for the crRevURL global also?
> 
> Is the test actually using it? I see it being set but I don't think it's used,
> right? Maybe just make it const.

It was being used inside commitPositionToHash. 

I've introduced a new type:

type crRevClient struct {
  HTTPClient *http.Client
  BaseURL    string
}

and made commitPositionToHash a method on the crRevClient.
This should get rid off the ugliness. thoughts?

[nishanths, 2016-08-17 22:19:24]

Description was changed from

==========
[no hurry to review] test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

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

to

==========
test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

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

[estaab, 2016-08-19 00:36:46]

On 2016/08/17 at 22:18:20, nishanths wrote:
> On 2016/08/17 18:47:43, estaab wrote:
> > lgtm
> > 
> >
https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
> > File go/src/infra/appengine/test-results/frontend/revision.go (right):
> > 
> >
https://codereview.chromium.org/2255433003/diff/60001/go/src/infra/appengine/...
> > go/src/infra/appengine/test-results/frontend/revision.go:66: func
> > commitPositionToHash(c context.Context, pos string) (string, error) {
> > On 2016/08/17 at 18:44:08, nishanths wrote:
> > > On 2016/08/17 18:34:50, estaab wrote:
> > > > How about having a separate function that takes a http.Client parameter
and
> > the
> > > > test uses it, and a wrapper that creates its own http.Client that
regular
> > code
> > > > calls? The global pattern isn't great.
> > > 
> > > Much better!
> > > Should I do the same for the crRevURL global also?
> > 
> > Is the test actually using it? I see it being set but I don't think it's
used,
> > right? Maybe just make it const.
> 
> It was being used inside commitPositionToHash. 
> 
> I've introduced a new type:
> 
> type crRevClient struct {
>   HTTPClient *http.Client
>   BaseURL    string
> }
> 
> and made commitPositionToHash a method on the crRevClient.
> This should get rid off the ugliness. thoughts?

Looks good.

[estaab, 2016-08-19 00:36:50]

lgtm

[nishanths, 2016-08-19 02:01:56]

The CQ bit was checked by nishanths@google.com

[commit-bot: I haz the power, 2016-08-19 02:01:59]

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-19 04:24:45]

Description was changed from

==========
test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

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

to

==========
test-results: Add revisions handler.

This adds tests and implementation for the GET /revision_range endpoint.

Related files from python implementation:
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...
  *
https://cs.chromium.org/chromium/infra/appengine/test_results/appengine_modul...

BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/6b46ad94077b0deec2161d7799fa7...
==========

[commit-bot: I haz the power, 2016-08-19 04:24:46]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/6b46ad94077b0deec2161d7799fa7...