Convert 'Testable' interface into an optional filter.

Convert 'Testable' interface into an optional filter.

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

Committed: https://chromium.googlesource.com/infra/infra/+/3cd6d14ddd093ea795b75b4c2ba78081cf56513f

[estaab, 2015-07-13 21:50:16]

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
File go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go (right):

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
What's our policy around having mixed case path names? It gives me an unsettling
feeling but if there's a precedent I'm fine with it. I see internally a lot of
teams use underscores in filenames but mixed case for the package name in the
code.

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go:5: package
featureBreaker
Why call the file brokenfeatures instead of featurebreaker?

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
File go/src/infra/gae/libs/gae/filters/featureBreaker/gi.go (right):

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/gi.go:15: *state
Is this just an untyped pointer?

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/gi.go:70: state :=
newState(defaultError)
Go noob question - where is newState defined and how would I go about finding
it?

[iannucci, 2015-07-13 22:59:22]

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
File go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go (right):

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/07/13 21:50:16, estaab wrote:
> What's our policy around having mixed case path names? It gives me an
unsettling
> feeling but if there's a precedent I'm fine with it. I see internally a lot of
> teams use underscores in filenames but mixed case for the package name in the
> code.

It's a go thing... golint apparently doesn't like underscores in package names.

/me just does what the nice computer says

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go:5: package
featureBreaker
On 2015/07/13 21:50:16, estaab wrote:
> Why call the file brokenfeatures instead of featurebreaker?

Historical silliness. I renamed the file.

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
File go/src/infra/gae/libs/gae/filters/featureBreaker/gi.go (right):

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/gi.go:15: *state
On 2015/07/13 21:50:16, estaab wrote:
> Is this just an untyped pointer?

No, it's a pointer to `type state struct`, but it's an 'embedded type'
(https://golang.org/doc/effective_go.html#embedding), so (*state)'s methods (in
this case `run`) are visible on (*giState) (which is why g.run works below.
Otherwise it would have to be g.s.run or something similar).

[iannucci, 2015-07-13 23:00:49]

Patchset #3 (id:40001) has been deleted

[dnj, 2015-07-13 23:42:36]

https://codereview.chromium.org/1227203004/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/filters/featureBreaker/featurebreaker.go (right):

https://codereview.chromium.org/1227203004/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/filters/featureBreaker/featurebreaker.go:97: defer
s.Unlock()
Either Unlock() here, or defer immediately after Lock(). I prefer the former, as
you don't need the lock after the check. The current form holds the lock through
"f()" if !ok.

For a more elegant expression, consider breaking this off into a separate
method:
func (s *state) getBroken(name string) (error, bool) {
  s.Lock()
  defer s.Unlock()

  return s.broken[name]
}

[iannucci, 2015-07-13 23:59:46]

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
File go/src/infra/gae/libs/gae/filters/featureBreaker/gi.go (right):

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/gi.go:70: state :=
newState(defaultError)
On 2015/07/13 21:50:16, estaab wrote:
> Go noob question - where is newState defined and how would I go about finding
> it?

personally I use github.com/nsf/gocode (plugged into vim using YouCompleteMe),
which has a go-to-definition functionality (so `gd` while cursor is on the
word). If you're using notepad.exe, then you'd have to look in *.go in this
package. Since it's lowercase, you know it's in THIS package somewhere. grep
would work for that.

https://codereview.chromium.org/1227203004/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/filters/featureBreaker/featurebreaker.go (right):

https://codereview.chromium.org/1227203004/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/filters/featureBreaker/featurebreaker.go:97: defer
s.Unlock()
On 2015/07/13 23:42:36, dnj wrote:
> Either Unlock() here, or defer immediately after Lock(). I prefer the former,
as
> you don't need the lock after the check. The current form holds the lock
through
> "f()" if !ok.
> 
> For a more elegant expression, consider breaking this off into a separate
> method:
> func (s *state) getBroken(name string) (error, bool) {
>   s.Lock()
>   defer s.Unlock()
> 
>   return s.broken[name]
> }

! you're right. I didn't want to have the defer in there at all, actually.
Fixed.

[estaab, 2015-07-14 00:49:22]

lgtm

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
File go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go (right):

