fix: Parse multiple label values for same key

kubernetes-sigs · Feb 3, 2023 · 35ab1e9 · 35ab1e9
1 parent 33c3d5e
commit 35ab1e9
Show file tree

Hide file tree

Showing 8 changed files with 148 additions and 34 deletions.
diff --git a/pkg/env/env.go b/pkg/env/env.go
@@ -512,20 +512,24 @@ func (e *testEnv) requireProcessing(kind, testName string, requiredRegexp, skipR
 	}
 
 	if labels != nil {
-		for k, v := range e.cfg.Labels() {
-			if labels[k] != v {
-				skip = true
-				message = fmt.Sprintf(`Skipping feature "%s": unmatched label "%s=%s"`, testName, k, labels[k])
-				return skip, message
+		for key, vals := range e.cfg.Labels() {
+			for _, v := range vals {
+				if !labels.Contains(key, v) {
+					skip = true
+					message = fmt.Sprintf(`Skipping feature "%s": unmatched label "%s=%s"`, testName, key, v)
+					return skip, message
+				}
 			}
 		}
 
 		// skip running a feature if labels matches with --skip-labels
-		for k, v := range e.cfg.SkipLabels() {
-			if labels[k] == v {
-				skip = true
-				message = fmt.Sprintf(`Skipping feature "%s": matched label provided in --skip-lables "%s=%s"`, testName, k, labels[k])
-				return skip, message
+		for key, vals := range e.cfg.SkipLabels() {
+			for _, v := range vals {
+				if labels.Contains(key, v) {
+					skip = true
+					message = fmt.Sprintf(`Skipping feature "%s": matched label provided in --skip-lables "%s=%s"`, testName, key, labels[key])
+					return skip, message
+				}
 			}
 		}
 	}
@@ -536,8 +540,10 @@ func (e *testEnv) requireProcessing(kind, testName string, requiredRegexp, skipR
 // copy to avoid mutation when we just want an informational copy.
 func deepCopyFeature(f types.Feature) types.Feature {
 	fcopy := features.New(f.Name())
-	for k, v := range f.Labels() {
-		fcopy = fcopy.WithLabel(k, v)
+	for k, vals := range f.Labels() {
+		for _, v := range vals {
+			fcopy = fcopy.WithLabel(k, v)
+		}
 	}
 	f.Steps()
 	for _, step := range f.Steps() {

diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go
@@ -450,12 +450,12 @@ func TestEnv_Test(t *testing.T) {
 
 					// Ensure changes aren't persisted to the afterEachFeature hook
 					labelMap := info.Labels()
-					labelMap["foo"] = "bar"
+					labelMap["foo"] = []string{"bar"}
 					return ctx, nil
 				}).AfterEachFeature(func(ctx context.Context, _ *envconf.Config, _ *testing.T, info features.Feature) (context.Context, error) {
 					val = append(val, "after-each-feature")
 					t.Logf("%#v, len(steps)=%v\n", info, len(info.Steps()))
-					if info.Labels()["foo"] == "bar" {
+					if info.Labels().Contains("foo", "bar") {
 						t.Errorf("Expected label from previous feature hook to not include foo:bar")
 					}
 					if len(info.Steps()) == 0 {

diff --git a/pkg/envconf/config.go b/pkg/envconf/config.go
@@ -36,9 +36,9 @@ type Config struct {
 	namespace               string
 	assessmentRegex         *regexp.Regexp
 	featureRegex            *regexp.Regexp
-	labels                  map[string]string
+	labels                  flags.LabelsMap
 	skipFeatureRegex        *regexp.Regexp
-	skipLabels              map[string]string
+	skipLabels              flags.LabelsMap
 	skipAssessmentRegex     *regexp.Regexp
 	parallelTests           bool
 	dryRun                  bool
@@ -203,24 +203,24 @@ func (c *Config) SkipFeatureRegex() *regexp.Regexp {
 }
 
 // WithLabels sets the environment label filters
-func (c *Config) WithLabels(lbls map[string]string) *Config {
+func (c *Config) WithLabels(lbls map[string][]string) *Config {
 	c.labels = lbls
 	return c
 }
 
 // Labels returns the environment's label filters
-func (c *Config) Labels() map[string]string {
+func (c *Config) Labels() map[string][]string {
 	return c.labels
 }
 
 // WithSkipLabels sets the environment label filters
-func (c *Config) WithSkipLabels(lbls map[string]string) *Config {
+func (c *Config) WithSkipLabels(lbls map[string][]string) *Config {
 	c.skipLabels = lbls
 	return c
 }
 
 // SkipLabels returns the environment's label filters
-func (c *Config) SkipLabels() map[string]string {
+func (c *Config) SkipLabels() map[string][]string {
 	return c.skipLabels
 }
 

diff --git a/pkg/features/builder.go b/pkg/features/builder.go
@@ -34,7 +34,7 @@ func New(name string) *FeatureBuilder {
 
 // WithLabel adds a test label key/value pair
 func (b *FeatureBuilder) WithLabel(key, value string) *FeatureBuilder {
-	b.feat.labels[key] = value
+	b.feat.labels[key] = append(b.feat.labels[key], value)
 	return b
 }
 

diff --git a/pkg/features/builder_test.go b/pkg/features/builder_test.go
@@ -58,13 +58,23 @@ func TestFeatureBuilder(t *testing.T) {
 		{
 			name: "with labels",
 			setup: func(t *testing.T) types.Feature {
-				return New("test").WithLabel("a", "b").WithLabel("c", "d").Feature()
+				return New("test").
+					WithLabel("a", "a").
+					WithLabel("a", "aa").
+					WithLabel("b", "b").
+					Feature()
 			},
 			eval: func(t *testing.T, f types.Feature) {
 				ft := f.(*defaultFeature) // nolint
 				if len(ft.labels) != 2 {
 					t.Error("unexpected labels len:", len(ft.labels))
 				}
+				if len(ft.labels["a"]) != 2 {
+					t.Errorf("unexpected label values for %q: %q", "a", ft.labels["a"])
+				}
+				if len(ft.labels["b"]) != 1 {
+					t.Errorf("unexpected label values for %q: %q", "b", ft.labels["b"])
+				}
 			},
 		},
 		{
@@ -116,7 +126,7 @@ func TestFeatureBuilder(t *testing.T) {
 				}).Feature()
 			},
 			eval: func(t *testing.T, f types.Feature) {
-				ft := f.(*defaultFeature) //nolint
+				ft := f.(*defaultFeature) // nolint
 				setups := GetStepsByLevel(ft.Steps(), types.LevelSetup)
 				if setups[0].Name() != "setup-test" {
 					t.Errorf("unexpected setup name: %s", setups[0].Name())
@@ -172,7 +182,7 @@ func TestFeatureBuilder(t *testing.T) {
 				}).Feature()
 			},
 			eval: func(t *testing.T, f types.Feature) {
-				ft := f.(*defaultFeature) //nolint
+				ft := f.(*defaultFeature) // nolint
 				setups := GetStepsByLevel(ft.Steps(), types.LevelTeardown)
 				if setups[0].Name() != "teardown-test" {
 					t.Errorf("unexpected teardown name: %s", setups[0].Name())

diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go
@@ -280,10 +280,10 @@ func ParseArgs(args []string) (*EnvFlags, error) {
 	}, nil
 }
 
-type LabelsMap map[string]string
+type LabelsMap map[string][]string
 
 func (m LabelsMap) String() string {
-	i := map[string]string(m)
+	i := map[string][]string(m)
 	return fmt.Sprint(i)
 }
 
@@ -295,8 +295,19 @@ func (m LabelsMap) Set(val string) error {
 		if len(kv) != 2 {
 			return fmt.Errorf("label format error: %s", label)
 		}
-		m[strings.TrimSpace(kv[0])] = strings.TrimSpace(kv[1])
+		k := strings.TrimSpace(kv[0])
+		v := strings.TrimSpace(kv[1])
+		m[k] = append(m[k], v)
 	}
 
 	return nil
 }
+
+func (m LabelsMap) Contains(key, val string) bool {
+	for _, v := range m[key] {
+		if val == v {
+			return true
+		}
+	}
+	return false
+}
diff --git a/pkg/flags/flags_test.go b/pkg/flags/flags_test.go
@@ -18,6 +18,7 @@ package flags
 
 import (
 	"flag"
+	"reflect"
 	"testing"
 )
 
@@ -29,8 +30,8 @@ func TestParseFlags(t *testing.T) {
 	}{
 		{
 			name:  "with all",
-			args:  []string{"-assess", "volume test", "--feature", "beta", "--labels", "k0=v0, k1=v1, k2=v2", "--skip-labels", "k0=v0, k1=v1", "-skip-features", "networking", "-skip-assessment", "volume test", "-parallel", "--dry-run", "--disable-graceful-teardown"},
-			flags: &EnvFlags{assess: "volume test", feature: "beta", labels: LabelsMap{"k0": "v0", "k1": "v1", "k2": "v2"}, skiplabels: LabelsMap{"k0": "v0", "k1": "v1"}, skipFeatures: "networking", skipAssessments: "volume test"},
+			args:  []string{"-assess", "volume test", "--feature", "beta", "--labels", "k0=v0, k0=v01, k1=v1, k1=v11, k2=v2", "--skip-labels", "k0=v0, k1=v1", "-skip-features", "networking", "-skip-assessment", "volume test", "-parallel", "--dry-run", "--disable-graceful-teardown"},
+			flags: &EnvFlags{assess: "volume test", feature: "beta", labels: LabelsMap{"k0": {"v0", "v01"}, "k1": {"v1", "v11"}, "k2": {"v2"}}, skiplabels: LabelsMap{"k0": {"v0"}, "k1": {"v1"}}, skipFeatures: "networking", skipAssessments: "volume test"},
 		},
 	}
 
@@ -50,14 +51,14 @@ func TestParseFlags(t *testing.T) {
 			}
 
 			for k, v := range testFlags.Labels() {
-				if test.flags.Labels()[k] != v {
-					t.Errorf("unmatched label %s=%s", k, test.flags.Labels()[k])
+				if !reflect.DeepEqual(test.flags.Labels()[k], v) {
+					t.Errorf("unmatched labels %s=%v", k, test.flags.Labels()[k])
 				}
 			}
 
 			for k, v := range testFlags.SkipLabels() {
-				if test.flags.SkipLabels()[k] != v {
-					t.Errorf("unmatched skip label %s=%s", k, test.flags.SkipLabels()[k])
+				if !reflect.DeepEqual(test.flags.SkipLabels()[k], v) {
+					t.Errorf("unmatched skip labels %s=%v", k, test.flags.Labels()[k])
 				}
 			}
 
@@ -83,3 +84,88 @@ func TestParseFlags(t *testing.T) {
 		})
 	}
 }
+
+func TestLabelsMap_Contains(t *testing.T) {
+	type args struct {
+		key string
+		val string
+	}
+	tests := []struct {
+		name string
+		m    LabelsMap
+		args args
+		want bool
+	}{
+		{
+			name: "empty map",
+			m:    LabelsMap{},
+			args: args{
+				key: "somekey",
+				val: "someval",
+			},
+			want: false,
+		},
+		{
+			name: "key does not exist",
+			m:    LabelsMap{"key0": {"val0"}},
+			args: args{
+				key: "key1",
+				val: "val1",
+			},
+			want: false,
+		},
+		{
+			// TODO (@embano1): #https://github.com/kubernetes-sigs/e2e-framework/issues/198
+			name: "lower-case key for upper case key does not exist",
+			m:    LabelsMap{"Key0": {"val0"}},
+			args: args{
+				key: "key1",
+				val: "val1",
+			},
+			want: false,
+		},
+		{
+			name: "value for existing key does not exist",
+			m:    LabelsMap{"key0": {"val0"}},
+			args: args{
+				key: "key0",
+				val: "val1",
+			},
+			want: false,
+		},
+		{
+			name: "value for map with one key with one value exists",
+			m:    LabelsMap{"key0": {"val0"}},
+			args: args{
+				key: "key0",
+				val: "val0",
+			},
+			want: true,
+		},
+		{
+			name: "value for map with one key with multiple values exists",
+			m:    LabelsMap{"key0": {"val0", "val1"}},
+			args: args{
+				key: "key0",
+				val: "val1",
+			},
+			want: true,
+		},
+		{
+			name: "value for map with multiple keys and values exists",
+			m:    LabelsMap{"key0": {"val0", "val1"}, "key1": {"val1"}},
+			args: args{
+				key: "key1",
+				val: "val1",
+			},
+			want: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := tt.m.Contains(tt.args.key, tt.args.val); got != tt.want {
+				t.Errorf("Contains() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/pkg/internal/types/types.go b/pkg/internal/types/types.go
@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"sigs.k8s.io/e2e-framework/pkg/envconf"
+	"sigs.k8s.io/e2e-framework/pkg/flags"
 )
 
 // EnvFunc represents a user-defined operation that
@@ -86,7 +87,7 @@ type Environment interface {
 	Run(*testing.M) int
 }
 
-type Labels map[string]string
+type Labels = flags.LabelsMap
 
 type Feature interface {
 	// Name is a descriptive text for the feature