Merge pull request #45571 from verb/fix-fakeruntime-assertcalls

Automatic merge from submit-queue

Fix AssertCalls usage for kubelet fake runtimes unit tests

Despite its name, AssertCalls() does not assert anything. It returns an error that should be checked. This was causing false negatives for a handful of unit tests, which are also fixed here.

Tests for the image manager needed to be rearranged in order to accommodate a potentially different sequence of calls each tick because the image puller changes behavior based on prior errors.

**What this PR does / why we need it**: Fixes broken unit tests

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: 

**Special notes for your reviewer**: 

**Release note**:

```release-note
NONE
```

pkg/kubelet/images/image_manager_test.go

@@ -31,60 +31,83 @@ import (
 	ctest "k8s.io/kubernetes/pkg/kubelet/container/testing"
 )
 
+type pullerExpects struct {
+	calls []string
+	err   error
+}
+
 type pullerTestCase struct {
-	containerImage  string
+	containerImage string
-	policy          v1.PullPolicy
+	policy         v1.PullPolicy
-	calledFunctions []string
+	inspectErr     error
-	inspectErr      error
+	pullerErr      error
-	pullerErr       error
+	expected       []pullerExpects
-	expectedErr     []error
 }
 
 func pullerTestCases() []pullerTestCase {
 	return []pullerTestCase{
 		{ // pull missing image
-			containerImage:  "missing_image",
+			containerImage: "missing_image",
-			policy:          v1.PullIfNotPresent,
+			policy:         v1.PullIfNotPresent,
-			calledFunctions: []string{"GetImageRef", "PullImage"},
+			inspectErr:     nil,
-			inspectErr:      nil,
+			pullerErr:      nil,
-			pullerErr:       nil,
+			expected: []pullerExpects{
-			expectedErr:     []error{nil}},
+				{[]string{"GetImageRef", "PullImage"}, nil},
+			}},
 
 		{ // image present, don't pull
-			containerImage:  "present_image",
+			containerImage: "present_image",
-			policy:          v1.PullIfNotPresent,
+			policy:         v1.PullIfNotPresent,
-			calledFunctions: []string{"GetImageRef"},
+			inspectErr:     nil,
-			inspectErr:      nil,
+			pullerErr:      nil,
-			pullerErr:       nil,
+			expected: []pullerExpects{
-			expectedErr:     []error{nil, nil, nil}},
+				{[]string{"GetImageRef"}, nil},
+				{[]string{"GetImageRef"}, nil},
+				{[]string{"GetImageRef"}, nil},
+			}},
 		// image present, pull it
 		{containerImage: "present_image",
-			policy:          v1.PullAlways,
+			policy:     v1.PullAlways,
-			calledFunctions: []string{"GetImageRef", "PullImage"},
+			inspectErr: nil,
-			inspectErr:      nil,
+			pullerErr:  nil,
-			pullerErr:       nil,
+			expected: []pullerExpects{
-			expectedErr:     []error{nil, nil, nil}},
+				{[]string{"GetImageRef", "PullImage"}, nil},
+				{[]string{"GetImageRef", "PullImage"}, nil},
+				{[]string{"GetImageRef", "PullImage"}, nil},
+			}},
 		// missing image, error PullNever
 		{containerImage: "missing_image",
-			policy:          v1.PullNever,
+			policy:     v1.PullNever,
-			calledFunctions: []string{"GetImageRef"},
+			inspectErr: nil,
-			inspectErr:      nil,
+			pullerErr:  nil,
-			pullerErr:       nil,
+			expected: []pullerExpects{
-			expectedErr:     []error{ErrImageNeverPull, ErrImageNeverPull, ErrImageNeverPull}},
+				{[]string{"GetImageRef"}, ErrImageNeverPull},
+				{[]string{"GetImageRef"}, ErrImageNeverPull},
+				{[]string{"GetImageRef"}, ErrImageNeverPull},
+			}},
 		// missing image, unable to inspect
 		{containerImage: "missing_image",
-			policy:          v1.PullIfNotPresent,
+			policy:     v1.PullIfNotPresent,
-			calledFunctions: []string{"GetImageRef"},
+			inspectErr: errors.New("unknown inspectError"),
-			inspectErr:      errors.New("unknown inspectError"),
+			pullerErr:  nil,
-			pullerErr:       nil,
+			expected: []pullerExpects{
-			expectedErr:     []error{ErrImageInspect, ErrImageInspect, ErrImageInspect}},
+				{[]string{"GetImageRef"}, ErrImageInspect},
+				{[]string{"GetImageRef"}, ErrImageInspect},
+				{[]string{"GetImageRef"}, ErrImageInspect},
+			}},
 		// missing image, unable to fetch
 		{containerImage: "typo_image",
-			policy:          v1.PullIfNotPresent,
+			policy:     v1.PullIfNotPresent,
-			calledFunctions: []string{"GetImageRef", "PullImage"},
+			inspectErr: nil,
-			inspectErr:      nil,
+			pullerErr:  errors.New("404"),
-			pullerErr:       errors.New("404"),
+			expected: []pullerExpects{
-			expectedErr:     []error{ErrImagePull, ErrImagePull, ErrImagePullBackOff, ErrImagePull, ErrImagePullBackOff, ErrImagePullBackOff}},
+				{[]string{"GetImageRef", "PullImage"}, ErrImagePull},
+				{[]string{"GetImageRef", "PullImage"}, ErrImagePull},
+				{[]string{"GetImageRef"}, ErrImagePullBackOff},
+				{[]string{"GetImageRef", "PullImage"}, ErrImagePull},
+				{[]string{"GetImageRef"}, ErrImagePullBackOff},
+				{[]string{"GetImageRef"}, ErrImagePullBackOff},
+			}},
 	}
 }
 
