New issue
Advanced search Search tips

Issue 894750 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Consider running golint in the pre-upload hook

Project Member Reported by nya@chromium.org, Oct 12

Issue description

golint is an official lint tool of Go.
https://github.com/golang/lint

It runs without setting up GOPATH, so it's a good candidate to run in the pre-upload hook.

I tried running it to tast-tests and got 98 warnings. Out of them, 68 were "exported X should have comment or be unexported" warning. Others were:

src/chromiumos/tast/local/bundles/cros/graphics/sshot/sshot.go:93:10: if block ends with a return statement, so drop this else and outdent its block
src/chromiumos/tast/local/bundles/cros/security/openfds/test.go:154:1: context.Context should be the first parameter of a function
src/chromiumos/tast/local/bundles/cros/security/selinux/coreutils.go:15:20: should omit type []string from declaration of var coreutilsFiles; it will be inferred from the right-hand side
src/chromiumos/tast/local/bundles/cros/ui/vkb/vkb.go:94:1: context.Context should be the first parameter of a function
src/chromiumos/tast/local/bundles/cros/vm/subtest/launch_terminal.go:22:8: const terminalUrlPrefix should be terminalURLPrefix
src/chromiumos/tast/local/bundles/cros/vm/subtest/linux_package_info.go:19:7: var packageId should be packageID
src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go:29:22: func parameter ownerId should be ownerID
src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go:29:40: func parameter appId should be appID
src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go:54:64: func parameter ownerId should be ownerID
src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go:54:82: func parameter appId should be appID
src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go:79:92: func parameter appId should be appID
src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go:136:94: func parameter appId should be appID
src/chromiumos/tast/local/chrome/chrome.go:63:38: exported func Auth returns unexported type chrome.option, which can be annoying to use
src/chromiumos/tast/local/chrome/chrome.go:73:23: exported func KeepCryptohome returns unexported type chrome.option, which can be annoying to use
src/chromiumos/tast/local/chrome/chrome.go:81:16: exported func NoLogin returns unexported type chrome.option, which can be annoying to use
src/chromiumos/tast/local/chrome/chrome.go:89:19: exported func ARCEnabled returns unexported type chrome.option, which can be annoying to use
src/chromiumos/tast/local/chrome/chrome.go:96:31: exported func ExtraArgs returns unexported type chrome.option, which can be annoying to use
src/chromiumos/tast/local/chrome/chrome.go:113:2: struct field testExtId should be testExtID
src/chromiumos/tast/local/chrome/extensions.go:58:6: func computeExtensionId should be computeExtensionID
src/chromiumos/tast/local/chrome/display/display.go:40:6: type name will be used as display.DisplayMode by other packages, and that stutters; consider calling this Mode
src/chromiumos/tast/local/chrome/display/display.go:102:6: type name will be used as display.DisplayProperties by other packages, and that stutters; consider calling this Properties
src/chromiumos/tast/local/graphics/deqp_parser.go:40:21: should omit type map[string]struct{} from declaration of var nonFailOutcomes; it will be inferred from the right-hand side
src/chromiumos/tast/local/perf/perf_test.go:18:6: func loadJson should be loadJSON
src/chromiumos/tast/local/session/session_manager.go:26:6: type name will be used as session.SessionManager by other packages, and that stutters; consider calling this Manager
src/chromiumos/tast/local/vm/container.go:124:1: error should be the last type when returning multiple items
src/chromiumos/tast/local/vm/container.go:124:84: method result packageId should be packageID
src/chromiumos/tast/local/vm/util.go:34:2: const terminaComponentLiveUrlFormat should be terminaComponentLiveURLFormat
src/chromiumos/tast/local/vm/util.go:35:2: const terminaComponentStagingUrlFormat should be terminaComponentStagingURLFormat
src/chromiumos/tast/local/vm/util.go:36:2: const terminaComponentUrlFormat should be terminaComponentURLFormat
src/chromiumos/tast/local/vm/util.go:58:1: comment on exported function CreateDefaultContainer should be of the form "CreateDefaultContainer ..."

Most of them make sense to me (with just an exception about session.SessionManager).

 
And fortunately, lint logic can be called as a Go function:
https://godoc.org/golang.org/x/lint

This means we can call it from tast-lint, allowing to check files modified by a commit only.

Interesting, I haven't used this before! Many of these seem very useful.

One concern is that it will probably warn about missing godoc comments on test functions, which is something that I've avoided pointing out in code reviews since it'd likely result in duplication with the Test.Desc field. Maybe we could exclude those warnings somehow...
Owner: nya@chromium.org
Status: Started (was: Available)
Yes, test function comment is a good point. I'll make sure to handle them specially.

We are getting more and more code reviews, so golint will save us some time. I'll try this.
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/741933cad7d0ee13096114caf393b71edc54d391

commit 741933cad7d0ee13096114caf393b71edc54d391
Author: Shuhei Takahashi <nya@chromium.org>
Date: Mon Oct 15 23:01:30 2018

dev-go/go-tools: Install astutil.

astutil is required by the latest golint.

BUG= chromium:894750 
TEST=emerge go-tools

Change-Id: I1e1542dcbae45b9295ac07047138767acb29dfef
Reviewed-on: https://chromium-review.googlesource.com/1280503
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/741933cad7d0ee13096114caf393b71edc54d391/dev-go/go-tools/go-tools-0.0.1.ebuild
[rename] https://crrev.com/741933cad7d0ee13096114caf393b71edc54d391/dev-go/go-tools/go-tools-0.0.1-r8.ebuild

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/e81c61cd34a5b3289de952671403b6686ccec842

