Consider running golint in the pre-upload hook |
||||
Issue descriptiongolint 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).
,
Oct 12
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...
,
Oct 15
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.
,
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
,
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
,
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
,
Oct 19
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.
,
Oct 19
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.
,
Oct 19
Oh, I wasn't aware the issue. Thanks.
,
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
,
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
,
Oct 19
|
||||
►
Sign in to add a comment |
||||
Comment 1 by nya@chromium.org
, Oct 12