@@ -102,7 +125,7 @@ func pullerTestEnv(c pullerTestCase, serialized bool) (puller ImageManager, fake
 	fakeRuntime = &ctest.FakeRuntime{}
 	fakeRecorder := &record.FakeRecorder{}
 
-	fakeRuntime.ImageList = []Image{{ID: "present_image"}}
+	fakeRuntime.ImageList = []Image{{ID: "present_image:latest"}}
 	fakeRuntime.Err = c.pullerErr
 	fakeRuntime.InspectErr = c.inspectErr
 
@@ -125,11 +148,12 @@ func TestParallelPuller(t *testing.T) {
 	for i, c := range cases {
 		puller, fakeClock, fakeRuntime, container := pullerTestEnv(c, false)
 
-		for tick, expected := range c.expectedErr {
+		for tick, expected := range c.expected {
+			fakeRuntime.CalledFunctions = nil
 			fakeClock.Step(time.Second)
 			_, _, err := puller.EnsureImageExists(pod, container, nil)
-			fakeRuntime.AssertCalls(c.calledFunctions)
+			assert.NoError(t, fakeRuntime.AssertCalls(expected.calls), "in test %d tick=%d", i, tick)
-			assert.Equal(t, expected, err, "in test %d tick=%d", i, tick)
+			assert.Equal(t, expected.err, err, "in test %d tick=%d", i, tick)
 		}
 	}
 }
@@ -149,11 +173,12 @@ func TestSerializedPuller(t *testing.T) {
 	for i, c := range cases {
 		puller, fakeClock, fakeRuntime, container := pullerTestEnv(c, true)
 
-		for tick, expected := range c.expectedErr {
+		for tick, expected := range c.expected {
+			fakeRuntime.CalledFunctions = nil
 			fakeClock.Step(time.Second)
 			_, _, err := puller.EnsureImageExists(pod, container, nil)
-			fakeRuntime.AssertCalls(c.calledFunctions)
+			assert.NoError(t, fakeRuntime.AssertCalls(expected.calls), "in test %d tick=%d", i, tick)
-			assert.Equal(t, expected, err, "in test %d tick=%d", i, tick)
+			assert.Equal(t, expected.err, err, "in test %d tick=%d", i, tick)
 		}
 	}
 }

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

@@ -60,7 +60,7 @@ func TestRemoveContainer(t *testing.T) {
 	expectedContainerLogSymlink := legacyLogSymlink(containerId, "foo", "bar", "new")
 	assert.Equal(t, fakeOS.Removes, []string{expectedContainerLogPath, expectedContainerLogSymlink})
 	// Verify container is removed
-	fakeRuntime.AssertCalls([]string{"RemoveContainer"})
+	assert.Contains(t, fakeRuntime.Called, "RemoveContainer")
 	containers, err := fakeRuntime.ListContainers(&runtimeapi.ContainerFilter{Id: containerId})
 	assert.NoError(t, err)
 	assert.Empty(t, containers)

pkg/kubelet/kuberuntime/kuberuntime_sandbox_test.go

@@ -57,7 +57,7 @@ func TestCreatePodSandbox(t *testing.T) {
 	}
 	id, _, err := m.createPodSandbox(pod, 1)
 	assert.NoError(t, err)
-	fakeRuntime.AssertCalls([]string{"RunPodSandbox"})
+	assert.Contains(t, fakeRuntime.Called, "RunPodSandbox")
 	sandboxes, err := fakeRuntime.ListPodSandbox(&runtimeapi.PodSandboxFilter{Id: id})
 	assert.NoError(t, err)
 	assert.Equal(t, len(sandboxes), 1)