test-results: Add revisions handler

go/src/infra/appengine/test-results/frontend/revision.go

+package frontend
+
+import (
+        "encoding/json"
+        "fmt"
+        "net/http"
+        "sync"
+
+        "golang.org/x/net/context"
+
+        "github.com/luci/gae/service/urlfetch"
+        "github.com/luci/luci-go/server/router"
+)
+
+// TODO(estaab): Comment copied from python implementation:
+//   Get rid of this one crrev.com supports this directly.
+//   See http://crbug.com/407198.
+
+func revisionHandler(c *router.Context) {
+        type result struct {
+                hash string
+                err  error
+        }
+        results := make([]result, 2)
+        wg := sync.WaitGroup{}
+        wg.Add(2)
+
+        go func() {
+                defer wg.Done()
+                hash, err := commitPositionToHash(c.Context, c.Request.FormValue("start"))
+                results[0] = result{hash, err}
+        }()
+        go func() {
+                defer wg.Done()
+                hash, err := commitPositionToHash(c.Context, c.Request.FormValue("end"))
+                results[1] = result{hash, err}
+        }()
+
+        wg.Wait()
+        for _, r := range results {
+                if r.err != nil {
+                        http.Error(c.Writer, r.err.Error(), http.StatusInternalServerError)
+                        return
+                }
+        }
+
+        http.Redirect(c.Writer, c.Request, fmt.Sprintf(
+                "https://chromium.googlesource.com/chromium/src/+log/%s^..%s?pretty=fuller",
+                results[0].hash,
+                results[1].hash,
+        ), http.StatusMovedPermanently)
+}
+
+var (
+        // crRevURL is the URL used by commitPositionToHash.
+        // It is global to allow switching in tests.
+        crRevURL = "https://cr-rev.appspot.com/_ah/api/crrev/v1/redirect"
+
+        // makeCrRevClient returns a HTTP client for use by commitPositionToHash.
+        // It is global to allow switching in tests.
+        makeCrRevClient = func(c context.Context) *http.Client {
+                return &http.Client{Transport: urlfetch.Get(c)}
+        }
+)
+
+func commitPositionToHash(c context.Context, pos string) (string, error) {

[estaab, 2016/08/17 18:34:50]

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]

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

[estaab, 2016/08/17 18:47:42]

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.

+        client := makeCrRevClient(c)
+        resp, err := client.Get(crRevURL + "/" + pos)
+        if err != nil {
+                return "", err
+        }
+        defer resp.Body.Close()
+
+        if resp.StatusCode != http.StatusOK {
+                return "", fmt.Errorf("HTTP status code %d from %s", resp.StatusCode, resp.Request.URL)
+        }
+
+        hash := struct {
+                Hash string `json:"git_sha"`
+        }{}
+        if err := json.NewDecoder(resp.Body).Decode(&hash); err != nil {
+                return "", err
+        }
+        return hash.Hash, nil
+}