.tools: Add per-commit Go checks (vet, fmt, and lint) #182
Conversation
The previous version of this script would always pass this check, because when 'fail == 0' was true, all of the messages had passed. That lead to logic like: a. If all checks passed for the commit and the verbose flag is set, print all the messages (which all passed). b. If all checks passed for the commit and the verbose flag is not set, don't print details about any messages (which all passed). c. If any checks failed, print all the failed messages. That wasn't printing successful checks when the verbose flag was true but some checks failed. This commit shifts the logic around to: A. If the verbose flag is set, print messages for all checks, using TAP's "ok" and "not ok" to distinguish between per-check pass/fail [1]. B. If the verbose flag is not set, only print the failed messages (but still keep the "not ok" prefix). In the non-verbose case, then (b/c) and (B) look the same (modulo my "not ok" prefix adjustment). And in the verbose case, I think (A) is preferable to (a/c), because there's no reason to *not* print successful checks when you've enabled the verbose flag. [1]: http://testanything.org/ Signed-off-by: W. Trevor King <wking@tremily.us>
Using TAP [1] makes it easy to consume the output of this tool in a variety of existing harnesses [2]. Only the verbose results are TAP compliant, mostly because folks aren't likely to care about verbosity if they're just piping the results into a TAP harness. If we end up wanting the non-verbose output to be TAP compliant, we'll have to switch to the trailing count approach explained in the "Unknown amount and failures" section of the spec [1], and we'll have to keep a running counter of failed checks (instead of the current len(DefaultRules)-based approach). [1]: http://testanything.org/tap-version-13-specification.html [2]: http://testanything.org/consumers.html Signed-off-by: W. Trevor King <wking@tremily.us>
Instead of just checking the tip, run 'go vet ...' on each commit in the series. This makes sure we catch errors with earlier commits even if they were fixed by subsequent commits. The bulk of the complexity here is checking out a previous commit's tree to a temporary directory, which is handled by the new GitCheckoutTree. I'd expect Git to be able to checkout an arbitrary tree to an arbitrary directory, but strangely it doesn't seem to be able to do that without side effects involving HEAD or the index. Instead, I call 'git archive ...' and pipe the results into tar to do the unpacking. You *can* unpack an archive using native Go, but there didn't seem to be a convenient tar.Reader.Extract(directory string) method. Shelling out to tar seemed more robust than writing a few dozen lines of Go, so that's what I've done. I'd expect shelling out is less portable, but we really only need this to run on Travis' Ubuntu environment [1]. We can work out a more portable approach if/when we need to support this script on a system without a compatible 'tar' command. [1]: http://docs.travis-ci.com/user/ci-environment/#Virtualization-environments Signed-off-by: W. Trevor King <wking@tremily.us>
Instead of just checking the tip, run 'golint ./...' on each commit in the series. This makes sure we catch errors with earlier commits even if they were fixed by subsequent commits. I've added ValidateResult.Detail to hold stdout from ExecTree calls, because golint just prints warnings to its stdout and always returns an exit code of 0. Then in the golint rule, I check vr.Detail and fail any result with content there. In the main block, I print YAML blocks [1] with vr.Detail, so folks consuming the TAP can figure out what golint was complaining about. The YAML block gets printed when the verbose flag is set or when there's an error (otherwise it's hard to figure out what golint was complaining about in non-verbose mode). The YAML library ensures we don't get bitten by YAML-sensitive characters in the Detail string (e.g. colons). The gopkg.in location is a nifty site allowing us to link to a specific API of the project (which is hosted on GitHub) [2]. [1]: http://testanything.org/tap-version-13-specification.html [2]: http://godoc.org/gopkg.in/yaml.v2 Signed-off-by: W. Trevor King <wking@tremily.us>
Instead of just checking the tip, run 'go fmt ./...' on each commit in the series. This makes sure we catch errors with earlier commits even if they were fixed by subsequent commits. Signed-off-by: W. Trevor King <wking@tremily.us>
wking
force-pushed
the
per-commit-go-checks
branch
from
34b0ca8
to
404a639
Compare
Watch for accidentally-injected trailing whitespace. The stdout for
failing checks is a bit hard to read:
not ok 2 - git show --check 2652b57a368e8fdd6745f2354f25c56e57326b16
---
message: "commit 2652b57a368e8fdd6745f2354f25c56e57326b16\nAuthor: W. Trevor King
<wking@tremily.us>\nDate: Sat Sep 12 08:32:15 2015 -0700\n\n .tools: Run 'git
show --check ...' on each commit\n \n Watch for accidentally-injected trailing
whitespace. The stdout for\n failing checks is a bit hard to read:\n \n not
ok 2 - exit status 2\n ---\n message: \"commit 09748ebfd739dbcee35a585a75f896a74aa1c2f0\\nAuthor:
W. Trevor King\n <wking@tremily.us>\\nDate: Sat Sep 12 08:32:15 2015 -0700\\n\\n
\ .tools: Run 'git\n show --check ...' on each commit\\n \\n Watch
for accidentally-injected trailing\n whitespace.\\n \\n Signed-off-by:
W. Trevor King <wking@tremily.us>\\n\\n.tools/validate.go:56:\n trailing
whitespace.\\n+\\t\\t\\treturn vr \\n\"\n ...\n \n Signed-off-by: W.
Trevor King <wking@tremily.us>\n\n.tools/validate.go:57: trailing whitespace.\n+\t\t\treturn
vr \n"
...
But having the command in the "not ok" line should make it easy for
folks to reproduce locally if they're not collecting the YAML blocks
with a TAP harness.
Signed-off-by: W. Trevor King <wking@tremily.us>
Sometimes Travis gives test results for more commits than I expect it to. This should make it easy to figure out why, so we can distinguish between "Travis is giving us a surprisingly large range" and "validate.go has a buggy GitCommits". Signed-off-by: W. Trevor King <wking@tremily.us>
wking
force-pushed
the
per-commit-go-checks
branch
from
ab6516f
to
44582cf
Compare
|
I added a |
Work around travis-ci/travis-ci#4596 until that is fixed upstream [1]. This avoids pulling in commits from the base tip that aren't reachable from the head tip (e.g. if master has advanced since the PR branched off, and the PR is against master). We only want to check commits that are in the head branch but not in the base branch (more details on the range syntax in [2]). Once the Travis bug does get fixed, the shell replacement will be a no-op. So we don't have to worry about checks breaking once the bug gets fixed, and can periodically poll the bug and remove the workaround at out leisure after the fix. [1]: travis-ci/travis-ci#4596 [2]: http://git-scm.com/docs/gitrevisions#_specifying_ranges Signed-off-by: W. Trevor King <wking@tremily.us>
This is travis-ci/travis-ci#4596. Until that gets fixed, I've pushed a workaround with 8831b20. |
|
honestly. Let's keep the TAP out for now. |
|
On Mon, Oct 05, 2015 at 12:16:37PM -0700, Vincent Batts wrote:
Ok (although I don't see a need to invent a different output syntax). |
| return ExecTree(c, "go", "fmt", "./...") | ||
| }, | ||
| func(c CommitEntry) (vr ValidateResult) { | ||
| vr = ExecTree(c, os.ExpandEnv("$HOME/gopath/bin/golint"), "./...") |
There was a problem hiding this comment.
but this is what we're doing in travis, and seems cleaner there.
There was a problem hiding this comment.
On Mon, Oct 05, 2015 at 12:20:58PM -0700, Vincent Batts wrote:
vr = ExecTree(c, os.ExpandEnv("$HOME/gopath/bin/golint"), "./...")but this is what we're doing in travis, and seems cleaner there.
You're not doing it for every commit in Travis, so you'd miss cases
where one commit breaks the format and a subsequent commit fixed it.
There was a problem hiding this comment.
fair, but not completely a concern right now.
|
Honestly, that is not something I see a big need for a change and would be On Mon, Oct 5, 2015 at 3:20 PM, W. Trevor King notifications@github.com
|
|
On Mon, Oct 05, 2015 at 12:26:10PM -0700, Vincent Batts wrote:
It doesn't currently print pass/fail for the individual checks, it FAIL where a single commit has both passing and failing checks, and we |
|
This was moved to a separate project out of this repo. |
Instead of just checking the tip, run the checks on each commit in the
series. This makes sure we catch errors with earlier commits even if
they were fixed by subsequent commits.
There are some details about low-level choices in the commit messages
themselves, but nothing that seems generic enough to be worth copying
out to the PR level.
This PR is based on #174, because as the output gets more verbose
(e.g. YAML blocks for golint's stdout) you'll likely want to feed it
into a TAP harness for compaction. The non-verbose output is still
pretty short, although there are now more tests which you can fail ;).
If the protobuf branch lands (#179), we can swap out the Go checkers
and swap in protoc-gen-lint. I'm not aware of any protobuf
equivalent to 'go fmt' though, which is unfortunate.