From e9ce625fd503c5f2b78c3d7e8328feec442e4644 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Wed, 29 Jul 2026 17:40:18 -0700 Subject: [PATCH] Verify WantDeleteCollections in TableTest Fixes #2455 Motivation: `TableRow.WantDeleteCollections` has existed since it was added to the struct, but `(r *TableRow) Test()` never compared it against the actual recorded DeleteCollection actions. A test author setting `WantDeleteCollections` gets no verification at all: whether the reconciler under test issues the expected DeleteCollection call, issues the wrong one, or issues none, the test silently passes either way. A maintainer comment on the issue speculated this couldn't be fixed because of an upstream fake-client limitation (kubernetes/kubernetes#105357), where DeleteCollection calls with a label selector don't actually remove matching objects from the fake ObjectTracker. That bug is real, but it's orthogonal to this issue: `reconciler/testing/actions.go`'s `ActionsByVerb()` already captures `delete-collection` verb actions into `Actions.DeleteCollections` from the fake clientset's recorded action list, independent of whether the tracker actually performed the deletion. So the missing verification in `Test()` is a separate, fixable gap in the test framework itself, not blocked by the upstream issue. Approach: Add a verification block in `Test()`, alongside the existing `WantDeletes` check, that compares `r.WantDeleteCollections` against `actions.DeleteCollections`. `DeleteCollectionActionImpl` has no `GetName()` (it targets a namespace + label/field selector, not a single named object), so it can't reuse the `WantDeletes` comparison verbatim. Instead, a new `deleteCollectionKey` helper builds a comparison key from resource + label selector string + field selector string (+ namespace, unless `SkipNamespaceValidation` is set), and the two sets of keys are compared the same way `WantDeletes`/`gotDeletes` are. Nil label/field selectors (as would result from a hand-written `DeleteCollectionActionImpl{}` literal in a test's `WantDeleteCollections`) are treated as `labels.Everything()` / `fields.Everything()`, matching what client-go's own `NewDeleteCollectionActionWithOptions` defaults them to internally, so constructing a "delete everything" expectation doesn't panic on a nil selector's `.String()`. This does not change behavior for any existing test: a repo-wide search found no existing use of `WantDeleteCollections` anywhere in knative/pkg, since it was never actually verified before. It only turns a previously-silent no-op field into a working one, so tests written against it going forward actually enforce their expectations. Validation: go build ./... go test ./reconciler/... -count=1 go test ./webhook/psbinding/... -count=1 both pass. Added `reconciler/testing/table_test.go` with `TestDeleteCollectionKey`, unit-testing the new helper directly: constructor-built vs. hand-written-literal actions key identically, nil-selector defaults match an explicit "everything" selector, and namespace affects the key unless `SkipNamespaceValidation` is set. /kind bug Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> --- reconciler/testing/table.go | 42 +++++++++++++++++++ reconciler/testing/table_test.go | 70 ++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 reconciler/testing/table_test.go diff --git a/reconciler/testing/table.go b/reconciler/testing/table.go index f749746633..85cc047f52 100644 --- a/reconciler/testing/table.go +++ b/reconciler/testing/table.go @@ -29,6 +29,8 @@ import ( "go.uber.org/zap" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" clientgotesting "k8s.io/client-go/testing" @@ -133,6 +135,25 @@ func objKey(o runtime.Object) string { return path.Join(typeOf, on.GetNamespace(), on.GetName()) } +// deleteCollectionKey builds a unique string representing a DeleteCollection +// call, so that got and want calls can be compared as sets. +func deleteCollectionKey(a clientgotesting.DeleteCollectionAction, skipNamespaceValidation bool) string { + lr := a.GetListRestrictions() + labelSelector, fieldSelector := lr.Labels, lr.Fields + if labelSelector == nil { + labelSelector = labels.Everything() + } + if fieldSelector == nil { + fieldSelector = fields.Everything() + } + + n := a.GetResource().Resource + "~~" + labelSelector.String() + "~~" + fieldSelector.String() + if !skipNamespaceValidation { + n += "~~" + a.GetNamespace() + } + return n +} + // Factory returns a Reconciler.Interface to perform reconciliation in table test, and // ActionRecorderList/EventList to capture k8s actions/events produced during reconciliation. type Factory func(*testing.T, *TableRow) (controller.Reconciler, ActionRecorderList, EventList) @@ -316,6 +337,27 @@ func (r *TableRow) Test(t *testing.T, factory Factory) { } } + // Build a set of unique strings that represent type{-namespace}-labelSelector-fieldSelector + // to catch missing or unexpected DeleteCollection calls. + gotDeleteCollections := make(sets.Set[string], len(actions.DeleteCollections)) + for _, w := range actions.DeleteCollections { + n := deleteCollectionKey(w, r.SkipNamespaceValidation) + gotDeleteCollections.Insert(n) + } + wantDeleteCollections := make(sets.Set[string], len(r.WantDeleteCollections)) + for _, w := range r.WantDeleteCollections { + n := deleteCollectionKey(w, r.SkipNamespaceValidation) + wantDeleteCollections.Insert(n) + } + if !gotDeleteCollections.Equal(wantDeleteCollections) { + if extra := gotDeleteCollections.Difference(wantDeleteCollections); len(extra) > 0 { + t.Error("Extra or unexpected delete-collections:", extra.UnsortedList()) + } + if missing := wantDeleteCollections.Difference(gotDeleteCollections); len(missing) > 0 { + t.Error("Missing delete-collections:", missing.UnsortedList()) + } + } + for i, want := range r.WantPatches { if i >= len(actions.Patches) { t.Errorf("Missing patch: %#v; raw: %s", want, string(want.GetPatch())) diff --git a/reconciler/testing/table_test.go b/reconciler/testing/table_test.go new file mode 100644 index 0000000000..5e93fe16d9 --- /dev/null +++ b/reconciler/testing/table_test.go @@ -0,0 +1,70 @@ +/* +Copyright 2026 The Knative Authors. + +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 testing + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" + clientgotesting "k8s.io/client-go/testing" +) + +func TestDeleteCollectionKey(t *testing.T) { + gvr := schema.GroupVersionResource{Resource: "pods"} + + fromListOptions := clientgotesting.NewDeleteCollectionAction(gvr, "foo", metav1.ListOptions{LabelSelector: "app=foo"}) + literalWithSelector := clientgotesting.DeleteCollectionActionImpl{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Resource: gvr, + }, + ListRestrictions: clientgotesting.ListRestrictions{ + Labels: labels.SelectorFromSet(labels.Set{"app": "foo"}), + }, + } + literalWithoutSelector := clientgotesting.DeleteCollectionActionImpl{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Resource: gvr, + }, + } + otherNamespace := clientgotesting.NewDeleteCollectionAction(gvr, "bar", metav1.ListOptions{LabelSelector: "app=foo"}) + + // An action built via the constructor (as the fake clientset produces) and an + // equivalent hand-built literal (as a test author would write in WantDeleteCollections) + // must key identically, or a correct WantDeleteCollections would never match. + if got, want := deleteCollectionKey(fromListOptions, false), deleteCollectionKey(literalWithSelector, false); got != want { + t.Errorf("deleteCollectionKey() = %q, want %q", got, want) + } + + // A DeleteCollection call with no selector at all (nil Labels/Fields) must not panic, + // and must key the same as an explicit "everything" selector. + everything := clientgotesting.NewDeleteCollectionAction(gvr, "foo", metav1.ListOptions{}) + if got, want := deleteCollectionKey(literalWithoutSelector, false), deleteCollectionKey(everything, false); got != want { + t.Errorf("deleteCollectionKey() = %q, want %q", got, want) + } + + // Differing namespaces must produce different keys unless namespace validation is skipped. + if got := deleteCollectionKey(fromListOptions, false); got == deleteCollectionKey(otherNamespace, false) { + t.Errorf("deleteCollectionKey() = %q, want distinct keys for different namespaces", got) + } + if got, want := deleteCollectionKey(fromListOptions, true), deleteCollectionKey(otherNamespace, true); got != want { + t.Errorf("deleteCollectionKey() = %q, want %q when SkipNamespaceValidation is set", got, want) + } +}