Improve golden test formatting (#110)

Make sure golden tests write all the generated definitions, and use the FileDefinition path so that they are formatted correctly.

I made the `CodeGenerator` less stateful by removing the `scanner` from it; it is no longer necessary to read the `Definitions` field from the `scanner` as `GenerateDefinitions` will return them directly.

hack/generator/cmd/gen/gen.go

@@ -7,6 +7,7 @@ package gen
 
 import (
 	"context"
+
 	"github.com/Azure/k8s-infra/hack/generator/pkg/codegen"
 	"github.com/Azure/k8s-infra/hack/generator/pkg/xcobra"
 	"github.com/spf13/cobra"

hack/generator/go.mod

@@ -3,31 +3,30 @@ module github.com/Azure/k8s-infra/hack/generator
 go 1.13
 
 require (
-	contrib.go.opencensus.io/exporter/jaeger v0.2.0 // indirect
 	github.com/devigned/tab v0.1.1
-	github.com/devigned/tab/opencensus v0.1.2 // indirect
-	github.com/golang/groupcache v0.0.0-20191227052852-215e87163ea7 // indirect
+	github.com/golang/protobuf v1.3.2 // indirect
+	github.com/onsi/ginkgo v1.11.0 // indirect
 	github.com/onsi/gomega v1.8.1
 	github.com/pelletier/go-toml v1.6.0 // indirect
 	github.com/sebdah/goldie/v2 v2.3.0
+	github.com/spf13/afero v1.2.2 // indirect
 	github.com/spf13/cast v1.3.1 // indirect
 	github.com/spf13/cobra v0.0.5
 	github.com/spf13/jwalterweatherman v1.1.0 // indirect
 	github.com/spf13/pflag v1.0.5
 	github.com/spf13/viper v1.6.1
-	github.com/uber/jaeger-client-go v2.21.1+incompatible // indirect
+	github.com/stretchr/testify v1.4.0 // indirect
 	github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
 	github.com/xeipuuv/gojsonschema v1.2.0
-	go.opencensus.io v0.22.2 // indirect
 	golang.org/x/net v0.0.0-20191209160850-c0dbc17a3553 // indirect
 	golang.org/x/sys v0.0.0-20191228213918-04cbcbbfeed8 // indirect
+	golang.org/x/text v0.3.2 // indirect
 	golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 // indirect
-	google.golang.org/api v0.15.0 // indirect
+	gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect
 	gopkg.in/ini.v1 v1.51.1 // indirect
+	gopkg.in/yaml.v2 v2.2.8 // indirect
 	gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c
-	k8s.io/apimachinery v0.18.3
 	k8s.io/klog/v2 v2.0.0
-	sigs.k8s.io/controller-runtime v0.6.0
 )
 
 replace github.com/xeipuuv/gojsonschema => github.com/devigned/gojsonschema v1.2.1-0.20191231010529-c593123f1e5d

hack/generator/pkg/astmodel/file_definition.go

@@ -11,9 +11,11 @@ import (
 	"go/format"
 	"go/parser"
 	"go/token"
-	"k8s.io/klog/v2"
+	"io"
 	"os"
 	"sort"
+
+	"k8s.io/klog/v2"
 )
 
 // FileDefinition is the content of a file we're generating
@@ -144,13 +146,13 @@ func createComments(lines ...string) ([]*ast.Comment, int) {
 	return result, length
 }
 
-// SaveTo writes this generated file to disk
-func (file FileDefinition) SaveTo(filePath string) error {
+// SaveToWriter writes the file to the specifier io.Writer
+func (file FileDefinition) SaveToWriter(filename string, dst io.Writer) error {
 	original := file.AsAst()
 
 	// Write generated source into a memory buffer
 	fset := token.NewFileSet()
-	fset.AddFile(filePath, 1, 102400)
+	fset.AddFile(filename, 1, 102400)
 
 	var buffer bytes.Buffer
 	err := format.Node(&buffer, fset, original)
@@ -160,18 +162,24 @@ func (file FileDefinition) SaveTo(filePath string) error {
 
 	// Parse it out of the buffer again so we can "go fmt" it
 	var toFormat ast.Node
-	toFormat, err = parser.ParseFile(fset, filePath, &buffer, parser.ParseComments)
+	toFormat, err = parser.ParseFile(fset, filename, &buffer, parser.ParseComments)
 	if err != nil {
 		klog.Errorf("Failed to reformat code (%s); keeping code as is.", err)
 		toFormat = original
 	}
 
-	// Write it to a file
+	return format.Node(dst, fset, toFormat)
+}
+
+// SaveToFile writes this generated file to disk
+func (file FileDefinition) SaveToFile(filePath string) error {
+
 	f, err := os.Create(filePath)
 	if err != nil {
 		return err
 	}
+
 	defer f.Close()
 
-	return format.Node(f, fset, toFormat)
+	return file.SaveToWriter(filePath, f)
 }

hack/generator/pkg/astmodel/package_definition.go

@@ -67,7 +67,7 @@ func emitFiles(filesToGenerate map[string][]Definition, outputDir string) error
 		genFile := NewFileDefinition(defs[0].Reference().PackageReference, defs...)
 		outputFile := filepath.Join(outputDir, fileName+"_types.go")
 		klog.V(5).Infof("Writing '%s'\n", outputFile)
-		err := genFile.SaveTo(outputFile)
+		err := genFile.SaveToFile(outputFile)
 		if err != nil {
 			return fmt.Errorf("error saving definitions to file '%v'(%w)", outputFile, err)
 		}

hack/generator/pkg/codegen/code_generator.go

@@ -8,21 +8,20 @@ package codegen
 import (
 	"context"
 	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
 	"github.com/Azure/k8s-infra/hack/generator/pkg/astmodel"
 	"github.com/Azure/k8s-infra/hack/generator/pkg/jsonast"
 	"github.com/xeipuuv/gojsonschema"
 	"gopkg.in/yaml.v3"
-	"io/ioutil"
-	"os"
-	"path/filepath"
 
 	"k8s.io/klog/v2"
 )
 
 type CodeGenerator struct {
 	configuration *Configuration
-	idFactory     astmodel.IdentifierFactory
-	scanner       *jsonast.SchemaScanner
 }
 
 func NewCodeGenerator(configurationFile string) (*CodeGenerator, error) {
@@ -36,14 +35,7 @@ func NewCodeGenerator(configurationFile string) (*CodeGenerator, error) {
 		return nil, fmt.Errorf("configuration loaded from '%v' is invalid (%w)", configurationFile, err)
 	}
 
-	idFactory := astmodel.NewIdentifierFactory()
-	scanner := jsonast.NewSchemaScanner(idFactory)
-
-	result := &CodeGenerator{
-		configuration: config,
-		idFactory:     idFactory,
-		scanner:       scanner,
-	}
+	result := &CodeGenerator{configuration: config}
 
 	return result, nil
 }
@@ -62,14 +54,15 @@ func (generator *CodeGenerator) Generate(ctx context.Context, outputFolder strin
 		return fmt.Errorf("error cleaning output folder '%v' (%w)", generator.configuration.SchemaURL, err)
 	}
 
+	scanner := jsonast.NewSchemaScanner(astmodel.NewIdentifierFactory())
+
 	klog.V(0).Infof("Walking JSON schema")
-	_, err = generator.scanner.ToNodes(ctx, schema.Root())
+	defs, err := scanner.GenerateDefinitions(ctx, schema.Root())
 	if err != nil {
 		return fmt.Errorf("failed to walk JSON schema (%w)", err)
 	}
-
-	// group definitions by package
-	packages, err := generator.CreatePackages()
+
+	packages, err := generator.CreatePackagesForDefinitions(defs)
 	if err != nil {
 		return fmt.Errorf("failed to assign generated definitions to packages (%w)", err)
 	}
@@ -105,9 +98,9 @@ func (generator *CodeGenerator) Generate(ctx context.Context, outputFolder strin
 	return nil
 }
 
-func (generator *CodeGenerator) CreatePackages() (map[astmodel.PackageReference]*astmodel.PackageDefinition, error) {
+func (generator *CodeGenerator) CreatePackagesForDefinitions(definitions []astmodel.Definition) ([]*astmodel.PackageDefinition, error) {
 	packages := make(map[astmodel.PackageReference]*astmodel.PackageDefinition)
-	for _, def := range generator.scanner.Definitions {
+	for _, def := range definitions {
 
 		shouldExport, reason := generator.configuration.ShouldExport(def)
 		defRef := def.Reference()
@@ -137,8 +130,13 @@ func (generator *CodeGenerator) CreatePackages() (map[astmodel.PackageReference]
 			}
 		}
 	}
-
-	return packages, nil
+
+	var pkgs []*astmodel.PackageDefinition
+	for _, pkg := range packages {
+		pkgs = append(pkgs, pkg)
+	}
+
+	return pkgs, nil
 }
 
 func loadConfiguration(configurationFile string) (*Configuration, error) {

hack/generator/pkg/jsonast/jsonast.go

@@ -38,7 +38,7 @@ type (
 
 	// A SchemaScanner is used to scan a JSON Schema extracting and collecting type definitions
 	SchemaScanner struct {
-		Definitions  map[astmodel.DefinitionName]astmodel.Definition
+		definitions  map[astmodel.DefinitionName]astmodel.Definition
 		TypeHandlers map[SchemaType]TypeHandler
 		Filters      []string
 		idFactory    astmodel.IdentifierFactory
@@ -47,13 +47,13 @@ type (
 
 // FindDefinition looks to see if we have seen the specified definiton before, returning its definition if we have.
 func (scanner *SchemaScanner) FindDefinition(ref astmodel.DefinitionName) (astmodel.Definition, bool) {
-	result, ok := scanner.Definitions[ref]
+	result, ok := scanner.definitions[ref]
 	return result, ok
 }
 
 // AddDefinition makes a record of the specified struct so that FindStruct() can return it when it is needed again.
 func (scanner *SchemaScanner) AddDefinition(def astmodel.Definition) {
-	scanner.Definitions[*def.Reference()] = def
+	scanner.definitions[*def.Reference()] = def
 }
 
 // Definitions for different kinds of JSON schema
@@ -84,7 +84,7 @@ func (use *UnknownSchemaError) Error() string {
 // NewSchemaScanner constructs a new scanner, ready for use
 func NewSchemaScanner(idFactory astmodel.IdentifierFactory) *SchemaScanner {
 	return &SchemaScanner{
-		Definitions:  make(map[astmodel.DefinitionName]astmodel.Definition),
+		definitions:  make(map[astmodel.DefinitionName]astmodel.Definition),
 		TypeHandlers: DefaultTypeHandlers(),
 		idFactory:    idFactory,
 	}
@@ -141,7 +141,7 @@ func (scanner *SchemaScanner) AddFilters(filters []string) {
 // 							- ARM specific resources. I'm not 100% sure why...
 //
 // 		allOf acts like composition which composites each schema from the child oneOf with the base reference from allOf.
-func (scanner *SchemaScanner) ToNodes(ctx context.Context, schema *gojsonschema.SubSchema, opts ...BuilderOption) (astmodel.Definition, error) {
+func (scanner *SchemaScanner) GenerateDefinitions(ctx context.Context, schema *gojsonschema.SubSchema, opts ...BuilderOption) ([]astmodel.Definition, error) {
 	ctx, span := tab.StartSpan(ctx, "ToNodes")
 	defer span.End()
 
@@ -192,7 +192,13 @@ func (scanner *SchemaScanner) ToNodes(ctx context.Context, schema *gojsonschema.
 
 	scanner.AddDefinition(root)
 
-	return root, nil
+	// produce the results
+	var defs []astmodel.Definition
+	for _, def := range scanner.definitions {
+		defs = append(defs, def)
+	}
+
+	return defs, nil
 }
 
 // DefaultTypeHandlers will create a default map of JSONType to AST transformers

hack/generator/pkg/jsonast/jsonast_test.go

@@ -9,8 +9,6 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"go/format"
-	"go/token"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -39,13 +37,20 @@ func runGoldenTest(t *testing.T, path string) {
 	}
 
 	scanner := NewSchemaScanner(astmodel.NewIdentifierFactory())
-	nodes, err := scanner.ToNodes(context.TODO(), schema.Root())
+	defs, err := scanner.GenerateDefinitions(context.TODO(), schema.Root())
 	if err != nil {
 		t.Fatal(fmt.Errorf("could not produce nodes from scanner: %w", err))
 	}
 
+	// put all definitions in one file, regardless
+	// the package reference isn't really used here
+	fileDef := astmodel.NewFileDefinition(defs[0].Reference().PackageReference, defs...)
+
 	buf := &bytes.Buffer{}
-	_ = format.Node(buf, token.NewFileSet(), nodes.AsDeclarations())
+	err = fileDef.SaveToWriter(path, buf)
+	if err != nil {
+		t.Fatal(fmt.Errorf("could not generate file: %w", err))
+	}
 
 	g.Assert(t, testName, buf.Bytes())
 }
@@ -56,7 +61,9 @@ func TestGolden(t *testing.T) {
 		name string
 		path string
 	}
+
 	testGroups := make(map[string][]Test)
+
 	// find all input .json files
 	err := filepath.Walk("testdata", func(path string, info os.FileInfo, err error) error {
 		if filepath.Ext(path) == ".json" {

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_field_if_other_fields_present.golden

@@ -1,8 +1,14 @@
-type
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+// This is a generated file. Please do not make manual changes.
+package v20200101
+
+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 /* Generated from: https://test.test/schemas/2020-01-01/test.json */
-Test struct {
+type Test struct {
 	Tags *struct {
 		OtherField           *string            `json:"otherField"`
 		additionalProperties map[string]float64 `json:"additionalProperties"`
 	} `json:"tags"`
-}
+}

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_map[string]interface{}_if_unset_and_no_other_fields_present.golden

@@ -1,5 +1,11 @@
-type
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+// This is a generated file. Please do not make manual changes.
+package v20200101
+
+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 /* Generated from: https://test.test/schemas/2020-01-01/test.json */
-Test struct {
+type Test struct {
 	Tags *map[string]interface{} `json:"tags"`
-}
+}

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_map_type_if_no_other_fields_present.golden

@@ -1,5 +1,11 @@
-type
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+// This is a generated file. Please do not make manual changes.
+package v20200101
+
+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 /* Generated from: https://test.test/schemas/2020-01-01/test.json */
-Test struct {
+type Test struct {
 	Tags *map[string]float64 `json:"tags"`
-}
+}

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_nothing_if_set_to_'false'.golden

@@ -1,6 +1,12 @@
-type
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+// This is a generated file. Please do not make manual changes.
+package v20200101
+
+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 /* Generated from: https://test.test/schemas/2020-01-01/test.json */
-Test struct {
+type Test struct {
 	Tags *struct {
 	} `json:"tags"`
-}
+}

hack/generator/pkg/jsonast/testdata/AdditionalProperties/Generates_nothing_if_unset_and_other_fields_present.golden

@@ -1,7 +1,13 @@
-type
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+// This is a generated file. Please do not make manual changes.
+package v20200101
+
+import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 /* Generated from: https://test.test/schemas/2020-01-01/test.json */
-Test struct {
+type Test struct {
 	Tags *struct {
 		OtherField *string `json:"otherField"`
 	} `json:"tags"`
-}
+}