Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG] automatic fixing not working #63

Closed
pohly opened this issue Feb 22, 2023 · 5 comments · Fixed by kubernetes/kubernetes#115953
Closed

[BUG] automatic fixing not working #63

pohly opened this issue Feb 22, 2023 · 5 comments · Fixed by kubernetes/kubernetes#115953
Assignees
@nunnatsa
Labels
bug Something isn't working

Comments

@pohly
Copy link

pohly commented Feb 22, 2023

Describe the bug

I tried ginkgolinter through golangci-lint v1.51.2 on the Kubernetes E2E test suite. When running with --fix, it reported an issue, but didn't fix it.

To Reproduce

  1. check out https://github.com/pohly/kubernetes/tree/lint-gomega
  2. patch test/e2e/storage/testsuites/subpath.go:
patch -p1 <<EOF
diff --git a/test/e2e/storage/testsuites/subpath.go b/test/e2e/storage/testsuites/subpath.go
index c8c2b787d81..e56948ce7d7 100644
--- a/test/e2e/storage/testsuites/subpath.go
+++ b/test/e2e/storage/testsuites/subpath.go
@@ -1007,7 +1007,7 @@ func testSubpathReconstruction(ctx context.Context, f *framework.Framework, host
                mountPointsAfter := storageutils.FindVolumeGlobalMountPoints(ctx, hostExec, podNode)
                s1 := mountPointsAfter.Difference(mountPoints)
                s2 := mountPoints.Difference(mountPointsAfter)
-               gomega.Expect(s1).To(gomega.BeEmpty(), "global mount points leaked: %v", s1)
+               gomega.Expect(len(s1)).To(gomega.Equal(0), "global mount points leaked: %v", s1)
                gomega.Expect(s2).To(gomega.BeEmpty(), "global mount points not found: %v", s2)
        }
 }

EOF
  1. hack/verify-golangci-lint.sh ./test/e2e/storage/testsuites

Once hack/verify-golangci-lint.sh has been used once, it is also possible to invoke golangci-lint manually:

$ _output/local/bin/golangci-lint run -v --fix ./test/e2e/storage/testsuites/
INFO [config_reader] Config search paths: [./ /nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites /nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage /nvme/gopath/src/k8s.io/kubernetes/test/e2e /nvme/gopath/src/k8s.io/kubernetes/test /nvme/gopath/src/k8s.io/kubernetes /nvme/gopath/src/k8s.io /nvme/gopath/src /nvme/gopath /nvme / /home/pohly] 
INFO [config_reader] Used config file .golangci.yaml 
INFO [lintersdb] Active 6 linters: [ginkgolinter gocritic ineffassign staticcheck stylecheck unused] 
INFO [loader] Go packages loading at mode 575 (compiled_files|deps|types_sizes|exports_file|files|imports|name) took 745.600412ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 10.598521ms 
INFO [linters_context/goanalysis] analyzers took 1m48.224534715s with top 10 stages: buildir: 1m42.856018911s, nilness: 3.116718852s, fact_purity: 828.873639ms, typedness: 527.014267ms, SA5012: 401.171985ms, ginkgolinter: 37.625298ms, directives: 31.602195ms, unused: 24.088056ms, SA4030: 21.908012ms, SA1012: 20.821358ms 
INFO [runner] Processors filtering stat (out/in): skip_files: 1/1, identifier_marker: 1/1, source_code: 1/1, path_prefixer: 1/1, sort_results: 1/1, filename_unadjuster: 1/1, autogenerated_exclude: 1/1, nolint: 1/1, uniq_by_line: 1/1, max_same_issues: 1/1, severity-rules: 1/1, cgo: 1/1, path_prettifier: 1/1, exclude: 1/1, skip_dirs: 1/1, exclude-rules: 1/1, diff: 1/1, max_per_file_from_linter: 1/1, max_from_linter: 1/1, path_shortener: 1/1 
INFO [runner] processing took 2.025624ms with stages: nolint: 1.712211ms, exclude-rules: 94.331µs, source_code: 76.895µs, identifier_marker: 49.305µs, autogenerated_exclude: 42.919µs, path_prettifier: 28.075µs, skip_files: 8.449µs, skip_dirs: 5.781µs, uniq_by_line: 1.739µs, max_from_linter: 1.209µs, cgo: 1.026µs, path_shortener: 945ns, filename_unadjuster: 856ns, max_same_issues: 634ns, exclude: 258ns, max_per_file_from_linter: 241ns, severity-rules: 212ns, path_prefixer: 198ns, diff: 187ns, sort_results: 153ns 
INFO [runner] linters took 13.03452066s with stages: goanalysis_metalinter: 13.032428247s 
INFO fixer took 0s with no stages                 
test/e2e/storage/testsuites/subpath.go:1010:3: ginkgo-linter: wrong length assertion; consider using `gomega.Expect(s1).To(gomega.BeEmpty(), "global mount points leaked: %v", s1)` instead (ginkgolinter)
		gomega.Expect(len(s1)).To(gomega.Equal(0), "global mount points leaked: %v", s1)
		^
