Add gometalinter tests #96

ghost · 2017-02-07T22:03:03Z

Trying out some of the other tests we want in .taskcluster.yml. There will likely be more added to this PR shortly.

djmitche · 2017-02-08T13:50:36Z

# cd /go/src/github.com/taskcluster/taskcluster-cli; git pull --ff-only

I have no idea where that could have come from, though.. does gometalinter do a git pull??? @walac?

ghost · 2017-02-09T05:06:41Z

Yes, seems weird. I have some things I'm going to mess around with tomorrow to troubleshoot. This is my first time seeing this type of error though, and I have run many similar commands in task creator.

ghost · 2017-02-09T21:05:42Z

The problem was in trying to checkout and then 'go get -u ./...' in that order. Being on a detached head messed this up. So now we get all our packages first, then checkout, then do the tests.

This wasn't a problem before because I was testing on branches before PR #67 was merged and didn't need 'go get -u ./...' at all. But after this merge I was getting import errors. So, good problem to have now, as .taskcluster.yml will be more robust for future changes.

ghost · 2017-02-09T21:39:58Z

This has some extra test features, using both 'gometalinter' and adding to 'go test':

Gometalinter:
This can check for a variety of Go warning and errors. We've included:
gotype - Syntactic and semantic analysis similar to the Go compiler.
golint - Google's (mostly stylistic) linter.
deadcode - Finds unused code. -- Just realized I've got this one (ironically) enabled twice.
staticcheck - Statically detect bugs, both obvious and subtle ones.
misspell - Finds commonly misspelled English words.
go vet --shadow - Reports variables that may have been unintentionally shadowed
gosimple - Report simplifications in code.
(Taken from: https://github.com/alecthomas/gometalinter/blob/master/README.md)

Test flags:
-v (verbose): logs and prints all tests
-race: checks for race conditions

djmitche · 2017-02-10T14:28:02Z

.taskcluster.yml

+          && go get -u github.com/alecthomas/gometalinter 
+          && gometalinter --install
+          && go get -u ./...
+          && git checkout {{ event.head.sha }} 


These lines have me a little worried -- it's running go get -u on the default branch (the one go get -t github.com/taskcluster/taskcluster-cli/... found), then switching to the commit we want to test. If those have different dependent packages, would this still work?

@walac this is pretty far over my golang head -- can you comment here?

This is to make gometalinter happy, as it doesn't look for packages inside vendor. Unfortunately, there is no much more we can do about it, since if we try to do go get after git checkout, it will fail due to the repo being in a detached state.

Does that mean that we're running our tests against the latest versions of everything, rather than the vendored versions? Or is this just updating gometalinter's own dependencies?

Would it be possible to use gometalinter --vendor?

@djmitche nope, tests run under vendor, just gometalinter

@t0xicCode oh, I was not aware of vendor option, thanks. @lteigrob could you please give it a try?

Yep, I'll add in that flag right now.

t0xicCode

Looks good to me 🚢

@djmitche

Changing review based on @djmitche's comments

t0xicCode · 2017-02-10T14:40:43Z

.taskcluster.yml

+          && go test -v -race ./...
+          && gometalinter --disable-all --enable=gotype --enable=golint 
+          --enable=deadcode --enable=staticcheck --enable=deadcode 
+          --enable=misspell --enable=vet --enable=vetshadow --enable=gosimple


Also, this doesn't run on any of the sub-packages.

Shoot. Right you are.

t0xicCode

A few adjustments (see comments)

t0xicCode

A few more points :)

t0xicCode · 2017-02-10T19:14:13Z

.taskcluster.yml

-          && go test ./...
+          && go get -u github.com/alecthomas/gometalinter 
+          && gometalinter --install --force
+          && go get -u ./...


We shouldn't need this anymore.

You're probably right, but what about with the added race condition check?

go test if fully compatible with vendoring. It's kinda funny the way it does it. When the code is compiled to be tested, the toolchain will pull the dependencies from vendor/, as expected. Then, when go test runs the tests, it will actually recurse into the folders in vendor/, but because govendor removes all of the test files from dependencies, no tests are run.

In essence, it matches the behaviour of un-vendored code.

t0xicCode · 2017-02-10T19:15:36Z

.taskcluster.yml

+          && go get -u ./...
+          && git checkout {{ event.head.sha }} 
+          && go test -v -race ./...
+          && gometalinter --vendor --disable-all --enable=gotype --enable=golint 


But we might have to run go install ./... before this. gotype uses the compiled data that is generated by install to run its checks.

ghost · 2017-02-10T20:10:37Z

Perhaps for discussion in the meeting:

Now that I'm checking subdirectories with gometalinter, my task is failing, I believe due to warnings. You can see this here:
https://tools.taskcluster.net/task-inspector/#d8c67_q0SLCiR61kYcU1Kw/0
(Go to Runs->Run0 and then Logs, and scrolling down you can see warnings with the type given in parentheses on the right of each)
where this task is the same but I've just omitted the specific gometalinter tests giving the warnings, and it's fine:
https://tools.taskcluster.net/task-inspector/#WH-069lNSCqkXdvOJibOCQ/0

From what I can tell this is due to gometalinter setting the exit status to 1 for any such warning. Is this what we want? If so, we'd just have to go through and fix up all these warnings to get everything up to this level of strictness. Alternatively, we allow these warnings but figure out how to keep exit status as zero. I'm not sure there's a way to do this using gometalinter as-is.

I'm sure it will fail, but I'll push the change for discussion purposes :) I'd like to talk more about @t0xicCode's comments too

t0xicCode · 2017-02-10T20:15:56Z

I'm actually building a PR to fix most of the linting issues.

ghost · 2017-02-10T20:18:51Z

Oh, perfect!

…nting

ghost · 2017-02-13T18:28:52Z

