review: tests, filecache, and minor changes

Author: ldez

pkg/commands/run.go

@@ -359,7 +359,7 @@ func (e *Executor) runAnalysis(ctx context.Context, args []string) ([]result.Iss
 	lintCtx.Log = e.log.Child(logutils.DebugKeyLintersContext)
 
 	runner, err := lint.NewRunner(e.cfg, e.log.Child(logutils.DebugKeyRunner),
-		e.goenv, e.EnabledLintersSet, e.lineCache, e.DBManager, lintCtx.Packages)
+		e.goenv, e.EnabledLintersSet, e.lineCache, e.fileCache, e.DBManager, lintCtx.Packages)
 	if err != nil {
 		return nil, err
 	}

pkg/fsutils/linecache.go

@@ -19,8 +19,6 @@ func NewLineCache(fc *FileCache) *LineCache {
 	}
 }
 
-func (lc *LineCache) GetFileCache() *FileCache { return lc.fileCache }
-
 // GetLine returns the index1-th (1-based index) line from the file on filePath
 func (lc *LineCache) GetLine(filePath string, index1 int) (string, error) {
 	if index1 == 0 { // some linters, e.g. gosec can do it: it really means first line

pkg/lint/runner.go

@@ -27,8 +27,10 @@ type Runner struct {
 	Log        logutils.Log
 }
 
-func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lintersdb.EnabledSet,
-	lineCache *fsutils.LineCache, dbManager *lintersdb.Manager, pkgs []*gopackages.Package) (*Runner, error) {
+func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env,
+	es *lintersdb.EnabledSet,
+	lineCache *fsutils.LineCache, fileCache *fsutils.FileCache,
+	dbManager *lintersdb.Manager, pkgs []*gopackages.Package) (*Runner, error) {
 	// Beware that some processors need to add the path prefix when working with paths
 	// because they get invoked before the path prefixer (exclude and severity rules)
 	// or process other paths (skip files).
@@ -98,8 +100,10 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lint
 			processors.NewSourceCode(lineCache, log.Child(logutils.DebugKeySourceCode)),
 			processors.NewPathShortener(),
 			getSeverityRulesProcessor(&cfg.Severity, log, files),
+
 			// The fixer still needs to see paths for the issues that are relative to the current directory.
-			processors.NewFixer(cfg, log, lineCache.GetFileCache()),
+			processors.NewFixer(cfg, log, fileCache),
+
 			// Now we can modify the issues for output.
 			processors.NewPathPrefixer(cfg.Output.PathPrefix),
 			processors.NewSortResults(cfg),

pkg/result/processors/fixer.go

@@ -16,6 +16,8 @@ import (
 	"github.com/golangci/golangci-lint/pkg/timeutils"
 )
 
+var _ Processor = Fixer{}
+
 type Fixer struct {
 	cfg       *config.Config
 	log       logutils.Log
@@ -76,8 +78,6 @@ func (f Fixer) Name() string {
 
 func (f Fixer) Finish() {}
 
-var _ Processor = Fixer{}
-
 func (f Fixer) fixIssuesInFile(filePath string, issues []result.Issue) error {
 	// TODO: don't read the whole file into memory: read line by line;
 	// can't just use bufio.scanner: it has a line length limit

test/fix_test.go

@@ -4,7 +4,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"sort"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -15,7 +14,9 @@ import (
 // value: "1"
 const envKeepTempFiles = "GL_KEEP_TEMP_FILES"
 
-func TestFix(t *testing.T) {
+func setupTestFix(t *testing.T) []string {
+	t.Helper()
+
 	testshared.SkipOnWindows(t)
 
 	tmpDir := filepath.Join(testdataDir, "fix.tmp")
@@ -34,44 +35,67 @@ func TestFix(t *testing.T) {
 
 	testshared.InstallGolangciLint(t)
 
-	sources := findSources(t, tmpDir, "in", "*.go")
+	return findSources(t, tmpDir, "in", "*.go")
+}
 
-	// The combination with --path gets tested for the first test case.
-	// This is arbitrary. It could also be tested for all of them,
-	// but then each test would have to run twice (with and without).
-	// To make this determinstic, the sources get sorted by name.
-	sort.Strings(sources)
+func TestFix(t *testing.T) {
+	sources := setupTestFix(t)
 
-	for i, input := range sources {
-		withPathPrefix := i == 0
+	for _, input := range sources {
 		input := input
 		t.Run(filepath.Base(input), func(t *testing.T) {
 			t.Parallel()
 
 			rc := testshared.ParseTestDirectives(t, input)
 			if rc == nil {
-				if withPathPrefix {
-					t.Errorf("The testcase %s should not get skipped, it's used for testing --path.", input)
-					return
-				}
 				t.Logf("Skipped: %s", input)
 				return
 			}
 
-			args := []string{
-				"--disable-all",
-				"--print-issued-lines=false",
-				"--print-linter-name=false",
-				"--out-format=line-number",
-				"--fix",
-			}
-			if withPathPrefix {
-				t.Log("Testing with --path-prefix.")
-				// This must not break fixing...
-				args = append(args, "--path-prefix=foobar/")
+			testshared.NewRunnerBuilder(t).
+				WithArgs("--disable-all",
+					"--print-issued-lines=false",
+					"--print-linter-name=false",
+					"--out-format=line-number",
+					"--fix").
+				WithRunContext(rc).
+				WithTargetPath(input).
+				Runner().
+				Run().
+				ExpectExitCode(rc.ExitCode)
+
+			output, err := os.ReadFile(input)
+			require.NoError(t, err)
+
+			expectedOutput, err := os.ReadFile(filepath.Join(testdataDir, "fix", "out", filepath.Base(input)))
+			require.NoError(t, err)
+
+			require.Equal(t, string(expectedOutput), string(output))
+		})
+	}
+}
+
+func TestFix_pathPrefix(t *testing.T) {
+	sources := setupTestFix(t)
+
+	for _, input := range sources {
+		input := input
+		t.Run(filepath.Base(input), func(t *testing.T) {
+			t.Parallel()
+
+			rc := testshared.ParseTestDirectives(t, input)
+			if rc == nil {
+				t.Logf("Skipped: %s", input)
+				return
 			}
+
 			testshared.NewRunnerBuilder(t).
-				WithArgs(args...).
+				WithArgs("--disable-all",
+					"--print-issued-lines=false",
+					"--print-linter-name=false",
+					"--out-format=line-number",
+					"--fix",
+					"--path-prefix=foobar/").
 				WithRunContext(rc).
 				WithTargetPath(input).
 				Runner().