Merge #74808

74808: dev: improve package argument handling r=postamar a=postamar This commit adds syntax and existence checks to handle bad package arguments in a more robust way. Fixes #73457. Release note: None Co-authored-by: Marius Posta <[email protected]>
cockroachdb · Jan 21, 2022 · 0a40796 · 0a40796
2 parents 6264a82 + a4da971
commit 0a40796
Show file tree

Hide file tree

Showing 8 changed files with 194 additions and 40 deletions.
diff --git a/pkg/cmd/dev/bench.go b/pkg/cmd/dev/bench.go
@@ -72,28 +72,26 @@ func (d *dev) bench(cmd *cobra.Command, commandLine []string) error {
 	}
 	benchesMap := make(map[string]bool)
 	for _, pkg := range pkgs {
-		pkg = strings.TrimPrefix(pkg, "//")
-		pkg = strings.TrimRight(pkg, "/")
-
-		if !strings.HasPrefix(pkg, "pkg/") {
-			return fmt.Errorf("malformed package %q, expecting %q", pkg, "pkg/{...}")
+		dir, isRecursive, tag, err := d.parsePkg(pkg)
+		if err != nil {
+			return err
 		}
-
-		if strings.HasSuffix(pkg, "/...") {
-			pkg = strings.TrimSuffix(pkg, "/...")
+		if isRecursive {
 			// Use `git grep` to find all Go files that contain benchmark tests.
-			out, err := d.exec.CommandContextSilent(ctx, "git", "grep", "-l", "^func Benchmark", "--", pkg+"/*_test.go")
+			out, err := d.exec.CommandContextSilent(ctx, "git", "grep", "-l", "^func Benchmark", "--", dir+"/*_test.go")
 			if err != nil {
 				return err
 			}
 			files := strings.Split(strings.TrimSpace(string(out)), "\n")
 			for _, file := range files {
-				dir, _ := filepath.Split(file)
+				dir, _ = filepath.Split(file)
 				dir = strings.TrimSuffix(dir, "/")
 				benchesMap[dir] = true
 			}
+		} else if tag != "" {
+			return fmt.Errorf("malformed package %q, tags not supported in 'bench' command", pkg)
 		} else {
-			benchesMap[pkg] = true
+			benchesMap[dir] = true
 		}
 	}
 	// De-duplicate and sort the list of benches to run.

diff --git a/pkg/cmd/dev/io/os/os.go b/pkg/cmd/dev/io/os/os.go
@@ -167,6 +167,26 @@ func (o *OS) Readlink(filename string) (string, error) {
 	return ret, err
 }
 
+// IsDir wraps around os.Stat, which returns the os.FileInfo of the named
+// directory. IsDir returns true if and only if it is an existing directory.
+// If there is an error, it will be of type *PathError.
+func (o *OS) IsDir(dirname string) (bool, error) {
+	command := fmt.Sprintf("find %s -type d", dirname)
+	o.logger.Print(command)
+
+	if o.Recording == nil {
+		// Do the real thing.
+		stat, err := os.Stat(dirname)
+		if err != nil {
+			return false, err
+		}
+		return stat.IsDir(), nil
+	}
+
+	res, err := o.replay(command)
+	return err == nil && res != "", err
+}
+
 // ReadFile wraps around ioutil.ReadFile, reading a file from disk and
 // returning the contents.
 func (o *OS) ReadFile(filename string) (string, error) {

diff --git a/pkg/cmd/dev/test.go b/pkg/cmd/dev/test.go
@@ -112,15 +112,12 @@ func (d *dev) test(cmd *cobra.Command, commandLine []string) error {
 
 	var testTargets []string
 	for _, pkg := range pkgs {
-		pkg = strings.TrimPrefix(pkg, "//")
-		pkg = strings.TrimPrefix(pkg, "./")
-		pkg = strings.TrimRight(pkg, "/")
-
-		if !strings.HasPrefix(pkg, "pkg/") {
-			return fmt.Errorf("malformed package %q, expecting %q", pkg, "pkg/{...}")
+		dir, isRecursive, tag, err := d.parsePkg(pkg)
+		if err != nil {
+			return err
 		}
-
-		if strings.HasSuffix(pkg, "/...") {
+		querySuffix := ""
+		if isRecursive {
 			// Similar to `go test`, we implement `...` expansion to allow
 			// callers to use the following pattern to test all packages under a
 			// named one:
@@ -145,26 +142,20 @@ func (d *dev) test(cmd *cobra.Command, commandLine []string) error {
 			// [1]: pkg/rpc/heartbeat.proto is one example of this pattern,
 			// where we define `Stringer` separately for the `RemoteOffset`
 			// type.
-			{
-				queryArgs := []string{fmt.Sprintf("kind(go_test,  //%s)", pkg)}
-				out, err := d.getQueryOutput(ctx, queryArgs...)
-				if err != nil {
-					return err
-				}
-				targets := strings.Split(strings.TrimSpace(string(out)), "\n")
-				testTargets = append(testTargets, targets...)
-			}
-		} else if strings.Contains(pkg, ":") {
-			testTargets = append(testTargets, pkg)
+			querySuffix = "/..."
 		} else {
-			queryArgs := []string{fmt.Sprintf("kind(go_test, //%s:all)", pkg)}
-			out, err := d.getQueryOutput(ctx, queryArgs...)
-			if err != nil {
-				return err
+			if tag == "" {
+				tag = "all"
 			}
-			tests := strings.Split(strings.TrimSpace(string(out)), "\n")
-			testTargets = append(testTargets, tests...)
+			querySuffix = ":" + tag
+		}
+		query := fmt.Sprintf("kind(go_test, //%s%s)", dir, querySuffix)
+		out, err := d.getQueryOutput(ctx, query)
+		if err != nil {
+			return err
 		}
+		tests := strings.Split(strings.TrimSpace(string(out)), "\n")
+		testTargets = append(testTargets, tests...)
 	}
 
 	args = append(args, testTargets...)

diff --git a/pkg/cmd/dev/testdata/bench.txt b/pkg/cmd/dev/testdata/bench.txt
@@ -1,9 +1,11 @@
 dev bench pkg/util/...
 ----
+find pkg/util -type d
 git grep -l '^func Benchmark' -- 'pkg/util/*_test.go'
 bazel run --config=test --test_sharding_strategy=disabled //pkg/util:util_test -- -test.run=- -test.bench=.
 bazel run --config=test --test_sharding_strategy=disabled //pkg/util/uuid:uuid_test -- -test.run=- -test.bench=.
 
 dev bench pkg/sql/parser --filter=BenchmarkParse
 ----
+find pkg/sql/parser -type d
 bazel run --config=test --test_sharding_strategy=disabled //pkg/sql/parser:parser_test -- -test.run=- -test.bench=BenchmarkParse
diff --git a/pkg/cmd/dev/testdata/recording/bench.txt b/pkg/cmd/dev/testdata/recording/bench.txt
@@ -1,3 +1,8 @@
+find pkg/util -type d
+----
+pkg/util
+pkg/util/uuid
+
 git grep -l '^func Benchmark' -- 'pkg/util/*_test.go'
 ----
 pkg/util/topk_test.go
@@ -11,5 +16,9 @@ bazel run --config=test --test_sharding_strategy=disabled //pkg/util:util_test -
 bazel run --config=test --test_sharding_strategy=disabled //pkg/util/uuid:uuid_test -- -test.run=- -test.bench=.
 ----
 
+find pkg/sql/parser -type d
+----
+pkg/sql/parser
+
 bazel run --config=test --test_sharding_strategy=disabled //pkg/sql/parser:parser_test -- -test.run=- -test.bench=BenchmarkParse
 ----
diff --git a/pkg/cmd/dev/testdata/recording/test.txt b/pkg/cmd/dev/testdata/recording/test.txt
@@ -1,3 +1,7 @@
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -11,7 +15,11 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
-bazel query 'kind(go_test,  //pkg/util/tracing/...)'
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
+bazel query 'kind(go_test, //pkg/util/tracing/...)'
 ----
 //pkg/util/tracing:tracing_test
 
@@ -24,6 +32,10 @@ Executed 0 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -37,6 +49,10 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -54,6 +70,10 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -67,6 +87,10 @@ Executed 0 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -80,6 +104,10 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -93,6 +121,10 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -106,6 +138,10 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -125,6 +161,10 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/testutils -type d
+----
+pkg/testutils
+
 bazel query 'kind(go_test, //pkg/testutils:all)'
 ----
 //pkg/testutils:testutils_test
@@ -154,6 +194,10 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/util/tracing -type d
+----
+pkg/util/tracing
+
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
@@ -167,6 +211,10 @@ Executed 1 out of 1 test: 1 test passes.
 ----
 ----
 
+find pkg/roachpb -type d
+----
+pkg/roachpb
+
 bazel query 'kind(go_test, //pkg/roachpb:all)'
 ----
 ----
@@ -178,8 +226,20 @@ bazel query 'kind(go_test, //pkg/roachpb:all)'
 bazel test //pkg/roachpb:roachpb_test //pkg/roachpb:string_test --test_env=GOTRACEBACK=all --test_output errors
 ----
 
-bazel test pkg/roachpb:string_test --test_env=GOTRACEBACK=all --test_output errors
+find pkg/roachpb -type d
+----
+pkg/roachpb
+
+bazel query 'kind(go_test, //pkg/roachpb:string_test)'
 ----
+//pkg/roachpb:string_test
+
+bazel test //pkg/roachpb:string_test --test_env=GOTRACEBACK=all --test_output errors
+----
+
+find pkg/testutils -type d
+----
+pkg/testutils
 
 bazel query 'kind(go_test, //pkg/testutils:all)'
 ----
@@ -192,10 +252,18 @@ go/src/github.com/cockroachdb/cockroach
 bazel test //pkg/testutils:testutils_test --test_env=GOTRACEBACK=all --test_env=COCKROACH_WORKSPACE=go/src/github.com/cockroachdb/cockroach --test_arg -rewrite --sandbox_writable_path=go/src/github.com/cockroachdb/cockroach/pkg/testutils --test_output errors
 ----
 
+find pkg/testutils -type d
+----
+pkg/testutils
+
 bazel query 'kind(go_test, //pkg/testutils:all)'
 ----
 //pkg/testutils:testutils_test
 
+find pkg/other/test -type d
+----
+pkg/other/test
+
 bazel query 'kind(go_test, //pkg/other/test:all)'
 ----
 //pkg/other/test:test_test