From 962f82f0b4a1128e431feec22c39490e95a23b6a Mon Sep 17 00:00:00 2001
From: pokom <mark.poko@grafana.com>
Date: Mon, 11 Dec 2023 17:05:38 -0500
Subject: [PATCH] feat(gke): Add unit tests for MachineTypes

This adds a suite of unit tests for MachineTypes and gets full coverage for stripOutKeyFromDescription.
Once this merges I'll likely do some small refactors for error handling for the MachineType methods that are called.
---
 pkg/google/compute/compute.go      |  4 --
 pkg/google/compute/compute_test.go | 75 +++++++++++++++++++++++++++++-
 pkg/google/compute/pricing_map.go  | 12 ++---
 3 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/pkg/google/compute/compute.go b/pkg/google/compute/compute.go
index e288b0f7..ed915f57 100644
--- a/pkg/google/compute/compute.go
+++ b/pkg/google/compute/compute.go
@@ -171,10 +171,6 @@ func getMachineFamily(machineType string) string {
 		return ""
 	}
 	split := strings.Split(machineType, "-")
-	if len(split) < 2 {
-		log.Printf("Machine type %s doesn't contain a -", machineType)
-		return ""
-	}
 	return strings.ToLower(split[0])
 }
 
diff --git a/pkg/google/compute/compute_test.go b/pkg/google/compute/compute_test.go
index 21a17968..6431b157 100644
--- a/pkg/google/compute/compute_test.go
+++ b/pkg/google/compute/compute_test.go
@@ -10,6 +10,7 @@ import (
 
 	billingv1 "cloud.google.com/go/billing/apiv1"
 	"cloud.google.com/go/billing/apiv1/billingpb"
+	"github.com/stretchr/testify/require"
 	"google.golang.org/api/compute/v1"
 )
 
@@ -47,6 +48,10 @@ func Test_stripOutKeyFromDescription(t *testing.T) {
 			description: "N1 Predefined Instance Core running in Americas",
 			want:        "N1 Predefined Instance Core",
 		},
+		"commitment v1: empty": {
+			description: "Commitment v1:",
+			want:        "",
+		},
 		"commitment v1": {
 			description: "Commitment v1: N2 Predefined Instance Core in Americas",
 			want:        "N2 Predefined Instance Core",
@@ -141,7 +146,7 @@ func Test_GetMachineFamily(t *testing.T) {
 	}
 }
 
-// development tests: Following tests are ment to be run locally and not suited for CI
+// development tests: Following tests are meant to be run locally and not suited for CI
 // If you need this tests for debugging purposes please run `TestGenerateTestFiles` first
 // and then you can run the rest of tests as needed.
 
@@ -245,3 +250,71 @@ func TestListInstances(t *testing.T) {
 		fmt.Printf("%v:%s\n", instance.Instance, instance.Family)
 	}
 }
+
+func TestNewMachineSpec(t *testing.T) {
+	tests := map[string]struct {
+		instance *compute.Instance
+		want     *MachineSpec
+	}{
+		"basic instance": {
+			instance: &compute.Instance{
+				Name:        "test",
+				MachineType: "abc/abc-def",
+				Zone:        "testing/abc-123",
+				Scheduling: &compute.Scheduling{
+					ProvisioningModel: "test",
+				},
+			},
+			want: &MachineSpec{
+				Instance:     "test",
+				Zone:         "abc-123",
+				Region:       "abc",
+				MachineType:  "abc-def",
+				Family:       "abc",
+				SpotInstance: false,
+			},
+		},
+		"machine type with no value": {
+			instance: &compute.Instance{
+				Name:        "test",
+				MachineType: "abc/",
+				Zone:        "testing/abc-123",
+				Scheduling: &compute.Scheduling{
+					ProvisioningModel: "test",
+				},
+			},
+			want: &MachineSpec{
+				Instance:     "test",
+				Zone:         "abc-123",
+				Region:       "abc",
+				MachineType:  "",
+				Family:       "",
+				SpotInstance: false,
+			},
+		},
+		"spot instance": {
+			instance: &compute.Instance{
+				Name:        "test",
+				MachineType: "abc/abc-def",
+				Zone:        "testing/abc-123",
+				Scheduling: &compute.Scheduling{
+					ProvisioningModel: "SPOT",
+				},
+			},
+			want: &MachineSpec{
+				Instance:     "test",
+				Zone:         "abc-123",
+				Region:       "abc",
+				MachineType:  "abc-def",
+				Family:       "abc",
+				SpotInstance: true,
+			},
+		},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := NewMachineSpec(test.instance)
+			require.Equal(t, got, test.want)
+		})
+	}
+}
diff --git a/pkg/google/compute/pricing_map.go b/pkg/google/compute/pricing_map.go
index ca69862e..963e2913 100644
--- a/pkg/google/compute/pricing_map.go
+++ b/pkg/google/compute/pricing_map.go
@@ -17,21 +17,21 @@ var (
 	ErrorInRegions     = errors.New("there is an error in the Regions data")
 	RegionNotFound     = errors.New("region wasn't found in pricing map")
 	FamilyTypeNotFound = errors.New("family wasn't found in pricing map for this region")
-	spot               = `(?P<spot>Spot Preemptible )`
-	machineType        = `(?P<machineType>\w{1,3})`
+	spotRegex          = `(?P<spot>Spot Preemptible )`
+	machineTypeRegex   = `(?P<machineType>\w{1,3})`
 	amd                = `(?P<amd> AMD)`
 	n1Suffix           = `(?: Predefined)`
 	resource           = `(?P<resource>Core|Ram)`
-	region             = `\w+(?: \w+){0,2}`
+	regionRegex        = `\w+(?: \w+){0,2}`
 	computeOptimized   = `(?P<optimized> ?Compute optimized)`
 	onDemandString     = fmt.Sprintf(`^%v?(?:%v|%v)%v?%v?(?: Instance)? %v running in %v$`,
-		spot,
-		machineType,
+		spotRegex,
+		machineTypeRegex,
 		computeOptimized,
 		n1Suffix,
 		amd,
 		resource,
-		region)
+		regionRegex)
 	reOnDemand = regexp.MustCompile(onDemandString)
 )