From 883ce07c70998e6aacd6b272dd56ac01abc1932c Mon Sep 17 00:00:00 2001 From: TheiLLeniumStudios <104288623+TheiLLeniumStudios@users.noreply.github.com> Date: Wed, 14 Jan 2026 12:35:48 +0100 Subject: [PATCH] test: remove redundant comments and clean up test code --- internal/pkg/controller/controller.go | 11 ++++------- internal/pkg/handler/create_test.go | 8 +------- internal/pkg/handler/delete_test.go | 8 ++------ internal/pkg/handler/handlers_test.go | 12 ++---------- internal/pkg/handler/update_test.go | 19 ++++--------------- internal/pkg/handler/upgrade_test.go | 26 ++++++++++---------------- 6 files changed, 23 insertions(+), 61 deletions(-) diff --git a/internal/pkg/controller/controller.go b/internal/pkg/controller/controller.go index bcc2d8c..9b7361c 100644 --- a/internal/pkg/controller/controller.go +++ b/internal/pkg/controller/controller.go @@ -42,16 +42,13 @@ type Controller struct { } // controllerInitialized flag determines whether controlled is being initialized -var secretControllerInitialized bool = false -var configmapControllerInitialized bool = false +var secretControllerInitialized = false +var configmapControllerInitialized = false var selectedNamespacesCache []string // NewController for initializing a Controller -func NewController( - client kubernetes.Interface, resource string, namespace string, ignoredNamespaces []string, namespaceLabelSelector string, resourceLabelSelector string, collectors metrics.Collectors) ( - *Controller, error, -) { - +func NewController(client kubernetes.Interface, resource string, namespace string, ignoredNamespaces []string, namespaceLabelSelector string, resourceLabelSelector string, collectors metrics.Collectors) (*Controller, + error) { if options.SyncAfterRestart { secretControllerInitialized = true configmapControllerInitialized = true diff --git a/internal/pkg/handler/create_test.go b/internal/pkg/handler/create_test.go index 8600cba..ef21f06 100644 --- a/internal/pkg/handler/create_test.go +++ b/internal/pkg/handler/create_test.go @@ -210,7 +210,6 @@ func TestResourceCreatedHandler_GetConfig(t *testing.T) { } func TestResourceCreatedHandler_GetConfig_Annotations(t *testing.T) { - // Test that annotations are properly captured in config cm := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "annotated-cm", @@ -236,7 +235,6 @@ func TestResourceCreatedHandler_GetConfig_Annotations(t *testing.T) { } func TestResourceCreatedHandler_GetConfig_Labels(t *testing.T) { - // Test that labels are properly captured in config secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "labeled-secret", @@ -270,7 +268,7 @@ func TestResourceCreatedHandler_Handle(t *testing.T) { { name: "Nil resource", resource: nil, - expectError: false, // logs error but returns nil + expectError: false, }, { name: "Valid ConfigMap - no workloads to update", @@ -315,7 +313,6 @@ func TestResourceCreatedHandler_Handle(t *testing.T) { } func TestResourceCreatedHandler_SHAConsistency(t *testing.T) { - // Test that same data produces same SHA data := map[string]string{"key": "value"} cm1 := &v1.ConfigMap{ @@ -333,12 +330,10 @@ func TestResourceCreatedHandler_SHAConsistency(t *testing.T) { config1, _ := handler1.GetConfig() config2, _ := handler2.GetConfig() - // Same data should produce same SHA assert.Equal(t, config1.SHAValue, config2.SHAValue) } func TestResourceCreatedHandler_SHADifference(t *testing.T) { - // Test that different data produces different SHA cm1 := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}, Data: map[string]string{"key": "value1"}, @@ -354,6 +349,5 @@ func TestResourceCreatedHandler_SHADifference(t *testing.T) { config1, _ := handler1.GetConfig() config2, _ := handler2.GetConfig() - // Different data should produce different SHA assert.NotEqual(t, config1.SHAValue, config2.SHAValue) } diff --git a/internal/pkg/handler/delete_test.go b/internal/pkg/handler/delete_test.go index 77fc448..812b0d1 100644 --- a/internal/pkg/handler/delete_test.go +++ b/internal/pkg/handler/delete_test.go @@ -129,7 +129,7 @@ func TestRemoveContainerEnvVars(t *testing.T) { }, }, }, - Env: []v1.EnvVar{}, // No env vars + Env: []v1.EnvVar{}, }, }, volumes: []v1.Volume{}, @@ -201,7 +201,6 @@ func TestRemoveContainerEnvVars(t *testing.T) { assert.Equal(t, tt.expected, result.Result) if tt.envVarRemoved { - // Verify env var was removed from container containers := deployment.Spec.Template.Spec.Containers for _, c := range containers { for _, env := range c.Env { @@ -215,7 +214,6 @@ func TestRemoveContainerEnvVars(t *testing.T) { } func TestInvokeDeleteStrategy(t *testing.T) { - // Save original strategy and restore after test originalStrategy := options.ReloadStrategy defer func() { options.ReloadStrategy = originalStrategy @@ -297,7 +295,6 @@ func TestInvokeDeleteStrategy(t *testing.T) { result := invokeDeleteStrategy(funcs, deployment, tt.config, true) - // Should return a valid result assert.NotNil(t, result) }) } @@ -345,12 +342,11 @@ func TestRemovePodAnnotations(t *testing.T) { VolumesFunc: mockVolumesFunc, PodAnnotationsFunc: mockPodAnnotationsFunc, PatchTemplatesFunc: mockPatchTemplatesFunc, - SupportsPatch: false, // No patch for annotations removal test + SupportsPatch: false, } result := removePodAnnotations(funcs, deployment, tt.config, true) - // Should return Updated since it sets the SHA to empty data hash assert.Equal(t, constants.Updated, result.Result) }) } diff --git a/internal/pkg/handler/handlers_test.go b/internal/pkg/handler/handlers_test.go index 4b56358..dedefcc 100644 --- a/internal/pkg/handler/handlers_test.go +++ b/internal/pkg/handler/handlers_test.go @@ -55,7 +55,7 @@ func TestResourceCreatedHandler_GetConfig_ConfigMap(t *testing.T) { assert.Equal(t, "default", config.Namespace) assert.Equal(t, constants.ConfigmapEnvVarPostfix, config.Type) assert.NotEmpty(t, config.SHAValue) - assert.Empty(t, oldSHA) // oldSHA is always empty for create handler + assert.Empty(t, oldSHA) } func TestResourceCreatedHandler_GetConfig_Secret(t *testing.T) { @@ -75,7 +75,6 @@ func TestResourceCreatedHandler_GetConfig_Secret(t *testing.T) { } func TestResourceCreatedHandler_GetConfig_InvalidResource(t *testing.T) { - // Test with an invalid resource type handler := ResourceCreatedHandler{ Resource: "invalid", Collectors: createTestCollectors(), @@ -83,7 +82,6 @@ func TestResourceCreatedHandler_GetConfig_InvalidResource(t *testing.T) { config, _ := handler.GetConfig() - // Config should be empty/zero for invalid resources assert.Empty(t, config.ResourceName) } @@ -95,7 +93,6 @@ func TestResourceCreatedHandler_Handle_NilResource(t *testing.T) { err := handler.Handle() - // Should not return error even with nil resource (just logs error) assert.NoError(t, err) } @@ -178,7 +175,6 @@ func TestResourceUpdatedHandler_GetConfig_ConfigMap(t *testing.T) { assert.Equal(t, constants.ConfigmapEnvVarPostfix, config.Type) assert.NotEmpty(t, config.SHAValue) assert.NotEmpty(t, oldSHA) - // SHAs should be different since data changed assert.NotEqual(t, config.SHAValue, oldSHA) } @@ -195,7 +191,6 @@ func TestResourceUpdatedHandler_GetConfig_ConfigMap_SameData(t *testing.T) { config, oldSHA := handler.GetConfig() assert.Equal(t, "test-cm", config.ResourceName) - // SHAs should be the same since data didn't change assert.Equal(t, config.SHAValue, oldSHA) } @@ -232,7 +227,6 @@ func TestResourceUpdatedHandler_GetConfig_Secret_SameData(t *testing.T) { config, oldSHA := handler.GetConfig() assert.Equal(t, "test-secret", config.ResourceName) - // SHAs should be the same since data didn't change assert.Equal(t, config.SHAValue, oldSHA) } @@ -270,16 +264,14 @@ func TestResourceUpdatedHandler_Handle_NilOldResource(t *testing.T) { err := handler.Handle() - // Should not return error (just logs error) assert.NoError(t, err) } func TestResourceUpdatedHandler_Handle_NoChange(t *testing.T) { - // When SHA values are the same, Handle should return nil without doing anything cm := createTestConfigMap(map[string]string{"key": "same-value"}) handler := ResourceUpdatedHandler{ Resource: cm, - OldResource: cm, // Same resource = same SHA + OldResource: cm, Collectors: createTestCollectors(), } diff --git a/internal/pkg/handler/update_test.go b/internal/pkg/handler/update_test.go index a10a6bf..1ae10d4 100644 --- a/internal/pkg/handler/update_test.go +++ b/internal/pkg/handler/update_test.go @@ -108,7 +108,7 @@ func TestResourceUpdatedHandler_GetConfig(t *testing.T) { expectedNS: "default", expectedType: constants.ConfigmapEnvVarPostfix, expectSHANotEmpty: true, - expectSHAChanged: false, // Only data affects SHA, not labels + expectSHAChanged: false, }, { name: "ConfigMap only annotations changed - SHA unchanged", @@ -132,7 +132,7 @@ func TestResourceUpdatedHandler_GetConfig(t *testing.T) { expectedNS: "default", expectedType: constants.ConfigmapEnvVarPostfix, expectSHANotEmpty: true, - expectSHAChanged: false, // Only data affects SHA, not annotations + expectSHAChanged: false, }, { name: "Secret data changed", @@ -257,7 +257,7 @@ func TestResourceUpdatedHandler_Handle(t *testing.T) { name: "Both resources nil", oldResource: nil, newResource: nil, - expectError: false, // logs error but returns nil + expectError: false, }, { name: "Old resource nil", @@ -299,7 +299,7 @@ func TestResourceUpdatedHandler_Handle(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}, Data: map[string]string{"key": "new"}, }, - expectError: false, // No error, but no workloads to update in test + expectError: false, }, { name: "Secret unchanged - no action", @@ -347,7 +347,6 @@ func TestResourceUpdatedHandler_Handle(t *testing.T) { } func TestResourceUpdatedHandler_GetConfig_Annotations(t *testing.T) { - // Test that annotations from the new resource are captured oldCM := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "cm", @@ -378,15 +377,12 @@ func TestResourceUpdatedHandler_GetConfig_Annotations(t *testing.T) { config, _ := handler.GetConfig() - // Should have new annotations assert.Equal(t, "new-value", config.ResourceAnnotations["new-annotation"]) - // Should not have old annotations _, hasOld := config.ResourceAnnotations["old-annotation"] assert.False(t, hasOld) } func TestResourceUpdatedHandler_GetConfig_Labels(t *testing.T) { - // Test that labels from the new resource are captured oldSecret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "secret", @@ -413,12 +409,10 @@ func TestResourceUpdatedHandler_GetConfig_Labels(t *testing.T) { config, _ := handler.GetConfig() - // Should have new labels assert.Equal(t, "v2", config.Labels["version"]) } func TestResourceUpdatedHandler_EmptyToNonEmpty(t *testing.T) { - // Test transition from empty data to non-empty data oldCM := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}, Data: map[string]string{}, @@ -440,7 +434,6 @@ func TestResourceUpdatedHandler_EmptyToNonEmpty(t *testing.T) { } func TestResourceUpdatedHandler_NonEmptyToEmpty(t *testing.T) { - // Test transition from non-empty data to empty data oldCM := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}, Data: map[string]string{"key": "value"}, @@ -462,7 +455,6 @@ func TestResourceUpdatedHandler_NonEmptyToEmpty(t *testing.T) { } func TestResourceUpdatedHandler_BinaryDataChange(t *testing.T) { - // Test ConfigMap binary data change oldCM := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}, BinaryData: map[string][]byte{"binary": []byte("old-binary")}, @@ -484,7 +476,6 @@ func TestResourceUpdatedHandler_BinaryDataChange(t *testing.T) { } func TestResourceUpdatedHandler_MixedDataAndBinaryData(t *testing.T) { - // Test ConfigMap with both Data and BinaryData oldCM := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}, Data: map[string]string{"text": "value"}, @@ -508,7 +499,6 @@ func TestResourceUpdatedHandler_MixedDataAndBinaryData(t *testing.T) { } func TestResourceUpdatedHandler_DifferentNamespaces(t *testing.T) { - // Edge case: what if namespaces are different (shouldn't happen in practice) oldCM := &v1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "ns1"}, Data: map[string]string{"key": "value"}, @@ -526,6 +516,5 @@ func TestResourceUpdatedHandler_DifferentNamespaces(t *testing.T) { config, _ := handler.GetConfig() - // Should use new resource's namespace assert.Equal(t, "ns2", config.Namespace) } diff --git a/internal/pkg/handler/upgrade_test.go b/internal/pkg/handler/upgrade_test.go index a518c38..8270132 100644 --- a/internal/pkg/handler/upgrade_test.go +++ b/internal/pkg/handler/upgrade_test.go @@ -203,7 +203,7 @@ func TestGetVolumeMountName(t *testing.T) { }, }, }, - mountType: constants.ConfigmapEnvVarPostfix, // Looking for configmap but volume is secret + mountType: constants.ConfigmapEnvVarPostfix, volumeName: "my-secret", expected: "", }, @@ -509,7 +509,7 @@ func TestUpdateEnvVar(t *testing.T) { envVar string shaData string expected constants.Result - newValue string // expected value after update + newValue string }{ { name: "Update existing env var with different value", @@ -567,7 +567,6 @@ func TestUpdateEnvVar(t *testing.T) { assert.Equal(t, tt.expected, result) if tt.expected == constants.Updated || tt.expected == constants.NotUpdated { - // Verify the value in the container for _, env := range tt.container.Env { if env.Name == tt.envVar { assert.Equal(t, tt.newValue, env.Value) @@ -658,7 +657,6 @@ func TestCreateReloadedAnnotations(t *testing.T) { }, } - // Use a simple func that doesn't require patch templates funcs := callbacks.RollingUpgradeFuncs{ SupportsPatch: false, } @@ -672,7 +670,6 @@ func TestCreateReloadedAnnotations(t *testing.T) { } else { assert.NoError(t, err) assert.NotNil(t, annotations) - // Verify annotation key exists _, exists := annotations[getReloaderAnnotationKey()] assert.True(t, exists) } @@ -782,7 +779,7 @@ func TestGetContainerUsingResource(t *testing.T) { }, autoReload: false, expectNil: false, - expectedName: "main-app", // Returns first container when init container has the mount + expectedName: "main-app", }, { name: "EnvFrom ConfigMap in regular container", @@ -846,7 +843,7 @@ func TestGetContainerUsingResource(t *testing.T) { ResourceName: "external-configmap", Type: constants.ConfigmapEnvVarPostfix, }, - autoReload: false, // Explicit annotation should use first container fallback + autoReload: false, expectNil: false, expectedName: "first-container", }, @@ -861,7 +858,7 @@ func TestGetContainerUsingResource(t *testing.T) { ResourceName: "unmounted-configmap", Type: constants.ConfigmapEnvVarPostfix, }, - autoReload: true, // Auto mode should NOT use first container fallback + autoReload: true, expectNil: true, }, { @@ -902,7 +899,7 @@ func TestGetContainerUsingResource(t *testing.T) { Type: constants.ConfigmapEnvVarPostfix, }, autoReload: false, - expectNil: true, // No regular containers to return + expectNil: true, }, { name: "CSI SecretProviderClass volume", @@ -1038,7 +1035,9 @@ func TestRetryOnConflict(t *testing.T) { matched bool err error }{ - {matched: false, err: apierrors.NewConflict(schema.GroupResource{Group: "", Resource: "deployments"}, "test", errors.New("conflict"))}, + {matched: false, + err: apierrors.NewConflict(schema.GroupResource{Group: "", Resource: "deployments"}, "test", + errors.New("conflict"))}, {matched: true, err: nil}, }, expectMatched: true, @@ -1086,7 +1085,6 @@ func TestRetryOnConflict(t *testing.T) { callCount := 0 fn := func(fetchResource bool) (bool, error) { if callCount >= len(tt.fnResults) { - // Should not happen in tests, but return success to prevent infinite loop return true, nil } result := tt.fnResults[callCount] @@ -1107,7 +1105,6 @@ func TestRetryOnConflict(t *testing.T) { } func TestGetVolumeMountNameCSI(t *testing.T) { - // Test CSI SecretProviderClass volume specifically tests := []struct { name string volumes []v1.Volume @@ -1303,11 +1300,9 @@ func TestSecretProviderClassAnnotationReloaded(t *testing.T) { } func TestInvokeReloadStrategy(t *testing.T) { - // Save original value and restore after test originalStrategy := options.ReloadStrategy defer func() { options.ReloadStrategy = originalStrategy }() - // Create a minimal deployment for testing deployment := createTestDeployment( []v1.Container{ { @@ -1365,14 +1360,13 @@ func TestInvokeReloadStrategy(t *testing.T) { name: "Env vars strategy with container found", reloadStrategy: constants.EnvVarsReloadStrategy, autoReload: false, - expectResult: constants.Updated, // Creates env var when not found + expectResult: constants.Updated, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { options.ReloadStrategy = tt.reloadStrategy - // Reset annotations for each test deployment.Spec.Template.Annotations = map[string]string{} result := invokeReloadStrategy(funcs, deployment, config, tt.autoReload)