Milo: Increase test coverage for appengine/buildbot

Milo: Increase test coverage for appengine/buildbot

Adds new tests and slight refactoring including:
* Move most of the html test data out of the buildbot module into the frontend module.  Because that code is only executed by the frontend test code, it makes more sense to put it there.  The intention is to do the same with the other modules.
* Because of the above, make the build implementation public.
* Add new test cases and test files.

This yields 81.1% test coverage for the buildbot module.

BUG=712431
Review-Url: https://codereview.chromium.org/2856273004
Committed: https://github.com/luci/luci-go/commit/23968256fadb58591bf2e8d1fce49eab04b721bb

[Ryan Tseng, 2017-05-04 01:22:23]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 01:22:32]

Dry run: 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, 2017-05-04 01:33:46]

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

[commit-bot: I haz the power, 2017-05-04 01:33:49]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/35e9deda64c5b010)

[Ryan Tseng, 2017-05-04 01:57:53]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 01:57:54]

Dry run: 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, 2017-05-04 02:10:23]

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

[commit-bot: I haz the power, 2017-05-04 02:10:24]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/35ea046bbed48810)

[Ryan Tseng, 2017-05-10 21:38:39]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 21:38:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hinoka, 2017-05-10 21:47:46]

Description was changed from

==========
Milo: Increase test coverage for appengine/buildbot

BUG=712431
==========

to

==========
Milo: Increase test coverage for appengine/buildbot

Adds new tests and slight refactoring including:
* Move most of the html test data out of the buildbot module into the frontend
module.  Because that code is only executed by the frontend test code, it makes
more sense to put it there.  The intention is to do the same with the other
modules.
* Because of the above, make the build implementation public.
* Add new test cases and test files.

This yields 81.1% test coverage for the buildbot module.

BUG=712431
==========

[hinoka, 2017-05-10 21:47:46]

hinoka@chromium.org changed reviewers:
+ nodir@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:51:19]

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

[commit-bot: I haz the power, 2017-05-10 21:51:20]

Dry run: Try jobs failed on following builders:
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/360d1e899cae8f10)

[Ryan Tseng, 2017-05-10 21:52:27]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 21:52:29]

Dry run: 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, 2017-05-10 21:59:19]

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

[commit-bot: I haz the power, 2017-05-10 21:59:19]

Dry run: This issue passed the CQ dry run.

[hinoka, 2017-05-10 22:38:55]

hinoka@chromium.org changed reviewers:
+ hinoka@chromium.org

[Ryan Tseng, 2017-05-16 23:46:00]

friendly bump

[nodir, 2017-05-18 03:17:15]

didn't finish reviewing yet

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/build_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/build_test.go:52: for _, tc := range TestCases {
why test cases where made public?

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/builder_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/builder_test.go:10: .
"github.com/smartystreets/goconvey/convey"
nit: blank line before dot imports

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/builder_test.go:20: // Seed a builder with 8 builds.
this is 20 builds, not 8?

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/builder_test.go:36: Number:      10,
are 10 builds with same number added intentionally?

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/builder_test.go:56: So(err.Error(), ShouldResemble,
"Cannot find builder not real in master fake.\nAvailable builders: \nfake")
this error message is hard to read. Please use %q instead of %s for
user-supplied data

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/buildinfo_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/buildinfo_test.go:137: Convey("Load an invalid build",
func() {
invalid or nonexisting? if the latter, the response code should be NotFound, not
InternalError.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/console_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/console_test.go:30: Revision: fmt.Sprintf("%d", i),
strconv.Itoa is probably simpler to use here

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/grpc_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/grpc_test.go:139: So(err, ShouldResemble,
grpc.Errorf(codes.Unauthenticated, "Unauthenticated request"))
why not NotFound?

i think you meant Forbidden. Otherwise, why does it return Unauthenticated? A
user may be well authenticated

[Ryan Tseng, 2017-05-18 23:51:02]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 23:51:11]

Dry run: 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, 2017-05-18 23:56:21]

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

[commit-bot: I haz the power, 2017-05-18 23:56:22]

Dry run: Try jobs failed on following builders:
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3636ca9b21edb410)
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3636ca9b17459010)

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

done reviewing

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/html_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/html_test.go:48: rc := request(c, map[string]string{})
passing nil map would be simpler and work too

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/html_test.go:81: rc := request(c, map[string]string{})
same

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/html_test.go:96: So(response.Code, ShouldEqual,
http.StatusInternalServerError)
i think "Failing" here actually means non-existent. In that case I think this
should be 404

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/common/m...
File milo/appengine/common/middleware.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/common/m...
milo/appengine/common/middleware.go:33: Loader:         
templates.FileSystemLoader("../frontend/templates"),
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-25 23:31:30]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 23:31:38]

Dry run: 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, 2017-05-25 23:36:07]

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

