storage: Get rid of TestListDeprecated

This commit extends the test cases of RunTestList
to include the things tested by TestListDeprecated
and subsequently deletes the test.

This additionally adds a test case for checking that
the list return the modified version of an object.

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go

@@ -502,7 +502,7 @@ func RunTestList(ctx context.Context, t *testing.T, store storage.Interface, ign
 
 	getAttrs := func(obj runtime.Object) (labels.Set, fields.Set, error) {
 		pod := obj.(*example.Pod)
-		return nil, fields.Set{"metadata.name": pod.Name}, nil
+		return nil, fields.Set{"metadata.name": pod.Name, "spec.nodeName": pod.Spec.NodeName}, nil
 	}
 
 	tests := []struct {
@@ -993,6 +993,30 @@ func RunTestList(ctx context.Context, t *testing.T, store storage.Interface, ign
 			rvMatch:     metav1.ResourceVersionMatchNotOlderThan,
 			expectedOut: []example.Pod{*preset[0], *preset[1], *preset[2], *preset[3], *preset[4]},
 		},
+		{
+			name:   "verify list returns updated version of object; filter returns one item, page size equal to total list size with current resource version and match=NotOlderThan",
+			prefix: "/pods",
+			pred: storage.SelectionPredicate{
+				Field: fields.OneTermEqualSelector("spec.nodeName", "fakeNode"),
+				Label: labels.Everything(),
+				Limit: 5,
+			},
+			rv:          list.ResourceVersion,
+			rvMatch:     metav1.ResourceVersionMatchNotOlderThan,
+			expectedOut: []example.Pod{*preset[0]},
+		},
+		{
+			name:   "verify list does not return deleted object; filter for deleted object, page size equal to total list size with current resource version and match=NotOlderThan",
+			prefix: "/pods",
+			pred: storage.SelectionPredicate{
+				Field: fields.OneTermEqualSelector("metadata.name", "baz"),
+				Label: labels.Everything(),
+				Limit: 5,
+			},
+			rv:          list.ResourceVersion,
+			rvMatch:     metav1.ResourceVersionMatchNotOlderThan,
+			expectedOut: []example.Pod{},
+		},
 	}
 
 	for _, tt := range tests {
@@ -1159,6 +1183,7 @@ func seedMultiLevelData(ctx context.Context, store storage.Interface) (string, [
 	//   - second/
 	//  |         - bar
 	//  |         - foo
+	//  |         - [deleted] baz
 	//  |
 	//   - third/
 	//  |         - barfoo
@@ -1166,6 +1191,7 @@ func seedMultiLevelData(ctx context.Context, store storage.Interface) (string, [
 	barFirst := &example.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "first", Name: "bar"}}
 	barSecond := &example.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "second", Name: "bar"}}
 	fooSecond := &example.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "second", Name: "foo"}}
+	bazSecond := &example.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "second", Name: "baz"}}
 	barfooThird := &example.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "third", Name: "barfoo"}}
 	fooThird := &example.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "third", Name: "foo"}}
 
@@ -1194,6 +1220,10 @@ func seedMultiLevelData(ctx context.Context, store storage.Interface) (string, [
 			key: computePodKey(fooThird),
 			obj: fooThird,
 		},
+		{
+			key: computePodKey(bazSecond),
+			obj: bazSecond,
+		},
 	}
 
 	// we want to figure out the resourceVersion before we create anything
@@ -1211,6 +1241,29 @@ func seedMultiLevelData(ctx context.Context, store storage.Interface) (string, [
 		}
 	}
 
