[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #301: [YUNIKORN-552] Add some unit tests to cover placeholder cleanup

[GitBox <gi...@apache.org>]

yangwwei commented on a change in pull request #301:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/301#discussion_r707519227



##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ func TestPlaceholderManagerStartStop(t *testing.T) {
 	time.Sleep(5 * time.Millisecond)
 	assert.Equal(t, mgr.isRunning(), false, "placeholder manager has not stopped")
 }
+
+func TestPlaceholderManagerCleanup(t *testing.T) {
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	pod1 := &v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name: "pod-01",
+			UID:  "UID-01",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()

Review comment:
       can we make sure mgr is stopped after the test?

##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ func TestPlaceholderManagerStartStop(t *testing.T) {
 	time.Sleep(5 * time.Millisecond)
 	assert.Equal(t, mgr.isRunning(), false, "placeholder manager has not stopped")
 }
+
+func TestPlaceholderManagerCleanup(t *testing.T) {
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	pod1 := &v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name: "pod-01",
+			UID:  "UID-01",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+	mgr.orphanPods["task01"] = pod1
+	assert.Equal(t, len(mgr.orphanPods), 1)
+	<-time.After(5 * time.Second)

Review comment:
       we can't wait for 5s in UT for this single test
   what we can do here is to add a function that can change the default 5s interval for the placeholder cleanup, e.g `setCleanupInterval(interval time.Duration)`. During this test, set the interval to 100millsecond, etc. And remember to set it back to 5s after the test.

##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -261,3 +261,24 @@ func TestPlaceholderManagerStartStop(t *testing.T) {
 	time.Sleep(5 * time.Millisecond)
 	assert.Equal(t, mgr.isRunning(), false, "placeholder manager has not stopped")
 }
+
+func TestPlaceholderManagerCleanup(t *testing.T) {
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	pod1 := &v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name: "pod-01",
+			UID:  "UID-01",
+		},
+	}
+	mgr := NewPlaceholderManager(mockedAPIProvider.GetAPIs())
+	mgr.Start()
+	assert.Equal(t, mgr.isRunning(), true, "manager should be running after start")
+	mgr.orphanPods["task01"] = pod1

Review comment:
       can we add more than 1 pod here?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org