[commit-bot: I haz the power, 2017-05-25 23:36:08]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/365ac534725b9910)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/365ac5346e182f10)

[Ryan Tseng, 2017-05-25 23:37:38]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 23:37:47]

Dry run: 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, 2017-05-25 23:43:57]

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

[commit-bot: I haz the power, 2017-05-25 23:43:58]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-05-26 18:02:08]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 18:02:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Tseng, 2017-05-26 18:07:54]

The CQ bit was unchecked by hinoka@google.com

[Ryan Tseng, 2017-05-26 18:07:58]

The CQ bit was checked by hinoka@google.com

[commit-bot: I haz the power, 2017-05-26 18:08:03]

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, 2017-05-26 18:08:04]

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

[commit-bot: I haz the power, 2017-05-26 18:08:05]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

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

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/build_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/build_test.go:52: for _, tc := range TestCases {
On 2017/05/18 03:17:15, nodir wrote:
> why test cases where made public?

(Mentioned in the CL desc) html rendering tests have been moved into the
frontend module, but to share the test data, TestCases need to be public. 
TestCases is basically just a file listing of what is in the testdata folder.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/builder_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/builder_test.go:10: .
"github.com/smartystreets/goconvey/convey"
On 2017/05/18 03:17:15, nodir wrote:
> nit: blank line before dot imports

Done.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/builder_test.go:20: // Seed a builder with 8 builds.
On 2017/05/18 03:17:15, nodir wrote:
> this is 20 builds, not 8?

Done.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/builder_test.go:36: Number:      10,
On 2017/05/18 03:17:15, nodir wrote:
> are 10 builds with same number added intentionally?

Nope, this is in the wrong indentation level.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/builder_test.go:56: So(err.Error(), ShouldResemble,
"Cannot find builder not real in master fake.\nAvailable builders: \nfake")
On 2017/05/18 03:17:15, nodir wrote:
> this error message is hard to read. Please use %q instead of %s for
> user-supplied data

Done.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/buildinfo_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/buildinfo_test.go:137: Convey("Load an invalid build",
func() {
On 2017/05/18 03:17:15, nodir wrote:
> invalid or nonexisting? if the latter, the response code should be NotFound,
not
> InternalError.

Done.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/console_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/console_test.go:30: Revision: fmt.Sprintf("%d", i),
On 2017/05/18 03:17:15, nodir wrote:
> strconv.Itoa is probably simpler to use here

Done.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/grpc_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/grpc_test.go:139: So(err, ShouldResemble,
grpc.Errorf(codes.Unauthenticated, "Unauthenticated request"))
On 2017/05/18 03:17:15, nodir wrote:
> why not NotFound?
> 
> i think you meant Forbidden. Otherwise, why does it return Unauthenticated? A
> user may be well authenticated

This is WAI.  buildbot has a global namespace for masters so we can't return
403.  Essentially
Anon + not found = Unauth
Anon + forbidden = Unauth
Logged in + not found = Not found
Logged in + forbidden = Not found

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/html_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/html_test.go:48: rc := request(c, map[string]string{})
On 2017/05/25 15:23:04, nodir wrote:
> passing nil map would be simpler and work too

Done.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/html_test.go:81: rc := request(c, map[string]string{})
On 2017/05/25 15:23:04, nodir wrote:
> same

Done.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/html_test.go:96: So(response.Code, ShouldEqual,
http.StatusInternalServerError)
On 2017/05/25 15:23:04, nodir wrote:
> i think "Failing" here actually means non-existent. In that case I think this
> should be 404

Done.

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/common/m...
File milo/appengine/common/middleware.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/common/m...
milo/appengine/common/middleware.go:33: Loader:         
templates.FileSystemLoader("../frontend/templates"),
On 2017/05/25 15:23:04, nodir wrote:
> 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)

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:05]

lgtm

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
File milo/appengine/buildbot/grpc_test.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/buildbot...
milo/appengine/buildbot/grpc_test.go:139: So(err, ShouldResemble,
grpc.Errorf(codes.Unauthenticated, "Unauthenticated request"))
On 2017/05/26 18:10:52, Ryan Tseng wrote:
> On 2017/05/18 03:17:15, nodir wrote:
> > why not NotFound?
> > 
> > i think you meant Forbidden. Otherwise, why does it return Unauthenticated?
A
> > user may be well authenticated
> 
> This is WAI.  buildbot has a global namespace for masters so we can't return
> 403.  Essentially
> Anon + not found = Unauth
> Anon + forbidden = Unauth
> Logged in + not found = Not found
> Logged in + forbidden = Not found

I see. It's good

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/common/m...
File milo/appengine/common/middleware.go (right):

https://codereview.chromium.org/2856273004/diff/50001/milo/appengine/common/m...
milo/appengine/common/middleware.go:33: Loader:         
templates.FileSystemLoader("../frontend/templates"),
On 2017/05/26 18:10:52, Ryan Tseng wrote:
> On 2017/05/25 15:23:04, nodir wrote:
> > 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)
> 
> 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.

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.

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
File milo/appengine/buildbot/builder.go (right):

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/builder.go:163: return fmt.Sprintf("Cannot find builder
%q in master %q.\nAvailable builders: \n%q",
i think the list of builders should be %s, otherwise it will be `Available
builders: "builder1\nbuilder2\nbuilder3"

also trailing space after "builder:" seems unnecessary

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/builder.go:164: e.master, e.builder, avail)
swap e.master and e.builder, see order in the format string

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
File milo/appengine/buildbot/builder_test.go (right):

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/builder_test.go:61: So(err.Error(), ShouldResemble,
"Cannot find builder \"fake\" in master \"not real builder\".\nAvailable
builders: \n\"fake\"")
should be `Cannot find builder "not real builder" in master "fake"...`

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
File milo/appengine/buildbot/buildinfo.go (right):

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/buildinfo.go:76: 
nit: not sure why this blank line is here

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
File milo/appengine/buildbot/html.go (right):

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/html.go:86: if err != nil {
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:44:13]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 22:44:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Tseng, 2017-05-26 22:49:23]

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
File milo/appengine/buildbot/builder.go (right):

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/builder.go:163: return fmt.Sprintf("Cannot find builder
%q in master %q.\nAvailable builders: \n%q",
On 2017/05/26 18:33:04, nodir wrote:
> i think the list of builders should be %s, otherwise it will be `Available
> builders: "builder1\nbuilder2\nbuilder3"
> 
> also trailing space after "builder:" seems unnecessary

Done.

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/builder.go:164: e.master, e.builder, avail)
On 2017/05/26 18:33:04, nodir wrote:
> swap e.master and e.builder, see order in the format string

