Merge pull request cockroachdb#18496 from tamird/better-lints

build: TestProtoMarshal correctly avoids Marshal methods
petermattis · Sep 20, 2017 · cc2d642 · cc2d642
2 parents c09d543 + 25e73c8
commit cc2d642
Show file tree

Hide file tree

Showing 20 changed files with 176 additions and 77 deletions.
diff --git a/build/style_test.go b/build/style_test.go
@@ -129,38 +129,72 @@ func TestStyle(t *testing.T) {
 
 	t.Run("TestEnvutil", func(t *testing.T) {
 		t.Parallel()
-		cmd, stderr, filter, err := dirCmd(pkg.Dir, "git", "grep", "-nE", `os\.(Getenv|LookupEnv)`, "--", "*.go")
-		if err != nil {
-			t.Fatal(err)
-		}
+		for _, tc := range []struct {
+			re       string
+			excludes []string
+		}{
+			{re: `\bos\.(Getenv|LookupEnv)\("COCKROACH`},
+			{
+				re: `\bos\.(Getenv|LookupEnv)\(`,
+				excludes: []string{
+					":!acceptance",
+					":!build/style_test.go",
+					":!ccl/acceptanceccl/backup_test.go",
+					":!ccl/sqlccl/backup_cloud_test.go",
+					":!ccl/storageccl/export_storage_test.go",
+					":!cmd",
+					":!util/envutil/env.go",
+					":!util/log/clog.go",
+					":!util/log/color.go",
+					":!util/sdnotify/sdnotify_unix.go",
+				},
+			},
+		} {
+			cmd, stderr, filter, err := dirCmd(
+				pkg.Dir,
+				"git",
+				append([]string{
+					"grep",
+					"-nE",
+					tc.re,
+					"--",
+					"*.go",
+				}, tc.excludes...)...,
+			)
+			if err != nil {
+				t.Fatal(err)
+			}
 
-		if err := cmd.Start(); err != nil {
-			t.Fatal(err)
-		}
+			if err := cmd.Start(); err != nil {
+				t.Fatal(err)
+			}
 
-		if err := stream.ForEach(stream.Sequence(
-			filter,
-			stream.GrepNot(`^cmd(/.*)?/\w+\.go\b`),
-			stream.GrepNot(`^build/style_test\.go\b`),
-			stream.GrepNot(`^ccl/(sqlccl/backup_cloud|storageccl/export_storage|acceptanceccl/backup)_test\.go\b`),
-			stream.GrepNot(`^acceptance(/.*)?/\w+\.go\b`),
-			stream.GrepNot(`^util/(log|envutil|sdnotify)/\w+\.go\b`),
-		), func(s string) {
-			t.Errorf(`%s <- forbidden; use "envutil" instead`, s)
-		}); err != nil {
-			t.Error(err)
-		}
+			if err := stream.ForEach(filter, func(s string) {
+				t.Errorf(`%s <- forbidden; use "envutil" instead`, s)
+			}); err != nil {
+				t.Error(err)
+			}
 
-		if err := cmd.Wait(); err != nil {
-			if out := stderr.String(); len(out) > 0 {
-				t.Fatalf("err=%s, stderr=%s", err, out)
+			if err := cmd.Wait(); err != nil {
+				if out := stderr.String(); len(out) > 0 {
+					t.Fatalf("err=%s, stderr=%s", err, out)
+				}
 			}
 		}
 	})
 
 	t.Run("TestSyncutil", func(t *testing.T) {
 		t.Parallel()
-		cmd, stderr, filter, err := dirCmd(pkg.Dir, "git", "grep", "-nE", `sync\.(RW)?Mutex`, "--", "*.go")
+		cmd, stderr, filter, err := dirCmd(
+			pkg.Dir,
+			"git",
+			"grep",
+			"-nE",
+			`\bsync\.(RW)?Mutex`,
+			"--",
+			"*.go",
+			":!util/syncutil/mutex_sync.go",
+		)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -169,10 +203,7 @@ func TestStyle(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		if err := stream.ForEach(stream.Sequence(
-			filter,
-			stream.GrepNot(`^util/syncutil/mutex_sync\.go\b`),
-		), func(s string) {
+		if err := stream.ForEach(filter, func(s string) {
 			t.Errorf(`%s <- forbidden; use "syncutil.{,RW}Mutex" instead`, s)
 		}); err != nil {
 			t.Error(err)
@@ -187,6 +218,7 @@ func TestStyle(t *testing.T) {
 
 	t.Run("TestTodoStyle", func(t *testing.T) {
 		t.Parallel()
+		// TODO(tamird): enforce presence of name.
 		cmd, stderr, filter, err := dirCmd(pkg.Dir, "git", "grep", "-nE", `\sTODO\([^)]*\)[^:]`, "--", "*.go")
 		if err != nil {
 			t.Fatal(err)
@@ -211,7 +243,23 @@ func TestStyle(t *testing.T) {
 
 	t.Run("TestTimeutil", func(t *testing.T) {
 		t.Parallel()
-		cmd, stderr, filter, err := dirCmd(pkg.Dir, "git", "grep", "-nE", `time\.(Now|Since)`, "--", "*.go")
+		cmd, stderr, filter, err := dirCmd(
+			pkg.Dir,
+			"git",
+			"grep",
+			"-nE",
+			`\btime\.(Now|Since)`,
+			"--",
+			"*.go",
+			":!util/log/clog_test.go",
+			":!util/log/clog.go",
+			":!util/log/crash_reporting.go",
+			":!util/log/file_test.go",
+			":!util/timeutil/now_unix.go",
+			":!util/timeutil/now_windows.go",
+			":!util/tracing/tracer_span.go",
+			":!util/tracing/tracer.go",
+		)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -220,10 +268,7 @@ func TestStyle(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		if err := stream.ForEach(stream.Sequence(
-			filter,
-			stream.GrepNot(`^util/(log|syncutil|timeutil|tracing)/\w+\.go\b`),
-		), func(s string) {
+		if err := stream.ForEach(filter, func(s string) {
 			t.Errorf(`%s <- forbidden; use "timeutil" instead`, s)
 		}); err != nil {
 			t.Error(err)
@@ -238,7 +283,18 @@ func TestStyle(t *testing.T) {
 
 	t.Run("TestGrpc", func(t *testing.T) {
 		t.Parallel()
-		cmd, stderr, filter, err := dirCmd(pkg.Dir, "git", "grep", "-nE", `grpc.NewServer\([^)]*\)`, "--", "*.go")
+		cmd, stderr, filter, err := dirCmd(
+			pkg.Dir,
+			"git",
+			"grep",
+			"-nE",
+			`\bgrpc\.NewServer\(`,
+			"--",
+			"*.go",
+			":!rpc/context_test.go",
+			":!rpc/context.go",
+			":!util/grpcutil/grpc_util_test.go",
+		)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -247,11 +303,7 @@ func TestStyle(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		if err := stream.ForEach(stream.Sequence(
-			filter,
-			stream.GrepNot(`^rpc/context(_test)?\.go\b`),
-			stream.GrepNot(`^util/grpcutil/`),
-		), func(s string) {
+		if err := stream.ForEach(filter, func(s string) {
 			t.Errorf(`%s <- forbidden; use "rpc.NewServer" instead`, s)
 		}); err != nil {
 			t.Error(err)
@@ -266,7 +318,17 @@ func TestStyle(t *testing.T) {
 
 	t.Run("TestProtoClone", func(t *testing.T) {
 		t.Parallel()
-		cmd, stderr, filter, err := dirCmd(pkg.Dir, "git", "grep", "-nE", `\.Clone\([^)]+\)`, "--", "*.go")
+		cmd, stderr, filter, err := dirCmd(
+			pkg.Dir,
+			"git",
+			"grep",
+			"-nE",
+			`\.Clone\([^)]`,
+			"--",
+			"*.go",
+			":!util/protoutil/clone_test.go",
+			":!util/protoutil/clone.go",
+		)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -277,8 +339,7 @@ func TestStyle(t *testing.T) {
 
 		if err := stream.ForEach(stream.Sequence(
 			filter,
-			stream.GrepNot(`protoutil\.Clone\([^)]+\)`),
-			stream.GrepNot(`^util/protoutil/clone(_test)?\.go\b`),
+			stream.GrepNot(`protoutil\.Clone\(`),
 		), func(s string) {
 			t.Errorf(`%s <- forbidden; use "protoutil.Clone" instead`, s)
 		}); err != nil {
@@ -294,7 +355,17 @@ func TestStyle(t *testing.T) {
 
 	t.Run("TestProtoMarshal", func(t *testing.T) {
 		t.Parallel()
-		cmd, stderr, filter, err := dirCmd(pkg.Dir, "git", "grep", "-nE", `\.Marshal\([^)]+\)`, "--", "*.go")
+		cmd, stderr, filter, err := dirCmd(
+			pkg.Dir,
+			"git",
+			"grep",
+			"-nE",
+			`\.Marshal\(`,
+			"--",
+			"*.go",
+			":!util/protoutil/marshal.go",
+			":!settings/settings_test.go",
+		)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -305,8 +376,7 @@ func TestStyle(t *testing.T) {
 
 		if err := stream.ForEach(stream.Sequence(
 			filter,
-			stream.GrepNot(`(json|yaml|protoutil|Field)\.Marshal`),
-			stream.GrepNot(`^util/protoutil/marshal(_test)?\.go\b`),
+			stream.GrepNot(`(json|yaml|protoutil|\.Field)\.Marshal\(`),
 		), func(s string) {
 			t.Errorf(`%s <- forbidden; use "protoutil.Marshal" instead`, s)
 		}); err != nil {
@@ -322,7 +392,16 @@ func TestStyle(t *testing.T) {
 
 	t.Run("TestProtoUnmarshal", func(t *testing.T) {
 		t.Parallel()
-		cmd, stderr, filter, err := dirCmd(pkg.Dir, "git", "grep", "-nE", `\.Unmarshal\([^)]+\)`, "--", "*.go")
+		cmd, stderr, filter, err := dirCmd(
+			pkg.Dir,
+			"git",
+			"grep",
+			"-nE",
+			`\.Unmarshal\(`,
+			"--",
+			"*.go",
+			":!*.pb.go",
+		)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -333,8 +412,7 @@ func TestStyle(t *testing.T) {
 
 		if err := stream.ForEach(stream.Sequence(
 			filter,
-			stream.GrepNot(`(json|jsonpb|yaml|proto)\.Unmarshal`),
-			stream.GrepNot(`\.pb\.go\b`),
+			stream.GrepNot(`(json|jsonpb|yaml|proto)\.Unmarshal\(`),
 		), func(s string) {
 			t.Errorf(`%s <- forbidden; use "proto.Unmarshal" instead`, s)
 		}); err != nil {
@@ -649,6 +727,9 @@ func TestStyle(t *testing.T) {
 	// TODO(tamird): replace this with errcheck.NewChecker() when
 	// https://github.com/dominikh/go-tools/issues/57 is fixed.
 	t.Run("TestErrCheck", func(t *testing.T) {
+		if testing.Short() {
+			t.Skip("short flag")
+		}
 		// errcheck uses 1GB of ram (as of 2017-02-18), so don't parallelize it.
 		cmd, stderr, filter, err := dirCmd(
 			pkg.Dir,
@@ -679,6 +760,9 @@ func TestStyle(t *testing.T) {
 	})
 
 	t.Run("TestReturnCheck", func(t *testing.T) {
+		if testing.Short() {
+			t.Skip("short flag")
+		}
 		// returncheck uses 1GB of ram (as of 2017-02-18), so don't parallelize it.
 		cmd, stderr, filter, err := dirCmd(pkg.Dir, "returncheck", pkgScope)
 		if err != nil {

diff --git a/pkg/ccl/acceptanceccl/backup_test.go b/pkg/ccl/acceptanceccl/backup_test.go
@@ -30,6 +30,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl"
 	"github.com/cockroachdb/cockroach/pkg/sql/jobs"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -58,8 +59,8 @@ type benchmarkTest struct {
 }
 
 func (bt *benchmarkTest) Start(ctx context.Context) {
-	licenseKey := os.Getenv("COCKROACH_DEV_LICENSE")
-	if licenseKey == "" {
+	licenseKey, ok := envutil.EnvString("COCKROACH_DEV_LICENSE", 0)
+	if !ok {
 		bt.b.Fatal("testing enterprise features requires setting COCKROACH_DEV_LICENSE")
 	}
 	bt.f = acceptance.MakeFarmer(bt.b, bt.prefix, acceptance.GetStopper())

diff --git a/pkg/ccl/sqlccl/backup.go b/pkg/ccl/sqlccl/backup.go
@@ -36,6 +36,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/interval"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
@@ -315,7 +316,7 @@ func writeBackupDescriptor(
 ) error {
 	sort.Sort(backupFileDescriptors(desc.Files))
 
-	descBuf, err := desc.Marshal()
+	descBuf, err := protoutil.Marshal(desc)
 	if err != nil {
 		return err
 	}

diff --git a/pkg/ccl/sqlccl/csv.go b/pkg/ccl/sqlccl/csv.go
@@ -41,6 +41,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/pkg/errors"
@@ -658,7 +659,7 @@ func finalizeCSVBackup(
 		backupDesc.NodeID = execCfg.NodeID.Get()
 		backupDesc.ClusterID = execCfg.ClusterID()
 	}
-	descBuf, err := backupDesc.Marshal()
+	descBuf, err := protoutil.Marshal(backupDesc)
 	if err != nil {
 		return err
 	}

diff --git a/pkg/ccl/sqlccl/load.go b/pkg/ccl/sqlccl/load.go
@@ -32,6 +32,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/engine"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 )
 
 // Load converts r into SSTables and backup descriptors. database is the name
@@ -218,7 +219,7 @@ func Load(
 		}
 	}
 
-	descBuf, err := backup.Marshal()
+	descBuf, err := protoutil.Marshal(&backup)
 	if err != nil {
 		return BackupDescriptor{}, errors.Wrap(err, "marshal backup descriptor")
 	}

diff --git a/pkg/ccl/sqlccl/restore.go b/pkg/ccl/sqlccl/restore.go
@@ -35,6 +35,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/interval"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
@@ -757,7 +758,7 @@ func restore(
 	// Get TableRekeys to use when importing raw data.
 	var rekeys []roachpb.ImportRequest_TableRekey
 	for i := range tables {
-		newDescBytes, err := sqlbase.WrapDescriptor(tables[i]).Marshal()
+		newDescBytes, err := protoutil.Marshal(sqlbase.WrapDescriptor(tables[i]))
 		if err != nil {
 			return failed, errors.Wrap(err, "marshalling descriptor")
 		}

diff --git a/pkg/ccl/storageccl/bench_test.go b/pkg/ccl/storageccl/bench_test.go
@@ -26,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/engine"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 )
 
 func BenchmarkAddSSTable(b *testing.B) {
@@ -175,7 +176,7 @@ func BenchmarkImport(b *testing.B) {
 					oldStartKey = sqlbase.MakeIndexKeyPrefix(tableDesc, tableDesc.PrimaryIndex.ID)
 					newDesc := *tableDesc
 					newDesc.ID = id
-					newDescBytes, err := sqlbase.WrapDescriptor(&newDesc).Marshal()
+					newDescBytes, err := protoutil.Marshal(sqlbase.WrapDescriptor(&newDesc))
 					if err != nil {
 						panic(err)
 					}