From 2e204c1776e826f39a031ce1c79535f37c26a9f2 Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Mon, 10 Jun 2024 22:28:33 +0800 Subject: [PATCH 01/10] update Signed-off-by: forsaken628 --- .../trial/trial_controller_test.go | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 975dd7bcb1c..282ec52d19b 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -256,21 +256,40 @@ func TestReconcileBatchJob(t *testing.T) { } g.Expect(c.Status().Update(ctx, batchJob)).NotTo(gomega.HaveOccurred()) + t.Log("before create") // Create the Trial trial = newFakeTrialBatchJob() g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) // Expect that Trial status is succeeded and metrics are properly populated // Metrics available because GetTrialObservationLog returns values + start := time.Now() g.Eventually(func() bool { if err = c.Get(ctx, trialKey, trial); err != nil { + t.Log(time.Since(start), err) return false } - return trial.IsSucceeded() && + ok := trial.IsSucceeded() && len(trial.Status.Observation.Metrics) > 0 && trial.Status.Observation.Metrics[0].Min == "0.11" && trial.Status.Observation.Metrics[0].Max == "0.99" && trial.Status.Observation.Metrics[0].Latest == "0.11" + if !ok { + switch { + case len(trial.Status.Conditions) == 0: + t.Log(time.Since(start), "not created") + case trial.IsFailed(), trial.IsKilled(), trial.IsMetricsUnavailable(), trial.IsEarlyStopped(): + t.Log(time.Since(start), trial.Status) + panic("trial failed") + case trial.IsRunning(): + t.Log(time.Since(start), "running") + case trial.IsCreated(): + t.Log(time.Since(start), "created") + default: + t.Logf("%s %+v", time.Since(start), trial.Status.Conditions) + } + } + return ok }, timeout).Should(gomega.BeTrue()) // Delete the Trial From 13e39b3c7dfbd87c393bde2b59e56a2714146502 Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Mon, 10 Jun 2024 22:47:53 +0800 Subject: [PATCH 02/10] fix Signed-off-by: forsaken628 --- pkg/controller.v1beta1/trial/trial_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 282ec52d19b..10241c1be4c 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -278,7 +278,7 @@ func TestReconcileBatchJob(t *testing.T) { switch { case len(trial.Status.Conditions) == 0: t.Log(time.Since(start), "not created") - case trial.IsFailed(), trial.IsKilled(), trial.IsMetricsUnavailable(), trial.IsEarlyStopped(): + case trial.IsFailed(), trial.IsKilled(), trial.IsEarlyStopped(): t.Log(time.Since(start), trial.Status) panic("trial failed") case trial.IsRunning(): From 6b6e2494ddd897975441a6b41c9d088cb84dd75a Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Mon, 10 Jun 2024 23:13:20 +0800 Subject: [PATCH 03/10] update Signed-off-by: forsaken628 --- pkg/controller.v1beta1/trial/trial_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 10241c1be4c..9bacd270e93 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -58,7 +58,7 @@ var trialKey = types.NamespacedName{Name: trialName, Namespace: namespace} var batchJobKey = types.NamespacedName{Name: batchJobName, Namespace: namespace} func init() { - logf.SetLogger(zap.New()) + logf.SetLogger(zap.New(zap.UseDevMode(true))) } func TestAdd(t *testing.T) { From ac0c04ebd1f4460619731f0992c3948fc3bba61d Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Mon, 10 Jun 2024 23:20:22 +0800 Subject: [PATCH 04/10] update Signed-off-by: forsaken628 --- pkg/controller.v1beta1/trial/trial_controller_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 9bacd270e93..f0e77d9cde1 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -277,16 +277,16 @@ func TestReconcileBatchJob(t *testing.T) { if !ok { switch { case len(trial.Status.Conditions) == 0: - t.Log(time.Since(start), "not created") + t.Log(time.Since(start), trial.UID, "not created") case trial.IsFailed(), trial.IsKilled(), trial.IsEarlyStopped(): - t.Log(time.Since(start), trial.Status) + t.Log(time.Since(start), trial.UID, trial.Status) panic("trial failed") case trial.IsRunning(): - t.Log(time.Since(start), "running") + t.Log(time.Since(start), trial.UID, "running") case trial.IsCreated(): - t.Log(time.Since(start), "created") + t.Log(time.Since(start), trial.UID, "created") default: - t.Logf("%s %+v", time.Since(start), trial.Status.Conditions) + t.Logf("%s %s %+v", time.Since(start), trial.UID, trial.Status.Conditions) } } return ok From 539ca6ea1a5e110624f707433b1a0d8a2326af58 Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Mon, 10 Jun 2024 23:41:34 +0800 Subject: [PATCH 05/10] update Signed-off-by: forsaken628 --- pkg/controller.v1beta1/trial/trial_controller_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index f0e77d9cde1..f75c05c8471 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -281,6 +281,8 @@ func TestReconcileBatchJob(t *testing.T) { case trial.IsFailed(), trial.IsKilled(), trial.IsEarlyStopped(): t.Log(time.Since(start), trial.UID, trial.Status) panic("trial failed") + case trial.IsMetricsUnavailable(): + t.Log(time.Since(start), trial.UID, "metrics unavailable", trial.Status.Conditions) case trial.IsRunning(): t.Log(time.Since(start), trial.UID, "running") case trial.IsCreated(): From f0d5fd77be3d1ae87ba49f857b4f56c266d2caca Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Tue, 11 Jun 2024 01:10:46 +0800 Subject: [PATCH 06/10] fix Signed-off-by: forsaken628 --- .../trial/trial_controller_test.go | 305 +++++++++--------- 1 file changed, 159 insertions(+), 146 deletions(-) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index f75c05c8471..3ac7ff3ad79 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -21,7 +21,6 @@ import ( "testing" "time" - "github.com/golang/mock/gomock" "github.com/onsi/gomega" "github.com/prometheus/client_golang/prometheus" "github.com/spf13/viper" @@ -43,7 +42,6 @@ import ( "github.com/kubeflow/katib/pkg/controller.v1beta1/consts" trialutil "github.com/kubeflow/katib/pkg/controller.v1beta1/trial/util" "github.com/kubeflow/katib/pkg/controller.v1beta1/util" - managerclientmock "github.com/kubeflow/katib/pkg/mock/v1beta1/trial/managerclient" ) const ( @@ -89,9 +87,7 @@ func TestAdd(t *testing.T) { func TestReconcileBatchJob(t *testing.T) { g := gomega.NewGomegaWithT(t) - mockCtrl := gomock.NewController(t) - defer mockCtrl.Finish() - mockManagerClient := managerclientmock.NewMockManagerClient(mockCtrl) + mockMC := &mockManagerClient{} // Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a // channel when it is finished. @@ -102,7 +98,7 @@ func TestReconcileBatchJob(t *testing.T) { r := &ReconcileTrial{ Client: mgr.GetClient(), scheme: mgr.GetScheme(), - ManagerClient: mockManagerClient, + ManagerClient: mockMC, recorder: mgr.GetEventRecorderFor(ControllerName), collector: trialutil.NewTrialsCollector(mgr.GetCache(), prometheus.NewRegistry()), } @@ -179,158 +175,162 @@ func TestReconcileBatchJob(t *testing.T) { }, } - mockManagerClient.EXPECT().GetTrialObservationLog(gomock.Any()).Return(observationLogAvailable, nil).Times(1) - mockManagerClient.EXPECT().GetTrialObservationLog(gomock.Any()).Return(observationLogUnavailable, nil).MinTimes(1) - mockManagerClient.EXPECT().DeleteTrialObservationLog(gomock.Any()).Return(nil, nil).AnyTimes() + t.Run(`Trial run with "Failed" BatchJob.`, func(t *testing.T) { + mockMC.msg = observationLogUnavailable + trial := newFakeTrialBatchJob() + batchJob := &batchv1.Job{} - // Test 1 - Trial run with "Failed" BatchJob. - trial := newFakeTrialBatchJob() - batchJob := &batchv1.Job{} + // Create the Trial + g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) - // Create the Trial - g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) + // Expect that BatchJob with appropriate name is created + g.Eventually(func() error { + return c.Get(ctx, batchJobKey, batchJob) + }, timeout).Should(gomega.Succeed()) - // Expect that BatchJob with appropriate name is created - g.Eventually(func() error { - return c.Get(ctx, batchJobKey, batchJob) - }, timeout).Should(gomega.Succeed()) + // Expect that Trial status is running + g.Eventually(func() bool { + if err = c.Get(ctx, trialKey, trial); err != nil { + return false + } + return trial.IsRunning() + }, timeout).Should(gomega.BeTrue()) + + // Manually update BatchJob status to failed + // Expect that Trial status is failed + g.Eventually(func() bool { + if err = c.Get(ctx, batchJobKey, batchJob); err != nil { + return false + } + batchJob.Status = batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{ + { + Type: batchv1.JobFailed, + Status: corev1.ConditionTrue, + Message: "BatchJob failed test message", + Reason: "BatchJob failed test reason", + }, + }, + } + if err = c.Status().Update(ctx, batchJob); err != nil { + return false + } - // Expect that Trial status is running - g.Eventually(func() bool { - if err = c.Get(ctx, trialKey, trial); err != nil { - return false - } - return trial.IsRunning() - }, timeout).Should(gomega.BeTrue()) - - // Manually update BatchJob status to failed - // Expect that Trial status is failed - g.Eventually(func() bool { - if err = c.Get(ctx, batchJobKey, batchJob); err != nil { - return false - } + if err = c.Get(ctx, trialKey, trial); err != nil { + return false + } + return trial.IsFailed() + }, timeout).Should(gomega.BeTrue()) + + // Delete the Trial + g.Expect(c.Delete(ctx, trial)).NotTo(gomega.HaveOccurred()) + + // Expect that Trial is deleted + // BatchJob can't be deleted because GC doesn't work in envtest and BatchJob stuck in termination phase. + // Ref: https://book.kubebuilder.io/reference/testing/envtest.html#testing-considerations. + g.Eventually(func() bool { + return errors.IsNotFound(c.Get(ctx, trialKey, &trialsv1beta1.Trial{})) + }, timeout).Should(gomega.BeTrue()) + + }) + + t.Run(`Trail with "Complete" BatchJob and Available metrics.`, func(t *testing.T) { + mockMC.msg = observationLogAvailable + batchJob := &batchv1.Job{} + batchJobCompleteMessage := "BatchJob completed test message" + batchJobCompleteReason := "BatchJob completed test reason" + // Update BatchJob status to Complete. + g.Expect(c.Get(ctx, batchJobKey, batchJob)).NotTo(gomega.HaveOccurred()) batchJob.Status = batchv1.JobStatus{ Conditions: []batchv1.JobCondition{ { - Type: batchv1.JobFailed, + Type: batchv1.JobComplete, Status: corev1.ConditionTrue, - Message: "BatchJob failed test message", - Reason: "BatchJob failed test reason", + Message: batchJobCompleteMessage, + Reason: batchJobCompleteReason, }, }, } - if err = c.Status().Update(ctx, batchJob); err != nil { - return false - } - - if err = c.Get(ctx, trialKey, trial); err != nil { - return false - } - return trial.IsFailed() - }, timeout).Should(gomega.BeTrue()) - - // Delete the Trial - g.Expect(c.Delete(ctx, trial)).NotTo(gomega.HaveOccurred()) - - // Expect that Trial is deleted - // BatchJob can't be deleted because GC doesn't work in envtest and BatchJob stuck in termination phase. - // Ref: https://book.kubebuilder.io/reference/testing/envtest.html#testing-considerations. - g.Eventually(func() bool { - return errors.IsNotFound(c.Get(ctx, trialKey, &trialsv1beta1.Trial{})) - }, timeout).Should(gomega.BeTrue()) - - // Test 2 - Trail with "Complete" BatchJob and Available metrics. - batchJobCompleteMessage := "BatchJob completed test message" - batchJobCompleteReason := "BatchJob completed test reason" - // Update BatchJob status to Complete. - g.Expect(c.Get(ctx, batchJobKey, batchJob)).NotTo(gomega.HaveOccurred()) - batchJob.Status = batchv1.JobStatus{ - Conditions: []batchv1.JobCondition{ - { - Type: batchv1.JobComplete, - Status: corev1.ConditionTrue, - Message: batchJobCompleteMessage, - Reason: batchJobCompleteReason, - }, - }, - } - g.Expect(c.Status().Update(ctx, batchJob)).NotTo(gomega.HaveOccurred()) - - t.Log("before create") - // Create the Trial - trial = newFakeTrialBatchJob() - g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) - - // Expect that Trial status is succeeded and metrics are properly populated - // Metrics available because GetTrialObservationLog returns values - start := time.Now() - g.Eventually(func() bool { - if err = c.Get(ctx, trialKey, trial); err != nil { - t.Log(time.Since(start), err) - return false - } - ok := trial.IsSucceeded() && - len(trial.Status.Observation.Metrics) > 0 && - trial.Status.Observation.Metrics[0].Min == "0.11" && - trial.Status.Observation.Metrics[0].Max == "0.99" && - trial.Status.Observation.Metrics[0].Latest == "0.11" - if !ok { - switch { - case len(trial.Status.Conditions) == 0: - t.Log(time.Since(start), trial.UID, "not created") - case trial.IsFailed(), trial.IsKilled(), trial.IsEarlyStopped(): - t.Log(time.Since(start), trial.UID, trial.Status) - panic("trial failed") - case trial.IsMetricsUnavailable(): - t.Log(time.Since(start), trial.UID, "metrics unavailable", trial.Status.Conditions) - case trial.IsRunning(): - t.Log(time.Since(start), trial.UID, "running") - case trial.IsCreated(): - t.Log(time.Since(start), trial.UID, "created") - default: - t.Logf("%s %s %+v", time.Since(start), trial.UID, trial.Status.Conditions) + g.Expect(c.Status().Update(ctx, batchJob)).NotTo(gomega.HaveOccurred()) + + t.Log("before create") + // Create the Trial + trial := newFakeTrialBatchJob() + g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) + + // Expect that Trial status is succeeded and metrics are properly populated + // Metrics available because GetTrialObservationLog returns values + start := time.Now() + g.Eventually(func() bool { + if err = c.Get(ctx, trialKey, trial); err != nil { + t.Log(time.Since(start), err) + return false } - } - return ok - }, timeout).Should(gomega.BeTrue()) - - // Delete the Trial - g.Expect(c.Delete(ctx, trial)).NotTo(gomega.HaveOccurred()) - - // Expect that Trial is deleted - g.Eventually(func() bool { - return errors.IsNotFound(c.Get(ctx, trialKey, &trialsv1beta1.Trial{})) - }, timeout).Should(gomega.BeTrue()) - - // Test 3 - Trail with "Complete" BatchJob and Unavailable metrics. - // Create the Trial - trial = newFakeTrialBatchJob() - g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) - - // Expect that Trial status is succeeded with "false" status and "metrics unavailable" reason. - // Metrics unavailable because GetTrialObservationLog returns "unavailable". - g.Eventually(func() bool { - if err = c.Get(ctx, trialKey, trial); err != nil { - return false - } - return trial.IsMetricsUnavailable() && - len(trial.Status.Observation.Metrics) > 0 && - trial.Status.Observation.Metrics[0].Min == consts.UnavailableMetricValue && - trial.Status.Observation.Metrics[0].Max == consts.UnavailableMetricValue && - trial.Status.Observation.Metrics[0].Latest == consts.UnavailableMetricValue - }, timeout).Should(gomega.BeTrue()) - - // Delete the Trial - g.Expect(c.Delete(ctx, trial)).NotTo(gomega.HaveOccurred()) - - // Expect that Trial is deleted - g.Eventually(func() bool { - return errors.IsNotFound(c.Get(ctx, trialKey, &trialsv1beta1.Trial{})) - }, timeout).Should(gomega.BeTrue()) - - // Test 4 - Update status for empty Trial - g.Expect(r.updateStatus(&trialsv1beta1.Trial{})).To(gomega.HaveOccurred()) - + ok := trial.IsSucceeded() && + len(trial.Status.Observation.Metrics) > 0 && + trial.Status.Observation.Metrics[0].Min == "0.11" && + trial.Status.Observation.Metrics[0].Max == "0.99" && + trial.Status.Observation.Metrics[0].Latest == "0.11" + if !ok { + switch { + case len(trial.Status.Conditions) == 0: + t.Log(time.Since(start), trial.UID, "not created") + case trial.IsFailed(), trial.IsKilled(), trial.IsEarlyStopped(): + t.Log(time.Since(start), trial.UID, trial.Status) + panic("trial failed") + case trial.IsMetricsUnavailable(): + t.Log(time.Since(start), trial.UID, "metrics unavailable", trial.Status.Conditions) + case trial.IsRunning(): + t.Log(time.Since(start), trial.UID, "running") + case trial.IsCreated(): + t.Log(time.Since(start), trial.UID, "created") + default: + t.Logf("%s %s %+v", time.Since(start), trial.UID, trial.Status.Conditions) + } + } + return ok + }, timeout).Should(gomega.BeTrue()) + + // Delete the Trial + g.Expect(c.Delete(ctx, trial)).NotTo(gomega.HaveOccurred()) + + // Expect that Trial is deleted + g.Eventually(func() bool { + return errors.IsNotFound(c.Get(ctx, trialKey, &trialsv1beta1.Trial{})) + }, timeout).Should(gomega.BeTrue()) + }) + + t.Run(`Trail with "Complete" BatchJob and Unavailable metrics.`, func(t *testing.T) { + mockMC.msg = observationLogUnavailable + // Create the Trial + trial := newFakeTrialBatchJob() + g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) + + // Expect that Trial status is succeeded with "false" status and "metrics unavailable" reason. + // Metrics unavailable because GetTrialObservationLog returns "unavailable". + g.Eventually(func() bool { + if err = c.Get(ctx, trialKey, trial); err != nil { + return false + } + return trial.IsMetricsUnavailable() && + len(trial.Status.Observation.Metrics) > 0 && + trial.Status.Observation.Metrics[0].Min == consts.UnavailableMetricValue && + trial.Status.Observation.Metrics[0].Max == consts.UnavailableMetricValue && + trial.Status.Observation.Metrics[0].Latest == consts.UnavailableMetricValue + }, timeout).Should(gomega.BeTrue()) + + // Delete the Trial + g.Expect(c.Delete(ctx, trial)).NotTo(gomega.HaveOccurred()) + + // Expect that Trial is deleted + g.Eventually(func() bool { + return errors.IsNotFound(c.Get(ctx, trialKey, &trialsv1beta1.Trial{})) + }, timeout).Should(gomega.BeTrue()) + }) + + t.Run("Update status for empty Trial", func(t *testing.T) { + g.Expect(r.updateStatus(&trialsv1beta1.Trial{})).To(gomega.HaveOccurred()) + }) } func TestGetObjectiveMetricValue(t *testing.T) { @@ -449,3 +449,16 @@ func newFakeTrialBatchJob() *trialsv1beta1.Trial { }, } } + +type mockManagerClient struct { + msg *api_pb.GetObservationLogReply +} + +func (c *mockManagerClient) GetTrialObservationLog(instance *trialsv1beta1.Trial) (*api_pb.GetObservationLogReply, error) { + return c.msg, nil +} + +func (c *mockManagerClient) DeleteTrialObservationLog(instance *trialsv1beta1.Trial) (*api_pb.DeleteObservationLogReply, error) { + c.msg = nil + return &api_pb.DeleteObservationLogReply{}, nil +} From 4a52a18435d60a94efd5710d1d9858d750877698 Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Tue, 11 Jun 2024 01:16:31 +0800 Subject: [PATCH 07/10] cleanup Signed-off-by: forsaken628 --- .../trial/trial_controller_test.go | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 3ac7ff3ad79..8eb01fee042 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -253,7 +253,6 @@ func TestReconcileBatchJob(t *testing.T) { } g.Expect(c.Status().Update(ctx, batchJob)).NotTo(gomega.HaveOccurred()) - t.Log("before create") // Create the Trial trial := newFakeTrialBatchJob() g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) @@ -266,29 +265,11 @@ func TestReconcileBatchJob(t *testing.T) { t.Log(time.Since(start), err) return false } - ok := trial.IsSucceeded() && + return trial.IsSucceeded() && len(trial.Status.Observation.Metrics) > 0 && trial.Status.Observation.Metrics[0].Min == "0.11" && trial.Status.Observation.Metrics[0].Max == "0.99" && trial.Status.Observation.Metrics[0].Latest == "0.11" - if !ok { - switch { - case len(trial.Status.Conditions) == 0: - t.Log(time.Since(start), trial.UID, "not created") - case trial.IsFailed(), trial.IsKilled(), trial.IsEarlyStopped(): - t.Log(time.Since(start), trial.UID, trial.Status) - panic("trial failed") - case trial.IsMetricsUnavailable(): - t.Log(time.Since(start), trial.UID, "metrics unavailable", trial.Status.Conditions) - case trial.IsRunning(): - t.Log(time.Since(start), trial.UID, "running") - case trial.IsCreated(): - t.Log(time.Since(start), trial.UID, "created") - default: - t.Logf("%s %s %+v", time.Since(start), trial.UID, trial.Status.Conditions) - } - } - return ok }, timeout).Should(gomega.BeTrue()) // Delete the Trial From b4caa68b5c01191461d1bb73644594511cbc17ce Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Tue, 11 Jun 2024 01:25:17 +0800 Subject: [PATCH 08/10] fix Signed-off-by: forsaken628 --- pkg/controller.v1beta1/trial/trial_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 8eb01fee042..36c412f7a59 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -231,7 +231,6 @@ func TestReconcileBatchJob(t *testing.T) { g.Eventually(func() bool { return errors.IsNotFound(c.Get(ctx, trialKey, &trialsv1beta1.Trial{})) }, timeout).Should(gomega.BeTrue()) - }) t.Run(`Trail with "Complete" BatchJob and Available metrics.`, func(t *testing.T) { From 5cb9b2b1643966994e5d2f9b68a48937c03dfc1c Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Tue, 11 Jun 2024 11:12:32 +0800 Subject: [PATCH 09/10] update Signed-off-by: forsaken628 --- pkg/controller.v1beta1/trial/trial_controller_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 36c412f7a59..4b82c225285 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -176,6 +176,7 @@ func TestReconcileBatchJob(t *testing.T) { } t.Run(`Trial run with "Failed" BatchJob.`, func(t *testing.T) { + g := gomega.NewGomegaWithT(t) mockMC.msg = observationLogUnavailable trial := newFakeTrialBatchJob() batchJob := &batchv1.Job{} @@ -234,6 +235,7 @@ func TestReconcileBatchJob(t *testing.T) { }) t.Run(`Trail with "Complete" BatchJob and Available metrics.`, func(t *testing.T) { + g := gomega.NewGomegaWithT(t) mockMC.msg = observationLogAvailable batchJob := &batchv1.Job{} batchJobCompleteMessage := "BatchJob completed test message" @@ -281,6 +283,7 @@ func TestReconcileBatchJob(t *testing.T) { }) t.Run(`Trail with "Complete" BatchJob and Unavailable metrics.`, func(t *testing.T) { + g := gomega.NewGomegaWithT(t) mockMC.msg = observationLogUnavailable // Create the Trial trial := newFakeTrialBatchJob() @@ -309,6 +312,7 @@ func TestReconcileBatchJob(t *testing.T) { }) t.Run("Update status for empty Trial", func(t *testing.T) { + g := gomega.NewGomegaWithT(t) g.Expect(r.updateStatus(&trialsv1beta1.Trial{})).To(gomega.HaveOccurred()) }) } From 317c40d32049033379175df67d0bd445bae69f37 Mon Sep 17 00:00:00 2001 From: forsaken628 Date: Fri, 14 Jun 2024 11:43:29 +0800 Subject: [PATCH 10/10] use gomock Signed-off-by: forsaken628 --- .../trial/trial_controller_test.go | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 4b82c225285..085f2e7c0fb 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -21,6 +21,7 @@ import ( "testing" "time" + "github.com/golang/mock/gomock" "github.com/onsi/gomega" "github.com/prometheus/client_golang/prometheus" "github.com/spf13/viper" @@ -42,6 +43,7 @@ import ( "github.com/kubeflow/katib/pkg/controller.v1beta1/consts" trialutil "github.com/kubeflow/katib/pkg/controller.v1beta1/trial/util" "github.com/kubeflow/katib/pkg/controller.v1beta1/util" + managerclientmock "github.com/kubeflow/katib/pkg/mock/v1beta1/trial/managerclient" ) const ( @@ -87,7 +89,9 @@ func TestAdd(t *testing.T) { func TestReconcileBatchJob(t *testing.T) { g := gomega.NewGomegaWithT(t) - mockMC := &mockManagerClient{} + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mockManagerClient := managerclientmock.NewMockManagerClient(mockCtrl) // Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a // channel when it is finished. @@ -98,7 +102,7 @@ func TestReconcileBatchJob(t *testing.T) { r := &ReconcileTrial{ Client: mgr.GetClient(), scheme: mgr.GetScheme(), - ManagerClient: mockMC, + ManagerClient: mockManagerClient, recorder: mgr.GetEventRecorderFor(ControllerName), collector: trialutil.NewTrialsCollector(mgr.GetCache(), prometheus.NewRegistry()), } @@ -177,7 +181,8 @@ func TestReconcileBatchJob(t *testing.T) { t.Run(`Trial run with "Failed" BatchJob.`, func(t *testing.T) { g := gomega.NewGomegaWithT(t) - mockMC.msg = observationLogUnavailable + mockManagerClient.EXPECT().DeleteTrialObservationLog(gomock.Any()).Return(nil, nil) + trial := newFakeTrialBatchJob() batchJob := &batchv1.Job{} @@ -236,7 +241,10 @@ func TestReconcileBatchJob(t *testing.T) { t.Run(`Trail with "Complete" BatchJob and Available metrics.`, func(t *testing.T) { g := gomega.NewGomegaWithT(t) - mockMC.msg = observationLogAvailable + gomock.InOrder( + mockManagerClient.EXPECT().GetTrialObservationLog(gomock.Any()).Return(observationLogAvailable, nil).MinTimes(1), + mockManagerClient.EXPECT().DeleteTrialObservationLog(gomock.Any()).Return(nil, nil), + ) batchJob := &batchv1.Job{} batchJobCompleteMessage := "BatchJob completed test message" batchJobCompleteReason := "BatchJob completed test reason" @@ -284,7 +292,10 @@ func TestReconcileBatchJob(t *testing.T) { t.Run(`Trail with "Complete" BatchJob and Unavailable metrics.`, func(t *testing.T) { g := gomega.NewGomegaWithT(t) - mockMC.msg = observationLogUnavailable + gomock.InOrder( + mockManagerClient.EXPECT().GetTrialObservationLog(gomock.Any()).Return(observationLogUnavailable, nil).MinTimes(1), + mockManagerClient.EXPECT().DeleteTrialObservationLog(gomock.Any()).Return(nil, nil), + ) // Create the Trial trial := newFakeTrialBatchJob() g.Expect(c.Create(ctx, trial)).NotTo(gomega.HaveOccurred()) @@ -433,16 +444,3 @@ func newFakeTrialBatchJob() *trialsv1beta1.Trial { }, } } - -type mockManagerClient struct { - msg *api_pb.GetObservationLogReply -} - -func (c *mockManagerClient) GetTrialObservationLog(instance *trialsv1beta1.Trial) (*api_pb.GetObservationLogReply, error) { - return c.msg, nil -} - -func (c *mockManagerClient) DeleteTrialObservationLog(instance *trialsv1beta1.Trial) (*api_pb.DeleteObservationLogReply, error) { - c.msg = nil - return &api_pb.DeleteObservationLogReply{}, nil -}