INFO File cache stats: 1 entries of total size 37.9KiB 
INFO Memory: 137 samples, avg is 2056.4MB, max is 3978.6MB 
INFO Execution took 13.798730346s

Note that the issue gets reported, which implies that https://github.com/golangci/golangci-lint/blob/7ac42b0dde93912bd521d0e89ebef8c191b1d10b/pkg/result/processors/fixer.go#L41-L73 didn't fix it - it's indeed still present in the file.

Expected behavior

It should fix the file.

Environment:

  • OS: Linux
  • Version: golangci-lint v1.51.2, ginkgolinter v0.8.1

Additional context

After removing the logcheck plugin, it is possible to run under dlv:

diff --git a/.golangci.yaml b/.golangci.yaml
index 29de7f79162..2321cd93af5 100644
--- a/.golangci.yaml
+++ b/.golangci.yaml
@@ -23,18 +23,11 @@ linters:
     - ginkgolinter
     - gocritic
     - ineffassign
-    - logcheck
     - staticcheck
     - stylecheck
     - unused
 
 linters-settings: # please keep this alphabetized
-  custom:
-    logcheck:
-      # Installed there by hack/verify-golangci-lint.sh.
-      path: _output/local/bin/logcheck.so
-      description: structured logging checker
-      original-url: k8s.io/klog/hack/tools
   gocritic:
     enabled-checks:
       - equalFold
$ (cd hack/tools && dlv debug --wd /nvme/gopath/src/k8s.io/kubernetes github.com/golangci/golangci-lint/cmd/golangci-lint -- run --fix -v ./test/e2e/storage/testsuites )
(dlv) b fixer.go:41
...
(dlv) c
...
> github.com/golangci/golangci-lint/pkg/result/processors.Fixer.Process() /nvme/gopath/pkg/mod/github.com/golangci/[email protected]/pkg/result/processors/fixer.go:41 (hits goroutine(1):1 total:1) (PC: 0x1af5e32)
    36:	
    37:	func (f Fixer) printStat() {
    38:		f.sw.PrintStages()
    39:	}
    40:	
