Repository: Khan/genqlient
Pull request #274 (2023-05-07 20:19:27)

Add some more tests for config validation, and fix some gaps
I set these tests up in #270, but I realized there's a lot more we could test unrelated to that PR. In this commit I add some more tests. They're not really exhaustive yet, but they did catch a few bugs, which I fixed:
- we weren't validating the package-name if you do set it (only if we guess it)
- If you omitted `generated`, you would try to write generated code to the directory containing `genqlient.yaml`, which makes no sense; now we default to `generated.go`. In the real world it's probably good to set explicitly, but it's actually very convenient in tests that we don't have to, and maybe in small projects too.

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable (n/a)
- [x] Included documentation, for new features (n/a)
- [x] Added an entry to the changelog (n/a)



Patch:
diff --git a/docs/genqlient.yaml b/docs/genqlient.yaml
index 1281321..ddfb760 100644
--- a/docs/genqlient.yaml
+++ b/docs/genqlient.yaml
@@ -26,7 +26,7 @@ operations:
 - "pkg/*.go"
 
 # The filename to which to write the generated code, relative to
-# genqlient.yaml.
+# genqlient.yaml. Default: generated.go.
 generated: generated/genqlient.go
 
 # The package name for the output code; defaults to the directory name of
@@ -77,7 +77,6 @@ context_type: context.Context
 # without making a query.
 client_getter: "github.com/you/yourpkg.GetClient"
 
-
 # If set, fields with a struct type will default to having
 # the "pointer: true, omitempty: true" flag.
 #