https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/07/13 22:59:21, iannucci wrote:
> On 2015/07/13 21:50:16, estaab wrote:
> > What's our policy around having mixed case path names? It gives me an
> unsettling
> > feeling but if there's a precedent I'm fine with it. I see internally a lot
of
> > teams use underscores in filenames but mixed case for the package name in
the
> > code.
> 
> It's a go thing... golint apparently doesn't like underscores in package
names.
> 
> /me just does what the nice computer says

Oh I get it for the package name, I just mean the directory name.

I tried to see what our other go code does but it seems the issue was completely
avoided by just concatenating lowercase words. Well, as long as other people
don't make names like some/path/first_second for package firstSecond this seems
ok, I just want us to be consistent for discoverability.

[iannucci, 2015-07-14 01:31:27]

On 2015/07/14 00:49:22, estaab wrote:
> lgtm
> 
>
https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
> File go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go
(right):
> 
>
https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
> go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go:1: //
> Copyright 2015 The Chromium Authors. All rights reserved.
> On 2015/07/13 22:59:21, iannucci wrote:
> > On 2015/07/13 21:50:16, estaab wrote:
> > > What's our policy around having mixed case path names? It gives me an
> > unsettling
> > > feeling but if there's a precedent I'm fine with it. I see internally a
lot
> of
> > > teams use underscores in filenames but mixed case for the package name in
> the
> > > code.
> > 
> > It's a go thing... golint apparently doesn't like underscores in package
> names.
> > 
> > /me just does what the nice computer says
> 
> Oh I get it for the package name, I just mean the directory name.
> 
> I tried to see what our other go code does but it seems the issue was
completely
> avoided by just concatenating lowercase words. Well, as long as other people
> don't make names like some/path/first_second for package firstSecond this
seems
> ok, I just want us to be consistent for discoverability.

Ah, yes... I was implying that foldername should == packagename (which I guess
isn't enforced, it just makes things awkward when you import a package but then
the packagename is different than what you'd expect). We could make a ruling
against this, however, if lowercasing things is more important. ¯\_(ツ)_/¯

[estaab, 2015-07-14 01:40:55]

On 2015/07/14 01:31:27, iannucci wrote:
> On 2015/07/14 00:49:22, estaab wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
> > File go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go
> (right):
> > 
> >
>
https://codereview.chromium.org/1227203004/diff/1/go/src/infra/gae/libs/gae/f...
> > go/src/infra/gae/libs/gae/filters/featureBreaker/brokenfeatures.go:1: //
> > Copyright 2015 The Chromium Authors. All rights reserved.
> > On 2015/07/13 22:59:21, iannucci wrote:
> > > On 2015/07/13 21:50:16, estaab wrote:
> > > > What's our policy around having mixed case path names? It gives me an
> > > unsettling
> > > > feeling but if there's a precedent I'm fine with it. I see internally a
> lot
> > of
> > > > teams use underscores in filenames but mixed case for the package name
in
> > the
> > > > code.
> > > 
> > > It's a go thing... golint apparently doesn't like underscores in package
> > names.
> > > 
> > > /me just does what the nice computer says
> > 
> > Oh I get it for the package name, I just mean the directory name.
> > 
> > I tried to see what our other go code does but it seems the issue was
> completely
> > avoided by just concatenating lowercase words. Well, as long as other people
> > don't make names like some/path/first_second for package firstSecond this
> seems
> > ok, I just want us to be consistent for discoverability.
> 
> Ah, yes... I was implying that foldername should == packagename (which I guess
> isn't enforced, it just makes things awkward when you import a package but
then
> the packagename is different than what you'd expect). We could make a ruling
> against this, however, if lowercasing things is more important. ¯\_(ツ)_/¯

Oh wait, I found something in the effective go guide:
"By convention, packages are given lower case, single-word names; there should
be no need for underscores or mixedCaps."

So I think we want featurebreaker for the package name, which is consistent with
or other infra go code as well.

[dnj, 2015-07-14 02:09:37]

lgtm

[iannucci, 2015-07-17 00:48:50]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-07-17 00:48:51]

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

[commit-bot: I haz the power, 2015-07-17 00:49:04]

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

[commit-bot: I haz the power, 2015-07-17 00:52:49]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/infra/infra/+/3cd6d14ddd093ea795b75b4c2ba78...