=>  41:	func (f Fixer) Process(issues []result.Issue) []result.Issue {
    42:		if !f.cfg.Issues.NeedFix {
    43:			return issues
    44:		}
    45:	
    46:		outIssues := make([]result.Issue, 0, len(issues))
...
> github.com/golangci/golangci-lint/pkg/result/processors.Fixer.Process() /nvme/gopath/pkg/mod/github.com/golangci/[email protected]/pkg/result/processors/fixer.go:51 (PC: 0x1af6236)
    46:		outIssues := make([]result.Issue, 0, len(issues))
    47:		issuesToFixPerFile := map[string][]result.Issue{}
    48:		for i := range issues {
    49:			issue := &issues[i]
    50:			if issue.Replacement == nil {
=>  51:				outIssues = append(outIssues, *issue)
    52:				continue
    53:			}
    54:	
    55:			issuesToFixPerFile[issue.FilePath()] = append(issuesToFixPerFile[issue.FilePath()], *issue)
    56:		}
(dlv) p issue
*github.com/golangci/golangci-lint/pkg/result.Issue {
	FromLinter: "ginkgolinter",
	Text: "ginkgo-linter: wrong length assertion; consider using `gomega.Expect(s1).To(gomega.BeEmpty(), \"global mount points leaked: %v\", s1)` instead",
	Severity: "",
	SourceLines: []string len: 1, cap: 1, [
		"\t\tgomega.Expect(len(s1)).To(gomega.Equal(0), \"global mount points leaked: %v\", s1)",
	],
	Replacement: *github.com/golangci/golangci-lint/pkg/result.Replacement nil,
	Pkg: *golang.org/x/tools/go/packages.Package {
		ID: "k8s.io/kubernetes/test/e2e/storage/testsuites",
		Name: "testsuites",
		PkgPath: "k8s.io/kubernetes/test/e2e/storage/testsuites",
		Errors: []golang.org/x/tools/go/packages.Error len: 0, cap: 0, nil,
		TypeErrors: []go/types.Error len: 0, cap: 0, nil,
		GoFiles: []string len: 19, cap: 32, [
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/base.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/capacity.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/disruptive.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/ephemeral.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/fsgroupchangepolicy.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/multivolume.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/provisioning.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/readwriteoncepod.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/snapshottable.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/snapshottable_stress.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/subpath.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/topology.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_expand.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_io.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_stress.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumelimits.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumemode.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumeperf.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumes.go",
		],
		CompiledGoFiles: []string len: 19, cap: 32, [
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/base.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/capacity.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/disruptive.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/ephemeral.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/fsgroupchangepolicy.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/multivolume.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/provisioning.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/readwriteoncepod.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/snapshottable.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/snapshottable_stress.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/subpath.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/topology.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_expand.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_io.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volume_stress.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumelimits.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumemode.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumeperf.go",
			"/nvme/gopath/src/k8s.io/kubernetes/test/e2e/storage/testsuites/volumes.go",
		],
		OtherFiles: []string len: 0, cap: 0, nil,
		EmbedFiles: []string len: 0, cap: 0, nil,
		EmbedPatterns: []string len: 0, cap: 0, nil,
		IgnoredFiles: []string len: 0, cap: 0, nil,
		ExportFile: "/home/pohly/.cache/go-build/9a/9aad1fdf1f98058a6de1e5d943ed660ef33ecc535d6e68664a073d95f053015b-d",
		Imports: map[string]*golang.org/x/tools/go/packages.Package [...],
		Types: *go/types.Package nil,
		Fset: *(*"go/token.FileSet")(0xc001530280),
		IllTyped: false,
		Syntax: []*go/ast.File len: 0, cap: 0, nil,
		TypesInfo: *go/types.Info nil,
		TypesSizes: go/types.Sizes nil,
		forTest: "",
		depsErrors: []*golang.org/x/tools/internal/packagesinternal.PackageError len: 0, cap: 0, nil,
		Module: *golang.org/x/tools/go/packages.Module nil,},
	LineRange: *github.com/golangci/golangci-lint/pkg/result.Range nil,
	Pos: go/token.Position {
		Filename: "test/e2e/storage/testsuites/subpath.go",
		Offset: 37472,
		Line: 1010,
		Column: 3,},
	HunkPos: 0,
	ExpectNoLint: false,
	ExpectedNoLintLinter: "",}

This shows that ginkgolinter didn't provide a replacement, and therefore fixing doesn't work.
@pohly pohly added the bug Something isn't working label Feb 22, 2023
@pohly
Copy link
Author

pohly commented Feb 22, 2023

The linter is correctly identifying 26 issues in k8s.io/kubernetes/test/e2e/...

That's a manageable number, so I can fix by hand. But being able to fix automatically would still be nicer 😄

@nunnatsa
Copy link
Owner

The auto fix does work with the ginkgolinter executable, using the -fix=true flag. I'll check why it's not working with golangci-lint.

@pohly pohly mentioned this issue Feb 22, 2023
Merged
@nunnatsa
Copy link
Owner

@pohly - I think it's a golangci-lint limitation. See here for example: golangci/golangci-lint#1779

The ginkgolinter does write SuggestedFixes and so it's working from the ginkgolinter executable. For now, golangci-lint dose not apply these suggested fixes, and it seems to be a work in progress.

Adding @ldez for reference.

@ldez
Copy link
Contributor

ldez commented Feb 22, 2023

Currently, we don't support SuggestedFixes but we have an internal way to handle fixes.

ex: https://github.com/golangci/golangci-lint/blob/610a2bd199a376bdbc37431ce2ba52d85f0ee307/pkg/golinters/godot.go#L93-L95

So it's not an issue with your linter.

@nunnatsa
Copy link
Owner

nunnatsa commented Feb 22, 2023 •
edited
Loading

Thanks @ldez . Closing the issue

@pohly pohly mentioned this issue Feb 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nunnatsa nunnatsa

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: fixing + linting gomega usage pohly/kubernetes
3 participants
@pohly @ldez @nunnatsa