From fa6b68c4104a3af9c2a149484234b6559c60c55f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 24 Sep 2024 16:39:02 +0200 Subject: [PATCH] add unit tests --- bundle/config/loader/process_include.go | 24 +- bundle/config/loader/process_include_test.go | 424 +++++++++++++----- .../testdata/{ => basic}/databricks.yml | 0 .../loader/testdata/{ => basic}/host.yml | 0 .../loader/testdata/format/databricks.yml | 2 + .../config/loader/testdata/format/foo.job.yml | 7 + 6 files changed, 333 insertions(+), 124 deletions(-) rename bundle/config/loader/testdata/{ => basic}/databricks.yml (100%) rename bundle/config/loader/testdata/{ => basic}/host.yml (100%) create mode 100644 bundle/config/loader/testdata/format/databricks.yml create mode 100644 bundle/config/loader/testdata/format/foo.job.yml diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index cc13d6900..85f3fd70d 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -3,12 +3,14 @@ package loader import ( "context" "fmt" + "sort" "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" + "golang.org/x/exp/maps" ) // Steps: @@ -121,10 +123,21 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos } msg := strings.Builder{} - msg.WriteString(fmt.Sprintf("We recommend only defining a single %s when a file has the %s extension.", typ, ext)) + msg.WriteString(fmt.Sprintf("We recommend only defining a single %s in a file with the %s extension.\n", typ, ext)) + + // Dedup the list of resources before adding them the diagnostic message. This + // is needed because we do not dedup earlier when gathering the resources and + // it's valid to define the same resource in both the resources and targets block. msg.WriteString("The following resources are defined or configured in this file:\n") + setOfLines := map[string]struct{}{} for _, r := range resources { - msg.WriteString(fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)) + setOfLines[fmt.Sprintf(" - %s (%s)\n", r.key, r.typ)] = struct{}{} + } + // Sort the line s to print to make the output deterministic. + listOfLines := maps.Keys(setOfLines) + sort.Strings(listOfLines) + for _, l := range listOfLines { + msg.WriteString(l) } locations := []dyn.Location{} @@ -133,6 +146,13 @@ func validateSingleResourceDefined(r *config.Root, ext, typ string) diag.Diagnos locations = append(locations, rr.value.Locations()...) paths = append(paths, rr.path) } + // Sort the locations and paths to make the output deterministic. + sort.Slice(locations, func(i, j int) bool { + return locations[i].String() < locations[j].String() + }) + sort.Slice(paths, func(i, j int) bool { + return paths[i].String() < paths[j].String() + }) return diag.Diagnostics{ { diff --git a/bundle/config/loader/process_include_test.go b/bundle/config/loader/process_include_test.go index 275b29fcc..ebd923575 100644 --- a/bundle/config/loader/process_include_test.go +++ b/bundle/config/loader/process_include_test.go @@ -10,13 +10,16 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestProcessInclude(t *testing.T) { b := &bundle.Bundle{ - RootPath: "testdata", + RootPath: "testdata/basic", Config: config.Root{ Workspace: config.Workspace{ Host: "foo", @@ -36,6 +39,38 @@ func TestProcessInclude(t *testing.T) { assert.Equal(t, "bar", b.Config.Workspace.Host) } +func TestProcessIncludeValidatesFileFormat(t *testing.T) { + b := &bundle.Bundle{ + RootPath: "testdata/format", + Config: config.Root{ + Bundle: config.Bundle{ + Name: "format_test", + }, + }, + } + + m := ProcessInclude(filepath.Join(b.RootPath, "foo.job.yml"), "foo.job.yml") + diags := bundle.Apply(context.Background(), b, m) + require.NoError(t, diags.Error()) + + // Assert that the diagnostics contain the expected information + assert.Len(t, diags, 1) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - bar (job)\n - foo (job)\n", + Locations: []dyn.Location{ + {File: "testdata/format/foo.job.yml", Line: 4, Column: 7}, + {File: "testdata/format/foo.job.yml", Line: 7, Column: 7}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.bar"), + dyn.MustPathFromString("resources.jobs.foo"), + }, + }, + }, diags) +} + func TestResourceNames(t *testing.T) { names := []string{} typ := reflect.TypeOf(config.Resources{}) @@ -72,125 +107,270 @@ func TestValidateFileFormat(t *testing.T) { }, }, } + onlyJobBundle := bundle.Bundle{Config: onlyJob} + + onlyPipeline := config.Root{ + Resources: config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "pipeline1": {}, + }, + }, + } + onlyPipelineBundle := bundle.Bundle{Config: onlyPipeline} + + bothJobAndPipeline := config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + }, + }, + Targets: map[string]*config.Target{ + "target1": { + Resources: &config.Resources{ + Pipelines: map[string]*resources.Pipeline{ + "pipeline1": {}, + }, + }, + }, + }, + } + bothJobAndPipelineBundle := bundle.Bundle{Config: bothJobAndPipeline} + + twoJobs := config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + "job2": {}, + }, + }, + } + twoJobsBundle := bundle.Bundle{Config: twoJobs} + + twoJobsTopLevelAndTarget := config.Root{ + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + }, + }, + Targets: map[string]*config.Target{ + "target1": { + Resources: &config.Resources{ + Jobs: map[string]*resources.Job{ + "job2": {}, + }, + }, + }, + }, + } + twoJobsTopLevelAndTargetBundle := bundle.Bundle{Config: twoJobsTopLevelAndTarget} + + twoJobsInTarget := config.Root{ + Targets: map[string]*config.Target{ + "target1": { + Resources: &config.Resources{ + Jobs: map[string]*resources.Job{ + "job1": {}, + "job2": {}, + }, + }, + }, + }, + } + twoJobsInTargetBundle := bundle.Bundle{Config: twoJobsInTarget} + + tcases := []struct { + name string + bundle *bundle.Bundle + expected diag.Diagnostics + fileName string + locations map[string]dyn.Location + }{ + { + name: "single job", + bundle: &onlyJobBundle, + expected: nil, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + }, + }, + { + name: "single pipeline", + bundle: &onlyPipelineBundle, + expected: nil, + fileName: "foo.pipeline.yml", + locations: map[string]dyn.Location{ + "resources.pipelines.pipeline1": {File: "foo.pipeline.yaml", Line: 1, Column: 1}, + }, + }, + { + name: "single job but extension is pipeline", + bundle: &onlyJobBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single pipeline in a file with the .pipeline.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n", + Locations: []dyn.Location{ + {File: "foo.pipeline.yml", Line: 1, Column: 1}, + {File: "foo.pipeline.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job1"), + }, + }, + }, + fileName: "foo.pipeline.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.pipeline.yml", Line: 1, Column: 1}, + "targets.target1.resources.jobs.job1": {File: "foo.pipeline.yml", Line: 2, Column: 2}, + }, + }, + { + name: "job and pipeline", + bundle: &bothJobAndPipelineBundle, + expected: nil, + fileName: "foo.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.yml", Line: 1, Column: 1}, + "targets.target1.resources.pipelines.pipeline1": {File: "foo.yml", Line: 2, Column: 2}, + }, + }, + { + name: "job and pipeline but extension is job", + bundle: &bothJobAndPipelineBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", + Locations: []dyn.Location{ + {File: "foo.job.yml", Line: 1, Column: 1}, + {File: "foo.job.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.pipelines.pipeline1"), + }, + }, + }, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + "targets.target1.resources.pipelines.pipeline1": {File: "foo.job.yml", Line: 2, Column: 2}, + }, + }, + { + name: "job and pipeline but extension is experiment", + bundle: &bothJobAndPipelineBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single experiment in a file with the .experiment.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - pipeline1 (pipeline)\n", + Locations: []dyn.Location{ + {File: "foo.experiment.yml", Line: 1, Column: 1}, + {File: "foo.experiment.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.pipelines.pipeline1"), + }, + }, + }, + fileName: "foo.experiment.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.experiment.yml", Line: 1, Column: 1}, + "targets.target1.resources.pipelines.pipeline1": {File: "foo.experiment.yml", Line: 2, Column: 2}, + }, + }, + { + name: "two jobs", + bundle: &twoJobsBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: "foo.job.yml", Line: 1, Column: 1}, + {File: "foo.job.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("resources.jobs.job2"), + }, + }, + }, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + "resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, + }, + }, + { + name: "two jobs but extension is simple yaml", + bundle: &twoJobsBundle, + expected: nil, + fileName: "foo.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.yml", Line: 1, Column: 1}, + "resources.jobs.job2": {File: "foo.yml", Line: 2, Column: 2}, + }, + }, + { + name: "two jobs in top level and target", + bundle: &twoJobsTopLevelAndTargetBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: "foo.job.yml", Line: 1, Column: 1}, + {File: "foo.job.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString("resources.jobs.job1"), + dyn.MustPathFromString("targets.target1.resources.jobs.job2"), + }, + }, + }, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + "targets.target1.resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, + }, + }, + { + name: "two jobs in target", + bundle: &twoJobsInTargetBundle, + expected: diag.Diagnostics{ + { + Severity: diag.Info, + Summary: "We recommend only defining a single job in a file with the .job.yml extension.\nThe following resources are defined or configured in this file:\n - job1 (job)\n - job2 (job)\n", + Locations: []dyn.Location{ + {File: "foo.job.yml", Line: 1, Column: 1}, + {File: "foo.job.yml", Line: 2, Column: 2}, + }, + Paths: []dyn.Path{ + dyn.MustPathFromString(("targets.target1.resources.jobs.job1")), + dyn.MustPathFromString("targets.target1.resources.jobs.job2"), + }, + }, + }, + fileName: "foo.job.yml", + locations: map[string]dyn.Location{ + "targets.target1.resources.jobs.job1": {File: "foo.job.yml", Line: 1, Column: 1}, + "targets.target1.resources.jobs.job2": {File: "foo.job.yml", Line: 2, Column: 2}, + }, + }, + } + + for _, tc := range tcases { + t.Run(tc.name, func(t *testing.T) { + for k, v := range tc.locations { + bundletest.SetLocation(tc.bundle, k, []dyn.Location{v}) + } + + diags := validateFileFormat(&tc.bundle.Config, tc.fileName) + assert.Equal(t, tc.expected, diags) + }) + } } - -// func TestGatherResources(t *testing.T) { -// // TODO: Add location tests? -// tcases := []struct { -// name string -// resources config.Resources -// targets map[string]*config.Target -// filenames map[string]string -// expected map[string]resource -// }{ -// { -// name: "empty", -// resources: config.Resources{}, -// expected: map[string]resource{}, -// }, -// { -// name: "one job", -// resources: config.Resources{ -// Jobs: map[string]*resources.Job{ -// "job1": {}, -// }, -// }, -// // TODO CONTINUE: Setting file names for the resources defined here. -// // and testing they are correctly aggregated. -// expected: map[string]resource{ -// "job1": { -// typ: "job", -// p: dyn.MustPathFromString("resources.jobs.job1"), -// }, -// }, -// }, -// { -// name: "three jobs", -// resources: config.Resources{ -// Jobs: map[string]*resources.Job{ -// "job1": {}, -// "job2": {}, -// "job3": {}, -// }, -// }, -// expected: []resource{ -// {"job1", "job"}, -// {"job2", "job"}, -// {"job3", "job"}, -// }, -// }, -// { -// name: "only one experiment target", -// resources: config.Resources{}, -// targets: map[string]*config.Target{ -// "target1": { -// Resources: &config.Resources{ -// Experiments: map[string]*resources.MlflowExperiment{ -// "experiment1": {}, -// }, -// }, -// }, -// }, -// expected: []resource{ -// {"experiment1", "experiment"}, -// }, -// }, -// { -// name: "multiple resources", -// resources: config.Resources{ -// Jobs: map[string]*resources.Job{ -// "job1": {}, -// }, -// Pipelines: map[string]*resources.Pipeline{ -// "pipeline1": {}, -// "pipeline2": {}, -// }, -// Experiments: map[string]*resources.MlflowExperiment{ -// "experiment1": {}, -// }, -// }, -// targets: map[string]*config.Target{ -// "target1": { -// Resources: &config.Resources{ -// ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ -// "model_serving_endpoint1": {}, -// }, -// }, -// }, -// "target2": { -// Resources: &config.Resources{ -// RegisteredModels: map[string]*resources.RegisteredModel{ -// "registered_model1": {}, -// }, -// }, -// }, -// }, -// expected: []resource{ -// {"job1", "job"}, -// {"pipeline1", "pipeline"}, -// {"pipeline2", "pipeline"}, -// {"experiment1", "experiment"}, -// {"model_serving_endpoint1", "model_serving_endpoint"}, -// {"registered_model1", "registered_model"}, -// }, -// }, -// } - -// for _, tc := range tcases { -// t.Run(tc.name, func(t *testing.T) { -// b := &bundle.Bundle{} - -// bundle.ApplyFunc(context.Background(), b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { -// b.Config.Resources = tc.resources -// b.Config.Targets = tc.targets -// return nil -// }) - -// res, err := gatherResources(&b.Config) -// require.NoError(t, err) - -// assert.Equal(t, tc.expected, res) -// }) -// } - -// } diff --git a/bundle/config/loader/testdata/databricks.yml b/bundle/config/loader/testdata/basic/databricks.yml similarity index 100% rename from bundle/config/loader/testdata/databricks.yml rename to bundle/config/loader/testdata/basic/databricks.yml diff --git a/bundle/config/loader/testdata/host.yml b/bundle/config/loader/testdata/basic/host.yml similarity index 100% rename from bundle/config/loader/testdata/host.yml rename to bundle/config/loader/testdata/basic/host.yml diff --git a/bundle/config/loader/testdata/format/databricks.yml b/bundle/config/loader/testdata/format/databricks.yml new file mode 100644 index 000000000..a621514c8 --- /dev/null +++ b/bundle/config/loader/testdata/format/databricks.yml @@ -0,0 +1,2 @@ +bundle: + name: format_test diff --git a/bundle/config/loader/testdata/format/foo.job.yml b/bundle/config/loader/testdata/format/foo.job.yml new file mode 100644 index 000000000..591aaa08c --- /dev/null +++ b/bundle/config/loader/testdata/format/foo.job.yml @@ -0,0 +1,7 @@ +resources: + jobs: + foo: + name: foo + + bar: + name: bar