Move dummy service implementations into their own package.

Move dummy service implementations into their own package.

R=dnj@chromium.org, estaab@chromium.org, martiniss@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/83ce2b26d4c53c1fa88034d5f5b3728881bb8200

[estaab, 2015-07-13 18:26:35]

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/dummy/dummy.go (right):

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy.go:19: func ni() error {
Add a comment saying what this does since the acronym isn't obvious and the
implementation uses (admittedly cool) reflection magic.

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy.go:72: // RDS returns a dummy RawDatastore
implementation suitable for embedding.
Do we typically include file-level comments describing at a high level what's
included? Having this comment near the top explaining that this file just
contains panicking implementations for the 4 gae interfaces could save readers a
couple minutes.

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy.go:75: func RDS() gae.RawDatastore {
return dummyRDSInst }
Could we spell these public functions out, at least? It's a little hard to read
as-is.

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/dummy/dummy.infra_testing (right):

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy.infra_testing:2: "expected_coverage_min":
29,
Is this json or can we include comments? It would be nice to have a few words
justifying lower coverage if possible.

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/dummy/dummy_test.go (right):

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy_test.go:18: p := func() {
panic(recover().(error).Error()) }
For my own understanding what is this doing? Is this super obvious for people
familiar with go or should it have a more descriptive name?

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/mathrand_test.go (right):

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/mathrand_test.go:19: func TestMathRand(t *testing.T) {
I'm a little confused by this being here. This is just a test for the standard
math/rand module?

[iannucci, 2015-07-14 01:35:53]

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/dummy/dummy.go (right):

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy.go:19: func ni() error {
On 2015/07/13 18:26:35, estaab wrote:
> Add a comment saying what this does since the acronym isn't obvious and the
> implementation uses (admittedly cool) reflection magic.

Done.

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy.go:72: // RDS returns a dummy RawDatastore
implementation suitable for embedding.
On 2015/07/13 18:26:35, estaab wrote:
> Do we typically include file-level comments describing at a high level what's
> included? Having this comment near the top explaining that this file just
> contains panicking implementations for the 4 gae interfaces could save readers
a
> couple minutes.

Added a doc.go :)

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy.go:75: func RDS() gae.RawDatastore {
return dummyRDSInst }
On 2015/07/13 18:26:35, estaab wrote:
> Could we spell these public functions out, at least? It's a little hard to
read
> as-is.

This is mimicing the format of the gae package itself (e.g. gae.GetRDS), as well
as the 2/3 letter prefixes there. It gets very (very) verbose when spelling them
all out. Note that this function will only ever show up elsewhere as
`dummy.RDS()`, which is pretty clear (imho :)). If you feel strongly about it, I
can expand them (but I would be loathe to expand the ones in the gae package
since they get used everywhere in client code).

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/dummy/dummy.infra_testing (right):

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy.infra_testing:2: "expected_coverage_min":
29,
On 2015/07/13 18:26:35, estaab wrote:
> Is this json or can we include comments? It would be nice to have a few words
> justifying lower coverage if possible.

It's json. If you look at the html output, it's pretty obvious why it's not
close to 100% (basically, I didn't feel like copy+pasting a test method for
every single method, which are all exactly the same). Having tunable coverage
annotations would be great here.

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/dummy/dummy_test.go (right):

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/dummy/dummy_test.go:18: p := func() {
panic(recover().(error).Error()) }
On 2015/07/13 18:26:35, estaab wrote:
> For my own understanding what is this doing? Is this super obvious for people
> familiar with go or should it have a more descriptive name?

it's maybe not completely obvious. I've added a comment :)

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/mathrand_test.go (right):

https://codereview.chromium.org/1226063003/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/mathrand_test.go:19: func TestMathRand(t *testing.T) {
On 2015/07/13 18:26:35, estaab wrote:
> I'm a little confused by this being here. This is just a test for the standard
> math/rand module?

gae (for better or worse.. actually probably just worse), has a "MathRand"
service (like RawDatastore and the rest), and this is a test for that. It should
be moved to a different, separate package like
https://github.com/luci/luci-go/tree/master/common/clock . I've added a TODO.

[estaab, 2015-07-14 01:47:36]

lgtm

[iannucci, 2015-07-14 17:20:14]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-07-14 17:20:14]

The patchset sent to the CQ was uploaded after l-g-t-m from estaab@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1226063003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-14 17:20:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1226063003/60001

[commit-bot: I haz the power, 2015-07-14 17:20:27]

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

[commit-bot: I haz the power, 2015-07-14 17:20:28]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Vadim Sh., 2015-07-14 17:21:59]

lgtm

[iannucci, 2015-07-14 17:22:03]

On 2015/07/14 17:20:28, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Aww... anyone else want to take a stab at it?

[iannucci, 2015-07-14 17:22:11]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-07-14 17:22:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1226063003/60001

[commit-bot: I haz the power, 2015-07-14 17:24:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/83ce2b26d4c53c1fa88034d5f5b37...