Repository: cockroachdb/pebble (go)

Issue:
metamorphic: error if unexpected flags are provided
There are metamorphic test flags that can only be used in conjunction with `--run/--compare`, and some flags that can only be used without `--run/--compare`. Currently, the ones that don't apply are being silently ignored. This has the potential to cause confusion. We should error out if any unexpected flags are provided.

Jira issue: PEBBLE-476

Pull request description:
metamorphic: error out if flags are used incorrectly
The metamorphic test has some flags specific to the "outer" Run, and
some flags specific to `--run-dir/--compare`. If a flag is passed that
doesn't apply to the mode, it is ignored.

In this change we instead error out the test. This will reduce sources
of confusion, especially when trying to run things manually.

Examples:
```
--- FAIL: TestMeta (0.00s)
    meta_test.go:96: flag "previous-ops" is not allowed with --run-dir

--- FAIL: TestMeta (0.00s)
    meta_test.go:118: flag "try-to-reduce" is only allowed with --compare or --run-dir
```

Fixes #4845

Patch:
diff --git a/internal/metamorphic/metaflags/meta_flags.go b/internal/metamorphic/metaflags/meta_flags.go
index 53997084..0568614a 100644
--- a/internal/metamorphic/metaflags/meta_flags.go
+++ b/internal/metamorphic/metaflags/meta_flags.go
@@ -134,23 +134,39 @@ type RunOnceFlags struct {
 
 func initRunOnceFlags(c *CommonFlags) *RunOnceFlags {
 	ro := &RunOnceFlags{CommonFlags: c}
-	flag.StringVar(&ro.RunDir, "run-dir", "",
+	ro.registerRunOnceFlags(flag.CommandLine)
+	return ro
+}
+
+func (ro *RunOnceFlags) registerRunOnceFlags(flagSet *flag.FlagSet) {
+	flagSet.StringVar(&ro.RunDir, "run-dir", "",
 		`directory containing the specific configuration to (re-)run (used for post-mortem debugging).
 Example: --run-dir _meta/231220-164251.3552792807512/random-003`)
 
-	flag.StringVar(&ro.Compare, "compare", "",
+	flagSet.StringVar(&ro.Compare, "compare", "",
 		`runs to compare, in the format _meta/test-root-dir/{run1,run2,...}. The result
 of each run is compared with the result of the first run.
 Example, --compare '_meta/231220-164251.3552792807512/{standard-000,random-025}'`)
 
-	flag.BoolVar(&ro.TryToReduce, "try-to-reduce", false,
+	flagSet.BoolVar(&ro.TryToReduce, "try-to-reduce", false,
 		`if set, we will try to reduce the number of operations that cause a failure. The
 verbose flag should be used with this flag.`)
 
-	flag.IntVar(&ro.ReduceAttempts, "reduce-attempts", 100,
+	flagSet.IntVar(&ro.ReduceAttempts, "reduce-attempts", 100,
 		`the number of attempts to reduce, for each probability; only used with --try-to-reduce.`)
+}
 
-	return ro
+// RunOnceOnlyFlagNames returns the names of the flags that are used only for
+// RunOnce/Compare.
+func RunOnceOnlyFlagNames() map[string]struct{} {
+	flagSet := flag.NewFlagSet("tmp", flag.ContinueOnError)
+	var ro RunOnceFlags
+	ro.registerRunOnceFlags(flagSet)
+	res := make(map[string]struct{})
+	flagSet.VisitAll(func(f *flag.Flag) {
+		res[f.Name] = struct{}{}
+	})
+	return res
 }
 
 // RunFlags contains flags that apply only to metamorphic.Run.
@@ -175,10 +191,15 @@ type RunFlags struct {
 
 func initRunFlags(c *CommonFlags) *RunFlags {
 	r := &RunFlags{CommonFlags: c}
-	flag.StringVar(&r.FS, "fs", "rand",
+	r.registerRunFlags(flag.CommandLine)
+	return r
+}
+
+func (r *RunFlags) registerRunFlags(flagSet *flag.FlagSet) {
+	flagSet.StringVar(&r.FS, "fs", "rand",
 		`force the tests to use either memory or disk-backed filesystems (valid: "mem", "disk", "rand")`)
 
-	flag.StringVar(&r.TraceFile, "trace-file", "",
+	flagSet.StringVar(&r.TraceFile, "trace-file", "",
 		"write an execution trace to `<run-dir>/file`")
 
 	ops := "uniform:5000-10000"
@@ -190,9 +211,9 @@ func initRunFlags(c *CommonFlags) *RunFlags {
 	if err := r.Ops.Set(ops); err != nil {
 		panic(err)
 	}
-	flag.Var(&r.Ops, "ops", "uniform:min-max")
+	flagSet.Var(&r.Ops, "ops", "uniform:min-max")
 
-	flag.StringVar(&r.InnerBinary, "inner-binary", "",
+	flagSet.StringVar(&r.InnerBinary, "inner-binary", "",
 		`binary to run for each instance of the test (this same binary by default); cannot be used
 with --run-dir or --compare`)
 
@@ -202,18 +223,30 @@ with --run-dir or --compare`)
 	// switches to the later Pebble SHA, setting the below options to point to
 	// the `ops` file and one of the previous run's data directories.
 
-	flag.StringVar(&r.PreviousOps, "previous-ops", "",
+	flagSet.StringVar(&r.PreviousOps, "previous-ops", "",
 		`paths to the ops files for the previous runs that resulted in the initial state
 (separated by commas); used to prepopulate the set of keys operations draw from.
 Must be used in conjunction with --initial-state.`)
 
-	flag.StringVar(&r.InitialStateDesc, "initial-state-desc", "",
+	flagSet.StringVar(&r.InitialStateDesc, "initial-state-desc", "",
 		`a human-readable description of the initial database state.
 If set this parameter is written to the OPTIONS to aid in
 debugging. It's intended to describe the lineage of a
 database's state, including sufficient information for
 reproduction (eg, SHA, prng seed, etc).`)
-	return r
+}
+
+// RunOnlyFlagNames returns the names of the flags that are used only for
+// metamorphic.Run.
+func RunOnlyFlagNames() map[string]struct{} {
+	flagSet := flag.NewFlagSet("tmp", flag.ContinueOnError)
+	var r RunFlags
+	r.registerRunFlags(flagSet)
+	res := make(map[string]struct{})
+	flagSet.VisitAll(func(f *flag.Flag) {
+		res[f.Name] = struct{}{}
+	})
+	return res
 }
 
 // InitRunOnceFlags initializes the flags that are used for a single run of the


Test patch:
diff --git a/internal/metamorphic/meta_test.go b/internal/metamorphic/meta_test.go
index 122fed3f..1728fee8 100644
--- a/internal/metamorphic/meta_test.go
+++ b/internal/metamorphic/meta_test.go
@@ -5,6 +5,7 @@
 package metamorphic
 
 import (
+	"flag"
 	"path/filepath"
 	"testing"
 
@@ -70,6 +71,13 @@ type option interface {
 func runTestMeta(t *testing.T, addtlOptions ...option) {
 	switch {
 	case runOnceFlags.Compare != "":
+		runOnlyFlags := metaflags.RunOnlyFlagNames()
+		flag.Visit(func(f *flag.Flag) {
+			if _, ok := runOnlyFlags[f.Name]; ok {
+				t.Fatalf("flag %q is not allowed with --compare", f.Name)
+			}
+		})
+
 		onceOpts := runOnceFlags.MakeRunOnceOptions()
 		for _, opt := range addtlOptions {
 			onceOpts = append(onceOpts, opt)
@@ -82,6 +90,13 @@ func runTestMeta(t *testing.T, addtlOptions ...option) {
 		metamorphic.Compare(t, testRootDir, runOnceFlags.Seed, runSubdirs, onceOpts...)
 
 	case runOnceFlags.RunDir != "":
+		runOnlyFlags := metaflags.RunOnlyFlagNames()
+		flag.Visit(func(f *flag.Flag) {
+			if _, ok := runOnlyFlags[f.Name]; ok {
+				t.Fatalf("flag %q is not allowed with --run-dir", f.Name)
+			}
+		})
+
 		// The --run-dir flag is specified either in the child process (see
 		// runOptions() below) or the user specified it manually in order to re-run
 		// a test.
@@ -97,6 +112,12 @@ func runTestMeta(t *testing.T, addtlOptions ...option) {
 			filepath.Join(runOnceFlags.RunDir, "history"), onceOpts...)
 
 	default:
+		runOnceOnlyFlags := metaflags.RunOnceOnlyFlagNames()
+		flag.Visit(func(f *flag.Flag) {
+			if _, ok := runOnceOnlyFlags[f.Name]; ok {
+				t.Fatalf("flag %q is only allowed with --compare or --run-dir", f.Name)
+			}
+		})
 		opts, err := runFlags.MakeRunOptions()
 		if err != nil {
 			t.Fatal(err)