From c57be6f7f5ea70389459de202d00f3fbd8be9849 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 17 Apr 2024 19:43:45 +0200 Subject: [PATCH] e2e framework: clarify Alpha/Beta requirement for feature gates We want: - To keep test annotations simple, using both WithFeatureGate and WithFeature should only be necessary when a test really has requirements that go beyond "feature gate needs to be enabled". - To run tests which depend only on feature gates being enabled in the ci-kubernetes-e2e-kind-alpha-features resp. ci-kubernetes-e2e-kind-beta-features, because otherwise we may have a proliferation of many bespoke jobs which only run very few tests. This would make testing more expensive for Kubernetes. - To enable those tests only once in the ci-kubernetes-e2e-kind-alpha-features and ci-kubernetes-e2e-kind-beta-features definition instead of having to update those each time feature gates change. This can be achieved by adding `Feature:Alpha` resp. `Feature:Beta` as Ginkgo labels instead of just `Alpha` and `Beta`. Then jobs which are configured to skip tests with feature dependencies via --label-filter=!/Feature:.+/ will skip tests which are labeled with just WithFeatureGate. The ci-kubernetes jobs can select to include such tests with a special regexp that mimicks a negative lookahead (see k8s.io/community/contributors/devel/sig-testing/e2e-tests.md) Note that removing WithFeature depends on first updating job definitions to use --label-filter or to skip based on the inline `[Alpha]` or `[Beta]` text, otherwise tests that were previously skipped because of WithFeature might start to run in jobs which don't have the feature gate enabled. --- test/e2e/framework/ginkgowrapper.go | 28 +++++++++++++++---- .../framework/internal/unittests/bugs/bugs.go | 4 +-- .../unittests/list-labels/listlabels_test.go | 2 +- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/test/e2e/framework/ginkgowrapper.go b/test/e2e/framework/ginkgowrapper.go index 25710efefd3..8517e4b5182 100644 --- a/test/e2e/framework/ginkgowrapper.go +++ b/test/e2e/framework/ginkgowrapper.go @@ -209,8 +209,9 @@ func registerInSuite(ginkgoCall func(string, ...interface{}) bool, args []interf case label: fullLabel := strings.Join(arg.parts, ":") addLabel(fullLabel) - if arg.extra != "" { - addLabel(arg.extra) + if arg.extraFeature != "" { + texts = append(texts, fmt.Sprintf("[%s]", arg.extraFeature)) + ginkgoArgs = append(ginkgoArgs, ginkgo.Label("Feature:"+arg.extraFeature)) } if fullLabel == "Serial" { ginkgoArgs = append(ginkgoArgs, ginkgo.Serial) @@ -309,6 +310,10 @@ func validateText(location types.CodeLocation, text string, labels []string) { recordTextBug(location, fmt.Sprintf("[%s] in plain text is deprecated and must be added through With%s instead", tag, tag)) } if deprecatedStability.Has(tag) { + if slices.Contains(labels, "Feature:"+tag) { + // Okay, was also set as label. + continue + } recordTextBug(location, fmt.Sprintf("[%s] in plain text is deprecated and must be added by defining the feature gate through WithFeatureGate instead", tag)) } if index := strings.Index(tag, ":"); index > 0 { @@ -353,6 +358,16 @@ func withFeature(name Feature) interface{} { // [k8s.io/apiserver/pkg/util/feature.DefaultMutableFeatureGate]. Once a // feature gate gets removed from there, the WithFeatureGate calls using it // also need to be removed. +// +// [Alpha] resp. [Beta] get added to the test name automatically depending +// on the current stability level of the feature. Feature:Alpha resp. +// Feature:Beta get added to the Ginkgo labels because this is a special +// requirement for how the cluster needs to be configured. +// +// If the test can run in any cluster that has alpha resp. beta features and +// API groups enabled, then annotating it with just WithFeatureGate is +// sufficient. Otherwise, WithFeature has to be used to define the additional +// requirements. func WithFeatureGate(featureGate featuregate.Feature) interface{} { return withFeatureGate(featureGate) } @@ -376,7 +391,7 @@ func withFeatureGate(featureGate featuregate.Feature) interface{} { } l := newLabel("FeatureGate", string(featureGate)) - l.extra = level + l.extraFeature = level return l } @@ -544,8 +559,9 @@ func withFlaky() interface{} { type label struct { // parts get concatenated with ":" to build the full label. parts []string - // extra is an optional fully-formed extra label. - extra string + // extra is an optional feature name. It gets added as [] + // to the test name and as Feature: to the labels. + extraFeature string // explanation gets set for each label to help developers // who pass a label to a ginkgo function. They need to use // the corresponding framework function instead. @@ -572,7 +588,7 @@ func TagsEqual(a, b interface{}) bool { if !ok { return false } - if al.extra != bl.extra { + if al.extraFeature != bl.extraFeature { return false } return slices.Equal(al.parts, bl.parts) diff --git a/test/e2e/framework/internal/unittests/bugs/bugs.go b/test/e2e/framework/internal/unittests/bugs/bugs.go index a70e042f892..114c82b8af5 100644 --- a/test/e2e/framework/internal/unittests/bugs/bugs.go +++ b/test/e2e/framework/internal/unittests/bugs/bugs.go @@ -133,12 +133,12 @@ ERROR: some/relative/path/buggy.go:200: with spaces // Used by unittests/list-labels. ListLabelsOutput = `The following labels can be used with 'ginkgo run --label-filter': - Alpha - Beta Conformance Disruptive Environment:Linux Environment:no-such-env + Feature:Alpha + Feature:Beta Feature:feature-foo Feature:no-such-feature FeatureGate:TestAlphaFeature diff --git a/test/e2e/framework/internal/unittests/list-labels/listlabels_test.go b/test/e2e/framework/internal/unittests/list-labels/listlabels_test.go index 95b0416d9a4..3ec9737865f 100644 --- a/test/e2e/framework/internal/unittests/list-labels/listlabels_test.go +++ b/test/e2e/framework/internal/unittests/list-labels/listlabels_test.go @@ -26,7 +26,7 @@ import ( "k8s.io/kubernetes/test/e2e/framework/internal/unittests/bugs" ) -func TestListTests(t *testing.T) { +func TestListLabels(t *testing.T) { bugs.Describe() framework.CheckForBugs = false output, code := unittests.GetFrameworkOutput(t, map[string]string{"list-labels": "true"})