Milo: Increase test coverage for appengine/buildbot

milo/appengine/common/middleware.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 common
 
 import (
         "net/http"
         "strings"
 
@@ skipping 12 matching lines @@
         "github.com/luci/luci-go/server/router"
         "github.com/luci/luci-go/server/templates"
 )
 
 var authconfig *auth.Config
 
 // GetTemplateBundles is used to render HTML templates. It provides base args
 // passed to all templates.
 func GetTemplateBundle() *templates.Bundle {
         return &templates.Bundle{
-                Loader:          templates.FileSystemLoader("templates"),
+                Loader:          templates.FileSystemLoader("../frontend/templates"),

[nodir, 2017/05/25 15:23:04]

when running a test, cwd is test location, and I assume this fixes usage of
GetTemplateBundle() in test. Note that it assumes that the test directory is a
sibling of frontend dir.
when running in prod, cwd is location of module yaml file. I am not sure current
directory will be called "frontend". Is it?

in general, personally i'd prefer tests in a package to test only code in that
package. That means buildbot package should check resp.MiloBuild that it
returns, not HTML. The frontend package should check that a valid resp.MiloBuild
provided by a test can be rendered by HTML templates properly. In that case you
don't have this problem, and your tests are more targeted (i.e. one bug causes
less tests to fail, not more)

[Ryan Tseng, 2017/05/26 18:10:52]

Acknowledged.  Yeah I think the main value of the template tests are probably to
see "does this template render without error?".  We can probably remove most of
the integration testing code without value loss.  That can be done in a separate
CL.

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

yeah, a separate CL is fine.

I don't think we pay close enough attention to the values in the expectation
files. I remember a couple of values in expectation files were incorrect. It is
hard to ensure they are correct because they have a lot of text. I think
targeted (=small) assertion tests that check the values would be more effective.

I definitely see the value of ensuring that a template renders without errors.
It does not require checking expectation files in though.

                 DebugMode:       info.IsDevAppServer,
                 DefaultTemplate: "base",
                 DefaultArgs: func(c context.Context) (templates.Args, error) {
                         r := getRequest(c)
                         path := r.URL.Path
                         loginURL, err := auth.LoginURL(c, path)
                         if err != nil {
                                 return nil, err
                         }
                         logoutURL, err := auth.LogoutURL(c, path)
@@ skipping 70 matching lines @@
         c.Context = WithRequest(c.Context, c.Request)
         next(c)
 }
 
 func getRequest(c context.Context) *http.Request {
         if req, ok := c.Value(&requestKey).(*http.Request); ok {
                 return req
         }
         panic("No http.request found in context")
 }