From ccd49e4d4b002460a3c391bd3dd795bc24c9a513 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 28 Sep 2023 17:01:34 +0200 Subject: [PATCH 1/5] Run tests to verify backend tag validation behavior --- internal/bundle/tags_test.go | 160 +++++++++++++++++++++++++++++++ internal/testutil/cloud.go | 50 ++++++++++ internal/testutil/requirement.go | 19 ++++ 3 files changed, 229 insertions(+) create mode 100644 internal/bundle/tags_test.go create mode 100644 internal/testutil/cloud.go create mode 100644 internal/testutil/requirement.go diff --git a/internal/bundle/tags_test.go b/internal/bundle/tags_test.go new file mode 100644 index 00000000000..8e70db28dc7 --- /dev/null +++ b/internal/bundle/tags_test.go @@ -0,0 +1,160 @@ +package bundle + +import ( + "context" + "strings" + "testing" + + "github.com/databricks/cli/internal" + "github.com/databricks/cli/internal/testutil" + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/service/compute" + "github.com/databricks/databricks-sdk-go/service/jobs" + "github.com/stretchr/testify/require" +) + +func testTags(t *testing.T, tags map[string]string) error { + var nodeTypeId string + switch testutil.GetCloud(t) { + case testutil.AWS: + nodeTypeId = "i3.xlarge" + case testutil.Azure: + nodeTypeId = "Standard_DS4_v2" + case testutil.GCP: + nodeTypeId = "n1-standard-4" + } + + w, err := databricks.NewWorkspaceClient() + require.NoError(t, err) + + ctx := context.Background() + resp, err := w.Jobs.Create(ctx, jobs.CreateJob{ + Name: internal.RandomName("test-tags-"), + Tasks: []jobs.Task{ + { + TaskKey: "test", + NewCluster: &compute.ClusterSpec{ + SparkVersion: "13.3.x-scala2.12", + NumWorkers: 1, + NodeTypeId: nodeTypeId, + }, + SparkPythonTask: &jobs.SparkPythonTask{ + PythonFile: "/doesnt_exist.py", + }, + }, + }, + Tags: tags, + }) + + if resp != nil { + t.Cleanup(func() { + w.Jobs.DeleteByJobId(ctx, resp.JobId) + }) + } + + return err +} + +func testTagKey(t *testing.T, key string) error { + return testTags(t, map[string]string{ + key: "value", + }) +} + +func testTagValue(t *testing.T, value string) error { + return testTags(t, map[string]string{ + "key": value, + }) +} + +func TestAccTagKeyAWS(t *testing.T) { + testutil.Require(t, testutil.AWS) + t.Parallel() + + t.Run("invalid", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "café") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, `The key must match the regular expression ^[\d \w\+\-=\.:\/@]*$.`) + }) + + t.Run("empty", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, ` the minimal length is 1, and the maximum length is 127.`) + }) +} + +func TestAccTagValueAWS(t *testing.T) { + testutil.Require(t, testutil.AWS) + t.Parallel() + + err := testTagValue(t, "café") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, `The value must match the regular expression ^[\d \w\+\-=\.:/@]*$.`) +} + +func TestAccTagKeyAzure(t *testing.T) { + testutil.Require(t, testutil.Azure) + t.Parallel() + + t.Run("invalid", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "café?") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, `The key must match the regular expression ^[^<>\*&%;\\\/\+\?]*$.`) + }) + + t.Run("empty", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, ` the minimal length is 1, and the maximum length is 512.`) + }) +} + +func TestAccTagValueAzure(t *testing.T) { + testutil.Require(t, testutil.Azure) + t.Parallel() + + // Azure puts no constraings on tag values. + err := testTagValue(t, "café?") + require.NoError(t, err) +} + +func TestAccTagKeyGCP(t *testing.T) { + testutil.Require(t, testutil.GCP) + t.Parallel() + + t.Run("invalid", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "café?") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, `The key must match the regular expression ^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$.`) + }) + + t.Run("empty", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, ` the minimal length is 1, and the maximum length is 63.`) + }) +} + +func TestAccTagValueGCP(t *testing.T) { + testutil.Require(t, testutil.GCP) + t.Parallel() + + err := testTagValue(t, "café?") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, `The value must match the regular expression ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$.`) +} diff --git a/internal/testutil/cloud.go b/internal/testutil/cloud.go new file mode 100644 index 00000000000..e8f741fae86 --- /dev/null +++ b/internal/testutil/cloud.go @@ -0,0 +1,50 @@ +package testutil + +import ( + "testing" + + "github.com/databricks/cli/internal" +) + +type Cloud int + +const ( + AWS Cloud = iota + Azure + GCP +) + +// Implement [Requirement]. +func (c Cloud) Verify(t *testing.T) { + if c != GetCloud(t) { + t.Skipf("Skipping %s-specific test", c) + } +} + +func (c Cloud) String() string { + switch c { + case AWS: + return "AWS" + case Azure: + return "Azure" + case GCP: + return "GCP" + default: + return "unknown" + } +} + +func GetCloud(t *testing.T) Cloud { + env := internal.GetEnvOrSkipTest(t, "CLOUD_ENV") + switch env { + case "aws": + return AWS + case "azure": + return Azure + case "gcp": + return GCP + default: + t.Fatalf("Unknown cloud environment: %s", env) + } + return -1 +} diff --git a/internal/testutil/requirement.go b/internal/testutil/requirement.go new file mode 100644 index 00000000000..53855e0b52f --- /dev/null +++ b/internal/testutil/requirement.go @@ -0,0 +1,19 @@ +package testutil + +import ( + "testing" +) + +// Requirement is the interface for test requirements. +type Requirement interface { + Verify(t *testing.T) +} + +// Require should be called at the beginning of a test to ensure that all +// requirements are met before running the test. +// If any requirement is not met, the test will be skipped. +func Require(t *testing.T, requirements ...Requirement) { + for _, r := range requirements { + r.Verify(t) + } +} From 7ea1f3530acf5899b3e41b4792a5b41238d89409 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 28 Sep 2023 17:04:14 +0200 Subject: [PATCH 2/5] Test unicode tag keys --- internal/bundle/tags_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/internal/bundle/tags_test.go b/internal/bundle/tags_test.go index 8e70db28dc7..220dd8afbbb 100644 --- a/internal/bundle/tags_test.go +++ b/internal/bundle/tags_test.go @@ -79,6 +79,14 @@ func TestAccTagKeyAWS(t *testing.T) { require.Contains(t, msg, `The key must match the regular expression ^[\d \w\+\-=\.:\/@]*$.`) }) + t.Run("unicode", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "🍎") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, ` contains non-latin1 characters.`) + }) + t.Run("empty", func(t *testing.T) { t.Parallel() err := testTagKey(t, "") @@ -110,6 +118,14 @@ func TestAccTagKeyAzure(t *testing.T) { require.Contains(t, msg, `The key must match the regular expression ^[^<>\*&%;\\\/\+\?]*$.`) }) + t.Run("unicode", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "🍎") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, ` contains non-latin1 characters.`) + }) + t.Run("empty", func(t *testing.T) { t.Parallel() err := testTagKey(t, "") @@ -140,6 +156,14 @@ func TestAccTagKeyGCP(t *testing.T) { require.Contains(t, msg, `The key must match the regular expression ^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$.`) }) + t.Run("unicode", func(t *testing.T) { + t.Parallel() + err := testTagKey(t, "🍎") + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, ` contains non-latin1 characters.`) + }) + t.Run("empty", func(t *testing.T) { t.Parallel() err := testTagKey(t, "") From 6f1ed032e4d46728fa41b36610dbfa13f0183ba4 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 28 Sep 2023 17:24:22 +0200 Subject: [PATCH 3/5] Reduce repetition --- internal/bundle/tags_test.go | 230 +++++++++++++++++++++++------------ 1 file changed, 153 insertions(+), 77 deletions(-) diff --git a/internal/bundle/tags_test.go b/internal/bundle/tags_test.go index 220dd8afbbb..443779e7aac 100644 --- a/internal/bundle/tags_test.go +++ b/internal/bundle/tags_test.go @@ -67,32 +67,59 @@ func testTagValue(t *testing.T, value string) error { }) } +type tagTestCase struct { + name string + value string + fn func(t *testing.T, value string) error + err string +} + +func runTagTestCases(t *testing.T, cases []tagTestCase) { + for i := range cases { + tc := cases[i] + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := tc.fn(t, tc.value) + if tc.err == "" { + require.NoError(t, err) + } else { + require.Error(t, err) + msg := strings.ReplaceAll(err.Error(), "\n", " ") + require.Contains(t, msg, tc.err) + } + }) + } +} + func TestAccTagKeyAWS(t *testing.T) { testutil.Require(t, testutil.AWS) t.Parallel() - t.Run("invalid", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "café") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, `The key must match the regular expression ^[\d \w\+\-=\.:\/@]*$.`) - }) - - t.Run("unicode", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "🍎") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, ` contains non-latin1 characters.`) - }) - - t.Run("empty", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, ` the minimal length is 1, and the maximum length is 127.`) + runTagTestCases(t, []tagTestCase{ + { + name: "invalid", + value: "café", + fn: testTagKey, + err: ` The key must match the regular expression ^[\d \w\+\-=\.:\/@]*$.`, + }, + { + name: "unicode", + value: "🍎", + fn: testTagKey, + err: ` contains non-latin1 characters.`, + }, + { + name: "empty", + value: "", + fn: testTagKey, + err: ` the minimal length is 1, and the maximum length is 127.`, + }, + { + name: "valid", + value: "cafe", + fn: testTagKey, + err: ``, + }, }) } @@ -100,38 +127,57 @@ func TestAccTagValueAWS(t *testing.T) { testutil.Require(t, testutil.AWS) t.Parallel() - err := testTagValue(t, "café") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, `The value must match the regular expression ^[\d \w\+\-=\.:/@]*$.`) + runTagTestCases(t, []tagTestCase{ + { + name: "invalid", + value: "café", + fn: testTagValue, + err: ` The value must match the regular expression ^[\d \w\+\-=\.:/@]*$.`, + }, + { + name: "unicode", + value: "🍎", + fn: testTagValue, + err: ` contains non-latin1 characters.`, + }, + { + name: "valid", + value: "cafe", + fn: testTagValue, + err: ``, + }, + }) } func TestAccTagKeyAzure(t *testing.T) { testutil.Require(t, testutil.Azure) t.Parallel() - t.Run("invalid", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "café?") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, `The key must match the regular expression ^[^<>\*&%;\\\/\+\?]*$.`) - }) - - t.Run("unicode", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "🍎") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, ` contains non-latin1 characters.`) - }) - - t.Run("empty", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, ` the minimal length is 1, and the maximum length is 512.`) + runTagTestCases(t, []tagTestCase{ + { + name: "invalid", + value: "café?", + fn: testTagKey, + err: ` The key must match the regular expression ^[^<>\*&%;\\\/\+\?]*$.`, + }, + { + name: "unicode", + value: "🍎", + fn: testTagKey, + err: ` contains non-latin1 characters.`, + }, + { + name: "empty", + value: "", + fn: testTagKey, + err: ` the minimal length is 1, and the maximum length is 512.`, + }, + { + name: "valid", + value: "cafe", + fn: testTagKey, + err: ``, + }, }) } @@ -139,37 +185,51 @@ func TestAccTagValueAzure(t *testing.T) { testutil.Require(t, testutil.Azure) t.Parallel() - // Azure puts no constraings on tag values. - err := testTagValue(t, "café?") - require.NoError(t, err) + runTagTestCases(t, []tagTestCase{ + { + name: "unicode", + value: "🍎", + fn: testTagValue, + err: ` contains non-latin1 characters.`, + }, + { + name: "valid", + value: "cafe", + fn: testTagValue, + err: ``, + }, + }) } func TestAccTagKeyGCP(t *testing.T) { testutil.Require(t, testutil.GCP) t.Parallel() - t.Run("invalid", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "café?") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, `The key must match the regular expression ^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$.`) - }) - - t.Run("unicode", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "🍎") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, ` contains non-latin1 characters.`) - }) - - t.Run("empty", func(t *testing.T) { - t.Parallel() - err := testTagKey(t, "") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, ` the minimal length is 1, and the maximum length is 63.`) + runTagTestCases(t, []tagTestCase{ + { + name: "invalid", + value: "café?", + fn: testTagKey, + err: ` The key must match the regular expression ^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$.`, + }, + { + name: "unicode", + value: "🍎", + fn: testTagKey, + err: ` contains non-latin1 characters.`, + }, + { + name: "empty", + value: "", + fn: testTagKey, + err: ` the minimal length is 1, and the maximum length is 63.`, + }, + { + name: "valid", + value: "cafe", + fn: testTagKey, + err: ``, + }, }) } @@ -177,8 +237,24 @@ func TestAccTagValueGCP(t *testing.T) { testutil.Require(t, testutil.GCP) t.Parallel() - err := testTagValue(t, "café?") - require.Error(t, err) - msg := strings.ReplaceAll(err.Error(), "\n", " ") - require.Contains(t, msg, `The value must match the regular expression ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$.`) + runTagTestCases(t, []tagTestCase{ + { + name: "invalid", + value: "café", + fn: testTagValue, + err: ` The value must match the regular expression ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$.`, + }, + { + name: "unicode", + value: "🍎", + fn: testTagValue, + err: ` contains non-latin1 characters.`, + }, + { + name: "valid", + value: "cafe", + fn: testTagValue, + err: ``, + }, + }) } From 3fb5671843dc072d0b9e007fbf0926b9d60ff7d7 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 28 Sep 2023 17:30:53 +0200 Subject: [PATCH 4/5] Keep internal/testutil dependency-free --- internal/testutil/cloud.go | 4 +--- internal/testutil/env.go | 9 +++++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/testutil/cloud.go b/internal/testutil/cloud.go index e8f741fae86..50bbf67f254 100644 --- a/internal/testutil/cloud.go +++ b/internal/testutil/cloud.go @@ -2,8 +2,6 @@ package testutil import ( "testing" - - "github.com/databricks/cli/internal" ) type Cloud int @@ -35,7 +33,7 @@ func (c Cloud) String() string { } func GetCloud(t *testing.T) Cloud { - env := internal.GetEnvOrSkipTest(t, "CLOUD_ENV") + env := GetEnvOrSkipTest(t, "CLOUD_ENV") switch env { case "aws": return AWS diff --git a/internal/testutil/env.go b/internal/testutil/env.go index 11a610189a0..39201c5b456 100644 --- a/internal/testutil/env.go +++ b/internal/testutil/env.go @@ -35,3 +35,12 @@ func CleanupEnvironment(t *testing.T) { t.Setenv("USERPROFILE", pwd) } } + +// GetEnvOrSkipTest proceeds with test only with that env variable +func GetEnvOrSkipTest(t *testing.T, name string) string { + value := os.Getenv(name) + if value == "" { + t.Skipf("Environment variable %s is missing", name) + } + return value +} From b584ecc08416e0429578f04acc29a0a1f0c3be63 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 28 Sep 2023 21:47:27 +0200 Subject: [PATCH 5/5] Move test to top level package --- internal/{bundle => }/tags_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) rename internal/{bundle => }/tags_test.go (98%) diff --git a/internal/bundle/tags_test.go b/internal/tags_test.go similarity index 98% rename from internal/bundle/tags_test.go rename to internal/tags_test.go index 443779e7aac..2dd3759acbd 100644 --- a/internal/bundle/tags_test.go +++ b/internal/tags_test.go @@ -1,11 +1,10 @@ -package bundle +package internal import ( "context" "strings" "testing" - "github.com/databricks/cli/internal" "github.com/databricks/cli/internal/testutil" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/service/compute" @@ -29,7 +28,7 @@ func testTags(t *testing.T, tags map[string]string) error { ctx := context.Background() resp, err := w.Jobs.Create(ctx, jobs.CreateJob{ - Name: internal.RandomName("test-tags-"), + Name: RandomName("test-tags-"), Tasks: []jobs.Task{ { TaskKey: "test",