From c0a9629d6f4cb9f880bf542992cefa4efa27a89a Mon Sep 17 00:00:00 2001 From: medmes Date: Fri, 17 Oct 2025 12:43:17 +0200 Subject: [PATCH 1/4] fix Integration testing leaked processes on DualCluster --- .../skrcontextimpl/dual_cluster.go | 55 +++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go b/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go index 521de7e2ba..e161aac91e 100644 --- a/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go +++ b/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go @@ -10,6 +10,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" + "fmt" "github.com/kyma-project/lifecycle-manager/internal/event" "github.com/kyma-project/lifecycle-manager/internal/remote" ) @@ -24,6 +25,7 @@ type DualClusterFactory struct { scheme *machineryruntime.Scheme event event.Event skrEnv *envtest.Environment + skrEnvs sync.Map } func NewDualClusterFactory(scheme *machineryruntime.Scheme, event event.Event) *DualClusterFactory { @@ -31,6 +33,7 @@ func NewDualClusterFactory(scheme *machineryruntime.Scheme, event event.Event) * clients: sync.Map{}, scheme: scheme, event: event, + skrEnvs: sync.Map{}, } } @@ -40,31 +43,44 @@ func (f *DualClusterFactory) Init(_ context.Context, kyma types.NamespacedName) return nil } - f.skrEnv = &envtest.Environment{ + skrEnv := &envtest.Environment{ ErrorIfCRDPathMissing: true, // Scheme: scheme, } - cfg, err := f.GetSkrEnv().Start() + + // Start the envtest and record the returned cfg + cfg, err := skrEnv.Start() if err != nil { return err } if cfg == nil { + // cleanup fast - if start returned nil cfg + _ = skrEnv.Stop() return ErrEmptyRestConfig } var authUser *envtest.AuthenticatedUser - authUser, err = f.GetSkrEnv().AddUser(envtest.User{ + authUser, err = skrEnv.AddUser(envtest.User{ Name: "skr-admin-account", Groups: []string{"system:masters"}, }, cfg) if err != nil { + _ = skrEnv.Stop() return err } skrClient, err := client.New(authUser.Config(), client.Options{Scheme: f.scheme}) + if err != nil { + _ = skrEnv.Stop() + return err + } newClient := remote.NewClientWithConfig(skrClient, authUser.Config()) f.clients.Store(kyma.Name, newClient) + f.skrEnv = skrEnv + // track this envtest so Stop() can stop all started envs + f.skrEnvs.Store(kyma.Name, skrEnv) + return err } @@ -89,8 +105,37 @@ func (f *DualClusterFactory) GetSkrEnv() *envtest.Environment { } func (f *DualClusterFactory) Stop() error { - if f.skrEnv == nil { + var errs []error + + f.skrEnvs.Range(func(key, value interface{}) bool { + name := "" + if ks, ok := key.(string); ok { + name = ks + } + + if env, ok := value.(*envtest.Environment); ok && env != nil { + if err := env.Stop(); err != nil { + if name != "" { + errs = append(errs, fmt.Errorf("stop envtest %q: %w", name, err)) + } else { + errs = append(errs, fmt.Errorf("stop envtest (unknown key): %w", err)) + } + } + } + + // remove entries so we don't double-stop later + f.skrEnvs.Delete(key) + if name != "" { + f.clients.Delete(name) + } + return true + }) + + // Clear skrEnv + f.skrEnv = nil + + if len(errs) == 0 { return nil } - return f.skrEnv.Stop() + return fmt.Errorf("errors stopping envtests: %w", errors.Join(errs...)) } From b18ee33344d2777730a129182964df6b9b977fe1 Mon Sep 17 00:00:00 2001 From: medmes Date: Fri, 17 Oct 2025 15:47:03 +0200 Subject: [PATCH 2/4] Add testing to dual_cluster factory code to verify the leaked processes fix --- .../skrcontextimpl/dual_cluster.go | 10 +- .../skrcontextimpl/dual_cluster_test.go | 117 ++++++++++++++++++ 2 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go diff --git a/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go b/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go index e161aac91e..1180b97618 100644 --- a/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go +++ b/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go @@ -17,7 +17,7 @@ import ( var ( ErrEmptyRestConfig = errors.New("rest.Config is nil") - errSkrEnvNotStarted = errors.New("SKR envtest environment not started") + ErrSkrEnvNotStarted = errors.New("SKR envtest environment not started") ) type DualClusterFactory struct { @@ -87,15 +87,19 @@ func (f *DualClusterFactory) Init(_ context.Context, kyma types.NamespacedName) func (f *DualClusterFactory) Get(kyma types.NamespacedName) (*remote.SkrContext, error) { value, ok := f.clients.Load(kyma.Name) if !ok { - return nil, errSkrEnvNotStarted + return nil, ErrSkrEnvNotStarted } skrClient, ok := value.(*remote.ConfigAndClient) if !ok { - return nil, errSkrEnvNotStarted + return nil, ErrSkrEnvNotStarted } return remote.NewSkrContext(skrClient, f.event), nil } +func (f *DualClusterFactory) StoreEnv(name string, env interface{}) { + f.skrEnvs.Store(name, env) +} + func (f *DualClusterFactory) InvalidateCache(_ types.NamespacedName) { // no-op } diff --git a/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go b/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go new file mode 100644 index 0000000000..95f52d2e55 --- /dev/null +++ b/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go @@ -0,0 +1,117 @@ +package skrcontextimpl_test + +import ( + "context" + "errors" + "runtime" + "testing" + + machineryruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + + testskrcontext "github.com/kyma-project/lifecycle-manager/tests/integration/commontestutils/skrcontextimpl" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newFactory() *testskrcontext.DualClusterFactory { + scheme := machineryruntime.NewScheme() + return testskrcontext.NewDualClusterFactory(scheme, nil) +} + +func Test_GetBeforeInit(t *testing.T) { + dualFactory := newFactory() + _, err := dualFactory.Get(types.NamespacedName{Name: "kymaUninitialized"}) + require.Error(t, err) + assert.ErrorIs(t, err, testskrcontext.ErrSkrEnvNotStarted) +} + +func Test_InitAndGet(t *testing.T) { + dualFactory := newFactory() + kyma := types.NamespacedName{Name: "kymaInit"} + require.NoError(t, dualFactory.Init(context.Background(), kyma)) + skrCtx, err := dualFactory.Get(kyma) + require.NoError(t, err) + require.NotNil(t, skrCtx) + require.NotNil(t, dualFactory.GetSkrEnv()) +} + +func Test_InitTwiceSameKyma(t *testing.T) { + dualFactory := newFactory() + kyma := types.NamespacedName{Name: "kymaSame"} + require.NoError(t, dualFactory.Init(context.Background(), kyma)) + envFirst := dualFactory.GetSkrEnv() + require.NotNil(t, envFirst) + require.NoError(t, dualFactory.Init(context.Background(), kyma)) + envSecond := dualFactory.GetSkrEnv() + assert.Equal(t, envFirst, envSecond) +} + +func Test_MultipleKymasAndStop(t *testing.T) { + dualFactory := newFactory() + kymaPrimary := types.NamespacedName{Name: "kymaPrimary"} + kymaSecondary := types.NamespacedName{Name: "kymaSecondary"} + + for _, k := range []types.NamespacedName{kymaPrimary, kymaSecondary} { + require.NoError(t, dualFactory.Init(context.Background(), k)) + _, err := dualFactory.Get(k) + require.NoError(t, err) + } + + require.NoError(t, dualFactory.Stop()) + assert.Nil(t, dualFactory.GetSkrEnv()) + + _, err := dualFactory.Get(kymaPrimary) + assert.Error(t, err) +} + +func Test_StopIdempotent(t *testing.T) { + dualFactory := newFactory() + kyma := types.NamespacedName{Name: "kymaLifecycle"} + require.NoError(t, dualFactory.Init(context.Background(), kyma)) + require.NoError(t, dualFactory.Stop()) + require.NoError(t, dualFactory.Stop()) +} + +func Test_NoLeakedProcesses(t *testing.T) { + dualFactory := newFactory() + kyma := types.NamespacedName{Name: "kymaGoroutineCheck"} + + before := runtime.NumGoroutine() + require.NoError(t, dualFactory.Init(context.Background(), kyma)) + require.NoError(t, dualFactory.Stop()) + + runtime.GC() + after := runtime.NumGoroutine() + assert.LessOrEqual(t, after, before+2) +} + +type fakeEnv struct { + name string + stopCalled bool + stopErr error +} + +func (fenv *fakeEnv) Stop() error { + fenv.stopCalled = true + return fenv.stopErr +} + +func Test_StopAggregatesErrors(t *testing.T) { + dualFactory := newFactory() + + envPrimary := &fakeEnv{name: "primary-env", stopErr: errors.New("primary stop failure")} + envSecondary := &fakeEnv{name: "secondary-env", stopErr: errors.New("secondary stop failure")} + envTertiary := &fakeEnv{name: "tertiary-env"} + + dualFactory.StoreEnv("primary-env", envPrimary) + dualFactory.StoreEnv("secondary-env", envSecondary) + dualFactory.StoreEnv("tertiary-env", envTertiary) + + err := dualFactory.Stop() + require.Error(t, err) + msg := err.Error() + assert.Contains(t, msg, "primary stop failure") + assert.Contains(t, msg, "secondary stop failure") + assert.Nil(t, dualFactory.GetSkrEnv()) +} From bbfa6500c70e37f69774e52e9d3f52f2624b1093 Mon Sep 17 00:00:00 2001 From: medmes Date: Mon, 20 Oct 2025 09:18:38 +0200 Subject: [PATCH 3/4] Added Unit tests for DualClusterFactory --- .../skrcontextimpl/dual_cluster.go | 20 +- .../skrcontextimpl/dual_cluster_test.go | 191 ++++++++++++------ 2 files changed, 142 insertions(+), 69 deletions(-) diff --git a/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go b/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go index 1180b97618..64bc4e3db2 100644 --- a/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go +++ b/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go @@ -20,6 +20,10 @@ var ( ErrSkrEnvNotStarted = errors.New("SKR envtest environment not started") ) +type Stopper interface { + Stop() error +} + type DualClusterFactory struct { clients sync.Map scheme *machineryruntime.Scheme @@ -111,18 +115,14 @@ func (f *DualClusterFactory) GetSkrEnv() *envtest.Environment { func (f *DualClusterFactory) Stop() error { var errs []error - f.skrEnvs.Range(func(key, value interface{}) bool { - name := "" - if ks, ok := key.(string); ok { - name = ks - } - - if env, ok := value.(*envtest.Environment); ok && env != nil { - if err := env.Stop(); err != nil { + f.skrEnvs.Range(func(key, value any) bool { + name, _ := key.(string) + if stopper, ok := value.(Stopper); ok && stopper != nil { + if err := stopper.Stop(); err != nil { if name != "" { - errs = append(errs, fmt.Errorf("stop envtest %q: %w", name, err)) + errs = append(errs, fmt.Errorf("stop %s: %w", name, err)) } else { - errs = append(errs, fmt.Errorf("stop envtest (unknown key): %w", err)) + errs = append(errs, fmt.Errorf("stop : %w", err)) } } } diff --git a/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go b/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go index 95f52d2e55..aea9ab8b3e 100644 --- a/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go +++ b/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go @@ -3,17 +3,25 @@ package skrcontextimpl_test import ( "context" "errors" - "runtime" - "testing" - + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" machineryruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "runtime" + "sync" + "testing" + "time" testskrcontext "github.com/kyma-project/lifecycle-manager/tests/integration/commontestutils/skrcontextimpl" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) +func TestDualClusterFactory(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "DualCluster Factory Suite") +} + func newFactory() *testskrcontext.DualClusterFactory { scheme := machineryruntime.NewScheme() return testskrcontext.NewDualClusterFactory(scheme, nil) @@ -21,69 +29,124 @@ func newFactory() *testskrcontext.DualClusterFactory { func Test_GetBeforeInit(t *testing.T) { dualFactory := newFactory() + _, err := dualFactory.Get(types.NamespacedName{Name: "kymaUninitialized"}) + require.Error(t, err) assert.ErrorIs(t, err, testskrcontext.ErrSkrEnvNotStarted) } -func Test_InitAndGet(t *testing.T) { +func Test_StopWithErrors(t *testing.T) { dualFactory := newFactory() - kyma := types.NamespacedName{Name: "kymaInit"} - require.NoError(t, dualFactory.Init(context.Background(), kyma)) - skrCtx, err := dualFactory.Get(kyma) - require.NoError(t, err) - require.NotNil(t, skrCtx) - require.NotNil(t, dualFactory.GetSkrEnv()) + envPrimary := &fakeEnv{name: "primary-env", stopErr: errors.New("primary stop failure")} + envSecondary := &fakeEnv{name: "secondary-env", stopErr: errors.New("secondary stop failure")} + envTertiary := &fakeEnv{name: "tertiary-env"} + dualFactory.StoreEnv("primary-env", envPrimary) + dualFactory.StoreEnv("secondary-env", envSecondary) + dualFactory.StoreEnv("tertiary-env", envTertiary) + + err := dualFactory.Stop() + + require.Error(t, err) + msg := err.Error() + assert.Contains(t, msg, "primary stop failure") + assert.Contains(t, msg, "secondary stop failure") + assert.Nil(t, dualFactory.GetSkrEnv()) + assert.True(t, envPrimary.stopCalled) + assert.True(t, envSecondary.stopCalled) + assert.True(t, envTertiary.stopCalled) } -func Test_InitTwiceSameKyma(t *testing.T) { +func Test_StopIdempotent(t *testing.T) { dualFactory := newFactory() - kyma := types.NamespacedName{Name: "kymaSame"} - require.NoError(t, dualFactory.Init(context.Background(), kyma)) - envFirst := dualFactory.GetSkrEnv() - require.NotNil(t, envFirst) - require.NoError(t, dualFactory.Init(context.Background(), kyma)) - envSecond := dualFactory.GetSkrEnv() - assert.Equal(t, envFirst, envSecond) + fakeEnv := &fakeEnv{name: "test-env"} + dualFactory.StoreEnv("test-env", fakeEnv) + + require.NoError(t, dualFactory.Stop()) + + assert.True(t, fakeEnv.stopCalled) } -func Test_MultipleKymasAndStop(t *testing.T) { +func Test_StopClearsAllEntries(t *testing.T) { dualFactory := newFactory() - kymaPrimary := types.NamespacedName{Name: "kymaPrimary"} - kymaSecondary := types.NamespacedName{Name: "kymaSecondary"} - - for _, k := range []types.NamespacedName{kymaPrimary, kymaSecondary} { - require.NoError(t, dualFactory.Init(context.Background(), k)) - _, err := dualFactory.Get(k) - require.NoError(t, err) + for range make([]struct{}, 5) { + fakeEnv := &fakeEnv{name: "test-env"} + dualFactory.StoreEnv("test-env", fakeEnv) } require.NoError(t, dualFactory.Stop()) - assert.Nil(t, dualFactory.GetSkrEnv()) - _, err := dualFactory.Get(kymaPrimary) + assert.Nil(t, dualFactory.GetSkrEnv()) + _, err := dualFactory.Get(types.NamespacedName{Name: "test-env"}) assert.Error(t, err) } -func Test_StopIdempotent(t *testing.T) { +func Test_ConcurrentStopCalls(t *testing.T) { dualFactory := newFactory() - kyma := types.NamespacedName{Name: "kymaLifecycle"} - require.NoError(t, dualFactory.Init(context.Background(), kyma)) - require.NoError(t, dualFactory.Stop()) - require.NoError(t, dualFactory.Stop()) -} + for range make([]struct{}, 10) { + fakeEnv := &fakeEnv{name: "test-env"} + dualFactory.StoreEnv("test-env", fakeEnv) + } + var waitGroup sync.WaitGroup + errors := make(chan error, 5) + + for range make([]struct{}, 5) { + waitGroup.Add(1) + go func() { + defer waitGroup.Done() + errors <- dualFactory.Stop() + }() + } + waitGroup.Wait() + close(errors) -func Test_NoLeakedProcesses(t *testing.T) { - dualFactory := newFactory() - kyma := types.NamespacedName{Name: "kymaGoroutineCheck"} + for err := range errors { + assert.NoError(t, err) + } +} +func Test_LeakPrevention_VerifyNoGoroutineLeaks(t *testing.T) { before := runtime.NumGoroutine() - require.NoError(t, dualFactory.Init(context.Background(), kyma)) - require.NoError(t, dualFactory.Stop()) + for range make([]struct{}, 3) { + dualFactory := newFactory() + for range make([]struct{}, 5) { + fakeEnv := &leakyFakeEnv{ + name: "test-env", + shouldSpawnGoroutine: true, + } + dualFactory.StoreEnv("test-env", fakeEnv) + } + require.NoError(t, dualFactory.Stop()) + time.Sleep(10 * time.Millisecond) + } + // Force garbage collection to clean up any lingering references runtime.GC() + runtime.GC() + time.Sleep(50 * time.Millisecond) + after := runtime.NumGoroutine() - assert.LessOrEqual(t, after, before+2) + assert.LessOrEqual(t, after, before+2, + "Expected no significant goroutine leaks. Before: %d, After: %d", before, after) +} + +func Test_LeakPrevention_VerifyStopperInterfaceHandling(t *testing.T) { + dualFactory := newFactory() + normalStopper := &fakeEnv{name: "normal"} + errorStopper := &fakeEnv{name: "error", stopErr: errors.New("stop error")} + leakyStopper := &leakyFakeEnv{name: "leaky", shouldSpawnGoroutine: true} + dualFactory.StoreEnv("normal", normalStopper) + dualFactory.StoreEnv("error", errorStopper) + dualFactory.StoreEnv("leaky", leakyStopper) + dualFactory.StoreEnv("non-stopper", "this is just a string") + + err := dualFactory.Stop() + + require.Error(t, err) + assert.Contains(t, err.Error(), "stop error") + assert.True(t, normalStopper.stopCalled) + assert.True(t, errorStopper.stopCalled) + assert.True(t, leakyStopper.stopCalled) } type fakeEnv struct { @@ -92,26 +155,36 @@ type fakeEnv struct { stopErr error } -func (fenv *fakeEnv) Stop() error { - fenv.stopCalled = true - return fenv.stopErr +func (f *fakeEnv) Stop() error { + f.stopCalled = true + return f.stopErr } -func Test_StopAggregatesErrors(t *testing.T) { - dualFactory := newFactory() +type leakyFakeEnv struct { + name string + stopCalled bool + stopErr error + shouldSpawnGoroutine bool + cancel context.CancelFunc +} - envPrimary := &fakeEnv{name: "primary-env", stopErr: errors.New("primary stop failure")} - envSecondary := &fakeEnv{name: "secondary-env", stopErr: errors.New("secondary stop failure")} - envTertiary := &fakeEnv{name: "tertiary-env"} +func (l *leakyFakeEnv) Stop() error { + l.stopCalled = true - dualFactory.StoreEnv("primary-env", envPrimary) - dualFactory.StoreEnv("secondary-env", envSecondary) - dualFactory.StoreEnv("tertiary-env", envTertiary) + if l.shouldSpawnGoroutine && l.cancel == nil { + // Simulate starting a background goroutine (like envtest) + ctx, cancel := context.WithCancel(context.Background()) + l.cancel = cancel - err := dualFactory.Stop() - require.Error(t, err) - msg := err.Error() - assert.Contains(t, msg, "primary stop failure") - assert.Contains(t, msg, "secondary stop failure") - assert.Nil(t, dualFactory.GetSkrEnv()) + go func() { + <-ctx.Done() // Wait for cancellation + }() + } + + // Clean up the goroutine + if l.cancel != nil { + l.cancel() + } + + return l.stopErr } From 893ede856002db65ed5479787da863771a7aa10a Mon Sep 17 00:00:00 2001 From: medmes Date: Mon, 20 Oct 2025 16:12:50 +0200 Subject: [PATCH 4/4] Addressed PR review comment --- .../skrcontextimpl/dual_cluster.go | 58 +++---- .../skrcontextimpl/dual_cluster_test.go | 150 +++++++----------- 2 files changed, 86 insertions(+), 122 deletions(-) diff --git a/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go b/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go index 64bc4e3db2..19b3a854b9 100644 --- a/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go +++ b/tests/integration/commontestutils/skrcontextimpl/dual_cluster.go @@ -28,8 +28,7 @@ type DualClusterFactory struct { clients sync.Map scheme *machineryruntime.Scheme event event.Event - skrEnv *envtest.Environment - skrEnvs sync.Map + SkrEnvs sync.Map } func NewDualClusterFactory(scheme *machineryruntime.Scheme, event event.Event) *DualClusterFactory { @@ -37,7 +36,7 @@ func NewDualClusterFactory(scheme *machineryruntime.Scheme, event event.Event) * clients: sync.Map{}, scheme: scheme, event: event, - skrEnvs: sync.Map{}, + SkrEnvs: sync.Map{}, } } @@ -81,9 +80,8 @@ func (f *DualClusterFactory) Init(_ context.Context, kyma types.NamespacedName) newClient := remote.NewClientWithConfig(skrClient, authUser.Config()) f.clients.Store(kyma.Name, newClient) - f.skrEnv = skrEnv // track this envtest so Stop() can stop all started envs - f.skrEnvs.Store(kyma.Name, skrEnv) + f.SkrEnvs.Store(kyma.Name, skrEnv) return err } @@ -100,8 +98,12 @@ func (f *DualClusterFactory) Get(kyma types.NamespacedName) (*remote.SkrContext, return remote.NewSkrContext(skrClient, f.event), nil } -func (f *DualClusterFactory) StoreEnv(name string, env interface{}) { - f.skrEnvs.Store(name, env) +func (f *DualClusterFactory) StoreEnv(name string, env *envtest.Environment) error { + if name == "" { + return errors.New("environment name cannot be empty") + } + f.SkrEnvs.Store(name, env) + return nil } func (f *DualClusterFactory) InvalidateCache(_ types.NamespacedName) { @@ -109,37 +111,37 @@ func (f *DualClusterFactory) InvalidateCache(_ types.NamespacedName) { } func (f *DualClusterFactory) GetSkrEnv() *envtest.Environment { - return f.skrEnv + var env *envtest.Environment + f.SkrEnvs.Range(func(key, value any) bool { + if e, ok := value.(*envtest.Environment); ok { + env = e + return false + } + return true + }) + return env } func (f *DualClusterFactory) Stop() error { var errs []error - f.skrEnvs.Range(func(key, value any) bool { - name, _ := key.(string) - if stopper, ok := value.(Stopper); ok && stopper != nil { + f.SkrEnvs.Range(func(key, value any) bool { + name, ok := key.(string) + if !ok { + return true + } + if stopper, ok := value.(Stopper); ok { if err := stopper.Stop(); err != nil { - if name != "" { - errs = append(errs, fmt.Errorf("stop %s: %w", name, err)) - } else { - errs = append(errs, fmt.Errorf("stop : %w", err)) - } + errs = append(errs, fmt.Errorf("stop %s: %w", name, err)) } } - - // remove entries so we don't double-stop later - f.skrEnvs.Delete(key) - if name != "" { - f.clients.Delete(name) - } + f.SkrEnvs.Delete(key) + f.clients.Delete(name) return true }) - // Clear skrEnv - f.skrEnv = nil - - if len(errs) == 0 { - return nil + if len(errs) > 0 { + return fmt.Errorf("errors stopping envtests: %w", errors.Join(errs...)) } - return fmt.Errorf("errors stopping envtests: %w", errors.Join(errs...)) + return nil } diff --git a/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go b/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go index aea9ab8b3e..46b75280cf 100644 --- a/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go +++ b/tests/integration/commontestutils/skrcontextimpl/dual_cluster_test.go @@ -1,18 +1,17 @@ package skrcontextimpl_test import ( - "context" "errors" + "sync" + "testing" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" machineryruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "runtime" - "sync" - "testing" - "time" + "sigs.k8s.io/controller-runtime/pkg/envtest" testskrcontext "github.com/kyma-project/lifecycle-manager/tests/integration/commontestutils/skrcontextimpl" ) @@ -36,14 +35,31 @@ func Test_GetBeforeInit(t *testing.T) { assert.ErrorIs(t, err, testskrcontext.ErrSkrEnvNotStarted) } +func Test_StoreEnv_ValidatesInput(t *testing.T) { + dualFactory := newFactory() + + err := dualFactory.StoreEnv("", &envtest.Environment{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "name cannot be empty") +} + func Test_StopWithErrors(t *testing.T) { dualFactory := newFactory() - envPrimary := &fakeEnv{name: "primary-env", stopErr: errors.New("primary stop failure")} - envSecondary := &fakeEnv{name: "secondary-env", stopErr: errors.New("secondary stop failure")} - envTertiary := &fakeEnv{name: "tertiary-env"} - dualFactory.StoreEnv("primary-env", envPrimary) - dualFactory.StoreEnv("secondary-env", envSecondary) - dualFactory.StoreEnv("tertiary-env", envTertiary) + + // Store real envtest.Environment first, then replace with test doubles + require.NoError(t, dualFactory.StoreEnv("primary-env", &envtest.Environment{})) + require.NoError(t, dualFactory.StoreEnv("secondary-env", &envtest.Environment{})) + require.NoError(t, dualFactory.StoreEnv("tertiary-env", &envtest.Environment{})) + + // Create test doubles + envPrimary := &fakeEnvTest{name: "primary-env", stopErr: errors.New("primary stop failure")} + envSecondary := &fakeEnvTest{name: "secondary-env", stopErr: errors.New("secondary stop failure")} + envTertiary := &fakeEnvTest{name: "tertiary-env"} + + // Replace with test doubles directly in the map + dualFactory.SkrEnvs.Store("primary-env", envPrimary) + dualFactory.SkrEnvs.Store("secondary-env", envSecondary) + dualFactory.SkrEnvs.Store("tertiary-env", envTertiary) err := dualFactory.Stop() @@ -51,46 +67,45 @@ func Test_StopWithErrors(t *testing.T) { msg := err.Error() assert.Contains(t, msg, "primary stop failure") assert.Contains(t, msg, "secondary stop failure") - assert.Nil(t, dualFactory.GetSkrEnv()) assert.True(t, envPrimary.stopCalled) assert.True(t, envSecondary.stopCalled) assert.True(t, envTertiary.stopCalled) } -func Test_StopIdempotent(t *testing.T) { +func Test_StopClearsAllEntriesAndIsIdempotent(t *testing.T) { dualFactory := newFactory() - fakeEnv := &fakeEnv{name: "test-env"} - dualFactory.StoreEnv("test-env", fakeEnv) - require.NoError(t, dualFactory.Stop()) + require.NoError(t, dualFactory.StoreEnv("test-env", &envtest.Environment{})) - assert.True(t, fakeEnv.stopCalled) -} - -func Test_StopClearsAllEntries(t *testing.T) { - dualFactory := newFactory() - for range make([]struct{}, 5) { - fakeEnv := &fakeEnv{name: "test-env"} - dualFactory.StoreEnv("test-env", fakeEnv) - } + fakeEnv := &fakeEnvTest{name: "test-env"} + dualFactory.SkrEnvs.Store("test-env", fakeEnv) require.NoError(t, dualFactory.Stop()) - + assert.True(t, fakeEnv.stopCalled) assert.Nil(t, dualFactory.GetSkrEnv()) + + // Verify entry is cleared _, err := dualFactory.Get(types.NamespacedName{Name: "test-env"}) - assert.Error(t, err) + require.Error(t, err) + + // Second stop should also succeed (idempotent) + require.NoError(t, dualFactory.Stop()) } func Test_ConcurrentStopCalls(t *testing.T) { dualFactory := newFactory() - for range make([]struct{}, 10) { - fakeEnv := &fakeEnv{name: "test-env"} - dualFactory.StoreEnv("test-env", fakeEnv) + + for range 10 { + require.NoError(t, dualFactory.StoreEnv("test-env", &envtest.Environment{})) } + + fakeEnv := &fakeEnvTest{name: "test-env"} + dualFactory.SkrEnvs.Store("test-env", fakeEnv) + var waitGroup sync.WaitGroup errors := make(chan error, 5) - for range make([]struct{}, 5) { + for range 5 { waitGroup.Add(1) go func() { defer waitGroup.Done() @@ -105,40 +120,17 @@ func Test_ConcurrentStopCalls(t *testing.T) { } } -func Test_LeakPrevention_VerifyNoGoroutineLeaks(t *testing.T) { - before := runtime.NumGoroutine() - for range make([]struct{}, 3) { - dualFactory := newFactory() - for range make([]struct{}, 5) { - fakeEnv := &leakyFakeEnv{ - name: "test-env", - shouldSpawnGoroutine: true, - } - dualFactory.StoreEnv("test-env", fakeEnv) - } - require.NoError(t, dualFactory.Stop()) - time.Sleep(10 * time.Millisecond) - } +func Test_StopperInterfaceHandling(t *testing.T) { + dualFactory := newFactory() - // Force garbage collection to clean up any lingering references - runtime.GC() - runtime.GC() - time.Sleep(50 * time.Millisecond) + require.NoError(t, dualFactory.StoreEnv("normal", &envtest.Environment{})) + require.NoError(t, dualFactory.StoreEnv("error", &envtest.Environment{})) - after := runtime.NumGoroutine() - assert.LessOrEqual(t, after, before+2, - "Expected no significant goroutine leaks. Before: %d, After: %d", before, after) -} + normalStopper := &fakeEnvTest{name: "normal"} + errorStopper := &fakeEnvTest{name: "error", stopErr: errors.New("stop error")} -func Test_LeakPrevention_VerifyStopperInterfaceHandling(t *testing.T) { - dualFactory := newFactory() - normalStopper := &fakeEnv{name: "normal"} - errorStopper := &fakeEnv{name: "error", stopErr: errors.New("stop error")} - leakyStopper := &leakyFakeEnv{name: "leaky", shouldSpawnGoroutine: true} - dualFactory.StoreEnv("normal", normalStopper) - dualFactory.StoreEnv("error", errorStopper) - dualFactory.StoreEnv("leaky", leakyStopper) - dualFactory.StoreEnv("non-stopper", "this is just a string") + dualFactory.SkrEnvs.Store("normal", normalStopper) + dualFactory.SkrEnvs.Store("error", errorStopper) err := dualFactory.Stop() @@ -146,45 +138,15 @@ func Test_LeakPrevention_VerifyStopperInterfaceHandling(t *testing.T) { assert.Contains(t, err.Error(), "stop error") assert.True(t, normalStopper.stopCalled) assert.True(t, errorStopper.stopCalled) - assert.True(t, leakyStopper.stopCalled) } -type fakeEnv struct { +type fakeEnvTest struct { name string stopCalled bool stopErr error } -func (f *fakeEnv) Stop() error { +func (f *fakeEnvTest) Stop() error { f.stopCalled = true return f.stopErr } - -type leakyFakeEnv struct { - name string - stopCalled bool - stopErr error - shouldSpawnGoroutine bool - cancel context.CancelFunc -} - -func (l *leakyFakeEnv) Stop() error { - l.stopCalled = true - - if l.shouldSpawnGoroutine && l.cancel == nil { - // Simulate starting a background goroutine (like envtest) - ctx, cancel := context.WithCancel(context.Background()) - l.cancel = cancel - - go func() { - <-ctx.Done() // Wait for cancellation - }() - } - - // Clean up the goroutine - if l.cancel != nil { - l.cancel() - } - - return l.stopErr -}