Add new tests to presubmit, fix errors

Add new tests to presubmit, fix errors

BUG=skia:3189
R=jcgregorio@google.com,rmistry@google.com

Committed: https://skia.googlesource.com/buildbot/+/ebe642c9cfa3b295f359f966e1124885fc8e3580

[borenet, 2014-12-05 15:39:28]

borenet@google.com changed reviewers:
+ jcgregorio@google.com, rmistry@google.com

[borenet, 2014-12-05 15:39:28]

I wanted to add errcheck as well, but there are LOTS of errors and they aren't
trivial to fix in general.  Should I...

A. Add it and spend all day fixing errors
B. Add it and let the presubmit be broken until we all fix our respective errors
C. Don't add it and ask really nicely for everyone to fix their own errors

?

I'm not particularly inclined to do A, and I suspect it won't get done with C.

https://codereview.chromium.org/777413002/diff/1/go/gitinfo/gitinfo.go
File go/gitinfo/gitinfo.go (right):

https://codereview.chromium.org/777413002/diff/1/go/gitinfo/gitinfo.go#newcod...
go/gitinfo/gitinfo.go:178: func (g *GitInfo) Log(begin, end string) (string,
error) {
"go vet" wants you to use a pointer receiver for structs containing a sync.Lock,
even if the lock isn't used in the method...

[jcgregorio, 2014-12-05 15:47:05]

On 2014/12/05 at 15:39:28, borenet wrote:
> I wanted to add errcheck as well, but there are LOTS of errors and they aren't
trivial to fix in general.  Should I...
> 
> A. Add it and spend all day fixing errors
> B. Add it and let the presubmit be broken until we all fix our respective
errors
> C. Don't add it and ask really nicely for everyone to fix their own errors
> 
> ?
> 
> I'm not particularly inclined to do A, and I suspect it won't get done with C.

Heather was talking about having a 'fix it' day soon. Maybe log it as a bug and
we can all fix the errcheck errors then?

> 
> https://codereview.chromium.org/777413002/diff/1/go/gitinfo/gitinfo.go
> File go/gitinfo/gitinfo.go (right):
> 
>
https://codereview.chromium.org/777413002/diff/1/go/gitinfo/gitinfo.go#newcod...
> go/gitinfo/gitinfo.go:178: func (g *GitInfo) Log(begin, end string) (string,
error) {
> "go vet" wants you to use a pointer receiver for structs containing a
sync.Lock, even if the lock isn't used in the method...

[rmistry, 2014-12-05 15:49:15]

On 2014/12/05 15:47:05, jcgregorio wrote:
> On 2014/12/05 at 15:39:28, borenet wrote:
> > I wanted to add errcheck as well, but there are LOTS of errors and they
aren't
> trivial to fix in general.  Should I...
> > 
> > A. Add it and spend all day fixing errors
> > B. Add it and let the presubmit be broken until we all fix our respective
> errors
> > C. Don't add it and ask really nicely for everyone to fix their own errors
> > 
> > ?
> > 
> > I'm not particularly inclined to do A, and I suspect it won't get done with
C.
> 
> Heather was talking about having a 'fix it' day soon. Maybe log it as a bug
and
> we can all fix the errcheck errors then?

+1

Also, add 'ct' to GO_DIRS.

> 
> > 
> > https://codereview.chromium.org/777413002/diff/1/go/gitinfo/gitinfo.go
> > File go/gitinfo/gitinfo.go (right):
> > 
> >
>
https://codereview.chromium.org/777413002/diff/1/go/gitinfo/gitinfo.go#newcod...
> > go/gitinfo/gitinfo.go:178: func (g *GitInfo) Log(begin, end string) (string,
> error) {
> > "go vet" wants you to use a pointer receiver for structs containing a
> sync.Lock, even if the lock isn't used in the method...

[jcgregorio, 2014-12-05 15:57:33]

https://codereview.chromium.org/777413002/diff/1/run_unittests
File run_unittests (right):

https://codereview.chromium.org/777413002/diff/1/run_unittests#newcode75
run_unittests:75: # which contain .go files and run "go vet" on them.
Doesn't this work?

$ go vet ./...

[borenet, 2014-12-05 16:07:00]

Added ct directory, used "./..." instead of searching for packages.

Filed https://code.google.com/p/skia/issues/detail?id=3189 to track errcheck
issues and went ahead and uploaded the errcheck addition here:
https://codereview.chromium.org/785533002

https://codereview.chromium.org/777413002/diff/1/run_unittests
File run_unittests (right):

https://codereview.chromium.org/777413002/diff/1/run_unittests#newcode75
run_unittests:75: # which contain .go files and run "go vet" on them.
On 2014/12/05 15:57:33, jcgregorio wrote:
> Doesn't this work?
> 
> $ go vet ./... 

It sure does!

[jcgregorio, 2014-12-05 16:16:24]

lgtm

[tfarina, 2014-12-05 16:25:11]

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

[tfarina, 2014-12-05 16:25:12]

https://codereview.chromium.org/777413002/diff/60001/run_unittests
File run_unittests (right):

https://codereview.chromium.org/777413002/diff/60001/run_unittests#newcode19
run_unittests:19: 'ct',
maybe we should start sorting this alphabetical? Could you add bug_chomper and
webtry to this party?

[borenet, 2014-12-05 18:14:06]

PTAL at patch set 6.

https://codereview.chromium.org/777413002/diff/60001/run_unittests
File run_unittests (right):

https://codereview.chromium.org/777413002/diff/60001/run_unittests#newcode19
run_unittests:19: 'ct',
On 2014/12/05 16:25:12, tfarina wrote:
> maybe we should start sorting this alphabetical? Could you add bug_chomper and
> webtry to this party?

Alphabetized.  Now that goimports and "go vet" work in the root directory, the
only thing this list is used for is "make testgo", which neither webtry or
bug_chomper support.

[borenet, 2014-12-08 14:13:05]

Friendly ping.

[borenet, 2014-12-09 12:52:28]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-12-09 12:52:49]

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

[commit-bot: I haz the power, 2014-12-09 12:53:06]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/buildbot/+/ebe642c9cfa3b295f359f966e1124885fc8e...