Shoot I'm blind, I even checked for this.

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
File milo/appengine/buildbot/builder_test.go (right):

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/builder_test.go:61: So(err.Error(), ShouldResemble,
"Cannot find builder \"fake\" in master \"not real builder\".\nAvailable
builders: \n\"fake\"")
On 2017/05/26 18:33:05, nodir wrote:
> should be `Cannot find builder "not real builder" in master "fake"...`

Done.

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
File milo/appengine/buildbot/buildinfo.go (right):

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/buildinfo.go:76: 
On 2017/05/26 18:33:05, nodir wrote:
> nit: not sure why this blank line is here

Done.

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
File milo/appengine/buildbot/html.go (right):

https://codereview.chromium.org/2856273004/diff/130001/milo/appengine/buildbo...
milo/appengine/buildbot/html.go:86: if err != nil {
On 2017/05/26 18:33:05, nodir wrote:
> 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

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

[commit-bot: I haz the power, 2017-05-26 22:50:05]

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

[commit-bot: I haz the power, 2017-05-26 22:50:06]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-05-26 23:40:35]

The CQ bit was checked by hinoka@google.com

[Ryan Tseng, 2017-05-26 23:40:35]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/2856273004/#ps150001
(title: "review")

[commit-bot: I haz the power, 2017-05-26 23:40:43]

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, 2017-05-26 23:45:37]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1495842035492470,
"parent_rev": "6e3d45742db9d47fb9ab3b5f61f4dd6c0f532a1d", "commit_rev":
"23968256fadb58591bf2e8d1fce49eab04b721bb"}

[commit-bot: I haz the power, 2017-05-26 23:45:40]

Description was changed from

==========
Milo: Increase test coverage for appengine/buildbot

Adds new tests and slight refactoring including:
* Move most of the html test data out of the buildbot module into the frontend
module.  Because that code is only executed by the frontend test code, it makes
more sense to put it there.  The intention is to do the same with the other
modules.
* Because of the above, make the build implementation public.
* Add new test cases and test files.

This yields 81.1% test coverage for the buildbot module.

BUG=712431
==========

to

==========
Milo: Increase test coverage for appengine/buildbot

Adds new tests and slight refactoring including:
* Move most of the html test data out of the buildbot module into the frontend
module.  Because that code is only executed by the frontend test code, it makes
more sense to put it there.  The intention is to do the same with the other
modules.
* Because of the above, make the build implementation public.
* Add new test cases and test files.

This yields 81.1% test coverage for the buildbot module.

BUG=712431

Review-Url: https://codereview.chromium.org/2856273004
Committed:
https://github.com/luci/luci-go/commit/23968256fadb58591bf2e8d1fce49eab04b721bb
==========

[commit-bot: I haz the power, 2017-05-26 23:45:42]

Committed patchset #9 (id:150001) as
https://github.com/luci/luci-go/commit/23968256fadb58591bf2e8d1fce49eab04b721bb