+	// For barFirst, we first create it with key /pods/first/bar and then we update
+	// it by changing its spec.nodeName. The point of doing this is to be able to
+	// test that if a pod with key /pods/first/bar is in fact returned, the returned
+	// pod is the updated one (i.e. with spec.nodeName changed).
+	preset[0].storedObj = &example.Pod{}
+	if err := store.GuaranteedUpdate(ctx, computePodKey(barFirst), preset[0].storedObj, true, nil,
+		func(input runtime.Object, _ storage.ResponseMeta) (output runtime.Object, ttl *uint64, err error) {
+			pod := input.(*example.Pod).DeepCopy()
+			pod.Spec.NodeName = "fakeNode"
+			return pod, nil, nil
+		}, nil); err != nil {
+		return "", nil, fmt.Errorf("failed to update object: %w", err)
+	}
+
+	// We now delete bazSecond provided it has been created first. We do this to enable
+	// testing cases that had an object exist initially and then was deleted and how this
+	// would be reflected in responses of different calls.
+	if err := store.Delete(ctx, computePodKey(bazSecond), preset[len(preset)-1].storedObj, nil, storage.ValidateAllObjectFunc, nil); err != nil {
+		return "", nil, fmt.Errorf("failed to delete object: %w", err)
+	}
+
+	// Since we deleted bazSecond (last element of preset), we remove it from preset.
+	preset = preset[:len(preset)-1]
 	var created []*example.Pod
 	for _, item := range preset {
 		created = append(created, item.storedObj)

staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go

@@ -19,7 +19,6 @@ package tests
 import (
 	"context"
 	"fmt"
-	"reflect"
 	goruntime "runtime"
 	"strconv"
 	"sync"
@@ -37,7 +36,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/apiserver/pkg/apis/example"
@@ -181,101 +179,6 @@ func TestList(t *testing.T) {
 	storagetesting.RunTestList(ctx, t, cacher, true)
 }
 
-// TODO(wojtek-t): We should extend the generic RunTestList test to cover the
-// scenarios that are not yet covered by it and get rid of this test.
-func TestListDeprecated(t *testing.T) {
-	server, etcdStorage := newEtcdTestStorage(t, etcd3testing.PathPrefix())
-	defer server.Terminate(t)
-	cacher, _, err := newTestCacher(etcdStorage)
-	if err != nil {
-		t.Fatalf("Couldn't create cacher: %v", err)
-	}
-	defer cacher.Stop()
-
-	podFoo := makeTestPod("foo")
-	podBar := makeTestPod("bar")
-	podBaz := makeTestPod("baz")
-
-	podFooPrime := makeTestPod("foo")
-	podFooPrime.Spec.NodeName = "fakeNode"
-
-	fooCreated := updatePod(t, etcdStorage, podFoo, nil)
-	_ = updatePod(t, etcdStorage, podBar, nil)
-	_ = updatePod(t, etcdStorage, podBaz, nil)
-
-	_ = updatePod(t, etcdStorage, podFooPrime, fooCreated)
-
-	// Create a pod in a namespace that contains "ns" as a prefix
-	// Make sure it is not returned in a watch of "ns"
-	podFooNS2 := makeTestPod("foo")
-	podFooNS2.Namespace += "2"
-	updatePod(t, etcdStorage, podFooNS2, nil)
-
-	deleted := example.Pod{}
-	if err := etcdStorage.Delete(context.TODO(), "pods/ns/bar", &deleted, nil, storage.ValidateAllObjectFunc, nil); err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-
-	// We first List directly from etcd by passing empty resourceVersion,
-	// to get the current etcd resourceVersion.
-	rvResult := &example.PodList{}
-	options := storage.ListOptions{
-		Predicate: storage.Everything,
-		Recursive: true,
-	}
-	if err := cacher.GetList(context.TODO(), "pods/ns", options, rvResult); err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	deletedPodRV := rvResult.ListMeta.ResourceVersion
-
-	result := &example.PodList{}
-	// We pass the current etcd ResourceVersion received from the above List() operation,
-	// since there is not easy way to get ResourceVersion of barPod deletion operation.
-	options = storage.ListOptions{
-		ResourceVersion: deletedPodRV,
-		Predicate:       storage.Everything,
-		Recursive:       true,
-	}
-	if err := cacher.GetList(context.TODO(), "pods/ns", options, result); err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
-	if result.ListMeta.ResourceVersion != deletedPodRV {
-		t.Errorf("Incorrect resource version: %v", result.ListMeta.ResourceVersion)
-	}
-	if len(result.Items) != 2 {
-		t.Errorf("Unexpected list result: %d", len(result.Items))
-	}
-	keys := sets.String{}
-	for _, item := range result.Items {
-		keys.Insert(item.Name)
-	}
-	if !keys.HasAll("foo", "baz") {
-		t.Errorf("Unexpected list result: %#v", result)
-	}
-	for _, item := range result.Items {
-		// unset fields that are set by the infrastructure
-		item.ResourceVersion = ""
-		item.CreationTimestamp = metav1.Time{}
-
-		if item.Namespace != "ns" {
-			t.Errorf("Unexpected namespace: %s", item.Namespace)
-		}
-
-		var expected *example.Pod
-		switch item.Name {
-		case "foo":
-			expected = podFooPrime
-		case "baz":
-			expected = podBaz
-		default:
-			t.Errorf("Unexpected item: %v", item)
-		}
-		if e, a := *expected, item; !reflect.DeepEqual(e, a) {
-			t.Errorf("Expected: %#v, got: %#v", e, a)
-		}
-	}
-}
-
 func verifyWatchEvent(t *testing.T, w watch.Interface, eventType watch.EventType, eventObject runtime.Object) {
 	_, _, line, _ := goruntime.Caller(1)
 	select {