Milo: Increase test coverage for appengine/buildbot

milo/appengine/buildbot/html.go

 // Copyright 2015 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 package buildbot
 
 import (
         "fmt"
         "net/http"
         "strconv"
@@ skipping 20 matching lines @@
                 common.ErrorPage(c, http.StatusBadRequest, "No build number")
                 return
         }
         num, err := strconv.Atoi(buildNum)
         if err != nil {
                 common.ErrorPage(c, http.StatusBadRequest,
                         fmt.Sprintf("%s does not look like a number", buildNum))
                 return
         }
 
-        result, err := build(c.Context, master, builder, num)
+        result, err := Build(c.Context, master, builder, num)
         if err != nil {
                 var code int
                 switch err {
                 case errBuildNotFound:
                         code = http.StatusNotFound
                 case errNotAuth:
                         code = http.StatusUnauthorized
                 default:
                         code = http.StatusInternalServerError
                 }
@@ skipping 24 matching lines @@
         if err != nil {
                 common.ErrorPage(c, http.StatusBadRequest, err.Error())
                 return
         }
         if limit < 0 {
                 limit = 25
         }
         cursor := c.Request.FormValue("cursor")
 
         result, err := builderImpl(c.Context, master, builder, limit, cursor)
         if err != nil {

[nodir, 2017/05/26 18:33:05]

we have 4 cases here essentially. I think using switch would make this code a
bit simpler and makes it impossible to forget a return statement

_, notFound := err.(errBuilderNotFound)
switch {
case notFound: ...
case err == errNotAuth: ...
case err != nil: ...
default:...
}
} // end of function

[Ryan Tseng, 2017/05/26 22:49:22]

I was reluctlant to do this because I wouldn't have been able to inline _,
notFound := err.(errBuilderNotFound).  But now that I tried it it doesn't look
too bad, changed

+                if _, ok := err.(errBuilderNotFound); ok {
+                        common.ErrorPage(c, http.StatusNotFound, err.Error())
+                        return
+                }
+                if err == errNotAuth {
+                        common.ErrorPage(c, http.StatusUnauthorized, err.Error())
+                        return
+                }
                 common.ErrorPage(c, http.StatusInternalServerError, err.Error())
                 return
         }
 
         // Render into the template
         templates.MustRender(c.Context, c.Writer, "pages/builder.html", templates.Args{
                 "Builder": result,
         })
 }