diff --git a/generate/config.go b/generate/config.go
index 65bc53c..94ee12c 100644
--- a/generate/config.go
+++ b/generate/config.go
@@ -145,6 +145,9 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {
 	for i := range c.Operations {
 		c.Operations[i] = pathJoin(baseDir, c.Operations[i])
 	}
+	if c.Generated == "" {
+		c.Generated = "generated.go"
+	}
 	c.Generated = pathJoin(baseDir, c.Generated)
 	if c.ExportOperations != "" {
 		c.ExportOperations = pathJoin(baseDir, c.ExportOperations)
@@ -164,17 +167,23 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {
 			"\nExample: \"github.com/Org/Repo/optional.Value\"")
 	}
 
-	if c.Package == "" {
+	if c.Package != "" {
+		if !token.IsIdentifier(c.Package) {
+			// No need for link here -- if you're already setting the package
+			// you know where to set the package.
+			return errorf(nil, "invalid package in genqlient.yaml: '%v' is not a valid identifier", c.Package)
+		}
+	} else {
 		abs, err := filepath.Abs(c.Generated)
 		if err != nil {
-			return errorf(nil, "unable to guess package-name: %v is not a valid identifier"+
+			return errorf(nil, "unable to guess package-name: %v"+
 				"\nSet package name in genqlient.yaml"+
 				"\nExample: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6", err)
 		}
 
 		base := filepath.Base(filepath.Dir(abs))
 		if !token.IsIdentifier(base) {
-			return errorf(nil, "unable to guess package-name: %v is not a valid identifier"+
+			return errorf(nil, "unable to guess package-name: '%v' is not a valid identifier"+
 				"\nSet package name in genqlient.yaml"+
 				"\nExample: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6", base)
 		}


Test patch:
diff --git a/generate/config_test.go b/generate/config_test.go
index 5e0b53e..004c8f4 100644
--- a/generate/config_test.go
+++ b/generate/config_test.go
@@ -12,6 +12,7 @@ import (
 
 const (
 	findConfigDir    = "testdata/find-config"
+	validConfigDir   = "testdata/valid-config"
 	invalidConfigDir = "testdata/invalid-config"
 )
 
@@ -114,18 +115,40 @@ func TestAbsoluteAndRelativePathsInConfigFiles(t *testing.T) {
 	require.Equal(t, "/tmp/genqlient.graphql", config.Operations[0])
 }
 
-func TestInvalidConfigs(t *testing.T) {
-	files, err := os.ReadDir(invalidConfigDir)
+func testAllSnapshots(
+	t *testing.T,
+	dir string,
+	testfunc func(t *testing.T, filename string),
+) {
+	files, err := os.ReadDir(dir)
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	for _, file := range files {
-		t.Run(file.Name(), func(t *testing.T) {
-			filename := filepath.Join(invalidConfigDir, file.Name())
-			_, err := ReadAndValidateConfig(filename)
-			require.Error(t, err)
-			testutil.Cupaloy.SnapshotT(t, err.Error())
+		name := file.Name()
+		if name[0] == '.' {
+			continue // editor backup files, etc.
+		}
+		t.Run(name, func(t *testing.T) {
+			filename := filepath.Join(dir, file.Name())
+			testfunc(t, filename)
 		})
 	}
 }
+
+func TestValidConfigs(t *testing.T) {
+	testAllSnapshots(t, validConfigDir, func(t *testing.T, filename string) {
+		config, err := ReadAndValidateConfig(filename)
+		require.NoError(t, err)
+		testutil.Cupaloy.SnapshotT(t, config)
+	})
+}
+
+func TestInvalidConfigs(t *testing.T) {
+	testAllSnapshots(t, invalidConfigDir, func(t *testing.T, filename string) {
+		_, err := ReadAndValidateConfig(filename)
+		require.Error(t, err)
+		testutil.Cupaloy.SnapshotT(t, err.Error())
+	})
+}
diff --git a/generate/generate_test.go b/generate/generate_test.go
index 6baab56..62390eb 100644
--- a/generate/generate_test.go
+++ b/generate/generate_test.go
@@ -164,20 +164,16 @@ func TestGenerateWithConfig(t *testing.T) {
 			Package:   "mypkg",
 		}},
 		{"ExportOperations", "", nil, &Config{
-			Generated:        "generated.go",
 			ExportOperations: "operations.json",
 		}},
 		{"CustomContext", "", nil, &Config{
-			Generated:   "generated.go",
 			ContextType: "github.com/Khan/genqlient/internal/testutil.MyContext",
 		}},
 		{"CustomContextWithAlias", "", nil, &Config{
-			Generated:   "generated.go",
 			ContextType: "github.com/Khan/genqlient/internal/testutil/junk---fun.name.MyContext",
 		}},
 		{"StructReferences", "", []string{"InputObject.graphql", "QueryWithStructs.graphql"}, &Config{
 			StructReferences: true,
-			Generated:        "generated.go",
 			Bindings: map[string]*TypeBinding{
 				"Date": {
 					Type:        "time.Time",
@@ -189,7 +185,6 @@ func TestGenerateWithConfig(t *testing.T) {
 		{"StructReferencesAndOptionalPointer", "", []string{"InputObject.graphql", "QueryWithStructs.graphql"}, &Config{
 			StructReferences: true,
 			Optional:         "pointer",
-			Generated:        "generated.go",
 			Bindings: map[string]*TypeBinding{
 				"Date": {
 					Type:        "time.Time",
@@ -204,48 +199,38 @@ func TestGenerateWithConfig(t *testing.T) {
 			},
 		}},
 		{"NoContext", "", nil, &Config{
-			Generated:   "generated.go",
 			ContextType: "-",
 		}},
 		{"ClientGetter", "", nil, &Config{
-			Generated:    "generated.go",
 			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromContext",
 		}},
 		{"ClientGetterCustomContext", "", nil, &Config{
-			Generated:    "generated.go",
 			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromMyContext",
 			ContextType:  "github.com/Khan/genqlient/internal/testutil.MyContext",
 		}},
 		{"ClientGetterNoContext", "", nil, &Config{
-			Generated:    "generated.go",
 			ClientGetter: "github.com/Khan/genqlient/internal/testutil.GetClientFromNowhere",
 			ContextType:  "-",
 		}},
 		{"Extensions", "", nil, &Config{
-			Generated:  "generated.go",
 			Extensions: true,
 		}},
 		{"OptionalValue", "", []string{"ListInput.graphql", "QueryWithSlices.graphql"}, &Config{
-			Generated: "generated.go",
-			Optional:  "value",
+			Optional: "value",
 		}},
 		{"OptionalPointer", "", []string{"ListInput.graphql", "QueryWithSlices.graphql"}, &Config{
-			Generated: "generated.go",
-			Optional:  "pointer",
+			Optional: "pointer",
 		}},
 		{"OptionalGeneric", "", []string{"ListInput.graphql", "QueryWithSlices.graphql"}, &Config{
-			Generated:           "generated.go",
 			Optional:            "generic",
 			OptionalGenericType: "github.com/Khan/genqlient/internal/testutil.Option",
 		}},
 		{"EnumRawCasingAll", "", []string{"QueryWithEnums.graphql"}, &Config{
-			Generated: "generated.go",
 			Casing: Casing{
 				AllEnums: CasingRaw,
 			},
 		}},
 		{"EnumRawCasingSpecific", "", []string{"QueryWithEnums.graphql"}, &Config{
-			Generated: "generated.go",
 			Casing: Casing{
 				Enums: map[string]CasingAlgorithm{"Role": CasingRaw},
 			},
diff --git a/generate/testdata/invalid-config/CantGuessPackage.yaml b/generate/testdata/invalid-config/CantGuessPackage.yaml
new file mode 100644
index 0000000..e69de29
diff --git a/generate/testdata/invalid-config/InvalidCasing.yaml b/generate/testdata/invalid-config/InvalidCasing.yaml
index 1f63cee..6775497 100644
--- a/generate/testdata/invalid-config/InvalidCasing.yaml
+++ b/generate/testdata/invalid-config/InvalidCasing.yaml
@@ -1,3 +1,4 @@
+package: invalidConfig
 casing:
   enums:
     MyType: bogus
diff --git a/generate/testdata/invalid-config/InvalidOptional.yaml b/generate/testdata/invalid-config/InvalidOptional.yaml
new file mode 100644
index 0000000..67151e6
--- /dev/null
+++ b/generate/testdata/invalid-config/InvalidOptional.yaml
@@ -0,0 +1,2 @@
+package: invalidConfig
+optional: bogus
diff --git a/generate/testdata/invalid-config/InvalidPackage.yaml b/generate/testdata/invalid-config/InvalidPackage.yaml
new file mode 100644
index 0000000..9f7372f
--- /dev/null
+++ b/generate/testdata/invalid-config/InvalidPackage.yaml
@@ -0,0 +1,1 @@
+package: bogus-package-name
diff --git a/generate/testdata/snapshots/TestGenerateWithConfig-PackageBindings-testdata-queries b/generate/testdata/snapshots/TestGenerateWithConfig-PackageBindings-testdata-queries-generated.go
similarity index 98%
rename from generate/testdata/snapshots/TestGenerateWithConfig-PackageBindings-testdata-queries
rename to generate/testdata/snapshots/TestGenerateWithConfig-PackageBindings-testdata-queries-generated.go
index 5ca60d3..5838a0a 100644
--- a/generate/testdata/snapshots/TestGenerateWithConfig-PackageBindings-testdata-queries
+++ b/generate/testdata/snapshots/TestGenerateWithConfig-PackageBindings-testdata-queries-generated.go
@@ -1,6 +1,6 @@
 // Code generated by github.com/Khan/genqlient, DO NOT EDIT.
 
-package testdata
+package queries
 
 import (
 	"context"
diff --git a/generate/testdata/snapshots/TestInvalidConfigs-CantGuessPackage.yaml b/generate/testdata/snapshots/TestInvalidConfigs-CantGuessPackage.yaml
new file mode 100644
index 0000000..2571f71
--- /dev/null
+++ b/generate/testdata/snapshots/TestInvalidConfigs-CantGuessPackage.yaml
@@ -0,0 +1,3 @@
+invalid config file testdata/invalid-config/CantGuessPackage.yaml: unable to guess package-name: 'invalid-config' is not a valid identifier
+Set package name in genqlient.yaml
+Example: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6
diff --git a/generate/testdata/snapshots/TestInvalidConfigs-InvalidOptional.yaml b/generate/testdata/snapshots/TestInvalidConfigs-InvalidOptional.yaml
new file mode 100644
index 0000000..fc5394b
--- /dev/null
+++ b/generate/testdata/snapshots/TestInvalidConfigs-InvalidOptional.yaml
@@ -0,0 +1,1 @@
+invalid config file testdata/invalid-config/InvalidOptional.yaml: optional must be one of: 'value' (default), 'pointer', or 'generic'
diff --git a/generate/testdata/snapshots/TestInvalidConfigs-InvalidPackage.yaml b/generate/testdata/snapshots/TestInvalidConfigs-InvalidPackage.yaml
new file mode 100644
index 0000000..a3d047a
--- /dev/null
+++ b/generate/testdata/snapshots/TestInvalidConfigs-InvalidPackage.yaml
@@ -0,0 +1,1 @@
+invalid config file testdata/invalid-config/InvalidPackage.yaml: invalid package in genqlient.yaml: 'bogus-package-name' is not a valid identifier
diff --git a/generate/testdata/snapshots/TestValidConfigs-Lists.yaml b/generate/testdata/snapshots/TestValidConfigs-Lists.yaml
new file mode 100644
index 0000000..7ff7046
--- /dev/null
+++ b/generate/testdata/snapshots/TestValidConfigs-Lists.yaml
@@ -0,0 +1,27 @@
+(*generate.Config)({
+  Schema: (generate.StringList) (len=2) {
+    (string) (len=42) "testdata/valid-config/first_schema.graphql",
+    (string) (len=43) "testdata/valid-config/second_schema.graphql"
+  },
+  Operations: (generate.StringList) (len=2) {
+    (string) (len=46) "testdata/valid-config/first_operations.graphql",
+    (string) (len=47) "testdata/valid-config/second_operations.graphql"
+  },
+  Generated: (string) (len=34) "testdata/valid-config/generated.go",
+  Package: (string) (len=11) "validConfig",
+  ExportOperations: (string) "",
+  ContextType: (string) (len=15) "context.Context",
+  ClientGetter: (string) "",
+  Bindings: (map[string]*generate.TypeBinding) <nil>,
+  PackageBindings: ([]*generate.PackageBinding) <nil>,
+  Casing: (generate.Casing) {
+    AllEnums: (generate.CasingAlgorithm) "",
+    Enums: (map[string]generate.CasingAlgorithm) <nil>
+  },
+  Optional: (string) "",
+  OptionalGenericType: (string) "",
+  StructReferences: (bool) false,
+  Extensions: (bool) false,
+  AllowBrokenFeatures: (bool) false,
+  baseDir: (string) (len=21) "testdata/valid-config"
+})
diff --git a/generate/testdata/snapshots/TestValidConfigs-Simple.yaml b/generate/testdata/snapshots/TestValidConfigs-Simple.yaml
new file mode 100644
index 0000000..77cc1b3
--- /dev/null
+++ b/generate/testdata/snapshots/TestValidConfigs-Simple.yaml
@@ -0,0 +1,21 @@
+(*generate.Config)({
+  Schema: (generate.StringList) <nil>,
+  Operations: (generate.StringList) <nil>,
+  Generated: (string) (len=34) "testdata/valid-config/generated.go",
+  Package: (string) (len=11) "validConfig",
+  ExportOperations: (string) "",
+  ContextType: (string) (len=15) "context.Context",
+  ClientGetter: (string) "",
+  Bindings: (map[string]*generate.TypeBinding) <nil>,
+  PackageBindings: ([]*generate.PackageBinding) <nil>,
+  Casing: (generate.Casing) {
+    AllEnums: (generate.CasingAlgorithm) "",
+    Enums: (map[string]generate.CasingAlgorithm) <nil>
+  },
+  Optional: (string) "",
+  OptionalGenericType: (string) "",
+  StructReferences: (bool) false,
+  Extensions: (bool) false,
+  AllowBrokenFeatures: (bool) false,
+  baseDir: (string) (len=21) "testdata/valid-config"
+})
diff --git a/generate/testdata/snapshots/TestValidConfigs-Strings.yaml b/generate/testdata/snapshots/TestValidConfigs-Strings.yaml
new file mode 100644
index 0000000..1638e8d
--- /dev/null
+++ b/generate/testdata/snapshots/TestValidConfigs-Strings.yaml
@@ -0,0 +1,25 @@
+(*generate.Config)({
+  Schema: (generate.StringList) (len=1) {
+    (string) (len=36) "testdata/valid-config/schema.graphql"
+  },
+  Operations: (generate.StringList) (len=1) {
+    (string) (len=40) "testdata/valid-config/operations.graphql"
+  },
+  Generated: (string) (len=34) "testdata/valid-config/generated.go",
+  Package: (string) (len=11) "validConfig",
+  ExportOperations: (string) "",
+  ContextType: (string) (len=15) "context.Context",
+  ClientGetter: (string) "",
+  Bindings: (map[string]*generate.TypeBinding) <nil>,
+  PackageBindings: ([]*generate.PackageBinding) <nil>,
+  Casing: (generate.Casing) {
+    AllEnums: (generate.CasingAlgorithm) "",
+    Enums: (map[string]generate.CasingAlgorithm) <nil>
+  },
+  Optional: (string) "",
+  OptionalGenericType: (string) "",
+  StructReferences: (bool) false,
+  Extensions: (bool) false,
+  AllowBrokenFeatures: (bool) false,
+  baseDir: (string) (len=21) "testdata/valid-config"
+})
diff --git a/generate/testdata/valid-config/Lists.yaml b/generate/testdata/valid-config/Lists.yaml
new file mode 100644
index 0000000..8348498
--- /dev/null
+++ b/generate/testdata/valid-config/Lists.yaml
@@ -0,0 +1,9 @@
+schema:
+- first_schema.graphql
+- second_schema.graphql
+
+operations:
+- first_operations.graphql
+- second_operations.graphql
+
+package: validConfig
diff --git a/generate/testdata/valid-config/Simple.yaml b/generate/testdata/valid-config/Simple.yaml
new file mode 100644
index 0000000..3084ef5
--- /dev/null
+++ b/generate/testdata/valid-config/Simple.yaml
@@ -0,0 +1,1 @@
+package: validConfig
diff --git a/generate/testdata/valid-config/Strings.yaml b/generate/testdata/valid-config/Strings.yaml
new file mode 100644
index 0000000..7830e0f
--- /dev/null
+++ b/generate/testdata/valid-config/Strings.yaml
@@ -0,0 +1,3 @@
+schema: schema.graphql
+operations: operations.graphql
+package: validConfig