diff --git a/featuregate/testing/feature_gate.go b/featuregate/testing/feature_gate.go index 907906e..9884df0 100644 --- a/featuregate/testing/feature_gate.go +++ b/featuregate/testing/feature_gate.go @@ -18,18 +18,35 @@ package testing import ( "fmt" + "strings" + "sync" "testing" "k8s.io/component-base/featuregate" ) -// SetFeatureGateDuringTest sets the specified gate to the specified value, and returns a function that restores the original value. -// Failures to set or restore cause the test to fail. +var ( + overrideLock sync.Mutex + featureFlagOverride map[featuregate.Feature]string +) + +func init() { + featureFlagOverride = map[featuregate.Feature]string{} +} + +// SetFeatureGateDuringTest sets the specified gate to the specified value for duration of the test. +// Fails when it detects second call to the same flag or is unable to set or restore feature flag. +// Returns empty cleanup function to maintain the old function signature that uses defer. +// TODO: Remove defer from calls to SetFeatureGateDuringTest and update hack/verify-test-featuregates.sh when we can do large scale code change. +// +// WARNING: Can leak set variable when called in test calling t.Parallel(), however second attempt to set the same feature flag will cause fatal. // // Example use: // // defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features., true)() func SetFeatureGateDuringTest(tb testing.TB, gate featuregate.FeatureGate, f featuregate.Feature, value bool) func() { + tb.Helper() + detectParallelOverrideCleanup := detectParallelOverride(tb, f) originalValue := gate.Enabled(f) // Specially handle AllAlpha and AllBeta @@ -50,12 +67,41 @@ func SetFeatureGateDuringTest(tb testing.TB, gate featuregate.FeatureGate, f fea tb.Errorf("error setting %s=%v: %v", f, value, err) } - return func() { + tb.Cleanup(func() { + tb.Helper() + detectParallelOverrideCleanup() if err := gate.(featuregate.MutableFeatureGate).Set(fmt.Sprintf("%s=%v", f, originalValue)); err != nil { tb.Errorf("error restoring %s=%v: %v", f, originalValue, err) } for _, cleanup := range cleanups { cleanup() } + }) + return func() {} +} + +func detectParallelOverride(tb testing.TB, f featuregate.Feature) func() { + tb.Helper() + overrideLock.Lock() + defer overrideLock.Unlock() + beforeOverrideTestName := featureFlagOverride[f] + if beforeOverrideTestName != "" && !sameTestOrSubtest(tb, beforeOverrideTestName) { + tb.Fatalf("Detected parallel setting of a feature gate by both %q and %q", beforeOverrideTestName, tb.Name()) + } + featureFlagOverride[f] = tb.Name() + + return func() { + tb.Helper() + overrideLock.Lock() + defer overrideLock.Unlock() + if afterOverrideTestName := featureFlagOverride[f]; afterOverrideTestName != tb.Name() { + tb.Fatalf("Detected parallel setting of a feature gate between both %q and %q", afterOverrideTestName, tb.Name()) + } + featureFlagOverride[f] = beforeOverrideTestName } } + +func sameTestOrSubtest(tb testing.TB, testName string) bool { + // Assumes that "/" is not used in test names. + return tb.Name() == testName || strings.HasPrefix(tb.Name(), testName+"/") +} diff --git a/featuregate/testing/feature_gate_test.go b/featuregate/testing/feature_gate_test.go index d16d210..a8ea878 100644 --- a/featuregate/testing/feature_gate_test.go +++ b/featuregate/testing/feature_gate_test.go @@ -19,6 +19,8 @@ package testing import ( gotest "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "k8s.io/component-base/featuregate" ) @@ -67,8 +69,11 @@ func TestSpecialGates(t *gotest.T) { "stable_default_off_set_on": true, } expect(t, gate, before) + t.Cleanup(func() { + expect(t, gate, before) + }) - cleanupAlpha := SetFeatureGateDuringTest(t, gate, "AllAlpha", true) + SetFeatureGateDuringTest(t, gate, "AllAlpha", true) expect(t, gate, map[featuregate.Feature]bool{ "AllAlpha": true, "AllBeta": false, @@ -89,7 +94,7 @@ func TestSpecialGates(t *gotest.T) { "stable_default_off_set_on": true, }) - cleanupBeta := SetFeatureGateDuringTest(t, gate, "AllBeta", true) + SetFeatureGateDuringTest(t, gate, "AllBeta", true) expect(t, gate, map[featuregate.Feature]bool{ "AllAlpha": true, "AllBeta": true, @@ -109,11 +114,6 @@ func TestSpecialGates(t *gotest.T) { "stable_default_off": false, "stable_default_off_set_on": true, }) - - // run cleanups in reverse order like defer would - cleanupBeta() - cleanupAlpha() - expect(t, gate, before) } func expect(t *gotest.T, gate featuregate.FeatureGate, expect map[featuregate.Feature]bool) { @@ -124,3 +124,127 @@ func expect(t *gotest.T, gate featuregate.FeatureGate, expect map[featuregate.Fe } } } + +func TestSetFeatureGateInTest(t *gotest.T) { + gate := featuregate.NewFeatureGate() + err := gate.Add(map[featuregate.Feature]featuregate.FeatureSpec{ + "feature": {PreRelease: featuregate.Alpha, Default: false}, + }) + require.NoError(t, err) + + assert.False(t, gate.Enabled("feature")) + defer SetFeatureGateDuringTest(t, gate, "feature", true)() + defer SetFeatureGateDuringTest(t, gate, "feature", true)() + + assert.True(t, gate.Enabled("feature")) + t.Run("Subtest", func(t *gotest.T) { + assert.True(t, gate.Enabled("feature")) + }) + + t.Run("ParallelSubtest", func(t *gotest.T) { + assert.True(t, gate.Enabled("feature")) + // Calling t.Parallel in subtest will resume the main test body + t.Parallel() + assert.True(t, gate.Enabled("feature")) + }) + assert.True(t, gate.Enabled("feature")) + + t.Run("OverwriteInSubtest", func(t *gotest.T) { + defer SetFeatureGateDuringTest(t, gate, "feature", false)() + assert.False(t, gate.Enabled("feature")) + }) + assert.True(t, gate.Enabled("feature")) +} + +func TestDetectLeakToMainTest(t *gotest.T) { + t.Cleanup(func() { + featureFlagOverride = map[featuregate.Feature]string{} + }) + gate := featuregate.NewFeatureGate() + err := gate.Add(map[featuregate.Feature]featuregate.FeatureSpec{ + "feature": {PreRelease: featuregate.Alpha, Default: false}, + }) + require.NoError(t, err) + + // Subtest setting feature gate and calling parallel will leak it out + t.Run("LeakingSubtest", func(t *gotest.T) { + fakeT := &ignoreFatalT{T: t} + defer SetFeatureGateDuringTest(fakeT, gate, "feature", true)() + // Calling t.Parallel in subtest will resume the main test body + t.Parallel() + // Leaked false from main test + assert.False(t, gate.Enabled("feature")) + }) + // Leaked true from subtest + assert.True(t, gate.Enabled("feature")) + fakeT := &ignoreFatalT{T: t} + defer SetFeatureGateDuringTest(fakeT, gate, "feature", false)() + assert.True(t, fakeT.fatalRecorded) +} + +func TestDetectLeakToOtherSubtest(t *gotest.T) { + t.Cleanup(func() { + featureFlagOverride = map[featuregate.Feature]string{} + }) + gate := featuregate.NewFeatureGate() + err := gate.Add(map[featuregate.Feature]featuregate.FeatureSpec{ + "feature": {PreRelease: featuregate.Alpha, Default: false}, + }) + require.NoError(t, err) + + subtestName := "Subtest" + // Subtest setting feature gate and calling parallel will leak it out + t.Run(subtestName, func(t *gotest.T) { + fakeT := &ignoreFatalT{T: t} + defer SetFeatureGateDuringTest(fakeT, gate, "feature", true)() + t.Parallel() + }) + // Add suffix to name to prevent tests with the same prefix. + t.Run(subtestName+"Suffix", func(t *gotest.T) { + // Leaked true + assert.True(t, gate.Enabled("feature")) + + fakeT := &ignoreFatalT{T: t} + defer SetFeatureGateDuringTest(fakeT, gate, "feature", false)() + assert.True(t, fakeT.fatalRecorded) + }) +} + +func TestCannotDetectLeakFromSubtest(t *gotest.T) { + t.Cleanup(func() { + featureFlagOverride = map[featuregate.Feature]string{} + }) + gate := featuregate.NewFeatureGate() + err := gate.Add(map[featuregate.Feature]featuregate.FeatureSpec{ + "feature": {PreRelease: featuregate.Alpha, Default: false}, + }) + require.NoError(t, err) + + defer SetFeatureGateDuringTest(t, gate, "feature", false)() + // Subtest setting feature gate and calling parallel will leak it out + t.Run("Subtest", func(t *gotest.T) { + defer SetFeatureGateDuringTest(t, gate, "feature", true)() + t.Parallel() + }) + // Leaked true + assert.True(t, gate.Enabled("feature")) +} + +type ignoreFatalT struct { + *gotest.T + fatalRecorded bool +} + +func (f *ignoreFatalT) Fatal(args ...any) { + f.T.Helper() + f.fatalRecorded = true + newArgs := []any{"[IGNORED]"} + newArgs = append(newArgs, args...) + f.T.Log(newArgs...) +} + +func (f *ignoreFatalT) Fatalf(format string, args ...any) { + f.T.Helper() + f.fatalRecorded = true + f.T.Logf("[IGNORED] "+format, args...) +} diff --git a/go.mod b/go.mod index 48b36bf..6037daa 100644 --- a/go.mod +++ b/go.mod @@ -25,8 +25,8 @@ require ( go.uber.org/zap v1.26.0 golang.org/x/sys v0.17.0 gopkg.in/yaml.v2 v2.4.0 - k8s.io/apimachinery v0.0.0-20240306164812-cbfe0a1feaa5 - k8s.io/client-go v0.0.0-20240306170515-0cdc0ce850af + k8s.io/apimachinery v0.0.0-20240307171817-d82afe1e363a + k8s.io/client-go v0.0.0-20240309200420-7ebe0ea60e0a k8s.io/klog/v2 v2.120.1 k8s.io/utils v0.0.0-20230726121419-3b25d923346b sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd @@ -76,14 +76,10 @@ require ( google.golang.org/protobuf v1.33.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/api v0.0.0-20240306165540-05aa4bceed70 // indirect + k8s.io/api v0.0.0-20240311194616-96558b97565e // indirect k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect sigs.k8s.io/yaml v1.3.0 // indirect ) -replace ( - k8s.io/api => k8s.io/api v0.0.0-20240306165540-05aa4bceed70 - k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20240306164812-cbfe0a1feaa5 - k8s.io/client-go => k8s.io/client-go v0.0.0-20240306170515-0cdc0ce850af -) +replace k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20240307160843-0407311be590 diff --git a/go.sum b/go.sum index 6f4b689..0b87084 100644 --- a/go.sum +++ b/go.sum @@ -210,12 +210,12 @@ gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -k8s.io/api v0.0.0-20240306165540-05aa4bceed70 h1:Kw6GufAvqr668v56lckDWoGHFG5wwjQRjYuf+PMt1us= -k8s.io/api v0.0.0-20240306165540-05aa4bceed70/go.mod h1:S69aw/5045kbDeBVmy89EQxSM7v5kHdLNzO4wt3OZ2o= -k8s.io/apimachinery v0.0.0-20240306164812-cbfe0a1feaa5 h1:YRP8FbAab9hlobsEfyUG7P6dC6hbVTLw0eFY/AnewmY= -k8s.io/apimachinery v0.0.0-20240306164812-cbfe0a1feaa5/go.mod h1:wEJvNDlfxMRaMhyv38SIHIEC9hah/xuzqUUhxIyUv7Y= -k8s.io/client-go v0.0.0-20240306170515-0cdc0ce850af h1:hU8WcNO9vZrUXLzKbyXj6FUlMuTecjBr7MmbGxfCkzM= -k8s.io/client-go v0.0.0-20240306170515-0cdc0ce850af/go.mod h1:g+S/ljjD+b0SS7OkKeZ6IBio03Ot9Q8s19hN3jHsl/Y= +k8s.io/api v0.0.0-20240311194616-96558b97565e h1:Z2GqjWWNuM6kr7OiUdpQvs+x7vUU96a+7YdYilq3Ku8= +k8s.io/api v0.0.0-20240311194616-96558b97565e/go.mod h1:RzL8aPQw9ZdVXCdY+Iz3AXnVX+jFyQNqcmzmS+2/Ur0= +k8s.io/apimachinery v0.0.0-20240307160843-0407311be590 h1:YFg0j+PVfNLayHtZ3gdTeW12q7HECwhvZm9fWZpXyXo= +k8s.io/apimachinery v0.0.0-20240307160843-0407311be590/go.mod h1:wEJvNDlfxMRaMhyv38SIHIEC9hah/xuzqUUhxIyUv7Y= +k8s.io/client-go v0.0.0-20240309200420-7ebe0ea60e0a h1:EMeP7Xj22C0bpj+nV6cxqBExDijGzRKs66AY4NDStYw= +k8s.io/client-go v0.0.0-20240309200420-7ebe0ea60e0a/go.mod h1:QDFa4l0JGh2EmATyNXW9l58yiTy4DIoVjrfyNgOxfOQ= k8s.io/klog/v2 v2.120.1 h1:QXU6cPEOIslTGvZaXvFWiP9VKyeet3sawzTOvdXb4Vw= k8s.io/klog/v2 v2.120.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE= k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 h1:BZqlfIlq5YbRMFko6/PM7FjZpUb45WallggurYhKGag=