From 836689f6ccc2e1c1511f123d9e453ce69e5d9481 Mon Sep 17 00:00:00 2001 From: Krishna Awasthi <140143710+opbot-xd@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:24:48 +0530 Subject: [PATCH] test: Unify duplicate test harness structs in restore and delete tests (#10362) * test: Unify duplicate test harness structs in restore and delete tests Extract the duplicated test harness (fakeRestorer, Harness struct, and AddResource/AddItems helpers) from restore_test.go and delete_item_action_handler_test.go into a shared pkg/test.Harness. Backup tests are intentionally excluded: the shared AddResource strips metadata.creationTimestamp and status (restore semantics), which would change what the backup tests tar up and assert on. Signed-off-by: opbot_xd * ci: retry Signed-off-by: opbot_xd * ci: retry Signed-off-by: opbot_xd --------- Signed-off-by: opbot_xd --- changelogs/unreleased/10362-opbot-xd | 1 + .../delete/delete_item_action_handler_test.go | 52 +++---------- pkg/restore/restore_test.go | 50 ++++--------- pkg/test/harness.go | 73 +++++++++++++++++++ 4 files changed, 98 insertions(+), 78 deletions(-) create mode 100644 changelogs/unreleased/10362-opbot-xd create mode 100644 pkg/test/harness.go diff --git a/changelogs/unreleased/10362-opbot-xd b/changelogs/unreleased/10362-opbot-xd new file mode 100644 index 000000000..551b7985b --- /dev/null +++ b/changelogs/unreleased/10362-opbot-xd @@ -0,0 +1 @@ +Unify duplicate test harness structs across restore and delete tests into a shared pkg/test.Harness diff --git a/internal/delete/delete_item_action_handler_test.go b/internal/delete/delete_item_action_handler_test.go index b7d4e6e6a..1ad978bca 100644 --- a/internal/delete/delete_item_action_handler_test.go +++ b/internal/delete/delete_item_action_handler_test.go @@ -26,9 +26,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" @@ -154,9 +151,9 @@ func TestInvokeDeleteItemActionsRunForCorrectItems(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // test harness contains the fake API server/discovery client - h := newHarness(t) + h, dh := newHarness(t) for _, r := range tc.apiResources { - h.addResource(t, r) + h.AddResource(t, dh, r) } // Get the plugins out of the map in order to use them. @@ -169,7 +166,7 @@ func TestInvokeDeleteItemActionsRunForCorrectItems(t *testing.T) { Backup: tc.backup, BackupReader: tc.tarball, Filesystem: fs, - DiscoveryHelper: h.discoveryHelper, + DiscoveryHelper: dh, Actions: actions, Log: log, } @@ -187,46 +184,15 @@ func TestInvokeDeleteItemActionsRunForCorrectItems(t *testing.T) { } } -// TODO: unify this with the test harness in pkg/restore/restore_test.go -type harness struct { - *test.APIServer - discoveryHelper discovery.Helper -} - -func newHarness(t *testing.T) *harness { +func newHarness(t *testing.T) (*test.Harness, discovery.Helper) { t.Helper() apiServer := test.NewAPIServer(t) log := logrus.StandardLogger() - - discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, log) + dh, err := discovery.NewHelper(apiServer.DiscoveryClient, log) require.NoError(t, err) - return &harness{ - APIServer: apiServer, - discoveryHelper: discoveryHelper, - } -} - -// addResource adds an APIResource and it's items to a faked API server for testing. -func (h *harness) addResource(t *testing.T, resource *test.APIResource) { - t.Helper() - - h.DiscoveryClient.WithAPIResource(resource) - require.NoError(t, h.discoveryHelper.Refresh()) - - for _, item := range resource.Items { - obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(item) - require.NoError(t, err) - - unstructuredObj := &unstructured.Unstructured{Object: obj} - if resource.Namespaced { - _, err = h.DynamicClient.Resource(resource.GVR()).Namespace(item.GetNamespace()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{}) - } else { - _, err = h.DynamicClient.Resource(resource.GVR()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{}) - } - require.NoError(t, err) - } + return test.NewHarness(t, apiServer), dh } // recordResourcesAction is a delete item action that can be configured to run @@ -306,14 +272,14 @@ func TestInvokeDeleteActionsReturnsPluginErrors(t *testing.T) { action := &failingAction{err: errors.New("could not delete artifact")} - h := newHarness(t) - h.addResource(t, test.Pods()) + h, dh := newHarness(t) + h.AddResource(t, dh, test.Pods()) c := &Context{ Backup: builder.ForBackup("velero", "velero").Result(), BackupReader: tarball, Filesystem: fs, - DiscoveryHelper: h.discoveryHelper, + DiscoveryHelper: dh, Actions: []velero.DeleteItemAction{action}, Log: log, } diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 5c75fff42..074053444 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -235,7 +235,7 @@ func TestRestorePVWithVolumeInfo(t *testing.T) { for _, r := range tc.apiResources { h.DiscoveryClient.WithAPIResource(r) } - require.NoError(t, h.restorer.discoveryHelper.Refresh()) + require.NoError(t, h.discoveryHelper.Refresh()) data := &Request{ Log: h.log, @@ -788,7 +788,7 @@ func TestRestoreResourceFiltering(t *testing.T) { for _, r := range tc.apiResources { h.DiscoveryClient.WithAPIResource(r) } - require.NoError(t, h.restorer.discoveryHelper.Refresh()) + require.NoError(t, h.discoveryHelper.Refresh()) // We need to fetch the policies using the actual function resPolicies, err := resourcepolicies.GetResourcePoliciesFromRestore(t.Context(), tc.restore, h.restorer.kbClient, h.log) @@ -868,7 +868,7 @@ func TestRestoreMustHaveResourceNamespaceEnforcement(t *testing.T) { for _, r := range tc.apiResources { h.DiscoveryClient.WithAPIResource(r) } - require.NoError(t, h.restorer.discoveryHelper.Refresh()) + require.NoError(t, h.discoveryHelper.Refresh()) resPolicies, err := resourcepolicies.GetResourcePoliciesFromRestore(t.Context(), tc.restore, h.restorer.kbClient, h.log) require.NoError(t, err) @@ -954,7 +954,7 @@ func TestRestoreNamespaceMapping(t *testing.T) { for _, r := range tc.apiResources { h.DiscoveryClient.WithAPIResource(r) } - require.NoError(t, h.restorer.discoveryHelper.Refresh()) + require.NoError(t, h.discoveryHelper.Refresh()) data := &Request{ Log: h.log, @@ -1038,7 +1038,7 @@ func TestRestoreResourcePriorities(t *testing.T) { for _, r := range tc.apiResources { h.DiscoveryClient.WithAPIResource(r) } - require.NoError(t, h.restorer.discoveryHelper.Refresh()) + require.NoError(t, h.discoveryHelper.Refresh()) data := &Request{ Log: h.log, @@ -1115,7 +1115,7 @@ func TestInvalidTarballContents(t *testing.T) { for _, r := range tc.apiResources { h.DiscoveryClient.WithAPIResource(r) } - require.NoError(t, h.restorer.discoveryHelper.Refresh()) + require.NoError(t, h.discoveryHelper.Refresh()) data := &Request{ Log: h.log, @@ -4548,10 +4548,10 @@ func assertNonEmptyResults(t *testing.T, typeMsg string, res ...Result) { } type harness struct { - *test.APIServer - - restorer *kubernetesRestorer - log logrus.FieldLogger + *test.Harness + discoveryHelper discovery.Helper + restorer *kubernetesRestorer + log logrus.FieldLogger } func newHarness(t *testing.T) *harness { @@ -4561,13 +4561,14 @@ func newHarness(t *testing.T) *harness { log := logrus.StandardLogger() kbClient := test.NewFakeControllerRuntimeClient(t) - discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, log) + dh, err := discovery.NewHelper(apiServer.DiscoveryClient, log) require.NoError(t, err) return &harness{ - APIServer: apiServer, + Harness: test.NewHarness(t, apiServer), + discoveryHelper: dh, restorer: &kubernetesRestorer{ - discoveryHelper: discoveryHelper, + discoveryHelper: dh, dynamicFactory: client.NewDynamicFactory(apiServer.DynamicClient), namespaceClient: apiServer.KubeClient.CoreV1().Namespaces(), resourceTerminatingTimeout: time.Minute, @@ -4586,28 +4587,7 @@ func newHarness(t *testing.T) *harness { func (h *harness) AddItems(t *testing.T, resource *test.APIResource) { t.Helper() - - h.DiscoveryClient.WithAPIResource(resource) - require.NoError(t, h.restorer.discoveryHelper.Refresh()) - - for _, item := range resource.Items { - obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(item) - require.NoError(t, err) - - unstructuredObj := &unstructured.Unstructured{Object: obj} - - // These fields have non-nil zero values in the unstructured objects. We remove - // them to make comparison easier in our tests. - unstructured.RemoveNestedField(unstructuredObj.Object, "metadata", "creationTimestamp") - unstructured.RemoveNestedField(unstructuredObj.Object, "status") - - if resource.Namespaced { - _, err = h.DynamicClient.Resource(resource.GVR()).Namespace(item.GetNamespace()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{}) - } else { - _, err = h.DynamicClient.Resource(resource.GVR()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{}) - } - require.NoError(t, err) - } + h.Harness.AddResource(t, h.discoveryHelper, resource) } func Test_resetVolumeBindingInfo(t *testing.T) { diff --git a/pkg/test/harness.go b/pkg/test/harness.go new file mode 100644 index 000000000..6fe1cf010 --- /dev/null +++ b/pkg/test/harness.go @@ -0,0 +1,73 @@ +/* +Copyright the Velero contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package test + +import ( + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +type refreshable interface { + Refresh() error +} + +// Harness wraps an APIServer and provides AddResource for seeding +// the fake API server with resources and items. +// It is shared across delete, restore, and backup test packages. +type Harness struct { + *APIServer +} + +// NewHarness creates a Harness from an existing APIServer. +func NewHarness(t *testing.T, apiServer *APIServer) *Harness { + t.Helper() + require.NotNil(t, apiServer, "apiServer must not be nil") + return &Harness{APIServer: apiServer} +} + +// AddResource registers an API resource with the discovery client, +// refreshes the discovery helper, and creates all of the resource's +// items in the fake dynamic client. +func (h *Harness) AddResource(t *testing.T, dh refreshable, resource *APIResource) { + t.Helper() + + h.DiscoveryClient.WithAPIResource(resource) + require.NoError(t, dh.Refresh()) + + for _, item := range resource.Items { + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(item) + require.NoError(t, err) + + unstructuredObj := &unstructured.Unstructured{Object: obj} + + // These fields have non-nil zero values in the unstructured objects. We remove + // them to make comparison easier in our tests. + unstructured.RemoveNestedField(unstructuredObj.Object, "metadata", "creationTimestamp") + unstructured.RemoveNestedField(unstructuredObj.Object, "status") + + if resource.Namespaced { + _, err = h.DynamicClient.Resource(resource.GVR()).Namespace(item.GetNamespace()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{}) + } else { + _, err = h.DynamicClient.Resource(resource.GVR()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{}) + } + require.NoError(t, err) + } +}