Nice, I'll just figure out the go install vs. go get -u issue and we should be good

t0xicCode · 2017-02-13T21:15:45Z

@lteigrob Could you rebase on top of master? I've merged the linting fixes that you approved :)

ghost · 2017-02-13T22:15:28Z

One does not simply git rebase it... @t0xicCode, I also had to fetch and merge with the remote gometalinter branch. Not sure if that's normal. I've looked at the code and I don't think I killed anything.

t0xicCode · 2017-02-13T22:24:49Z

I may have managed to do it 😄 but I manually removed a bunch of duplicated commits.

t0xicCode

Single point

t0xicCode · 2017-02-13T22:26:27Z

.taskcluster.yml

+          && go get -u github.com/alecthomas/gometalinter 
+          && gometalinter --install --force
+          && go install ./...
+          && git checkout {{ event.head.sha }} 


You should checkout before running go install or make, in case the dependencies changed.

t0xicCode · 2017-02-13T22:28:48Z

Also, you'll have to do a force pull (git fetch && git reset --hard @{upstream}) before continuing.

djmitche · 2017-02-14T18:02:53Z

.taskcluster.yml

+          && go install ./...
+          && git checkout {{ event.head.sha }} 
+          && go test -v -race ./...
+          && gometalinter --vendor --disable-all --enable=gotype --enable=golint 


Consider --skip=apis/ to skip the auto-generated source (for #100)

ghost · 2017-02-17T03:39:32Z

Cool, this sequence seems to be working now. I'm sure @djmitche will be happy :)

Aaand I got excited and forgot to do the force pull :( If things need to be fixed we can discuss tomorrow.

Finally, I'm working on getting '--skip=apis' to work. Like everything else since adding in gometalinter, it's not as simple as it seems.

djmitche · 2017-02-17T16:11:04Z

Success on Taskcluster!! Should we merge this?

t0xicCode

This seems good. I'd still like to see the code rebased and the duplicate commits removed to keep history clean. @lteigrob you can run git rebase -i master to remove any of the duplicates and reorder the commits.

djmitche · 2017-02-17T16:26:25Z

I can just squash it when merging.. that'll be easier, I think.

t0xicCode · 2017-02-17T16:26:51Z

Yeah, that works too

…

On Feb 17, 2017 11:26 AM, "Dustin J. Mitchell" ***@***.***> wrote: I can just squash it when merging.. that'll be easier, I think. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#96 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNclUAKpxD7FicSyIb2r8AeFM-IAx25ks5rdcoxgaJpZM4L6GXn> .

lteigrob added 2 commits February 7, 2017 15:56

Add gometalinter tests

4068b56

Add go get -u command for imports

1ef929a

Rearrange to get packages before checkout

285a744

Add test flags

f7f8f43

djmitche reviewed Feb 10, 2017

View reviewed changes

t0xicCode previously approved these changes Feb 10, 2017

View reviewed changes

t0xicCode reviewed Feb 10, 2017

View reviewed changes

t0xicCode suggested changes Feb 10, 2017

View reviewed changes

Add vendor flag

377e217

t0xicCode reviewed Feb 10, 2017

View reviewed changes

Run gometalinter through subpackages

0a5d25f

lteigrob added 2 commits February 13, 2017 11:43

Merge remote-tracking branch 'origin/resolve-linting' into resolve-li…

1876d8f

…nting

Rebase for fixes, lose misspell check

4b4962a

Replace 'go get -u' with 'go install'

9fb7e95

lteigrob added 6 commits February 13, 2017 15:41

Add gometalinter tests

64b66ae

Add go get -u command for imports

8c7d19b

Rearrange to get packages before checkout

70822b6

Add test flags

8dea95e

Add vendor flag

653681b

Run gometalinter through subpackages

86b73c9

lteigrob added 3 commits February 13, 2017 15:41

Rebase for fixes, lose misspell check

7373414

Replace 'go get -u' with 'go install'

7266fa0

Merge remote-tracking branch 'origin/gometalinter' into gometalinter

fe4a30d

t0xicCode force-pushed the gometalinter branch from fe4a30d to 7266fa0 Compare February 13, 2017 22:23

t0xicCode reviewed Feb 13, 2017

View reviewed changes

djmitche reviewed Feb 14, 2017

View reviewed changes

Move checkout to earlier in sequence

5918fd5

Add flag to skip apis directory

cb0789e

t0xicCode approved these changes Feb 17, 2017

View reviewed changes

djmitche merged commit 711b3a2 into master Feb 17, 2017

ghost deleted the gometalinter branch February 17, 2017 18:57

ghost mentioned this pull request Feb 17, 2017

Round out taskclutster-github testing #92

Closed

Add gometalinter tests #96

Add gometalinter tests #96

Conversation

ghost commented Feb 7, 2017

djmitche commented Feb 8, 2017 • edited Loading

ghost commented Feb 9, 2017

ghost commented Feb 9, 2017

ghost commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0xicCode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0xicCode left a comment • edited Loading

Choose a reason for hiding this comment

t0xicCode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 10, 2017

t0xicCode commented Feb 10, 2017

ghost commented Feb 10, 2017

ghost commented Feb 13, 2017

t0xicCode commented Feb 13, 2017

ghost commented Feb 13, 2017

t0xicCode commented Feb 13, 2017

t0xicCode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0xicCode commented Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

ghost commented Feb 17, 2017

djmitche commented Feb 17, 2017

t0xicCode left a comment

Choose a reason for hiding this comment

djmitche commented Feb 17, 2017

t0xicCode commented Feb 17, 2017 via email

djmitche commented Feb 8, 2017 •

edited

Loading

t0xicCode left a comment •

edited

Loading

t0xicCode commented Feb 13, 2017 •

edited

Loading