From efa9a577c7f755035ebc51d0b517fd71a4d78f38 Mon Sep 17 00:00:00 2001 From: Anand Kumar Date: Fri, 20 Mar 2026 14:03:06 +0530 Subject: [PATCH 1/7] CM-902: Add and improve unit tests - Add unit tests for modified functions to improve coverage - Align trustmanager utils tests with table-driven style - Add table-driven tests for refactor safety across controller packages - Update common and deployment test files; remove obsolete istiocsr test Made-with: Cursor --- .../certmanager/credentials_request_test.go | 223 ++++++++ .../certmanager/deployment_overrides_test.go | 157 ++++++ pkg/controller/common/client_test.go | 101 ++++ pkg/controller/common/errors_test.go | 332 +++++++++++ .../common/fakes/fake_ctrl_client_test.go | 166 ++++++ pkg/controller/common/utils_test.go | 269 +++++++++ pkg/controller/istiocsr/utils_test.go | 523 ++++++++++++++++++ .../trustmanager/controller_test.go | 31 ++ pkg/controller/trustmanager/utils_test.go | 143 +++++ .../v1alpha1/certmanagerconfig_test.go | 79 +++ .../applyconfigurations/utils_test.go | 60 ++ pkg/operator/assets/bindata_test.go | 268 +++++++++ 12 files changed, 2352 insertions(+) create mode 100644 pkg/controller/certmanager/credentials_request_test.go create mode 100644 pkg/controller/common/client_test.go create mode 100644 pkg/controller/common/errors_test.go create mode 100644 pkg/controller/common/fakes/fake_ctrl_client_test.go create mode 100644 pkg/controller/common/utils_test.go create mode 100644 pkg/controller/istiocsr/utils_test.go create mode 100644 pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go create mode 100644 pkg/operator/applyconfigurations/utils_test.go create mode 100644 pkg/operator/assets/bindata_test.go diff --git a/pkg/controller/certmanager/credentials_request_test.go b/pkg/controller/certmanager/credentials_request_test.go new file mode 100644 index 000000000..57b7da53c --- /dev/null +++ b/pkg/controller/certmanager/credentials_request_test.go @@ -0,0 +1,223 @@ +package certmanager + +import ( + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/cache" + + configv1 "github.com/openshift/api/config/v1" + configinformersv1 "github.com/openshift/client-go/config/informers/externalversions/config/v1" + configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" +) + +const testSecretName = "cloud-creds" + +func TestWithCloudCredentials(t *testing.T) { + tests := []struct { + name string + deploymentName string + secretName string + secretInStore bool + platformType configv1.PlatformType + wantErr string + wantVolumes int + wantMountPath string + wantAWSEnv bool + noInfra bool // if true, infra indexer is left empty so Get("cluster") fails + }{ + { + name: "non-controller deployment no-op", + deploymentName: certmanagerWebhookDeployment, + secretName: testSecretName, + platformType: configv1.AWSPlatformType, + wantVolumes: 0, + }, + { + name: "empty secret name returns nil", + deploymentName: certmanagerControllerDeployment, + secretName: "", + platformType: configv1.AWSPlatformType, + wantVolumes: 0, + }, + { + name: "secret not found returns retry error", + deploymentName: certmanagerControllerDeployment, + secretName: "missing-secret", + secretInStore: false, + platformType: configv1.AWSPlatformType, + wantErr: "Retrying", + wantVolumes: 0, + }, + { + name: "AWS adds volume, mount and env", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.AWSPlatformType, + wantVolumes: 1, + wantMountPath: awsCredentialsDir, + wantAWSEnv: true, + }, + { + name: "GCP adds volume and mount, no AWS env", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.GCPPlatformType, + wantVolumes: 1, + wantMountPath: gcpCredentialsDir, + wantAWSEnv: false, + }, + { + name: "unsupported platform returns error", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.PlatformType("Unsupported"), + wantErr: "unsupported cloud provider", + wantVolumes: 0, + }, + { + name: "infra not found returns error", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.AWSPlatformType, + wantErr: "cluster", + noInfra: true, + wantVolumes: 0, + }, + { + name: "Azure platform is unsupported", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.AzurePlatformType, + wantErr: "unsupported cloud provider", + wantVolumes: 0, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var kubeClient *fake.Clientset + if tt.secretInStore { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: tt.secretName, Namespace: "cert-manager"}, + } + kubeClient = fake.NewSimpleClientset(secret) + } else { + kubeClient = fake.NewSimpleClientset() + } + if tt.name == "secret not found returns retry error" { + kubeClient = fake.NewSimpleClientset(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: "cert-manager"}, + }) + } + kubeInformers := informers.NewSharedInformerFactory(kubeClient, 0) + if tt.secretInStore || tt.wantErr != "" { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: tt.secretName, Namespace: "cert-manager"}, + } + if tt.wantErr == "Retrying" { + secret.Name = "other" + } + kubeInformers.Core().V1().Secrets().Informer().GetStore().Add(secret) + } + stopCh := make(chan struct{}) + defer close(stopCh) + kubeInformers.Start(stopCh) + if tt.secretInStore || tt.wantErr != "" { + if !cache.WaitForCacheSync(stopCh, kubeInformers.Core().V1().Secrets().Informer().HasSynced) { + t.Fatal("secret informer did not sync") + } + } + + infraIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + if !tt.noInfra { + infra := &configv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: configv1.InfrastructureStatus{ + PlatformStatus: &configv1.PlatformStatus{Type: tt.platformType}, + }, + } + _ = infraIndexer.Add(infra) + } + infraInformer := &fakeInfrastructureInformer{lister: configlistersv1.NewInfrastructureLister(infraIndexer)} + + hook := withCloudCredentials( + kubeInformers.Core().V1().Secrets(), + infraInformer, + tt.deploymentName, + tt.secretName, + ) + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "controller"}}, + }, + }, + }, + } + err := hook(nil, deployment) + + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErr) + } + if !strings.Contains(err.Error(), tt.wantErr) && !apierrors.IsNotFound(err) { + t.Errorf("error = %v, want substring %q or NotFound", err, tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if n := len(deployment.Spec.Template.Spec.Volumes); n != tt.wantVolumes { + t.Errorf("volumes count = %d, want %d", n, tt.wantVolumes) + } + if tt.wantVolumes > 0 { + if deployment.Spec.Template.Spec.Volumes[0].Name != cloudCredentialsVolumeName { + t.Errorf("volume name = %q, want %q", deployment.Spec.Template.Spec.Volumes[0].Name, cloudCredentialsVolumeName) + } + if tt.wantMountPath != "" && len(deployment.Spec.Template.Spec.Containers[0].VolumeMounts) > 0 { + if deployment.Spec.Template.Spec.Containers[0].VolumeMounts[0].MountPath != tt.wantMountPath { + t.Errorf("mount path = %q, want %q", deployment.Spec.Template.Spec.Containers[0].VolumeMounts[0].MountPath, tt.wantMountPath) + } + } + var hasAWS bool + for _, e := range deployment.Spec.Template.Spec.Containers[0].Env { + if e.Name == "AWS_SDK_LOAD_CONFIG" { + hasAWS = true + break + } + } + if hasAWS != tt.wantAWSEnv { + t.Errorf("AWS_SDK_LOAD_CONFIG present = %v, want %v", hasAWS, tt.wantAWSEnv) + } + } + }) + } +} + +// fakeInfrastructureInformer implements configinformersv1.InfrastructureInformer using a fixed lister. +type fakeInfrastructureInformer struct { + lister configlistersv1.InfrastructureLister +} + +func (f *fakeInfrastructureInformer) Informer() cache.SharedIndexInformer { + return nil +} + +func (f *fakeInfrastructureInformer) Lister() configlistersv1.InfrastructureLister { + return f.lister +} + +var _ configinformersv1.InfrastructureInformer = (*fakeInfrastructureInformer)(nil) diff --git a/pkg/controller/certmanager/deployment_overrides_test.go b/pkg/controller/certmanager/deployment_overrides_test.go index aa8c111c9..7de566490 100644 --- a/pkg/controller/certmanager/deployment_overrides_test.go +++ b/pkg/controller/certmanager/deployment_overrides_test.go @@ -2,6 +2,7 @@ package certmanager import ( "reflect" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -9,9 +10,14 @@ import ( "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/cache" + corelistersv1 "k8s.io/client-go/listers/core/v1" "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" "github.com/openshift/cert-manager-operator/pkg/operator/assets" + "github.com/openshift/cert-manager-operator/pkg/operator/operatorclient" ) func TestUnsupportedConfigOverrides(t *testing.T) { @@ -457,3 +463,154 @@ func TestParseArgMap(t *testing.T) { t.Fatalf("unexpected update to arg map, diff = %v", cmp.Diff(wantMap, argMap)) } } + +// TestWithOperandImageOverrideHook covers the modified hook (parameter rename). +func TestWithOperandImageOverrideHook(t *testing.T) { + originalImage := "quay.io/jetstack/cert-manager-controller:v1.19.2" + deployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: certmanagerControllerDeployment}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "controller", Image: originalImage, Args: []string{"--v=2"}}, + }, + }, + }, + }, + } + err := withOperandImageOverrideHook(nil, deployment) + require.NoError(t, err) + require.Len(t, deployment.Spec.Template.Spec.Containers, 1) + + // Verify image was overridden + newImage := deployment.Spec.Template.Spec.Containers[0].Image + require.NotEmpty(t, newImage, "image should be set") + + // certManagerImage resolves image; at least args should contain acme solver for controller + args := deployment.Spec.Template.Spec.Containers[0].Args + var hasAcme bool + var acmeImage string + for _, a := range args { + if strings.HasPrefix(a, "--acme-http01-solver-image=") { + hasAcme = true + acmeImage = strings.TrimPrefix(a, "--acme-http01-solver-image=") + break + } + } + require.True(t, hasAcme, "controller deployment should get acme-http01-solver-image arg") + require.NotEmpty(t, acmeImage, "acme solver image value should not be empty") +} + +// TestWithProxyEnv covers the modified hook (parameter rename). +func TestWithProxyEnv(t *testing.T) { + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "c", Env: []corev1.EnvVar{{Name: "EXISTING", Value: "x"}}}}, + }, + }, + }, + } + err := withProxyEnv(nil, deployment) + require.NoError(t, err) + // mergeContainerEnvs merges; we should have at least EXISTING plus any proxy vars from env + require.NotEmpty(t, deployment.Spec.Template.Spec.Containers[0].Env) +} + +// TestWithCAConfigMap covers the modified hook (parameter rename); empty name and success path. +func TestWithCAConfigMap(t *testing.T) { + t.Run("empty configmap name returns nil", func(t *testing.T) { + hook := withCAConfigMap(nil, nil, "") + dep := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "c"}}}, + }, + }, + } + err := hook(nil, dep) + require.NoError(t, err) + }) + t.Run("configmap not found returns retry error", func(t *testing.T) { + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + // empty indexer -> Get("my-ca") will return not found + lister := corelistersv1.NewConfigMapLister(indexer) + fakeInformer := &fakeConfigMapInformer{lister: lister} + hook := withCAConfigMap(fakeInformer, nil, "my-ca") + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "c"}}, + }, + }, + }, + } + err := hook(nil, deployment) + require.Error(t, err) + require.True(t, strings.Contains(err.Error(), "Retrying") || apierrors.IsNotFound(err), "expected retry or NotFound") + }) + t.Run("configmap exists adds volume and volume mounts", func(t *testing.T) { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "trusted-ca", Namespace: operatorclient.TargetNamespace}, + } + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + require.NoError(t, indexer.Add(cm)) + lister := corelistersv1.NewConfigMapLister(indexer) + fakeInformer := &fakeConfigMapInformer{lister: lister} + hook := withCAConfigMap(fakeInformer, nil, "trusted-ca") + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "c1"}, {Name: "c2"}}, + }, + }, + }, + } + err := hook(nil, deployment) + require.NoError(t, err) + require.NotEmpty(t, deployment.Spec.Template.Spec.Volumes) + require.Equal(t, trustedCAVolumeName, deployment.Spec.Template.Spec.Volumes[len(deployment.Spec.Template.Spec.Volumes)-1].Name) + for i := range deployment.Spec.Template.Spec.Containers { + require.NotEmpty(t, deployment.Spec.Template.Spec.Containers[i].VolumeMounts) + } + }) +} + +// fakeConfigMapInformer implements coreinformersv1.ConfigMapInformer for tests. +type fakeConfigMapInformer struct { + lister corelistersv1.ConfigMapLister +} + +func (f *fakeConfigMapInformer) Informer() cache.SharedIndexInformer { return nil } +func (f *fakeConfigMapInformer) Lister() corelistersv1.ConfigMapLister { + return f.lister +} + +// TestWithSABoundToken covers the modified hook (parameter rename). +func TestWithSABoundToken(t *testing.T) { + deployment := &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "controller"}}, + }, + }, + }, + } + err := withSABoundToken(nil, deployment) + require.NoError(t, err) + require.NotEmpty(t, deployment.Spec.Template.Spec.Volumes) + var found bool + for _, v := range deployment.Spec.Template.Spec.Volumes { + if v.Name == boundSATokenVolumeName { + found = true + break + } + } + require.True(t, found) + require.NotEmpty(t, deployment.Spec.Template.Spec.Containers[0].VolumeMounts) +} diff --git a/pkg/controller/common/client_test.go b/pkg/controller/common/client_test.go new file mode 100644 index 000000000..ff10003ec --- /dev/null +++ b/pkg/controller/common/client_test.go @@ -0,0 +1,101 @@ +package common + +import ( + "context" + "net/http" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/config" + "sigs.k8s.io/controller-runtime/pkg/healthz" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" +) + +// fakeManager implements manager.Manager for unit testing NewClient. +// Only GetClient is used by NewClient; other methods return nil/zero/no-op. +type fakeManager struct { + client client.Client +} + +func (f *fakeManager) GetClient() client.Client { return f.client } +func (f *fakeManager) GetCache() cache.Cache { return nil } +func (f *fakeManager) GetScheme() *runtime.Scheme { return nil } +func (f *fakeManager) GetConfig() *rest.Config { return &rest.Config{} } +func (f *fakeManager) GetHTTPClient() *http.Client { return &http.Client{} } +func (f *fakeManager) GetFieldIndexer() client.FieldIndexer { return nil } +func (f *fakeManager) GetEventRecorderFor(string) record.EventRecorder { return nil } +func (f *fakeManager) GetRESTMapper() meta.RESTMapper { return nil } +func (f *fakeManager) GetAPIReader() client.Reader { return f.client } +func (f *fakeManager) Start(context.Context) error { return nil } +func (f *fakeManager) Add(manager.Runnable) error { return nil } +func (f *fakeManager) Elected() <-chan struct{} { ch := make(chan struct{}); close(ch); return ch } +func (f *fakeManager) AddMetricsServerExtraHandler(string, http.Handler) error { return nil } +func (f *fakeManager) AddHealthzCheck(string, healthz.Checker) error { return nil } +func (f *fakeManager) AddReadyzCheck(string, healthz.Checker) error { return nil } +func (f *fakeManager) GetWebhookServer() webhook.Server { return nil } +func (f *fakeManager) GetLogger() logr.Logger { return logr.Discard() } +func (f *fakeManager) GetControllerOptions() config.Controller { return config.Controller{} } + +// TestNewClient provides table-driven tests for NewClient(m manager.Manager) (CtrlClient, error). +// Uses a nil client to avoid depending on controller-runtime/pkg/client/fake (not in vendor). +func TestNewClient(t *testing.T) { + var cl client.Client = nil + mgr := &fakeManager{client: cl} + + tests := []struct { + name string + m manager.Manager + expectPanic bool + }{ + { + name: "happy path - valid manager returns CtrlClient", + m: mgr, + expectPanic: false, + }, + { + name: "nil manager - GetClient panics", + m: nil, + expectPanic: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.expectPanic { + defer func() { + r := recover() + if r == nil { + t.Fatal("expected panic but got none") + } + t.Logf("NewClient(nil) panicked as expected: %v", r) + }() + } + got, err := NewClient(tt.m) + if !tt.expectPanic { + require.NoError(t, err) + require.NotNil(t, got) + var _ CtrlClient = got + } + }) + } +} + +// TestNewClient_GetClientReturnsSameClient verifies the returned client wraps the manager's client. +func TestNewClient_GetClientReturnsSameClient(t *testing.T) { + var cl client.Client = nil + mgr := &fakeManager{client: cl} + ctrlClient, err := NewClient(mgr) + require.NoError(t, err) + require.NotNil(t, ctrlClient) + impl, ok := ctrlClient.(*ctrlClientImpl) + require.True(t, ok, "NewClient must return *ctrlClientImpl") + assert.Equal(t, cl, impl.Client, "client must be the manager's client") +} diff --git a/pkg/controller/common/errors_test.go b/pkg/controller/common/errors_test.go new file mode 100644 index 000000000..00db91b18 --- /dev/null +++ b/pkg/controller/common/errors_test.go @@ -0,0 +1,332 @@ +package common + +import ( + "fmt" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func TestReconcileErrors(t *testing.T) { + tests := []struct { + name string + fn func() (*ReconcileError, error) + checkReason ErrorReason + checkErr bool + wantNil bool + }{ + { + name: "NewIrrecoverableError", + fn: func() (*ReconcileError, error) { + e := NewIrrecoverableError(fmt.Errorf("x"), "message: %s", "arg") + return e, nil + }, + checkReason: IrrecoverableError, + checkErr: true, + }, + { + name: "NewIrrecoverableError nil input returns nil", + fn: func() (*ReconcileError, error) { + return NewIrrecoverableError(nil, "msg"), nil + }, + wantNil: true, + }, + { + name: "NewMultipleInstanceError", + fn: func() (*ReconcileError, error) { + return NewMultipleInstanceError(fmt.Errorf("multiple")), nil + }, + checkReason: MultipleInstanceError, + checkErr: true, + }, + { + name: "NewMultipleInstanceError nil input returns nil", + fn: func() (*ReconcileError, error) { + return NewMultipleInstanceError(nil), nil + }, + wantNil: true, + }, + { + name: "NewRetryRequiredError", + fn: func() (*ReconcileError, error) { + return NewRetryRequiredError(fmt.Errorf("x"), "retry: %d", 1), nil + }, + checkReason: RetryRequiredError, + checkErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, _ := tt.fn() + if tt.wantNil { + if got != nil { + t.Errorf("expected nil, got %v", got) + } + return + } + if got == nil { + t.Fatal("expected non-nil error") + } + if tt.checkReason != "" && got.Reason != tt.checkReason { + t.Errorf("Reason = %v, want %v", got.Reason, tt.checkReason) + } + if tt.checkErr && got.Err == nil { + t.Error("Err should be set") + } + }) + } +} + +func TestFromClientError(t *testing.T) { + tests := []struct { + name string + clientErr error + wantIrrecover bool + wantRetry bool + }{ + { + name: "Unauthorized is irrecoverable", + clientErr: apierrors.NewUnauthorized("forbidden"), + wantIrrecover: true, + }, + { + name: "Forbidden is irrecoverable", + clientErr: apierrors.NewForbidden(schema.GroupResource{Resource: "test"}, "resource", fmt.Errorf("forbidden")), + wantIrrecover: true, + }, + { + name: "Invalid is irrecoverable", + clientErr: apierrors.NewInvalid(schema.GroupKind{Kind: "Pod"}, "test-pod", nil), + wantIrrecover: true, + }, + { + name: "BadRequest is irrecoverable", + clientErr: apierrors.NewBadRequest("bad request"), + wantIrrecover: true, + }, + { + name: "ServiceUnavailable is irrecoverable", + clientErr: apierrors.NewServiceUnavailable("service unavailable"), + wantIrrecover: true, + }, + { + name: "Conflict is retry required", + clientErr: apierrors.NewConflict(schema.GroupResource{Resource: "test"}, "resource", fmt.Errorf("conflict")), + wantRetry: true, + }, + { + name: "NotFound is retry required", + clientErr: apierrors.NewNotFound(schema.GroupResource{Resource: "test"}, "resource"), + wantRetry: true, + }, + { + name: "AlreadyExists is retry required", + clientErr: apierrors.NewAlreadyExists(schema.GroupResource{Resource: "test"}, "resource"), + wantRetry: true, + }, + { + name: "nil error returns nil", + clientErr: nil, + wantRetry: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := FromClientError(tt.clientErr, "client err: %s", "context") + if tt.clientErr == nil { + if err != nil { + t.Errorf("expected nil for nil input, got %v", err) + } + return + } + if err == nil { + t.Fatal("expected non-nil error") + } + if tt.wantIrrecover && !IsIrrecoverableError(err) { + t.Error("expected irrecoverable") + } + if tt.wantRetry && !IsRetryRequiredError(err) { + t.Error("expected retry required") + } + // Verify message formatting works + if err.Message != "client err: context" { + t.Errorf("Message = %q, want %q", err.Message, "client err: context") + } + }) + } +} + +func TestFromError(t *testing.T) { + tests := []struct { + name string + inputErr error + wantNil bool + wantReason ErrorReason + wantMessage string + }{ + { + name: "preserves irrecoverable error", + inputErr: NewIrrecoverableError(fmt.Errorf("base"), "irrecoverable"), + wantReason: IrrecoverableError, + wantMessage: "wrapped: %s", + }, + { + name: "preserves retry error as retry", + inputErr: NewRetryRequiredError(fmt.Errorf("base"), "retry"), + wantReason: RetryRequiredError, + wantMessage: "wrapped: %s", + }, + { + name: "converts plain error to retry", + inputErr: fmt.Errorf("plain error"), + wantReason: RetryRequiredError, + wantMessage: "converted", + }, + { + name: "nil error returns nil", + inputErr: nil, + wantNil: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := FromError(tt.inputErr, tt.wantMessage, "arg") + if tt.wantNil { + if err != nil { + t.Errorf("expected nil, got %v", err) + } + return + } + if err == nil { + t.Fatal("expected non-nil error") + } + if err.Reason != tt.wantReason { + t.Errorf("Reason = %v, want %v", err.Reason, tt.wantReason) + } + }) + } +} + +func TestIsIrrecoverableError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "true for ReconcileError with IrrecoverableError reason", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg"), + want: true, + }, + { + name: "false for plain error", + err: fmt.Errorf("plain"), + want: false, + }, + { + name: "false for RetryRequiredError", + err: NewRetryRequiredError(fmt.Errorf("x"), "msg"), + want: false, + }, + { + name: "false for nil", + err: nil, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsIrrecoverableError(tt.err) + if got != tt.want { + t.Errorf("IsIrrecoverableError() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsRetryRequiredError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "true for RetryRequiredError", + err: NewRetryRequiredError(fmt.Errorf("x"), "msg"), + want: true, + }, + { + name: "false for IrrecoverableError", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg"), + want: false, + }, + { + name: "false for plain error", + err: fmt.Errorf("plain"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsRetryRequiredError(tt.err) + if got != tt.want { + t.Errorf("IsRetryRequiredError() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsMultipleInstanceError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "true for MultipleInstanceError", + err: NewMultipleInstanceError(fmt.Errorf("multiple")), + want: true, + }, + { + name: "false for IrrecoverableError", + err: NewIrrecoverableError(fmt.Errorf("x"), "msg"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsMultipleInstanceError(tt.err) + if got != tt.want { + t.Errorf("IsMultipleInstanceError() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestReconcileError_Error(t *testing.T) { + tests := []struct { + name string + err *ReconcileError + wantMsg string + }{ + { + name: "formats message with error", + err: NewIrrecoverableError(fmt.Errorf("base"), "message"), + wantMsg: "message: base", + }, + { + name: "formats with arguments", + err: NewRetryRequiredError(fmt.Errorf("err"), "retry: %d", 3), + wantMsg: "retry: 3: err", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.err.Error() + if got != tt.wantMsg { + t.Errorf("Error() = %q, want %q", got, tt.wantMsg) + } + }) + } +} diff --git a/pkg/controller/common/fakes/fake_ctrl_client_test.go b/pkg/controller/common/fakes/fake_ctrl_client_test.go new file mode 100644 index 000000000..014ba332b --- /dev/null +++ b/pkg/controller/common/fakes/fake_ctrl_client_test.go @@ -0,0 +1,166 @@ +package fakes + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// TestFakeCtrlClient_Exists documents behavior of Exists with client.ObjectKey (refactor safety). +func TestFakeCtrlClient_Exists(t *testing.T) { + tests := []struct { + name string + setupStub func(*FakeCtrlClient) + key client.ObjectKey + obj client.Object + expectedOk bool + expectError bool + errorMsg string + }{ + { + name: "happy path - stub returns true", + setupStub: func(f *FakeCtrlClient) { + f.ExistsReturns(true, nil) + }, + key: client.ObjectKey{Namespace: "ns", Name: "cm"}, + obj: &corev1.ConfigMap{}, + expectedOk: true, + expectError: false, + }, + { + name: "happy path - stub returns false (not found)", + setupStub: func(f *FakeCtrlClient) { + f.ExistsReturns(false, nil) + }, + key: client.ObjectKey{Namespace: "ns", Name: "missing"}, + obj: &corev1.ConfigMap{}, + expectedOk: false, + expectError: false, + }, + { + name: "error case - stub returns error", + setupStub: func(f *FakeCtrlClient) { + f.ExistsReturns(false, errors.New("api server error")) + }, + key: client.ObjectKey{Namespace: "ns", Name: "x"}, + obj: &corev1.ConfigMap{}, + expectError: true, + errorMsg: "api server error", + }, + { + name: "boundary - empty ObjectKey", + setupStub: func(f *FakeCtrlClient) { f.ExistsReturns(false, nil) }, + key: client.ObjectKey{}, + obj: &corev1.ConfigMap{}, + expectedOk: false, + expectError: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fake := &FakeCtrlClient{} + if tt.setupStub != nil { + tt.setupStub(fake) + } + ok, err := fake.Exists(context.Background(), tt.key, tt.obj) + if tt.expectError { + require.Error(t, err) + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg) + } + return + } + require.NoError(t, err) + assert.Equal(t, tt.expectedOk, ok) + }) + } +} + +// TestFakeCtrlClient_ExistsArgsForCall verifies ExistsCalls / ExistsArgsForCall use client.ObjectKey. +func TestFakeCtrlClient_ExistsArgsForCall(t *testing.T) { + fake := &FakeCtrlClient{} + fake.ExistsReturns(true, nil) + key := client.ObjectKey{Namespace: "cert-manager", Name: "trusted-ca"} + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace}} + _, _ = fake.Exists(context.Background(), key, obj) + require.Equal(t, 1, fake.ExistsCallCount()) + ctx, gotKey, gotObj := fake.ExistsArgsForCall(0) + require.NotNil(t, ctx) + assert.Equal(t, key, gotKey, "ExistsArgsForCall must return client.ObjectKey") + assert.Same(t, obj, gotObj) +} + +// TestFakeCtrlClient_Get documents behavior of Get with client.ObjectKey. +func TestFakeCtrlClient_Get(t *testing.T) { + tests := []struct { + name string + setupStub func(*FakeCtrlClient) + key client.ObjectKey + obj client.Object + expectError bool + errorMsg string + }{ + { + name: "happy path - stub returns nil", + setupStub: func(f *FakeCtrlClient) { + f.GetStub = func(_ context.Context, _ client.ObjectKey, obj client.Object) error { + cm := &corev1.ConfigMap{} + cm.SetName("x") + cm.SetNamespace("ns") + cm.DeepCopyInto(obj.(*corev1.ConfigMap)) + return nil + } + }, + key: client.ObjectKey{Namespace: "ns", Name: "x"}, + obj: &corev1.ConfigMap{}, + expectError: false, + }, + { + name: "error case - stub returns error", + setupStub: func(f *FakeCtrlClient) { + f.GetReturns(errors.New("not found")) + }, + key: client.ObjectKey{Namespace: "ns", Name: "missing"}, + obj: &corev1.ConfigMap{}, + expectError: true, + errorMsg: "not found", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fake := &FakeCtrlClient{} + if tt.setupStub != nil { + tt.setupStub(fake) + } + err := fake.Get(context.Background(), tt.key, tt.obj) + if tt.expectError { + require.Error(t, err) + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg) + } + return + } + require.NoError(t, err) + }) + } +} + +// TestFakeCtrlClient_GetArgsForCall verifies GetCalls / GetArgsForCall use client.ObjectKey. +func TestFakeCtrlClient_GetArgsForCall(t *testing.T) { + fake := &FakeCtrlClient{} + fake.GetReturns(nil) + key := client.ObjectKey{Namespace: "default", Name: "secret"} + obj := &corev1.Secret{} + _ = fake.Get(context.Background(), key, obj) + require.Equal(t, 1, fake.GetCallCount()) + ctx, gotKey, gotObj := fake.GetArgsForCall(0) + require.NotNil(t, ctx) + assert.Equal(t, key, gotKey, "GetArgsForCall must return client.ObjectKey") + assert.Same(t, obj, gotObj) +} diff --git a/pkg/controller/common/utils_test.go b/pkg/controller/common/utils_test.go new file mode 100644 index 000000000..bba04b90a --- /dev/null +++ b/pkg/controller/common/utils_test.go @@ -0,0 +1,269 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// TestUpdateNamespace provides table-driven tests for UpdateNamespace(obj, newNamespace). +// Refactor may move this to ctrlutil; tests ensure behavior (side-effect on obj) is preserved. +func TestUpdateNamespace(t *testing.T) { + tests := []struct { + name string + obj client.Object + newNamespace string + expectedNs string + description string + }{ + { + name: "happy path - set new namespace", + obj: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "old-ns"}, + }, + newNamespace: "new-ns", + expectedNs: "new-ns", + description: "namespace is updated in place", + }, + { + name: "edge case - empty string namespace", + obj: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}, + }, + newNamespace: "", + expectedNs: "", + description: "empty namespace is allowed", + }, + { + name: "boundary - long namespace name", + obj: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "s", Namespace: "ns"}, + }, + newNamespace: "openshift-cert-manager-operator", + expectedNs: "openshift-cert-manager-operator", + description: "long namespace is set correctly", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + UpdateNamespace(tt.obj, tt.newNamespace) + assert.Equal(t, tt.expectedNs, tt.obj.GetNamespace(), tt.description) + }) + } +} + +// TestUpdateResourceLabels provides table-driven tests for UpdateResourceLabels(obj, labels). +func TestUpdateResourceLabels(t *testing.T) { + tests := []struct { + name string + obj client.Object + labels map[string]string + expectedLen int + checkLabels map[string]string + description string + }{ + { + name: "happy path - set labels", + obj: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + }, + labels: map[string]string{"key": "value", "a": "b"}, + expectedLen: 2, + checkLabels: map[string]string{"key": "value", "a": "b"}, + description: "labels replace existing", + }, + { + name: "edge case - nil labels", + obj: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"old": "v"}}, + }, + labels: nil, + expectedLen: 0, + checkLabels: nil, + description: "nil labels clears (SetLabels(nil) behavior)", + }, + { + name: "edge case - empty map", + obj: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Labels: map[string]string{"a": "1"}}, + }, + labels: map[string]string{}, + expectedLen: 0, + checkLabels: map[string]string{}, + description: "empty map clears labels", + }, + { + name: "single element", + obj: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "s"}}, + labels: map[string]string{"only": "one"}, + expectedLen: 1, + checkLabels: map[string]string{"only": "one"}, + description: "single label", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + UpdateResourceLabels(tt.obj, tt.labels) + got := tt.obj.GetLabels() + require.Len(t, got, tt.expectedLen, tt.description) + for k, v := range tt.checkLabels { + assert.Equal(t, v, got[k], "label %q", k) + } + }) + } +} + +func TestHasObjectChanged_DifferentTypes(t *testing.T) { + sa := &corev1.ServiceAccount{} + cm := &corev1.ConfigMap{} + if HasObjectChanged(sa, cm) { + t.Error("HasObjectChanged should return false for different types") + } +} + +func TestHasObjectChanged_SameTypeDifferentLabels(t *testing.T) { + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, + } + fetched := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "2"}}, + } + if !HasObjectChanged(desired, fetched) { + t.Error("HasObjectChanged should return true when labels differ") + } +} + +func TestHasObjectChanged_SameTypeSameLabels(t *testing.T) { + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, + } + fetched := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, + } + if HasObjectChanged(desired, fetched) { + t.Error("HasObjectChanged should return false when labels match") + } +} + +func TestObjectMetadataModified(t *testing.T) { + tests := []struct { + name string + desired map[string]string + fetched map[string]string + wantDiff bool + }{ + {"same", map[string]string{"a": "1"}, map[string]string{"a": "1"}, false}, + {"different value", map[string]string{"a": "1"}, map[string]string{"a": "2"}, true}, + {"different key", map[string]string{"a": "1"}, map[string]string{"b": "1"}, true}, + {"both nil", nil, nil, false}, + {"desired nil fetched has labels", nil, map[string]string{"a": "1"}, true}, + {"desired has labels fetched nil", map[string]string{"a": "1"}, nil, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Labels: tt.desired}} + f := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Labels: tt.fetched}} + got := ObjectMetadataModified(d, f) + if got != tt.wantDiff { + t.Errorf("ObjectMetadataModified() = %v, want %v", got, tt.wantDiff) + } + }) + } +} + +func TestContainsAnnotation(t *testing.T) { + obj := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"present": "yes"}, + }, + } + if !ContainsAnnotation(obj, "present") { + t.Error("ContainsAnnotation should be true for present key") + } + if ContainsAnnotation(obj, "absent") { + t.Error("ContainsAnnotation should be false for absent key") + } + empty := &corev1.ServiceAccount{} + if ContainsAnnotation(empty, "any") { + t.Error("ContainsAnnotation should be false when annotations nil") + } +} + +func TestAddAnnotation(t *testing.T) { + t.Run("adds when missing", func(t *testing.T) { + obj := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{}} + added := AddAnnotation(obj, "key", "value") + if !added { + t.Error("AddAnnotation should return true when adding") + } + if obj.GetAnnotations()["key"] != "value" { + t.Errorf("annotation value = %q, want %q", obj.GetAnnotations()["key"], "value") + } + }) + t.Run("no change when present", func(t *testing.T) { + obj := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"key": "existing"}, + }, + } + added := AddAnnotation(obj, "key", "new") + if added { + t.Error("AddAnnotation should return false when already present") + } + if obj.GetAnnotations()["key"] != "existing" { + t.Errorf("annotation should be unchanged: %q", obj.GetAnnotations()["key"]) + } + }) +} + +// Ensure we can pass any client.Object to HasObjectChanged (e.g. *appsv1.Deployment). +func TestHasObjectChanged_WithDifferentObjectTypes(t *testing.T) { + tests := []struct { + name string + desired client.Object + fetched client.Object + wantChanged bool + }{ + { + name: "Deployment with different labels", + desired: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "deploy", Labels: map[string]string{"a": "1"}}, + }, + fetched: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "deploy", Labels: map[string]string{"a": "2"}}, + }, + wantChanged: true, + }, + { + name: "Deployment with same labels", + desired: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "deploy", Labels: map[string]string{"a": "1"}}, + }, + fetched: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "deploy", Labels: map[string]string{"a": "1"}}, + }, + wantChanged: false, + }, + { + name: "ConfigMap with different labels", + desired: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Labels: map[string]string{"key": "val1"}}, + }, + fetched: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Labels: map[string]string{"key": "val2"}}, + }, + wantChanged: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := HasObjectChanged(tt.desired, tt.fetched) + assert.Equal(t, tt.wantChanged, got) + }) + } +} diff --git a/pkg/controller/istiocsr/utils_test.go b/pkg/controller/istiocsr/utils_test.go new file mode 100644 index 000000000..4c4e00f69 --- /dev/null +++ b/pkg/controller/istiocsr/utils_test.go @@ -0,0 +1,523 @@ +package istiocsr + +import ( + "fmt" + "strings" + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" +) + +// baseDeployment returns a minimal deployment for spec comparison tests. +func baseDeployment(replicas int32, image string) *appsv1.Deployment { + return &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: &replicas, + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "x"}}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"app": "x"}}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "c", + Image: image, + Args: []string{"--arg"}, + Ports: []corev1.ContainerPort{{ContainerPort: 8080}}, + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{HTTPGet: &corev1.HTTPGetAction{Path: "/ready"}}, + InitialDelaySeconds: 0, + PeriodSeconds: 1, + }, + SecurityContext: &corev1.SecurityContext{}, + Resources: corev1.ResourceRequirements{}, + }}, + ServiceAccountName: "sa", + }, + }, + }, + } +} + +func TestHasObjectChanged(t *testing.T) { + tests := []struct { + name string + desired client.Object + fetched client.Object + wantPanic bool + panicSubstr string + wantChanged bool + }{ + { + name: "different types panics", + desired: &rbacv1.ClusterRole{}, + fetched: &rbacv1.ClusterRoleBinding{}, + wantPanic: true, + panicSubstr: "same type", + }, + { + name: "mismatched types (CR vs CRB) panics", + desired: &rbacv1.ClusterRole{}, + fetched: &rbacv1.ClusterRoleBinding{}, + wantPanic: true, + panicSubstr: "same type", + }, + { + name: "matching ClusterRole identical", + desired: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"get"}}}, + }, + fetched: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"get"}}}, + }, + wantChanged: false, + }, + { + name: "ClusterRole rules different", + desired: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"get"}, APIGroups: []string{""}}}, + }, + fetched: &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"list"}}}, + }, + wantChanged: true, + }, + { + name: "matching ClusterRoleBinding identical", + desired: &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "x"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + }, + fetched: &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "x"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + }, + wantChanged: false, + }, + { + name: "Deployment labels different", + desired: func() *appsv1.Deployment { d := baseDeployment(1, "img"); d.Labels = map[string]string{"a": "1"}; return d }(), + fetched: func() *appsv1.Deployment { d := baseDeployment(1, "img"); d.Labels = map[string]string{"a": "2"}; return d }(), + wantChanged: true, + }, + { + name: "ConfigMap identical", + desired: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Data: map[string]string{"k": "v"}, + }, + fetched: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Data: map[string]string{"k": "v"}, + }, + wantChanged: false, + }, + { + name: "Service identical", + desired: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + fetched: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + wantChanged: false, + }, + { + name: "unsupported type (Pod) panics", + desired: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "a"}}, + fetched: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "a"}}, + wantPanic: true, + panicSubstr: "unsupported", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.wantPanic { + defer func() { + r := recover() + if r == nil { + t.Error("expected panic") + } + if msg := fmt.Sprintf("%v", r); tt.panicSubstr != "" && !strings.Contains(msg, tt.panicSubstr) { + t.Errorf("panic message = %q, want substring %q", msg, tt.panicSubstr) + } + }() + } + got := hasObjectChanged(tt.desired, tt.fetched) + if !tt.wantPanic && got != tt.wantChanged { + t.Errorf("hasObjectChanged() = %v, want %v", got, tt.wantChanged) + } + }) + } +} + +func TestDeploymentSpecModified(t *testing.T) { + tests := []struct { + name string + desired *appsv1.Deployment + fetched *appsv1.Deployment + wantTrue bool + }{ + { + name: "identical", + desired: baseDeployment(1, "img"), + fetched: baseDeployment(1, "img"), + wantTrue: false, + }, + { + name: "replicas different", + desired: baseDeployment(1, "img"), + fetched: baseDeployment(2, "img"), + wantTrue: true, + }, + { + name: "image different", + desired: baseDeployment(1, "img:v1"), + fetched: baseDeployment(1, "img:v2"), + wantTrue: true, + }, + { + name: "selector match labels different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Selector.MatchLabels = map[string]string{"app": "x"} + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Selector.MatchLabels = map[string]string{"app": "y"} + return d + }(), + wantTrue: true, + }, + { + name: "template labels different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Labels = map[string]string{"app": "x"} + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Labels = map[string]string{"app": "other"} + return d + }(), + wantTrue: true, + }, + { + name: "container count different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers = append(d.Spec.Template.Spec.Containers, corev1.Container{Name: "sidecar"}) + return d + }(), + wantTrue: true, + }, + { + name: "args different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Args = []string{"--arg=a"} + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Args = []string{"--arg=b"} + return d + }(), + wantTrue: true, + }, + { + name: "container name different", + desired: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Name = "c" + return d + }(), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Name = "other" + return d + }(), + wantTrue: true, + }, + { + name: "ports length different", + desired: baseDeployment(1, "img"), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Ports = append(d.Spec.Template.Spec.Containers[0].Ports, corev1.ContainerPort{ContainerPort: 9090}) + return d + }(), + wantTrue: true, + }, + { + name: "ports content different", + desired: baseDeployment(1, "img"), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].Ports = []corev1.ContainerPort{{ContainerPort: 9090}} + return d + }(), + wantTrue: true, + }, + { + name: "readiness probe path different", + desired: baseDeployment(1, "img"), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.Containers[0].ReadinessProbe.HTTPGet.Path = "/other" + return d + }(), + wantTrue: true, + }, + { + name: "serviceAccountName different", + desired: baseDeployment(1, "img"), + fetched: func() *appsv1.Deployment { + d := baseDeployment(1, "img") + d.Spec.Template.Spec.ServiceAccountName = "other-sa" + return d + }(), + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := deploymentSpecModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("deploymentSpecModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} + +func TestHasObjectChanged_RoleAndRoleBinding(t *testing.T) { + t.Run("Role rules different", func(t *testing.T) { + desired := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"get"}, APIGroups: []string{""}}}, + } + fetched := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + Rules: []rbacv1.PolicyRule{{Verbs: []string{"list"}}}, + } + if !hasObjectChanged(desired, fetched) { + t.Error("hasObjectChanged() = false, want true for Role rules different") + } + }) + t.Run("ClusterRoleBinding RoleRef different", func(t *testing.T) { + desired := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "role-a"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + fetched := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "role-b"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + if !hasObjectChanged(desired, fetched) { + t.Error("hasObjectChanged() = false, want true for RoleRef different") + } + }) + t.Run("ClusterRoleBinding Subjects different", func(t *testing.T) { + desired := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "x"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + fetched := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "ClusterRole", Name: "x"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "other", Namespace: "z"}}, + } + if !hasObjectChanged(desired, fetched) { + t.Error("hasObjectChanged() = false, want true for Subjects different") + } + }) + t.Run("RoleBinding RoleRef different", func(t *testing.T) { + desired := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "Role", Name: "role-a"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + fetched := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"a": "1"}}, + RoleRef: rbacv1.RoleRef{APIGroup: "rbac", Kind: "Role", Name: "role-b"}, + Subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Name: "y", Namespace: "z"}}, + } + if !hasObjectChanged(desired, fetched) { + t.Error("hasObjectChanged() = false, want true for RoleBinding RoleRef different") + } + }) +} + +func TestServiceSpecModified(t *testing.T) { + tests := []struct { + name string + desired *corev1.Service + fetched *corev1.Service + wantTrue bool + }{ + { + name: "identical", + desired: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + fetched: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + wantTrue: false, + }, + { + name: "different type", + desired: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + fetched: &corev1.Service{ + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{{Port: 443}}, + Selector: map[string]string{"app": "x"}, + }, + }, + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := serviceSpecModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("serviceSpecModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} + +func TestCertificateSpecModified(t *testing.T) { + tests := []struct { + name string + desired *certmanagerv1.Certificate + fetched *certmanagerv1.Certificate + wantTrue bool + }{ + { + name: "identical", + desired: &certmanagerv1.Certificate{Spec: certmanagerv1.CertificateSpec{DNSNames: []string{"a.example.com"}}}, + fetched: &certmanagerv1.Certificate{Spec: certmanagerv1.CertificateSpec{DNSNames: []string{"a.example.com"}}}, + wantTrue: false, + }, + { + name: "different DNSNames", + desired: &certmanagerv1.Certificate{Spec: certmanagerv1.CertificateSpec{DNSNames: []string{"a.example.com"}}}, + fetched: &certmanagerv1.Certificate{Spec: certmanagerv1.CertificateSpec{DNSNames: []string{"b.example.com"}}}, + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := certificateSpecModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("certificateSpecModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} + +func TestConfigMapDataModified(t *testing.T) { + tests := []struct { + name string + desired *corev1.ConfigMap + fetched *corev1.ConfigMap + wantTrue bool + }{ + { + name: "identical", + desired: &corev1.ConfigMap{Data: map[string]string{"key": "v1"}}, + fetched: &corev1.ConfigMap{Data: map[string]string{"key": "v1"}}, + wantTrue: false, + }, + { + name: "different value", + desired: &corev1.ConfigMap{Data: map[string]string{"key": "v1"}}, + fetched: &corev1.ConfigMap{Data: map[string]string{"key": "v2"}}, + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := configMapDataModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("configMapDataModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} + +func TestNetworkPolicySpecModified(t *testing.T) { + tests := []struct { + name string + desired *networkingv1.NetworkPolicy + fetched *networkingv1.NetworkPolicy + wantTrue bool + }{ + { + name: "identical", + desired: &networkingv1.NetworkPolicy{Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}}}, + fetched: &networkingv1.NetworkPolicy{Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}}}, + wantTrue: false, + }, + { + name: "different PolicyTypes", + desired: &networkingv1.NetworkPolicy{Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}}}, + fetched: &networkingv1.NetworkPolicy{Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeEgress}}}, + wantTrue: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := networkPolicySpecModified(tt.desired, tt.fetched) + if got != tt.wantTrue { + t.Errorf("networkPolicySpecModified() = %v, want %v", got, tt.wantTrue) + } + }) + } +} diff --git a/pkg/controller/trustmanager/controller_test.go b/pkg/controller/trustmanager/controller_test.go index 65fe79c76..3efd817a0 100644 --- a/pkg/controller/trustmanager/controller_test.go +++ b/pkg/controller/trustmanager/controller_test.go @@ -10,6 +10,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -370,3 +371,33 @@ func TestProcessReconcileRequest(t *testing.T) { }) } } + +func TestCleanUp(t *testing.T) { + tests := []struct { + name string + trustManager *v1alpha1.TrustManager + wantRequeue bool + wantErr bool + }{ + { + name: "returns false and nil", + trustManager: &v1alpha1.TrustManager{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + }, + wantRequeue: false, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &Reconciler{eventRecorder: record.NewFakeRecorder(10)} + requeue, err := r.cleanUp(tt.trustManager) + if (err != nil) != tt.wantErr { + t.Errorf("cleanUp() error = %v, wantErr %v", err, tt.wantErr) + } + if requeue != tt.wantRequeue { + t.Errorf("cleanUp() requeue = %v, want %v", requeue, tt.wantRequeue) + } + }) + } +} diff --git a/pkg/controller/trustmanager/utils_test.go b/pkg/controller/trustmanager/utils_test.go index d791ddeb0..7be9ba56a 100644 --- a/pkg/controller/trustmanager/utils_test.go +++ b/pkg/controller/trustmanager/utils_test.go @@ -1,9 +1,16 @@ package trustmanager import ( + "fmt" + "strings" "testing" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "github.com/openshift/cert-manager-operator/api/operator/v1alpha1" + "github.com/openshift/cert-manager-operator/pkg/controller/common" ) func TestGetTrustNamespace(t *testing.T) { @@ -112,6 +119,62 @@ func TestGetResourceAnnotations(t *testing.T) { } } +func TestManagedLabelsModified(t *testing.T) { + tests := []struct { + name string + desired map[string]string + existing map[string]string + want bool + }{ + { + name: "identical labels returns not modified", + desired: map[string]string{"a": "1"}, + existing: map[string]string{"a": "1"}, + want: false, + }, + { + name: "different value for same key returns modified", + desired: map[string]string{"a": "1"}, + existing: map[string]string{"a": "2"}, + want: true, + }, + { + name: "existing has extra labels beyond desired still not modified", + desired: map[string]string{"a": "1"}, + existing: map[string]string{"a": "1", "b": "2"}, + want: false, + }, + { + name: "desired label missing on existing returns modified", + desired: map[string]string{"a": "1"}, + existing: map[string]string{}, + want: true, + }, + { + name: "nil desired labels returns not modified", + desired: nil, + existing: map[string]string{"a": "1"}, + want: false, + }, + { + name: "both nil labels returns not modified", + desired: nil, + existing: nil, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + desired := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Labels: tt.desired}} + existing := &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Labels: tt.existing}} + got := managedLabelsModified(desired, existing) + if got != tt.want { + t.Errorf("managedLabelsModified() = %v, want %v", got, tt.want) + } + }) + } +} + func TestValidateTrustManagerConfig(t *testing.T) { tests := []struct { name string @@ -199,3 +262,83 @@ func TestUpdateResourceAnnotations(t *testing.T) { }) } } + +func mustEncode(t *testing.T, encoder runtime.Encoder, obj runtime.Object) []byte { + t.Helper() + b, err := runtime.Encode(encoder, obj) + if err != nil { + t.Fatalf("encode: %v", err) + } + return b +} + +// TestDecodeServiceAccountObjBytes exercises common.DecodeObjBytes for ServiceAccount assets (same path as serviceaccounts.go). +func TestDecodeServiceAccountObjBytes(t *testing.T) { + validSA := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "test-sa", Namespace: "test-ns"}, + } + configMapObj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "ns"}, + Data: map[string]string{"k": "v"}, + } + gv := corev1.SchemeGroupVersion + encoder := codecs.LegacyCodec(gv) + tests := []struct { + name string + getBytes func(t *testing.T) []byte + wantPanic bool + panicSubstr string + wantName string + wantNamespace string + }{ + { + name: "valid ServiceAccount bytes returns SA", + getBytes: func(t *testing.T) []byte { + return mustEncode(t, encoder, validSA) + }, + wantPanic: false, + wantName: "test-sa", + wantNamespace: "test-ns", + }, + { + name: "wrong type bytes panics", + getBytes: func(t *testing.T) []byte { + return mustEncode(t, encoder, configMapObj) + }, + wantPanic: true, + panicSubstr: "ServiceAccount", + }, + { + name: "invalid bytes panics", + getBytes: func(t *testing.T) []byte { + return []byte("not valid yaml or json") + }, + wantPanic: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + objBytes := tt.getBytes(t) + if tt.wantPanic { + defer func() { + r := recover() + if r == nil { + t.Error("expected panic") + } + if tt.panicSubstr != "" { + msg := fmt.Sprintf("%v", r) + if !strings.Contains(msg, tt.panicSubstr) && !strings.Contains(msg, "interface") { + t.Errorf("panic message = %q, want substring %q", msg, tt.panicSubstr) + } + } + }() + } + got := common.DecodeObjBytes[*corev1.ServiceAccount](codecs, gv, objBytes) + if !tt.wantPanic { + if got.Name != tt.wantName || got.Namespace != tt.wantNamespace { + t.Errorf("DecodeObjBytes() = %s/%s, want %s/%s", got.Name, got.Namespace, tt.wantName, tt.wantNamespace) + } + } + }) + } +} diff --git a/pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go b/pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go new file mode 100644 index 000000000..051478ab6 --- /dev/null +++ b/pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go @@ -0,0 +1,79 @@ +package v1alpha1 + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" +) + +// TestCertManagerConfigApplyConfiguration_WithIssuerRef provides table-driven tests for +// WithIssuerRef(value v1.IssuerReference). Refactor may change param type from ObjectReference to IssuerReference. +func TestCertManagerConfigApplyConfiguration_WithIssuerRef(t *testing.T) { + tests := []struct { + name string + input cmmeta.IssuerReference + chained bool + expected *cmmeta.IssuerReference + }{ + { + name: "happy path - set IssuerRef", + input: cmmeta.IssuerReference{ + Name: "my-issuer", + Kind: "ClusterIssuer", + Group: "cert-manager.io", + }, + chained: false, + expected: &cmmeta.IssuerReference{ + Name: "my-issuer", + Kind: "ClusterIssuer", + Group: "cert-manager.io", + }, + }, + { + name: "edge case - zero value IssuerRef", + input: cmmeta.IssuerReference{}, + chained: false, + expected: &cmmeta.IssuerReference{}, + }, + { + name: "chained call returns receiver", + input: cmmeta.IssuerReference{Name: "issuer", Kind: "Issuer", Group: "cert-manager.io"}, + chained: true, + expected: &cmmeta.IssuerReference{Name: "issuer", Kind: "Issuer", Group: "cert-manager.io"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := CertManagerConfig() + require.NotNil(t, b) + got := b.WithIssuerRef(tt.input) + require.Same(t, b, got, "WithIssuerRef must return receiver for chaining") + require.NotNil(t, b.IssuerRef) + assert.Equal(t, tt.expected.Name, b.IssuerRef.Name) + assert.Equal(t, tt.expected.Kind, b.IssuerRef.Kind) + assert.Equal(t, tt.expected.Group, b.IssuerRef.Group) + if tt.chained { + got2 := got.WithIssuerRef(cmmeta.IssuerReference{Name: "other"}) + assert.Same(t, got, got2) + assert.Equal(t, "other", b.IssuerRef.Name) + } + }) + } +} + +// TestCertManagerConfigApplyConfiguration_WithIssuerRef_nilReceiver documents that calling WithIssuerRef on nil receiver panics. +func TestCertManagerConfigApplyConfiguration_WithIssuerRef_nilReceiver(t *testing.T) { + var b *CertManagerConfigApplyConfiguration + panicked := false + func() { + defer func() { + if recover() != nil { + panicked = true + } + }() + _ = b.WithIssuerRef(cmmeta.IssuerReference{Name: "x"}) + }() + assert.True(t, panicked, "calling WithIssuerRef on nil receiver must panic") +} diff --git a/pkg/operator/applyconfigurations/utils_test.go b/pkg/operator/applyconfigurations/utils_test.go new file mode 100644 index 000000000..9f138f3f7 --- /dev/null +++ b/pkg/operator/applyconfigurations/utils_test.go @@ -0,0 +1,60 @@ +package applyconfigurations + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + managedfields "k8s.io/apimachinery/pkg/util/managedfields" +) + +// TestNewTypeConverter ensures the type converter is created from the scheme and +// can be used for managed fields. Refactoring may change the implementation +// (e.g. internal.Parser()); these tests guard behavior. +func TestNewTypeConverter(t *testing.T) { + tests := []struct { + name string + scheme *runtime.Scheme + expectError bool + description string + }{ + { + name: "happy path - valid scheme returns non-nil converter", + scheme: runtime.NewScheme(), + expectError: false, + description: "NewTypeConverter must return a non-nil TypeConverter for a valid scheme", + }, + { + name: "nil scheme - may panic or return converter", + scheme: nil, + expectError: false, + description: "Document behavior: nil scheme may panic in NewSchemeTypeConverter; refactor should preserve or explicitly reject", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var conv managedfields.TypeConverter + didPanic := false + func() { + defer func() { + if r := recover(); r != nil { + didPanic = true + if tt.scheme == nil { + t.Logf("NewTypeConverter(nil) panicked: %v", r) + } + } + }() + conv = NewTypeConverter(tt.scheme) + }() + if tt.scheme != nil { + require.False(t, didPanic, "NewTypeConverter(non-nil scheme) must not panic") + require.NotNil(t, conv, "NewTypeConverter must not return nil for non-nil scheme") + } + // Document: for nil scheme, either conv is nil (if we returned) or we panicked. + if !didPanic && conv != nil { + assert.NotNil(t, conv, tt.description) + } + }) + } +} diff --git a/pkg/operator/assets/bindata_test.go b/pkg/operator/assets/bindata_test.go new file mode 100644 index 000000000..bedc402e7 --- /dev/null +++ b/pkg/operator/assets/bindata_test.go @@ -0,0 +1,268 @@ +package assets + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // Known asset from bindata (cert-manager-tokenrequest RoleBinding). + tokenRequestRBAsset = "cert-manager-deployment/controller/cert-manager-tokenrequest-rb.yaml" + // Known directory in the bintree. + certManagerDeploymentDir = "cert-manager-deployment" + controllerDir = "cert-manager-deployment/controller" +) + +// TestAsset provides table-driven tests for Asset(name string) ([]byte, error). +func TestAsset(t *testing.T) { + tests := []struct { + name string + input string + expectedMin int + expectError bool + errorMsg string + }{ + { + name: "happy path - valid asset name returns bytes", + input: tokenRequestRBAsset, + expectedMin: 50, + expectError: false, + }, + { + name: "happy path - backslash normalized to slash", + input: strings.ReplaceAll(tokenRequestRBAsset, "/", "\\"), + expectedMin: 50, + expectError: false, + }, + { + name: "error case - empty name", + input: "", + expectError: true, + errorMsg: "not found", + }, + { + name: "error case - unknown asset", + input: "nonexistent/path.yaml", + expectError: true, + errorMsg: "not found", + }, + { + name: "error case - typo in asset name", + input: "cert-manager-deployment/controller/cert-manager-tokenrequest-rb.yam", + expectError: true, + errorMsg: "not found", + }, + { + name: "boundary - single known asset from AssetNames", + input: tokenRequestRBAsset, + expectedMin: 1, + expectError: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data, err := Asset(tt.input) + if tt.expectError { + require.Error(t, err) + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg) + } + return + } + require.NoError(t, err) + require.NotNil(t, data) + assert.GreaterOrEqual(t, len(data), tt.expectedMin, "asset content length") + }) + } +} + +// TestAssetNames ensures AssetNames() returns a non-empty list of known keys. +func TestAssetNames(t *testing.T) { + tests := []struct { + name string + expectEmpty bool + mustContain string + description string + }{ + { + name: "returns non-empty list", + expectEmpty: false, + mustContain: tokenRequestRBAsset, + description: "AssetNames must include tokenrequest-rb asset", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + names := AssetNames() + if tt.expectEmpty { + assert.Empty(t, names) + return + } + require.NotEmpty(t, names, "AssetNames() should not be empty") + assert.Contains(t, names, tt.mustContain, tt.description) + }) + } +} + +// TestAssetDir provides table-driven tests for AssetDir(name string) ([]string, error). +func TestAssetDir(t *testing.T) { + tests := []struct { + name string + input string + expectError bool + errorMsg string + minChildren int + description string + }{ + { + name: "happy path - root dir returns top-level children", + input: "", + expectError: false, + minChildren: 1, + description: "empty name returns root children (e.g. cert-manager-deployment)", + }, + { + name: "happy path - cert-manager-deployment dir", + input: certManagerDeploymentDir, + expectError: false, + minChildren: 1, + description: "directory returns child names", + }, + { + name: "happy path - controller subdir", + input: controllerDir, + expectError: false, + minChildren: 1, + description: "controller dir has at least one child", + }, + { + name: "error case - path to file not dir", + input: tokenRequestRBAsset, + expectError: true, + errorMsg: "not found", + description: "AssetDir on a file path returns error", + }, + { + name: "error case - nonexistent path", + input: "nonexistent/dir", + expectError: true, + errorMsg: "not found", + description: "nonexistent path returns error", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + children, err := AssetDir(tt.input) + if tt.expectError { + require.Error(t, err) + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg) + } + return + } + require.NoError(t, err) + require.NotNil(t, children) + assert.GreaterOrEqual(t, len(children), tt.minChildren, tt.description) + }) + } +} + +// TestAssetInfo provides table-driven tests for AssetInfo(name string) (os.FileInfo, error). +func TestAssetInfo(t *testing.T) { + tests := []struct { + name string + input string + expectError bool + errorMsg string + checkInfo func(t *testing.T, fi os.FileInfo) + }{ + { + name: "happy path - valid asset returns FileInfo", + input: tokenRequestRBAsset, + expectError: false, + checkInfo: func(t *testing.T, fi os.FileInfo) { + require.NotNil(t, fi) + assert.False(t, fi.IsDir()) + assert.Equal(t, "cert-manager-deployment/controller/cert-manager-tokenrequest-rb.yaml", fi.Name()) + }, + }, + { + name: "error case - asset not found", + input: "not/found.yaml", + expectError: true, + errorMsg: "not found", + }, + { + name: "error case - empty name", + input: "", + expectError: true, + errorMsg: "not found", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fi, err := AssetInfo(tt.input) + if tt.expectError { + require.Error(t, err) + if tt.errorMsg != "" { + assert.Contains(t, err.Error(), tt.errorMsg) + } + return + } + require.NoError(t, err) + if tt.checkInfo != nil { + tt.checkInfo(t, fi) + } + }) + } +} + +// TestMustAsset verifies MustAsset panics on error and returns bytes on success. +func TestMustAsset(t *testing.T) { + t.Run("happy path - valid name returns bytes", func(t *testing.T) { + data := MustAsset(tokenRequestRBAsset) + require.NotNil(t, data) + assert.GreaterOrEqual(t, len(data), 50) + }) + t.Run("panic on unknown asset", func(t *testing.T) { + defer func() { + r := recover() + require.NotNil(t, r, "MustAsset must panic when asset not found") + assert.Contains(t, r.(string), "not found") + }() + _ = MustAsset("nonexistent.yaml") + }) + t.Run("panic on empty name", func(t *testing.T) { + defer func() { + r := recover() + require.NotNil(t, r) + }() + _ = MustAsset("") + }) +} + +// TestCertManagerDeploymentControllerCertManagerTokenrequestRbYaml verifies the tokenrequest-rb +// asset is loadable via Asset (the generator functions are package-private). +func TestCertManagerDeploymentControllerCertManagerTokenrequestRbYaml(t *testing.T) { + data, err := Asset(tokenRequestRBAsset) + require.NoError(t, err) + require.NotNil(t, data) + // Content must look like a RoleBinding (YAML with kind and apiVersion). + str := string(data) + assert.Contains(t, str, "RoleBinding") + assert.Contains(t, str, "rbac.authorization.k8s.io") +} + +// TestCertManagerDeploymentControllerCertManagerTokenrequestRbYamlBytes verifies the same +// content is returned by loading the asset (Bytes is internal; we test via Asset). +func TestCertManagerDeploymentControllerCertManagerTokenrequestRbYamlBytes(t *testing.T) { + data, err := Asset(tokenRequestRBAsset) + require.NoError(t, err) + require.NotNil(t, data) + // Bytes() is used by the internal *Yaml() function; Asset returns the same bytes. + assert.Contains(t, string(data), "cert-manager-tokenrequest") +} From bfb2ac68b4dbaaec827781ae083ab9b03468a514 Mon Sep 17 00:00:00 2001 From: Anand Kumar Date: Mon, 23 Mar 2026 16:48:38 +0530 Subject: [PATCH 2/7] test: strengthen NewClient and validateTrustManagerConfig coverage - Use a non-nil sentinel client.Client in client tests so happy path asserts manager client is wired by pointer identity, not nil==nil. - Add a valid TrustManagerConfig case to TestValidateTrustManagerConfig to exercise validateTrustManagerConfig success path. Made-with: Cursor --- pkg/controller/common/client_test.go | 67 +++++++++++++++++++++-- pkg/controller/trustmanager/utils_test.go | 13 +++++ 2 files changed, 76 insertions(+), 4 deletions(-) diff --git a/pkg/controller/common/client_test.go b/pkg/controller/common/client_test.go index ff10003ec..8df3da405 100644 --- a/pkg/controller/common/client_test.go +++ b/pkg/controller/common/client_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/rest" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/cache" @@ -45,10 +46,65 @@ func (f *fakeManager) GetWebhookServer() webhook.Server { return nil } func (f *fakeManager) GetLogger() logr.Logger { return logr.Discard() } func (f *fakeManager) GetControllerOptions() config.Controller { return config.Controller{} } +// sentinelClient is a non-nil client.Client stub so NewClient tests can assert manager wiring +// (pointer identity) without controller-runtime's fake client (not vendored). +type sentinelClient struct{} + +type noopSubResourceWriter struct{} + +func (noopSubResourceWriter) Create(context.Context, client.Object, client.Object, ...client.SubResourceCreateOption) error { + return nil +} +func (noopSubResourceWriter) Update(context.Context, client.Object, ...client.SubResourceUpdateOption) error { + return nil +} +func (noopSubResourceWriter) Patch(context.Context, client.Object, client.Patch, ...client.SubResourcePatchOption) error { + return nil +} + +type noopSubResourceClient struct{} + +func (noopSubResourceClient) Get(context.Context, client.Object, client.Object, ...client.SubResourceGetOption) error { + return nil +} +func (noopSubResourceClient) Create(context.Context, client.Object, client.Object, ...client.SubResourceCreateOption) error { + return nil +} +func (noopSubResourceClient) Update(context.Context, client.Object, ...client.SubResourceUpdateOption) error { + return nil +} +func (noopSubResourceClient) Patch(context.Context, client.Object, client.Patch, ...client.SubResourcePatchOption) error { + return nil +} + +func (*sentinelClient) Get(context.Context, client.ObjectKey, client.Object, ...client.GetOption) error { + return nil +} +func (*sentinelClient) List(context.Context, client.ObjectList, ...client.ListOption) error { return nil } +func (*sentinelClient) Apply(context.Context, runtime.ApplyConfiguration, ...client.ApplyOption) error { + return nil +} +func (*sentinelClient) Create(context.Context, client.Object, ...client.CreateOption) error { return nil } +func (*sentinelClient) Delete(context.Context, client.Object, ...client.DeleteOption) error { return nil } +func (*sentinelClient) Update(context.Context, client.Object, ...client.UpdateOption) error { return nil } +func (*sentinelClient) Patch(context.Context, client.Object, client.Patch, ...client.PatchOption) error { + return nil +} +func (*sentinelClient) DeleteAllOf(context.Context, client.Object, ...client.DeleteAllOfOption) error { + return nil +} +func (*sentinelClient) Status() client.SubResourceWriter { return noopSubResourceWriter{} } +func (*sentinelClient) SubResource(string) client.SubResourceClient { return noopSubResourceClient{} } +func (*sentinelClient) Scheme() *runtime.Scheme { return nil } +func (*sentinelClient) RESTMapper() meta.RESTMapper { return nil } +func (*sentinelClient) GroupVersionKindFor(runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{}, nil +} +func (*sentinelClient) IsObjectNamespaced(runtime.Object) (bool, error) { return false, nil } + // TestNewClient provides table-driven tests for NewClient(m manager.Manager) (CtrlClient, error). -// Uses a nil client to avoid depending on controller-runtime/pkg/client/fake (not in vendor). func TestNewClient(t *testing.T) { - var cl client.Client = nil + var cl client.Client = &sentinelClient{} mgr := &fakeManager{client: cl} tests := []struct { @@ -83,6 +139,9 @@ func TestNewClient(t *testing.T) { require.NoError(t, err) require.NotNil(t, got) var _ CtrlClient = got + impl, ok := got.(*ctrlClientImpl) + require.True(t, ok, "NewClient must return *ctrlClientImpl") + assert.True(t, impl.Client == cl, "wrapped client must be the exact manager client instance") } }) } @@ -90,12 +149,12 @@ func TestNewClient(t *testing.T) { // TestNewClient_GetClientReturnsSameClient verifies the returned client wraps the manager's client. func TestNewClient_GetClientReturnsSameClient(t *testing.T) { - var cl client.Client = nil + var cl client.Client = &sentinelClient{} mgr := &fakeManager{client: cl} ctrlClient, err := NewClient(mgr) require.NoError(t, err) require.NotNil(t, ctrlClient) impl, ok := ctrlClient.(*ctrlClientImpl) require.True(t, ok, "NewClient must return *ctrlClientImpl") - assert.Equal(t, cl, impl.Client, "client must be the manager's client") + assert.True(t, impl.Client == cl, "client must be the exact same manager client instance") } diff --git a/pkg/controller/trustmanager/utils_test.go b/pkg/controller/trustmanager/utils_test.go index 7be9ba56a..5d4d2f628 100644 --- a/pkg/controller/trustmanager/utils_test.go +++ b/pkg/controller/trustmanager/utils_test.go @@ -212,6 +212,19 @@ func TestValidateTrustManagerConfig(t *testing.T) { tm: testTrustManager().WithAnnotations(map[string]string{"invalid/key/with/extra/slash": "val"}), wantErr: `spec.controllerConfig.annotations: Invalid value:`, }, + { + name: "non-empty trustManagerConfig is valid", + tm: &v1alpha1.TrustManager{ + Spec: v1alpha1.TrustManagerSpec{ + TrustManagerConfig: v1alpha1.TrustManagerConfig{ + LogLevel: 2, + LogFormat: "json", + TrustNamespace: "cert-manager", + }, + }, + }, + wantErr: false, + }, } for _, tt := range tests { From b4fe99970f0af2219cdb99623b2c9a2f808ab915 Mon Sep 17 00:00:00 2001 From: Anand Kumar Date: Tue, 24 Mar 2026 14:12:17 +0530 Subject: [PATCH 3/7] fix: align unit tests with ObjectMetadataModified and trustManagerBuilder - Use ObjectMetadataModified in common utils tests (HasObjectChanged does not exist). - Fix ValidateTrustManagerConfig case to use trustManagerBuilder and drop invalid wantErr. Made-with: Cursor --- pkg/controller/common/utils_test.go | 24 +++++++++++------------ pkg/controller/trustmanager/utils_test.go | 21 ++++++++++---------- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/pkg/controller/common/utils_test.go b/pkg/controller/common/utils_test.go index bba04b90a..582b950d8 100644 --- a/pkg/controller/common/utils_test.go +++ b/pkg/controller/common/utils_test.go @@ -118,35 +118,35 @@ func TestUpdateResourceLabels(t *testing.T) { } } -func TestHasObjectChanged_DifferentTypes(t *testing.T) { +func TestObjectMetadataModified_DifferentTypes(t *testing.T) { sa := &corev1.ServiceAccount{} cm := &corev1.ConfigMap{} - if HasObjectChanged(sa, cm) { - t.Error("HasObjectChanged should return false for different types") + if ObjectMetadataModified(sa, cm) { + t.Error("ObjectMetadataModified should return false when both objects have the same labels (including both empty)") } } -func TestHasObjectChanged_SameTypeDifferentLabels(t *testing.T) { +func TestObjectMetadataModified_SameTypeDifferentLabels(t *testing.T) { desired := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, } fetched := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "2"}}, } - if !HasObjectChanged(desired, fetched) { - t.Error("HasObjectChanged should return true when labels differ") + if !ObjectMetadataModified(desired, fetched) { + t.Error("ObjectMetadataModified should return true when labels differ") } } -func TestHasObjectChanged_SameTypeSameLabels(t *testing.T) { +func TestObjectMetadataModified_SameTypeSameLabels(t *testing.T) { desired := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, } fetched := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{Name: "x", Labels: map[string]string{"a": "1"}}, } - if HasObjectChanged(desired, fetched) { - t.Error("HasObjectChanged should return false when labels match") + if ObjectMetadataModified(desired, fetched) { + t.Error("ObjectMetadataModified should return false when labels match") } } @@ -221,8 +221,8 @@ func TestAddAnnotation(t *testing.T) { }) } -// Ensure we can pass any client.Object to HasObjectChanged (e.g. *appsv1.Deployment). -func TestHasObjectChanged_WithDifferentObjectTypes(t *testing.T) { +// Ensure we can pass any client.Object to ObjectMetadataModified (e.g. *appsv1.Deployment). +func TestObjectMetadataModified_WithDifferentObjectTypes(t *testing.T) { tests := []struct { name string desired client.Object @@ -262,7 +262,7 @@ func TestHasObjectChanged_WithDifferentObjectTypes(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := HasObjectChanged(tt.desired, tt.fetched) + got := ObjectMetadataModified(tt.desired, tt.fetched) assert.Equal(t, tt.wantChanged, got) }) } diff --git a/pkg/controller/trustmanager/utils_test.go b/pkg/controller/trustmanager/utils_test.go index 5d4d2f628..a113ace02 100644 --- a/pkg/controller/trustmanager/utils_test.go +++ b/pkg/controller/trustmanager/utils_test.go @@ -213,17 +213,16 @@ func TestValidateTrustManagerConfig(t *testing.T) { wantErr: `spec.controllerConfig.annotations: Invalid value:`, }, { - name: "non-empty trustManagerConfig is valid", - tm: &v1alpha1.TrustManager{ - Spec: v1alpha1.TrustManagerSpec{ - TrustManagerConfig: v1alpha1.TrustManagerConfig{ - LogLevel: 2, - LogFormat: "json", - TrustNamespace: "cert-manager", - }, - }, - }, - wantErr: false, + name: "non-empty trustManagerConfig with explicit fields is valid", + tm: func() *trustManagerBuilder { + b := testTrustManager() + b.Spec.TrustManagerConfig = v1alpha1.TrustManagerConfig{ + LogLevel: 2, + LogFormat: "json", + TrustNamespace: "cert-manager", + } + return b + }(), }, } From d149df8914d5416aad3d910f5fcb3449827e8e8c Mon Sep 17 00:00:00 2001 From: Anand Kumar Date: Tue, 24 Mar 2026 15:23:56 +0530 Subject: [PATCH 4/7] fix: harden NewClient and tests from review feedback Return (nil, error) from NewClient when manager is nil instead of panicking. Tighten withCloudCredentials tests: decoy-secret flag, NotFound only when expected, required VolumeMounts when wantMountPath is set. Remove duplicate bindata Asset case; format MustAsset panic recovery safely. Made-with: Cursor --- .../certmanager/credentials_request_test.go | 83 +++++++++++-------- pkg/controller/common/client.go | 3 + pkg/controller/common/client_test.go | 45 +++++----- pkg/operator/assets/bindata_test.go | 9 +- 4 files changed, 73 insertions(+), 67 deletions(-) diff --git a/pkg/controller/certmanager/credentials_request_test.go b/pkg/controller/certmanager/credentials_request_test.go index 57b7da53c..d9883781d 100644 --- a/pkg/controller/certmanager/credentials_request_test.go +++ b/pkg/controller/certmanager/credentials_request_test.go @@ -21,16 +21,18 @@ const testSecretName = "cloud-creds" func TestWithCloudCredentials(t *testing.T) { tests := []struct { - name string - deploymentName string - secretName string - secretInStore bool - platformType configv1.PlatformType - wantErr string - wantVolumes int - wantMountPath string - wantAWSEnv bool - noInfra bool // if true, infra indexer is left empty so Get("cluster") fails + name string + deploymentName string + secretName string + secretInStore bool + decoySecretOnly bool // lister has a different secret name so Get(secretName) fails (not brittle on tt.name) + platformType configv1.PlatformType + wantErr string + wantNotFoundOK bool // if true, apierrors.IsNotFound(err) is an acceptable match for this case + wantVolumes int + wantMountPath string + wantAWSEnv bool + noInfra bool // if true, infra indexer is left empty so Get("cluster") fails }{ { name: "non-controller deployment no-op", @@ -47,13 +49,14 @@ func TestWithCloudCredentials(t *testing.T) { wantVolumes: 0, }, { - name: "secret not found returns retry error", - deploymentName: certmanagerControllerDeployment, - secretName: "missing-secret", - secretInStore: false, - platformType: configv1.AWSPlatformType, - wantErr: "Retrying", - wantVolumes: 0, + name: "secret not found returns retry error", + deploymentName: certmanagerControllerDeployment, + secretName: "missing-secret", + secretInStore: false, + decoySecretOnly: true, + platformType: configv1.AWSPlatformType, + wantErr: "Retrying", + wantVolumes: 0, }, { name: "AWS adds volume, mount and env", @@ -85,14 +88,15 @@ func TestWithCloudCredentials(t *testing.T) { wantVolumes: 0, }, { - name: "infra not found returns error", - deploymentName: certmanagerControllerDeployment, - secretName: testSecretName, - secretInStore: true, - platformType: configv1.AWSPlatformType, - wantErr: "cluster", - noInfra: true, - wantVolumes: 0, + name: "infra not found returns error", + deploymentName: certmanagerControllerDeployment, + secretName: testSecretName, + secretInStore: true, + platformType: configv1.AWSPlatformType, + wantErr: "cluster", + wantNotFoundOK: true, + noInfra: true, + wantVolumes: 0, }, { name: "Azure platform is unsupported", @@ -112,20 +116,19 @@ func TestWithCloudCredentials(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: tt.secretName, Namespace: "cert-manager"}, } kubeClient = fake.NewSimpleClientset(secret) - } else { - kubeClient = fake.NewSimpleClientset() - } - if tt.name == "secret not found returns retry error" { + } else if tt.decoySecretOnly { kubeClient = fake.NewSimpleClientset(&corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: "cert-manager"}, }) + } else { + kubeClient = fake.NewSimpleClientset() } kubeInformers := informers.NewSharedInformerFactory(kubeClient, 0) if tt.secretInStore || tt.wantErr != "" { secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: tt.secretName, Namespace: "cert-manager"}, } - if tt.wantErr == "Retrying" { + if tt.decoySecretOnly { secret.Name = "other" } kubeInformers.Core().V1().Secrets().Informer().GetStore().Add(secret) @@ -172,8 +175,14 @@ func TestWithCloudCredentials(t *testing.T) { if err == nil { t.Fatalf("expected error containing %q, got nil", tt.wantErr) } - if !strings.Contains(err.Error(), tt.wantErr) && !apierrors.IsNotFound(err) { - t.Errorf("error = %v, want substring %q or NotFound", err, tt.wantErr) + matchSubstring := strings.Contains(err.Error(), tt.wantErr) + matchNotFound := tt.wantNotFoundOK && apierrors.IsNotFound(err) + if !matchSubstring && !matchNotFound { + if tt.wantNotFoundOK { + t.Errorf("error = %v, want substring %q or NotFound", err, tt.wantErr) + } else { + t.Errorf("error = %v, want substring %q", err, tt.wantErr) + } } return } @@ -187,9 +196,13 @@ func TestWithCloudCredentials(t *testing.T) { if deployment.Spec.Template.Spec.Volumes[0].Name != cloudCredentialsVolumeName { t.Errorf("volume name = %q, want %q", deployment.Spec.Template.Spec.Volumes[0].Name, cloudCredentialsVolumeName) } - if tt.wantMountPath != "" && len(deployment.Spec.Template.Spec.Containers[0].VolumeMounts) > 0 { - if deployment.Spec.Template.Spec.Containers[0].VolumeMounts[0].MountPath != tt.wantMountPath { - t.Errorf("mount path = %q, want %q", deployment.Spec.Template.Spec.Containers[0].VolumeMounts[0].MountPath, tt.wantMountPath) + if tt.wantMountPath != "" { + mounts := deployment.Spec.Template.Spec.Containers[0].VolumeMounts + if len(mounts) == 0 { + t.Fatalf("expected VolumeMount for mount path %q, got none (containers[0].VolumeMounts is empty)", tt.wantMountPath) + } + if mounts[0].MountPath != tt.wantMountPath { + t.Errorf("mount path = %q, want %q", mounts[0].MountPath, tt.wantMountPath) } } var hasAWS bool diff --git a/pkg/controller/common/client.go b/pkg/controller/common/client.go index f7d025ee2..4397b77fc 100644 --- a/pkg/controller/common/client.go +++ b/pkg/controller/common/client.go @@ -39,6 +39,9 @@ type CtrlClient interface { // reads from the same cache that the controller's watches use, preventing // cache mismatch issues. func NewClient(m manager.Manager) (CtrlClient, error) { + if m == nil { + return nil, fmt.Errorf("nil manager") + } return &ctrlClientImpl{ Client: m.GetClient(), }, nil diff --git a/pkg/controller/common/client_test.go b/pkg/controller/common/client_test.go index 8df3da405..d5ec2a8a8 100644 --- a/pkg/controller/common/client_test.go +++ b/pkg/controller/common/client_test.go @@ -108,41 +108,36 @@ func TestNewClient(t *testing.T) { mgr := &fakeManager{client: cl} tests := []struct { - name string - m manager.Manager - expectPanic bool + name string + m manager.Manager + wantError bool }{ { - name: "happy path - valid manager returns CtrlClient", - m: mgr, - expectPanic: false, + name: "happy path - valid manager returns CtrlClient", + m: mgr, + wantError: false, }, { - name: "nil manager - GetClient panics", - m: nil, - expectPanic: true, + name: "nil manager returns error", + m: nil, + wantError: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.expectPanic { - defer func() { - r := recover() - if r == nil { - t.Fatal("expected panic but got none") - } - t.Logf("NewClient(nil) panicked as expected: %v", r) - }() - } got, err := NewClient(tt.m) - if !tt.expectPanic { - require.NoError(t, err) - require.NotNil(t, got) - var _ CtrlClient = got - impl, ok := got.(*ctrlClientImpl) - require.True(t, ok, "NewClient must return *ctrlClientImpl") - assert.True(t, impl.Client == cl, "wrapped client must be the exact manager client instance") + if tt.wantError { + require.Error(t, err) + require.Nil(t, got) + assert.Contains(t, err.Error(), "nil manager") + return } + require.NoError(t, err) + require.NotNil(t, got) + var _ CtrlClient = got + impl, ok := got.(*ctrlClientImpl) + require.True(t, ok, "NewClient must return *ctrlClientImpl") + assert.True(t, impl.Client == cl, "wrapped client must be the exact manager client instance") }) } } diff --git a/pkg/operator/assets/bindata_test.go b/pkg/operator/assets/bindata_test.go index bedc402e7..7d084f1ad 100644 --- a/pkg/operator/assets/bindata_test.go +++ b/pkg/operator/assets/bindata_test.go @@ -1,6 +1,7 @@ package assets import ( + "fmt" "os" "strings" "testing" @@ -56,12 +57,6 @@ func TestAsset(t *testing.T) { expectError: true, errorMsg: "not found", }, - { - name: "boundary - single known asset from AssetNames", - input: tokenRequestRBAsset, - expectedMin: 1, - expectError: false, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -232,7 +227,7 @@ func TestMustAsset(t *testing.T) { defer func() { r := recover() require.NotNil(t, r, "MustAsset must panic when asset not found") - assert.Contains(t, r.(string), "not found") + assert.Contains(t, fmt.Sprintf("%v", r), "not found") }() _ = MustAsset("nonexistent.yaml") }) From add71f89fcb60b8657019c8b9bcdde2524462662 Mon Sep 17 00:00:00 2001 From: Anand Kumar Date: Tue, 24 Mar 2026 15:43:14 +0530 Subject: [PATCH 5/7] test(deployment): require NotFound and substring for infra error case When wantNotFoundOK is set, pass only if apierrors.IsNotFound(err) and the error message contains wantErr, avoiding unrelated NotFound matches. Made-with: Cursor --- .../certmanager/credentials_request_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/controller/certmanager/credentials_request_test.go b/pkg/controller/certmanager/credentials_request_test.go index d9883781d..bb0f1d438 100644 --- a/pkg/controller/certmanager/credentials_request_test.go +++ b/pkg/controller/certmanager/credentials_request_test.go @@ -28,7 +28,7 @@ func TestWithCloudCredentials(t *testing.T) { decoySecretOnly bool // lister has a different secret name so Get(secretName) fails (not brittle on tt.name) platformType configv1.PlatformType wantErr string - wantNotFoundOK bool // if true, apierrors.IsNotFound(err) is an acceptable match for this case + wantNotFoundOK bool // if true, err must be NotFound and still contain wantErr in the message wantVolumes int wantMountPath string wantAWSEnv bool @@ -176,10 +176,15 @@ func TestWithCloudCredentials(t *testing.T) { t.Fatalf("expected error containing %q, got nil", tt.wantErr) } matchSubstring := strings.Contains(err.Error(), tt.wantErr) - matchNotFound := tt.wantNotFoundOK && apierrors.IsNotFound(err) - if !matchSubstring && !matchNotFound { + var ok bool + if tt.wantNotFoundOK { + ok = apierrors.IsNotFound(err) && matchSubstring + } else { + ok = matchSubstring + } + if !ok { if tt.wantNotFoundOK { - t.Errorf("error = %v, want substring %q or NotFound", err, tt.wantErr) + t.Errorf("error = %v, want NotFound with message containing %q", err, tt.wantErr) } else { t.Errorf("error = %v, want substring %q", err, tt.wantErr) } From 0ea04a5027393b9cfeab3e21b9713cb0c6f1c0b5 Mon Sep 17 00:00:00 2001 From: Anand Kumar Date: Mon, 30 Mar 2026 12:26:04 +0530 Subject: [PATCH 6/7] Remove unit tests for generated bindata and applyconfiguration code Drop tests that only exercised go-bindata and applyconfiguration-gen output. Coverage should focus on hand-written operator logic; generated packages are not maintained as test targets. Made-with: Cursor --- .../v1alpha1/certmanagerconfig_test.go | 79 ------ .../applyconfigurations/utils_test.go | 60 ---- pkg/operator/assets/bindata_test.go | 263 ------------------ 3 files changed, 402 deletions(-) delete mode 100644 pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go delete mode 100644 pkg/operator/applyconfigurations/utils_test.go delete mode 100644 pkg/operator/assets/bindata_test.go diff --git a/pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go b/pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go deleted file mode 100644 index 051478ab6..000000000 --- a/pkg/operator/applyconfigurations/operator/v1alpha1/certmanagerconfig_test.go +++ /dev/null @@ -1,79 +0,0 @@ -package v1alpha1 - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" -) - -// TestCertManagerConfigApplyConfiguration_WithIssuerRef provides table-driven tests for -// WithIssuerRef(value v1.IssuerReference). Refactor may change param type from ObjectReference to IssuerReference. -func TestCertManagerConfigApplyConfiguration_WithIssuerRef(t *testing.T) { - tests := []struct { - name string - input cmmeta.IssuerReference - chained bool - expected *cmmeta.IssuerReference - }{ - { - name: "happy path - set IssuerRef", - input: cmmeta.IssuerReference{ - Name: "my-issuer", - Kind: "ClusterIssuer", - Group: "cert-manager.io", - }, - chained: false, - expected: &cmmeta.IssuerReference{ - Name: "my-issuer", - Kind: "ClusterIssuer", - Group: "cert-manager.io", - }, - }, - { - name: "edge case - zero value IssuerRef", - input: cmmeta.IssuerReference{}, - chained: false, - expected: &cmmeta.IssuerReference{}, - }, - { - name: "chained call returns receiver", - input: cmmeta.IssuerReference{Name: "issuer", Kind: "Issuer", Group: "cert-manager.io"}, - chained: true, - expected: &cmmeta.IssuerReference{Name: "issuer", Kind: "Issuer", Group: "cert-manager.io"}, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - b := CertManagerConfig() - require.NotNil(t, b) - got := b.WithIssuerRef(tt.input) - require.Same(t, b, got, "WithIssuerRef must return receiver for chaining") - require.NotNil(t, b.IssuerRef) - assert.Equal(t, tt.expected.Name, b.IssuerRef.Name) - assert.Equal(t, tt.expected.Kind, b.IssuerRef.Kind) - assert.Equal(t, tt.expected.Group, b.IssuerRef.Group) - if tt.chained { - got2 := got.WithIssuerRef(cmmeta.IssuerReference{Name: "other"}) - assert.Same(t, got, got2) - assert.Equal(t, "other", b.IssuerRef.Name) - } - }) - } -} - -// TestCertManagerConfigApplyConfiguration_WithIssuerRef_nilReceiver documents that calling WithIssuerRef on nil receiver panics. -func TestCertManagerConfigApplyConfiguration_WithIssuerRef_nilReceiver(t *testing.T) { - var b *CertManagerConfigApplyConfiguration - panicked := false - func() { - defer func() { - if recover() != nil { - panicked = true - } - }() - _ = b.WithIssuerRef(cmmeta.IssuerReference{Name: "x"}) - }() - assert.True(t, panicked, "calling WithIssuerRef on nil receiver must panic") -} diff --git a/pkg/operator/applyconfigurations/utils_test.go b/pkg/operator/applyconfigurations/utils_test.go deleted file mode 100644 index 9f138f3f7..000000000 --- a/pkg/operator/applyconfigurations/utils_test.go +++ /dev/null @@ -1,60 +0,0 @@ -package applyconfigurations - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/runtime" - managedfields "k8s.io/apimachinery/pkg/util/managedfields" -) - -// TestNewTypeConverter ensures the type converter is created from the scheme and -// can be used for managed fields. Refactoring may change the implementation -// (e.g. internal.Parser()); these tests guard behavior. -func TestNewTypeConverter(t *testing.T) { - tests := []struct { - name string - scheme *runtime.Scheme - expectError bool - description string - }{ - { - name: "happy path - valid scheme returns non-nil converter", - scheme: runtime.NewScheme(), - expectError: false, - description: "NewTypeConverter must return a non-nil TypeConverter for a valid scheme", - }, - { - name: "nil scheme - may panic or return converter", - scheme: nil, - expectError: false, - description: "Document behavior: nil scheme may panic in NewSchemeTypeConverter; refactor should preserve or explicitly reject", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var conv managedfields.TypeConverter - didPanic := false - func() { - defer func() { - if r := recover(); r != nil { - didPanic = true - if tt.scheme == nil { - t.Logf("NewTypeConverter(nil) panicked: %v", r) - } - } - }() - conv = NewTypeConverter(tt.scheme) - }() - if tt.scheme != nil { - require.False(t, didPanic, "NewTypeConverter(non-nil scheme) must not panic") - require.NotNil(t, conv, "NewTypeConverter must not return nil for non-nil scheme") - } - // Document: for nil scheme, either conv is nil (if we returned) or we panicked. - if !didPanic && conv != nil { - assert.NotNil(t, conv, tt.description) - } - }) - } -} diff --git a/pkg/operator/assets/bindata_test.go b/pkg/operator/assets/bindata_test.go deleted file mode 100644 index 7d084f1ad..000000000 --- a/pkg/operator/assets/bindata_test.go +++ /dev/null @@ -1,263 +0,0 @@ -package assets - -import ( - "fmt" - "os" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -const ( - // Known asset from bindata (cert-manager-tokenrequest RoleBinding). - tokenRequestRBAsset = "cert-manager-deployment/controller/cert-manager-tokenrequest-rb.yaml" - // Known directory in the bintree. - certManagerDeploymentDir = "cert-manager-deployment" - controllerDir = "cert-manager-deployment/controller" -) - -// TestAsset provides table-driven tests for Asset(name string) ([]byte, error). -func TestAsset(t *testing.T) { - tests := []struct { - name string - input string - expectedMin int - expectError bool - errorMsg string - }{ - { - name: "happy path - valid asset name returns bytes", - input: tokenRequestRBAsset, - expectedMin: 50, - expectError: false, - }, - { - name: "happy path - backslash normalized to slash", - input: strings.ReplaceAll(tokenRequestRBAsset, "/", "\\"), - expectedMin: 50, - expectError: false, - }, - { - name: "error case - empty name", - input: "", - expectError: true, - errorMsg: "not found", - }, - { - name: "error case - unknown asset", - input: "nonexistent/path.yaml", - expectError: true, - errorMsg: "not found", - }, - { - name: "error case - typo in asset name", - input: "cert-manager-deployment/controller/cert-manager-tokenrequest-rb.yam", - expectError: true, - errorMsg: "not found", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - data, err := Asset(tt.input) - if tt.expectError { - require.Error(t, err) - if tt.errorMsg != "" { - assert.Contains(t, err.Error(), tt.errorMsg) - } - return - } - require.NoError(t, err) - require.NotNil(t, data) - assert.GreaterOrEqual(t, len(data), tt.expectedMin, "asset content length") - }) - } -} - -// TestAssetNames ensures AssetNames() returns a non-empty list of known keys. -func TestAssetNames(t *testing.T) { - tests := []struct { - name string - expectEmpty bool - mustContain string - description string - }{ - { - name: "returns non-empty list", - expectEmpty: false, - mustContain: tokenRequestRBAsset, - description: "AssetNames must include tokenrequest-rb asset", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - names := AssetNames() - if tt.expectEmpty { - assert.Empty(t, names) - return - } - require.NotEmpty(t, names, "AssetNames() should not be empty") - assert.Contains(t, names, tt.mustContain, tt.description) - }) - } -} - -// TestAssetDir provides table-driven tests for AssetDir(name string) ([]string, error). -func TestAssetDir(t *testing.T) { - tests := []struct { - name string - input string - expectError bool - errorMsg string - minChildren int - description string - }{ - { - name: "happy path - root dir returns top-level children", - input: "", - expectError: false, - minChildren: 1, - description: "empty name returns root children (e.g. cert-manager-deployment)", - }, - { - name: "happy path - cert-manager-deployment dir", - input: certManagerDeploymentDir, - expectError: false, - minChildren: 1, - description: "directory returns child names", - }, - { - name: "happy path - controller subdir", - input: controllerDir, - expectError: false, - minChildren: 1, - description: "controller dir has at least one child", - }, - { - name: "error case - path to file not dir", - input: tokenRequestRBAsset, - expectError: true, - errorMsg: "not found", - description: "AssetDir on a file path returns error", - }, - { - name: "error case - nonexistent path", - input: "nonexistent/dir", - expectError: true, - errorMsg: "not found", - description: "nonexistent path returns error", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - children, err := AssetDir(tt.input) - if tt.expectError { - require.Error(t, err) - if tt.errorMsg != "" { - assert.Contains(t, err.Error(), tt.errorMsg) - } - return - } - require.NoError(t, err) - require.NotNil(t, children) - assert.GreaterOrEqual(t, len(children), tt.minChildren, tt.description) - }) - } -} - -// TestAssetInfo provides table-driven tests for AssetInfo(name string) (os.FileInfo, error). -func TestAssetInfo(t *testing.T) { - tests := []struct { - name string - input string - expectError bool - errorMsg string - checkInfo func(t *testing.T, fi os.FileInfo) - }{ - { - name: "happy path - valid asset returns FileInfo", - input: tokenRequestRBAsset, - expectError: false, - checkInfo: func(t *testing.T, fi os.FileInfo) { - require.NotNil(t, fi) - assert.False(t, fi.IsDir()) - assert.Equal(t, "cert-manager-deployment/controller/cert-manager-tokenrequest-rb.yaml", fi.Name()) - }, - }, - { - name: "error case - asset not found", - input: "not/found.yaml", - expectError: true, - errorMsg: "not found", - }, - { - name: "error case - empty name", - input: "", - expectError: true, - errorMsg: "not found", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - fi, err := AssetInfo(tt.input) - if tt.expectError { - require.Error(t, err) - if tt.errorMsg != "" { - assert.Contains(t, err.Error(), tt.errorMsg) - } - return - } - require.NoError(t, err) - if tt.checkInfo != nil { - tt.checkInfo(t, fi) - } - }) - } -} - -// TestMustAsset verifies MustAsset panics on error and returns bytes on success. -func TestMustAsset(t *testing.T) { - t.Run("happy path - valid name returns bytes", func(t *testing.T) { - data := MustAsset(tokenRequestRBAsset) - require.NotNil(t, data) - assert.GreaterOrEqual(t, len(data), 50) - }) - t.Run("panic on unknown asset", func(t *testing.T) { - defer func() { - r := recover() - require.NotNil(t, r, "MustAsset must panic when asset not found") - assert.Contains(t, fmt.Sprintf("%v", r), "not found") - }() - _ = MustAsset("nonexistent.yaml") - }) - t.Run("panic on empty name", func(t *testing.T) { - defer func() { - r := recover() - require.NotNil(t, r) - }() - _ = MustAsset("") - }) -} - -// TestCertManagerDeploymentControllerCertManagerTokenrequestRbYaml verifies the tokenrequest-rb -// asset is loadable via Asset (the generator functions are package-private). -func TestCertManagerDeploymentControllerCertManagerTokenrequestRbYaml(t *testing.T) { - data, err := Asset(tokenRequestRBAsset) - require.NoError(t, err) - require.NotNil(t, data) - // Content must look like a RoleBinding (YAML with kind and apiVersion). - str := string(data) - assert.Contains(t, str, "RoleBinding") - assert.Contains(t, str, "rbac.authorization.k8s.io") -} - -// TestCertManagerDeploymentControllerCertManagerTokenrequestRbYamlBytes verifies the same -// content is returned by loading the asset (Bytes is internal; we test via Asset). -func TestCertManagerDeploymentControllerCertManagerTokenrequestRbYamlBytes(t *testing.T) { - data, err := Asset(tokenRequestRBAsset) - require.NoError(t, err) - require.NotNil(t, data) - // Bytes() is used by the internal *Yaml() function; Asset returns the same bytes. - assert.Contains(t, string(data), "cert-manager-tokenrequest") -} From a3e30f9b0c7cede083547c6d29aad6e8b79c4934 Mon Sep 17 00:00:00 2001 From: Anand Kumar Date: Thu, 2 Apr 2026 12:20:48 +0530 Subject: [PATCH 7/7] client: drop nil manager check from NewClient The manager is always non-nil when controllers construct the client; removing the guard simplifies the code path for test coverage. Made-with: Cursor --- pkg/controller/common/client.go | 3 -- pkg/controller/common/client_test.go | 51 ++++------------------------ 2 files changed, 7 insertions(+), 47 deletions(-) diff --git a/pkg/controller/common/client.go b/pkg/controller/common/client.go index 4397b77fc..f7d025ee2 100644 --- a/pkg/controller/common/client.go +++ b/pkg/controller/common/client.go @@ -39,9 +39,6 @@ type CtrlClient interface { // reads from the same cache that the controller's watches use, preventing // cache mismatch issues. func NewClient(m manager.Manager) (CtrlClient, error) { - if m == nil { - return nil, fmt.Errorf("nil manager") - } return &ctrlClientImpl{ Client: m.GetClient(), }, nil diff --git a/pkg/controller/common/client_test.go b/pkg/controller/common/client_test.go index d5ec2a8a8..585448716 100644 --- a/pkg/controller/common/client_test.go +++ b/pkg/controller/common/client_test.go @@ -102,54 +102,17 @@ func (*sentinelClient) GroupVersionKindFor(runtime.Object) (schema.GroupVersionK } func (*sentinelClient) IsObjectNamespaced(runtime.Object) (bool, error) { return false, nil } -// TestNewClient provides table-driven tests for NewClient(m manager.Manager) (CtrlClient, error). +// TestNewClient verifies NewClient returns a CtrlClient that wraps the same client instance +// as manager.GetClient() (cache-consistent wiring). func TestNewClient(t *testing.T) { var cl client.Client = &sentinelClient{} mgr := &fakeManager{client: cl} - tests := []struct { - name string - m manager.Manager - wantError bool - }{ - { - name: "happy path - valid manager returns CtrlClient", - m: mgr, - wantError: false, - }, - { - name: "nil manager returns error", - m: nil, - wantError: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := NewClient(tt.m) - if tt.wantError { - require.Error(t, err) - require.Nil(t, got) - assert.Contains(t, err.Error(), "nil manager") - return - } - require.NoError(t, err) - require.NotNil(t, got) - var _ CtrlClient = got - impl, ok := got.(*ctrlClientImpl) - require.True(t, ok, "NewClient must return *ctrlClientImpl") - assert.True(t, impl.Client == cl, "wrapped client must be the exact manager client instance") - }) - } -} - -// TestNewClient_GetClientReturnsSameClient verifies the returned client wraps the manager's client. -func TestNewClient_GetClientReturnsSameClient(t *testing.T) { - var cl client.Client = &sentinelClient{} - mgr := &fakeManager{client: cl} - ctrlClient, err := NewClient(mgr) + got, err := NewClient(mgr) require.NoError(t, err) - require.NotNil(t, ctrlClient) - impl, ok := ctrlClient.(*ctrlClientImpl) + require.NotNil(t, got) + var _ CtrlClient = got + impl, ok := got.(*ctrlClientImpl) require.True(t, ok, "NewClient must return *ctrlClientImpl") - assert.True(t, impl.Client == cl, "client must be the exact same manager client instance") + assert.True(t, impl.Client == cl, "wrapped client must be the exact manager client instance") }