commit e81c61cd34a5b3289de952671403b6686ccec842
Author: Shuhei Takahashi <nya@chromium.org>
Date: Mon Oct 15 23:01:30 2018

dev-go/golint: Install golang.org/x/lint.

Install Go package golang.org/x/lint. In addition, we need to
update the package to latest since the old version wrongly refers
github.com/golang/lint and thus the build fails.

BUG= chromium:894750 
TEST=emerge golint
TEST=equery f golint
CQ-DEPEND=I1e1542dcbae45b9295ac07047138767acb29dfef

Change-Id: I4c9f3fd72a3e2c56124b13554498168c1abf3536
Reviewed-on: https://chromium-review.googlesource.com/1280504
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/e81c61cd34a5b3289de952671403b6686ccec842/dev-go/golint/Manifest
[rename] https://crrev.com/e81c61cd34a5b3289de952671403b6686ccec842/dev-go/golint/golint-0.0.1-r6.ebuild
[modify] https://crrev.com/e81c61cd34a5b3289de952671403b6686ccec842/dev-go/golint/golint-0.0.1.ebuild

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/62c1f450149f8f88302b7e711784a787e9fa9da6

commit 62c1f450149f8f88302b7e711784a787e9fa9da6
Author: Shuhei Takahashi <nya@chromium.org>
Date: Fri Oct 19 02:01:27 2018

tast-cmd: Add dev-go/golint to DEPEND.

The library will be used in tast-lint.

BUG= chromium:894750 
TEST=cros_workon --host start tast-cmd; sudo emerge tast-cmd

Change-Id: Ib95413fa2739d35cec5e922f9dddaf4aad3368c8
Reviewed-on: https://chromium-review.googlesource.com/1287349
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/62c1f450149f8f88302b7e711784a787e9fa9da6/chromeos-base/tast-cmd/tast-cmd-9999.ebuild

Cc: keiichiw@chromium.org
How do you think about also adding "go vet"?
https://golang.org/cmd/vet/

It's executed during emerge.
https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1239657
But, we don't run emerge while modifying tast-tests. So, we notice this kind of mistakes only after trybot worked.
 Issue 888259  explains why running "go vet" from a pre-upload hook doesn't seem to be feasible. In short, it needs to run within the chroot.
Oh, I wasn't aware the issue. Thanks.
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/7d6f7efb83c41c8f355e341467cfff6ac9283cf3

commit 7d6f7efb83c41c8f355e341467cfff6ac9283cf3
Author: Shuhei Takahashi <nya@chromium.org>
Date: Fri Oct 19 19:19:37 2018

tast-tests: Fix golint errors.

BUG= chromium:894750 
TEST=fast_build.sh -T
TEST=tools/run_lint.sh **/*.go

Change-Id: I0bf861cb7531ce3c54115bbbf806f348f75e10a7
Reviewed-on: https://chromium-review.googlesource.com/1287449
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/security/selinux_process_context.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/security/selinux/file_label_utils.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/security/openfds/test.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/session/session_manager.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/vm/util.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/vm/container.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/debugd/debugd.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/vm/subtest/launch_terminal.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/chrome/extensions.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/chrome/extensions_test.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/security/open_fds.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/graphics/deqp_parser.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/vm/concierge.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/ui/vkb/vkb.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/graphics/sshot/sshot.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/perf/perf.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/perf/perf_test.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/chrome/chrome.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/ui/virtual_keyboard_typing.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/vm/subtest/linux_package_info.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/chrome/display/display.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/bundles/cros/security/selinux/coreutils.go
[modify] https://crrev.com/7d6f7efb83c41c8f355e341467cfff6ac9283cf3/src/chromiumos/tast/local/audio/cras.go

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/d66f23bb39290fd1416f577e35d69696bef283f1

commit d66f23bb39290fd1416f577e35d69696bef283f1
Author: Shuhei Takahashi <nya@chromium.org>
Date: Fri Oct 19 19:19:37 2018

tast-lint: Run Golint.

We run golint and take problems whose confidence is >=0.8.

We filter results in addition in some ways:
- Lines marked "// NOLINT" are ignored. This is because golint is
  actually designed to make "suggestions"; it sometimes gives
  false positive but does not provide a way to selectively ignore
  errors by itself. Nevertheless, we believe running golint in
  the pre-upload hook is worth doing.
- unexported-type-in-api is ignored.
- Test main functions are allowed to be exported without
  comments.

Also, -debug option is added to turn on verbose output. It can be
used to print the information required to add more filtering
in the case of false positives.

CQ-DEPEND=CL:1287349
CQ-DEPEND=CL:1287449
BUG= chromium:894750 
TEST=fast_build.sh -T
TEST=tools/run_lint.sh

Change-Id: Ic095877d0069ca917e9fb406b087cf458bcac78d
Reviewed-on: https://chromium-review.googlesource.com/1287469
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/d66f23bb39290fd1416f577e35d69696bef283f1/src/chromiumos/cmd/tast-lint/check/golint.go
[modify] https://crrev.com/d66f23bb39290fd1416f577e35d69696bef283f1/src/chromiumos/cmd/tast-lint/main.go
[add] https://crrev.com/d66f23bb39290fd1416f577e35d69696bef283f1/src/chromiumos/cmd/tast-lint/check/golint_test.go
[modify] https://crrev.com/d66f23bb39290fd1416f577e35d69696bef283f1/tools/run_lint.sh

Status: Fixed (was: Started)

Sign in to add a comment