From 96eee93da0b66d78f65f8a766d3dbf6c4e6311eb Mon Sep 17 00:00:00 2001 From: Michael Rea Date: Thu, 6 Apr 2023 18:39:44 +0000 Subject: [PATCH 1/2] Add support for tags. --- go/metadata/metadata.go | 20 ++++++++++++++++++++ web/internal/metadata.bzl | 3 +++ web/internal/web_test.bzl | 1 + 3 files changed, 24 insertions(+) diff --git a/go/metadata/metadata.go b/go/metadata/metadata.go index c53b8c2d..5feccd1e 100644 --- a/go/metadata/metadata.go +++ b/go/metadata/metadata.go @@ -50,6 +50,8 @@ type Metadata struct { DebuggerPort int `json:"debuggerPort,omitempty"` // A list of WebTestFiles with named files in them. WebTestFiles []*WebTestFiles `json:"webTestFiles,omitempty"` + // A list of tags. + Tags []string `json:"tags,omitempty"` // An object for any additional metadata fields on this object. Extension `json:"extension,omitempty"` } @@ -107,6 +109,23 @@ func Merge(m1, m2 *Metadata) (*Metadata, error) { return nil, err } + // Merged tags are the concatenated list of unique tags. Note that it is + // important that order is preserved. + var tags []string + tagMap := make(map[string]bool) + for _, t := range m1.Tags { + if !tagMap[t] { + tags = append(tags, t) + tagMap[t] = true + } + } + for _, t := range m2.Tags { + if !tagMap[t] { + tags = append(tags, t) + tagMap[t] = true + } + } + extension := m1.Extension if extension == nil { extension = m2.Extension @@ -127,6 +146,7 @@ func Merge(m1, m2 *Metadata) (*Metadata, error) { ConfigLabel: configLabel, DebuggerPort: debuggerPort, WebTestFiles: webTestFiles, + Tags: tags, Extension: extension, }, nil } diff --git a/web/internal/metadata.bzl b/web/internal/metadata.bzl index 96dccb50..45fab41c 100644 --- a/web/internal/metadata.bzl +++ b/web/internal/metadata.bzl @@ -49,6 +49,7 @@ def _create_file( label = None, test_label = None, web_test_files = None, + tags = None, extension = None): """Generates a web_test metadata file with specified contents. @@ -80,6 +81,8 @@ def _create_file( fields["testLabel"] = str(test_label) if web_test_files: fields["webTestFiles"] = web_test_files + if tags: + fields["tags"] = tags if extension: if type(extension) == type({}): extension = struct(**extension) diff --git a/web/internal/web_test.bzl b/web/internal/web_test.bzl index 0e0e4174..47dab09d 100644 --- a/web/internal/web_test.bzl +++ b/web/internal/web_test.bzl @@ -103,6 +103,7 @@ def _generate_default_test(ctx): config_label = ctx.attr.config.label, label = ctx.label, test_label = ctx.attr.test.label, + tags = ctx.attr.tags.label, ) metadata.merge_files( From d33ff4cc31023631215c3199fc6432899e272c8b Mon Sep 17 00:00:00 2001 From: Michael Rea Date: Thu, 6 Apr 2023 18:47:12 +0000 Subject: [PATCH 2/2] Implement merging of --enable/disable-features and --enable/disable-blink-features in WTL capabilities. --- go/metadata/capabilities/capabilities.go | 159 +++++++++++++++ go/metadata/capabilities/capabilities_test.go | 186 ++++++++++++++++++ 2 files changed, 345 insertions(+) diff --git a/go/metadata/capabilities/capabilities.go b/go/metadata/capabilities/capabilities.go index a24d991d..49bd3a1b 100644 --- a/go/metadata/capabilities/capabilities.go +++ b/go/metadata/capabilities/capabilities.go @@ -20,6 +20,7 @@ import ( "fmt" "reflect" "regexp" + "sort" "strings" ) @@ -725,6 +726,10 @@ func mergeLists(m1, m2 []interface{}) []interface{} { // have prefix "--" or "-", then both same arguments in m1 and m2 will remain. func mergeArgs(m1, m2 []interface{}) []interface{} { m2Opts := map[string]bool{} + leftFeatures := []string{} + rightFeatures := []string{} + leftBlinkFeatures := []string{} + rightBlinkFeatures := []string{} m2Copy := make([]interface{}, 0, len(m2)) for _, a := range m2 { @@ -735,6 +740,15 @@ func mergeArgs(m1, m2 []interface{}) []interface{} { } if strings.HasPrefix(arg, "-") { tokens := strings.Split(arg, "=") + // If config is "--enable/disable-features", we need to merge them with a specail algorithm, see function mergeFeatures. + if strings.Compare(tokens[0], "--enable-features") == 0 || strings.Compare(tokens[0], "--disable-features") == 0 { + rightFeatures = append(rightFeatures, arg) + continue + // If config is "--enable/disable-blink-features", we need to merge them with a specail algorithm, see function mergeFeatures. + } else if strings.Compare(tokens[0], "--enable-blink-features") == 0 || strings.Compare(tokens[0], "--disable-blink-features") == 0 { + rightBlinkFeatures = append(rightBlinkFeatures, arg) + continue + } m2Opts[tokens[0]] = true } } @@ -747,6 +761,13 @@ func mergeArgs(m1, m2 []interface{}) []interface{} { if arg, ok := a.(string); ok { if strings.HasPrefix(arg, "-") { tokens := strings.Split(arg, "=") + if strings.Compare(tokens[0], "--enable-features") == 0 || strings.Compare(tokens[0], "--disable-features") == 0 { + leftFeatures = append(leftFeatures, arg) + continue + } else if strings.Compare(tokens[0], "--enable-blink-features") == 0 || strings.Compare(tokens[0], "--disable-blink-features") == 0 { + leftBlinkFeatures = append(leftBlinkFeatures, arg) + continue + } // Skip options from m1 that are redefined in m2 if m2Opts[tokens[0]] { continue @@ -755,11 +776,149 @@ func mergeArgs(m1, m2 []interface{}) []interface{} { } nl = append(nl, a) } + for _, v := range mergeFeatures("features", leftFeatures, rightFeatures) { + nl = append(nl, v) + } + for _, v := range mergeFeatures("blink-features", leftBlinkFeatures, rightBlinkFeatures) { + nl = append(nl, v) + } nl = append(nl, m2Copy...) return nl } +// mergeFeatures extract four important sets from input and return merged features of "--enable/disable-features" or "--enable/disable-blink-features". +// An example: +// Left Args: Enable: f1, f2; Disable: f2, f3 +// Right Args: Enable: f4; Disable: f1, f3 +// +// 1) disable-features is union of left-hand and right-hand disabled features +// f2,f3 U f1,f3 = f1,f2,f3 +// 2) enabled-features is union of left-hand and right-hand enabled features +// f1,f2 U f4 = f1,f2,f4 +// 3) remove right-hand enabled features from disabled features +// f1,f2,f3 - f4 = f1,f2,f3 +// 4) remove right-hand disabled features from enabled features +// f1,f2,f4 - f1,f3 = f2,f4 +// +// Final result is: Enable: f1,f2,f3; Disable: f2, f4. +// +// Note: in this example, f2 will be kept in both sides, since chrome allow a feature to be enable and disabled as the same time. +// If this case happens, f2 will be considered disabled by default. +func mergeFeatures(argName string, leftFeatures []string, rightFeatures []string) []string { + result := []string{} + if len(leftFeatures) == 0 && len(rightFeatures) == 0 { + return result + } else if len(leftFeatures) == 0 { + result = append(result, rightFeatures...) + return result + } else if len(rightFeatures) == 0 { + result = append(result, leftFeatures...) + return result + } + enableLeftFeatures := map[string]bool{} + enableRightFeatures := map[string]bool{} + disableLeftFeatures := map[string]bool{} + disableRightFeatures := map[string]bool{} + for _, arg := range leftFeatures { + tokens := strings.Split(arg, "=") + if strings.Compare(tokens[0], "--enable-"+argName) == 0 && tokens[1] != "" { + features := strings.Split(tokens[1], ",") + for _, f := range features { + enableLeftFeatures[f] = true + } + } else if strings.Compare(tokens[0], "--disable-"+argName) == 0 && tokens[1] != "" { + features := strings.Split(tokens[1], ",") + for _, f := range features { + disableLeftFeatures[f] = true + } + } + } + for _, arg := range rightFeatures { + tokens := strings.Split(arg, "=") + if strings.Compare(tokens[0], "--enable-"+argName) == 0 { + // if right enable feature value is empty, then clear all enable features. + if tokens[1] == "" { + enableLeftFeatures = map[string]bool{} + continue + } + features := strings.Split(tokens[1], ",") + for _, f := range features { + enableRightFeatures[f] = true + } + } else if strings.Compare(tokens[0], "--disable-"+argName) == 0 { + // if right disable feature value is empty, then clear all disable features. + if tokens[1] == "" { + disableLeftFeatures = map[string]bool{} + continue + } + features := strings.Split(tokens[1], ",") + for _, f := range features { + disableRightFeatures[f] = true + } + } + } + + result = append(result, mergeSingleTypeFeatures(argName, enableLeftFeatures, enableRightFeatures, disableLeftFeatures, disableRightFeatures)...) + return result +} + +// mergeSingleTypeFeatures return merged result of one type of features, the input feature could be "--enable/disable-features" or "--enable/disable-blink-features". +func mergeSingleTypeFeatures(argName string, enableLeftFeatures map[string]bool, enableRightFeatures map[string]bool, disableLeftFeatures map[string]bool, disableRightFeatures map[string]bool) []string { + result := []string{} + enableFeatures := map[string]bool{} + disableFeatures := map[string]bool{} + // Step 1: union enable-features and union disable-features. + for f := range enableLeftFeatures { + enableFeatures[f] = true + } + for f := range enableRightFeatures { + enableFeatures[f] = true + } + for f := range disableLeftFeatures { + disableFeatures[f] = true + } + for f := range disableRightFeatures { + disableFeatures[f] = true + } + // Step 2: remove right-hand enabled features from unioned disabled features and remove right-hand disabled features from unioned enabled features. + for f := range enableRightFeatures { + if !disableRightFeatures[f] && disableFeatures[f] { + delete(disableFeatures, f) + } + } + for f := range disableRightFeatures { + if !enableRightFeatures[f] && enableFeatures[f] { + delete(enableFeatures, f) + } + } + // Step 3: generate final result with sorted features from union enable-features and union disable-features. + if len(enableFeatures) > 0 { + sortedEnableFeatures := []string{} + for f := range enableFeatures { + sortedEnableFeatures = append(sortedEnableFeatures, f) + } + sort.Strings(sortedEnableFeatures) + s := "--enable-" + argName + "=" + for _, sf := range sortedEnableFeatures { + s = s + sf + "," + } + result = append(result, s[0:len(s)-1]) + } + if len(disableFeatures) > 0 { + sortedDisableFeatures := []string{} + for f := range disableFeatures { + sortedDisableFeatures = append(sortedDisableFeatures, f) + } + sort.Strings(sortedDisableFeatures) + s := "--disable-" + argName + "=" + for _, sf := range sortedDisableFeatures { + s = s + sf + "," + } + result = append(result, s[0:len(s)-1]) + } + return result +} // CanReuseSession returns true if the "google:canReuseSession" is set. func CanReuseSession(caps *Capabilities) bool { reuse, _ := caps.AlwaysMatch["google:canReuseSession"].(bool) diff --git a/go/metadata/capabilities/capabilities_test.go b/go/metadata/capabilities/capabilities_test.go index e5322153..4d47af63 100644 --- a/go/metadata/capabilities/capabilities_test.go +++ b/go/metadata/capabilities/capabilities_test.go @@ -585,6 +585,192 @@ func TestMerge(t *testing.T) { } } +func TestMergeFeatures(t *testing.T) { + testCases := []struct { + name string + input1 map[string]interface{} + input2 map[string]interface{} + result map[string]interface{} + }{ + { + name: "merge features that only exist on one side", + input1: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f2", + }, + }, + input2: map[string]interface{}{ + "args": []interface{}{ + "--disable-blink-features=f3,f5", + }, + }, + result: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f2", + "--disable-blink-features=f3,f5", + }, + }, + }, + { + name: "merge only --enable-feature and disable-blink-feature", + input1: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f2", + "--disable-blink-features=f3,f5", + }, + }, + input2: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f4", + "--disable-blink-features=f3,f5", + }, + }, + result: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f2,f4", + "--disable-blink-features=f3,f5", + }, + }, + }, + { + name: "merge --enable/disable-feature with a left hand side feature stated both in enable and disable, result should keep both if none of them is covered by right side args", + input1: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f3", + "--disable-features=f3,f4", + }, + }, + input2: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f4", + "--disable-features=f1,f5", + }, + }, + result: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f3,f4", + "--disable-features=f1,f3,f5", + }, + }, + }, + { + name: "merge --enable/disable-feature with a right hand side feature stated both in enable and disable, result should keep both", + input1: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f2,f4", + "--disable-features=f3,f4", + }, + }, + input2: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f4", + "--disable-features=f4,f5", + }, + }, + result: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f2,f4", + "--disable-features=f3,f4,f5", + }, + }, + }, + { + name: "merge both --enable/disable-feature and --enable/disable-blink-feature", + input1: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f2", + "--disable-features=f3,f4", + "--enable-blink-features=f1", + "--disable-blink-features=f3", + }, + }, + input2: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f4", + "--disable-features=f1,f5", + "--enable-blink-features=f3", + "--disable-blink-features=f1", + }, + }, + result: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f2,f4", + "--disable-features=f1,f3,f5", + "--enable-blink-features=f3", + "--disable-blink-features=f1", + }, + }, + }, + { + name: "merge only --enable/disable-feature with a feature stated twice in one place", + input1: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f1,f2", + "--disable-features=f3", + }, + }, + input2: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f4", + "--disable-features=f3", + }, + }, + result: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f2,f4", + "--disable-features=f3", + }, + }, + }, + { + name: "merge only --enable-feature with a feature has empty value", + input1: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=", + }, + }, + input2: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f4,f1", + }, + }, + result: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f4", + }, + }, + }, + { + name: "merge only --enable-feature with a feature has empty value on the right", + input1: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=f1,f4", + "--disable-features=f1,f3", + }, + }, + input2: map[string]interface{}{ + "args": []interface{}{ + "--enable-features=", + "--disable-features=f2,f5", + }, + }, + result: map[string]interface{}{ + "args": []interface{}{ + "--disable-features=f1,f2,f3,f5", + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if result := Merge(tc.input1, tc.input2); !reflect.DeepEqual(tc.result, result) { + t.Errorf("Got Merge(%+v, %+v) == %+v, expected %+v", tc.input1, tc.input2, result, tc.result) + } + }) + } +} + func TestFromNewSessionArgs(t *testing.T) { testCases := []struct { name string