Introduced error constants and replaced reflect with cmp #2289

tariq-hasan · 2024-03-17T23:41:05Z

What this PR does / why we need it:

Following the suggestion #2281 (review) from @tenzen-y this code change provides a more thorough comparison of errors through the introduction of error constants and the replacement of replace.DeepEqual with cmp.Diff.

In addition some of the test cases in generator_test.go have been revised to allow for a more thorough comparison of error messages.

Finally cosmetic changes have been applied to some of the tests in order to match the style of the code in config_test.go that compares error constants.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2268

Checklist:

Docs included if any changes are user facing

tenzen-y

I reviewed only against experiments. So, could you refine other codes based on my comments?

tenzen-y · 2024-04-04T16:39:06Z

pkg/controller.v1beta1/experiment/manifest/generator.go

+	ErrConfigMapNotFound                  = errors.New("ConfigMap not found")
+	ErrConvertStringToUnstructuredFailed  = errors.New("ConvertStringToUnstructured failed")
+	ErrConvertUnstructuredToStringFailed  = errors.New("ConvertUnstructuredToString failed")
+	ErrParamNotFoundInParameterAssignment = errors.New("unable to find non-meta paramater from TrialParameters in ParameterAssignment")
+	ErrParamNotFoundInTrialParameters     = errors.New("unable to find parameter from ParameterAssignment in TrialParameters")
+	ErrTemplatePathNotFound               = errors.New("TemplatePath not found in ConfigMap")


Suggested change

ErrConfigMapNotFound = errors.New("ConfigMap not found")

ErrConvertStringToUnstructuredFailed = errors.New("ConvertStringToUnstructured failed")

ErrConvertUnstructuredToStringFailed = errors.New("ConvertUnstructuredToString failed")

ErrParamNotFoundInParameterAssignment = errors.New("unable to find non-meta paramater from TrialParameters in ParameterAssignment")

ErrParamNotFoundInTrialParameters = errors.New("unable to find parameter from ParameterAssignment in TrialParameters")

ErrTemplatePathNotFound = errors.New("TemplatePath not found in ConfigMap")

errConfigMapNotFound = errors.New("configMap not found")

errConvertStringToUnstructuredFailed = errors.New("failed to convert string to unstructured")

errConvertUnstructuredToStringFailed = errors.New("failed to convert unstructured to string")

errParamNotFoundInParameterAssignment = errors.New("unable to find non-meta parameter from TrialParameters in ParameterAssignment")

errParamNotFoundInTrialParameters = errors.New("unable to find parameter from ParameterAssignment in TrialParameters")

errTrialTemplateNotFound = errors.New("unable to find trial template in ConfigMap")

It seems that these errors do not need to be exposed, right?
Also, should we make errors more explainable?

The comment has been addressed in this commit.

tenzen-y · 2024-04-04T16:48:33Z

pkg/controller.v1beta1/experiment/manifest/generator_test.go

@@ -245,18 +237,17 @@ spec:

 	expectedRunSpec, err := util.ConvertStringToUnstructured(expectedStr)
 	if err != nil {


Suggested change

if err != nil {

if diff := cmp.Diff(errConvertStringToUnstructuredFailed, err, cmpopts.EquateErrors()); len(diff) != 0 {

Should we evaluate if the error is appropriate?

expectedRunSpec is not evaluated using a function in generator.go so I am not sure if the errors defined in generator.go are applicable here.

I see. So, could we use this error? https://github.com/kubernetes/apimachinery/blob/e696ec55a32e2177cd5f2fe1ded91d9485aa5c64/pkg/util/yaml/decoder.go#L255

If it is not working well, could you define a dedicated error as well?

katib/pkg/controller.v1beta1/util/unstructured.go

Line 48 in 7df05c2

func ConvertStringToUnstructured(in string) (*unstructured.Unstructured, error) {

I think we might need to define a dedicated error since the YAMLSyntaxError struct's field err cannot be exported and we therefore cannot initialize a YAMLSyntaxError struct in generator_test.go to define the expected error.

I think it is better to move both errConvertStringToUnstructuredFailed and errConvertUnstructuredToStringFailed to katib/pkg/controller.v1beta1/util/unstructured.go.

Please let me know your thoughts.

P.S.: We can also define errConvertObjectToUnstructured and use it in generator_test.go.

@tariq-hasan I was missing any context here. Sorry for your confusion.
I meant like this.

if diff := cmp.Diff(nil, err, cmpopts.EquateErrors()); len(diff) != 0 { t.Errorf("ConvertStringToUnstructured failed (-want,+got):\n%s", diff) }

I addressed the comment in this commit.

tenzen-y · 2024-04-04T16:49:01Z

pkg/controller.v1beta1/experiment/manifest/generator_test.go

@@ -245,18 +237,17 @@ spec:

 	expectedRunSpec, err := util.ConvertStringToUnstructured(expectedStr)
 	if err != nil {
-		t.Errorf("ConvertStringToUnstructured failed: %v", err)
+		t.Errorf("%v: %v", ErrConvertStringToUnstructuredFailed, err)


Suggested change

t.Errorf("%v: %v", ErrConvertStringToUnstructuredFailed, err)

t.Errorf("Unexpected error from ConvertStringToUnstructured (-want,+got):\n%s", diff)

The above comment applies here as well.

tenzen-y · 2024-04-04T16:50:26Z

pkg/controller.v1beta1/suggestion/composer/composer_test.go

-		reflect.DeepEqual(expected.Labels, actual.Labels) &&
-		reflect.DeepEqual(expected.Annotations, actual.Annotations) &&
+		cmp.Equal(expected.Labels, actual.Labels) &&
+		cmp.Equal(expected.Annotations, actual.Annotations) &&


I think that these changes are not needed.

The comment has been addressed in this commit.

tenzen-y · 2024-04-04T16:51:42Z

pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient_test.go

@@ -578,8 +578,8 @@ func TestConvertTrialObservation(t *testing.T) {
 	}
 	for _, tc := range tcs {


Suggested change

for _, tc := range tcs {

for _, tc := range tcs {

t.Run(testDescription, func(T *t.testing){

The comment has been addressed in this commit.

tenzen-y · 2024-04-04T16:52:23Z

pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient_test.go

-		if !reflect.DeepEqual(actualObservation, tc.expectedObservation) {
-			t.Errorf("Case: %v failed.\nExpected observation: %v \ngot: %v", tc.testDescription, tc.expectedObservation, actualObservation)
+		if diff := cmp.Diff(actualObservation, tc.expectedObservation); diff != "" {
+			t.Errorf("Case: %v failed. Diff (-want,+got):\n%s", tc.testDescription, diff)


Suggested change

t.Errorf("Case: %v failed. Diff (-want,+got):\n%s", tc.testDescription, diff)

t.Errorf("Unexpected observation (-want,+got):\n%s", diff)

The comment has been addressed in this commit.

tenzen-y · 2024-04-04T16:53:48Z

pkg/suggestion/v1beta1/goptuna/converter.go

@@ -311,7 +311,7 @@ func toGoptunaParams(
 			}
 			ir := float64(p)
 			internalParams[name] = ir
-			// externalParams[name] = p is prohibited because of reflect.DeepEqual() will be false
+			// externalParams[name] = p is prohibited because of cmp.Diff() will not be empty


I could not follow this. Could you clarify?

The comment has been addressed in this commit.

tenzen-y · 2024-04-04T16:54:44Z

pkg/suggestion/v1beta1/goptuna/sample.go

-		if reflect.DeepEqual(ktrial.Params, trials[i].Params) {
+		if diff := cmp.Diff(ktrial.Params, trials[i].Params); diff == "" {


This change is not needed.

The comment has been addressed in this commit.

tenzen-y · 2024-04-06T18:40:24Z

@tariq-hasan You should follow this guide to pass DCO check.

https://github.com/kubeflow/katib/pull/2289/checks?check_run_id=23472655010

tenzen-y

Thank you for this update!
I left some comments.

tenzen-y · 2024-04-10T13:50:23Z

pkg/controller.v1beta1/experiment/manifest/generator.go

@@ -86,7 +96,7 @@ func (g *DefaultGenerator) GetRunSpecWithHyperParameters(experiment *experiments
 	// Convert Trial template to unstructured
 	runSpec, err := util.ConvertStringToUnstructured(replacedTemplate)
 	if err != nil {
-		return nil, fmt.Errorf("ConvertStringToUnstructured failed: %v", err)
+		return nil, fmt.Errorf("%w: %s", errConvertStringToUnstructuredFailed, err.Error())


Suggested change

return nil, fmt.Errorf("%w: %s", errConvertStringToUnstructuredFailed, err.Error())

return nil, fmt.Errorf("%w: %w", errConvertStringToUnstructuredFailed, err)

Could you just wrap errors?

This has been fixed in this commit.

tenzen-y · 2024-04-10T13:50:37Z

pkg/controller.v1beta1/experiment/manifest/generator.go

@@ -108,7 +118,7 @@ func (g *DefaultGenerator) applyParameters(experiment *experimentsv1beta1.Experi
 	if trialSpec == nil {
 		trialSpec, err = util.ConvertStringToUnstructured(trialTemplate)
 		if err != nil {
-			return "", fmt.Errorf("ConvertStringToUnstructured failed: %v", err)
+			return "", fmt.Errorf("%w: %s", errConvertStringToUnstructuredFailed, err.Error())


Suggested change

return "", fmt.Errorf("%w: %s", errConvertStringToUnstructuredFailed, err.Error())

return "", fmt.Errorf("%w: %w", errConvertStringToUnstructuredFailed, err)

Same as above.

tenzen-y · 2024-04-10T13:51:07Z

pkg/controller.v1beta1/experiment/manifest/generator.go

@@ -194,20 +205,20 @@ func (g *DefaultGenerator) GetTrialTemplate(instance *experimentsv1beta1.Experim
 	if trialSource.TrialSpec != nil {
 		trialTemplateString, err = util.ConvertUnstructuredToString(trialSource.TrialSpec)
 		if err != nil {
-			return "", fmt.Errorf("ConvertUnstructuredToString failed: %v", err)
+			return "", fmt.Errorf("%w: %s", errConvertUnstructuredToStringFailed, err.Error())


Suggested change

return "", fmt.Errorf("%w: %s", errConvertUnstructuredToStringFailed, err.Error())

return "", fmt.Errorf("%w: %w", errConvertUnstructuredToStringFailed, err)

Same as above.

tenzen-y · 2024-04-10T13:51:24Z

pkg/controller.v1beta1/experiment/manifest/generator.go

 		}
 	} else {
 		configMapNS := trialSource.ConfigMap.ConfigMapNamespace
 		configMapName := trialSource.ConfigMap.ConfigMapName
 		templatePath := trialSource.ConfigMap.TemplatePath
 		configMap, err := g.client.GetConfigMap(configMapName, configMapNS)
 		if err != nil {
-			return "", fmt.Errorf("GetConfigMap failed: %v", err)
+			return "", fmt.Errorf("%w: %v", errConfigMapNotFound, err)


Suggested change

return "", fmt.Errorf("%w: %v", errConfigMapNotFound, err)

return "", fmt.Errorf("%w: %w", errConfigMapNotFound, err)

Same as above.

tenzen-y · 2024-04-10T13:55:01Z

pkg/controller.v1beta1/experiment/manifest/generator_test.go

@@ -245,18 +237,17 @@ spec:

 	expectedRunSpec, err := util.ConvertStringToUnstructured(expectedStr)
 	if err != nil {


I see. So, could we use this error? https://github.com/kubernetes/apimachinery/blob/e696ec55a32e2177cd5f2fe1ded91d9485aa5c64/pkg/util/yaml/decoder.go#L255

If it is not working well, could you define a dedicated error as well?

katib/pkg/controller.v1beta1/util/unstructured.go

Line 48 in 7df05c2

func ConvertStringToUnstructured(in string) (*unstructured.Unstructured, error) {

tenzen-y · 2024-04-10T14:06:05Z

pkg/webhook/v1beta1/pod/inject_webhook_test.go

 			}
-		}
+			if err == nil && !equality.Semantic.DeepEqual(tc.Pod.Spec.Containers, tc.WantPod.Spec.Containers) {


Could you apply the cmp approach here as well?

Same as above.

tenzen-y · 2024-04-10T14:06:42Z

pkg/webhook/v1beta1/pod/inject_webhook_test.go

+	if err == nil && !equality.Semantic.DeepEqual(tc.Pod, tc.WantPod) {
+		t.Errorf("Unexpected error from mutateMetricsCollectorVolume, expected pod: %v, got: %v", tc.WantPod, tc.Pod)


Could you apply the cmp approach here as well?

Same as above.

tenzen-y · 2024-04-10T14:07:40Z

pkg/webhook/v1beta1/pod/inject_webhook_test.go

 		},
 	}

-	for _, tc := range testCases {
+	for _, tc := range cases {
 		collectorKind := getSidecarContainerName(tc.CollectorKind)


Suggested change

collectorKind := getSidecarContainerName(tc.CollectorKind)

t.Run(...

collectorKind := getSidecarContainerName(tc.CollectorKind)

Could we use the t.Run?

Same as above.

tenzen-y · 2024-04-10T14:09:53Z

pkg/webhook/v1beta1/pod/inject_webhook_test.go

 		},
 	}

-	for _, tc := range testCases {
+	for name, tc := range cases {
 		isPrimary := isPrimaryPod(tc.podLabels, tc.primaryPodLabels)


Suggested change

isPrimary := isPrimaryPod(tc.podLabels, tc.primaryPodLabels)

t.Run(...

isPrimary := isPrimaryPod(tc.podLabels, tc.primaryPodLabels)

Could we use the t.Run?

Same as above.

tenzen-y · 2024-04-10T14:10:14Z

pkg/webhook/v1beta1/pod/inject_webhook_test.go

 		},
 	}

-	for _, tc := range testCases {
+	for _, tc := range cases {
 		mutatePodMetadata(tc.pod, tc.trial)


Suggested change

mutatePodMetadata(tc.pod, tc.trial)

t.Run(...

mutatePodMetadata(tc.pod, tc.trial)

ditto.

Same as above.

tenzen-y

@tariq-hasan Thank you for this update!
Otherwise lgtm.
I left a comment for one suggestion.

tenzen-y · 2024-04-15T18:22:31Z

pkg/controller.v1beta1/experiment/manifest/generator_test.go

@@ -245,18 +237,17 @@ spec:

 	expectedRunSpec, err := util.ConvertStringToUnstructured(expectedStr)
 	if err != nil {


@tariq-hasan I was missing any context here. Sorry for your confusion.
I meant like this.

if diff := cmp.Diff(nil, err, cmpopts.EquateErrors()); len(diff) != 0 { t.Errorf("ConvertStringToUnstructured failed (-want,+got):\n%s", diff) }

tenzen-y · 2024-04-15T18:26:39Z

pkg/controller.v1beta1/experiment/manifest/generator_test.go

+		Instance                       *experimentsv1beta1.Experiment
+		ParameterAssignments           []commonapiv1beta1.ParameterAssignment


Suggested change

Instance *experimentsv1beta1.Experiment

ParameterAssignments []commonapiv1beta1.ParameterAssignment

instance *experimentsv1beta1.Experiment

parameterAssignments []commonapiv1beta1.ParameterAssignment

I found these fields are exported. We should not export test fields at all points.
But this is irrelevant to this PR. So, could you open dedicated PR so that we can avoid to export test fields.

I can open a separate PR once this one is merged.

tenzen-y · 2024-04-16T05:12:32Z

@tariq-hasan LGTM, but the latest commit seems to violate DCO.

tariq-hasan · 2024-04-16T05:13:54Z

@tariq-hasan LGTM, but the latest commit seems to violate DCO.

I addressed the issue. I am wondering if I should squash the commits given that the changes are final now.

tenzen-y · 2024-04-16T05:15:52Z

@tariq-hasan LGTM, but the latest commit seems to violate DCO.

I addressed the issue. I am wondering if I should squash the commits given that the changes are final now.

The prow automatically squash commits into one. So, I'm ok with either way.

tariq-hasan · 2024-04-16T05:47:41Z

Makes sense.

After reviewing the PR I find that these are the final set of changes

introduce error constants
replace reflect with cmp for error comparison
define test cases as maps instead of slices
rewrote test cases for TestGetRunSpecWithHP

I was thinking if each of these should ideally be its own commit so that we maintain a clear separation of concerns.

tenzen-y · 2024-04-16T05:52:14Z

Makes sense.

After reviewing the PR I find that these are the final set of changes

introduce error constants

replace reflect with cmp for error comparison

define test cases as maps instead of slices

rewrote test cases for TestGetRunSpecWithHP

I was thinking if each of these should ideally be its own commit so that we maintain a clear separation of concerns.

I think that those sets depend on each. So, those should be included in a single PR.

Note that: We have no way to select the merge method merge. The prow enforces the squash method.

tariq-hasan · 2024-04-16T05:53:39Z

Makes sense.

tenzen-y · 2024-04-16T05:54:03Z

@tariq-hasan Could you address this error: https://github.com/kubeflow/katib/actions/runs/8700291109/job/23861062475?pr=2289?

tenzen-y · 2024-04-16T05:56:30Z

@tariq-hasan Could you address this error: https://github.com/kubeflow/katib/actions/runs/8700291109/job/23861062475?pr=2289?

I guess that we could avoid those errors after you rebase this PR.

tariq-hasan · 2024-04-16T05:58:27Z

Okay. I thought that these might have been flakiness issues as the CI workflow seems to pass and fail quite unpredictably for this test case.

I am rebasing the PR.

tenzen-y · 2024-04-16T06:09:58Z

Okay. I thought that these might have been flakiness issues as the CI workflow seems to pass and fail quite unpredictably for this test case.

I am rebasing the PR.

No, actually these errors were brought to us by the kubernetes issue w/o supporting SSA for the fake client.
And, I addressed that issue recently in the previous katib PR.
So, rebasing should fix those errors.

tariq-hasan · 2024-04-16T06:11:09Z

Got it.

tenzen-y · 2024-04-16T06:17:24Z

@tariq-hasan Uhm, it seems that meaningful errors occur. Could you investigate them?

tariq-hasan · 2024-04-16T06:19:53Z

I am working through them now.

tenzen-y · 2024-04-16T06:20:48Z

I am working through them now.

Thank you!

tariq-hasan · 2024-04-16T06:39:57Z

I did run into the issues here before.

The errors are not always reproducible however. I usually get these errors in the first one to three runs of make test on my local and then the errors disappear for subsequent runs.

If I remove the kube-apiserver and etcd processes from my local and run make test then I find that the errors sometimes reappear.

That said I now restarted my local and I am running make test ENVTEST_K8S_VERSION=1.27 (or 1.28 or 1.29) but I do not see the issues appearing.

In the few cases where I ran into the errors I did root-cause that the errors were due to the order of execution of this code with the order of execution for this code.

Not sure if you have any thoughts of what might be happening here.

tariq-hasan · 2024-06-14T20:26:10Z

Hi @tariq-hasan, are you still working on this PR ?

Hi @andreyvelich I will try to complete it ASAP. Sorry about the delay.

tariq-hasan · 2024-06-25T00:29:53Z

@tariq-hasan I merged #2316. So could you rebase this PR?

I rebased the PR.

I am now working on fixing the error in generator_test.go.

As a side note, I had to revert the change in suggestionclient_test.go as comparison using cmp threw the following error:

panic: cannot handle unexported field at {*api_v1_beta1.Observation}.state:
        "github.com/kubeflow/katib/pkg/apis/manager/v1beta1".Observation
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]

tariq-hasan · 2024-06-29T17:39:39Z

I added this commit to tie up each mock method call with the execution of the associated test case.

Please let me know if you have any feedback and if this would fix the issue.

tenzen-y

Basically, lgtm
Thank you for doing this!

tenzen-y · 2024-08-15T17:35:53Z

pkg/controller.v1beta1/experiment/manifest/generator_test.go

-		err                  bool
-		testDescription      string
+	cases := map[string]struct {
+		prepareConfigMap               func()


Suggested change

prepareConfigMap func()

mockConfigMapGetter *gomock.Call

Could you use a more strict type?

tenzen-y · 2024-08-15T17:36:21Z

pkg/controller.v1beta1/experiment/manifest/generator_test.go

+			prepareConfigMap: func() {
+				c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
+					map[string]string{templatePath: trialSpec}, nil)
+			},


Suggested change

prepareConfigMap: func() {

c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(

map[string]string{templatePath: trialSpec}, nil)

},

mockConfigMapGetter: c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(

map[string]string{templatePath: trialSpec}, nil)

tenzen-y · 2024-08-15T17:37:56Z

pkg/controller.v1beta1/experiment/manifest/generator_test.go

+			prepareConfigMap: func() {
+				c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
+					map[string]string{templatePath: trialSpec}, nil)
+			},


TBH, I think that we should use the client-go fake client instead of go mock.
But, that is another problem. So, could you open an issue to replace go mock with fake client-go?

Done. Ref: #2408

tenzen-y · 2024-08-18T10:54:46Z

@tariq-hasan Thank you for the updates.
Could you address the test error? Otherwise lgtm.

Signed-off-by: tariq-hasan <[email protected]>

tariq-hasan · 2024-08-18T17:29:20Z

@tariq-hasan Thank you for the updates. Could you address the test error? Otherwise lgtm.

I addressed the error by retaining the strict type checking while wrapping the mock method call within a function to ensure that the ordering of the mock method calls aligns with the test execution cases.

tariq-hasan · 2024-08-18T17:50:56Z

/rerun-all

tenzen-y

Thank you for the update!
This was not a trivial task, great contributions!

/lgtm
/approve

google-oss-prow · 2024-08-18T18:32:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tenzen-y]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tariq-hasan · 2024-08-19T02:32:08Z

Thank you for kindly helping with the PR.

google-oss-prow bot requested review from andreyvelich, anencore94 and johnugeorge March 17, 2024 23:41

google-oss-prow bot added the size/XL label Mar 17, 2024

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch from 25bea40 to 64c60db Compare March 17, 2024 23:48

tenzen-y reviewed Apr 4, 2024

View reviewed changes

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch 2 times, most recently from 4c6db3c to 4b174b6 Compare April 7, 2024 20:08

tenzen-y reviewed Apr 10, 2024

View reviewed changes

tenzen-y reviewed Apr 15, 2024

View reviewed changes

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch from e8afe14 to ce09054 Compare April 16, 2024 05:13

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch from ce09054 to 6d33aa5 Compare April 16, 2024 06:07

tariq-hasan mentioned this pull request Apr 25, 2024

Make test fields private in Go unit tests #2316

Merged

1 task

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch from 6d33aa5 to 87a6abc Compare April 26, 2024 03:58

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch 3 times, most recently from 2480ef5 to a578685 Compare June 25, 2024 00:26

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch 3 times, most recently from fa2cfa2 to 4e1acc7 Compare June 29, 2024 17:30

tenzen-y reviewed Aug 15, 2024

View reviewed changes

tariq-hasan mentioned this pull request Aug 18, 2024

Replace go mock with client-go fake client in unit tests #2408

Open

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch 4 times, most recently from 0c34001 to c8b7f49 Compare August 18, 2024 07:03

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch from c8b7f49 to f058446 Compare August 18, 2024 17:22

tariq-hasan added 2 commits August 18, 2024 13:23

introduced error constants and replaced reflect with cmp

7648fb7

Signed-off-by: tariq-hasan <[email protected]>

fix order of mock method calls

0cd9b85

Signed-off-by: tariq-hasan <[email protected]>

tariq-hasan force-pushed the replace-reflect.DeepEqual-with-cmp.Diff branch from f058446 to 0cd9b85 Compare August 18, 2024 17:23

tenzen-y reviewed Aug 18, 2024

View reviewed changes

google-oss-prow bot assigned tenzen-y Aug 18, 2024

google-oss-prow bot added the lgtm label Aug 18, 2024

google-oss-prow bot added the approved label Aug 18, 2024

google-oss-prow bot merged commit abd1c42 into kubeflow:master Aug 18, 2024
63 checks passed

tariq-hasan deleted the replace-reflect.DeepEqual-with-cmp.Diff branch August 19, 2024 02:31

		@@ -245,18 +237,17 @@ spec:

		expectedRunSpec, err := util.ConvertStringToUnstructured(expectedStr)
		if err != nil {

	if err != nil {
	if diff := cmp.Diff(errConvertStringToUnstructuredFailed, err, cmpopts.EquateErrors()); len(diff) != 0 {

	t.Errorf("%v: %v", ErrConvertStringToUnstructuredFailed, err)
	t.Errorf("Unexpected error from ConvertStringToUnstructured (-want,+got):\n%s", diff)

		@@ -578,8 +578,8 @@ func TestConvertTrialObservation(t *testing.T) {
		}
		for _, tc := range tcs {

	for _, tc := range tcs {
	for _, tc := range tcs {
	t.Run(testDescription, func(T *t.testing){

	t.Errorf("Case: %v failed. Diff (-want,+got):\n%s", tc.testDescription, diff)
	t.Errorf("Unexpected observation (-want,+got):\n%s", diff)

		if reflect.DeepEqual(ktrial.Params, trials[i].Params) {
		if diff := cmp.Diff(ktrial.Params, trials[i].Params); diff == "" {

	return nil, fmt.Errorf("%w: %s", errConvertStringToUnstructuredFailed, err.Error())
	return nil, fmt.Errorf("%w: %w", errConvertStringToUnstructuredFailed, err)

	return "", fmt.Errorf("%w: %s", errConvertStringToUnstructuredFailed, err.Error())
	return "", fmt.Errorf("%w: %w", errConvertStringToUnstructuredFailed, err)

	return "", fmt.Errorf("%w: %v", errConfigMapNotFound, err)
	return "", fmt.Errorf("%w: %w", errConfigMapNotFound, err)

		if err == nil && !equality.Semantic.DeepEqual(tc.Pod, tc.WantPod) {
		t.Errorf("Unexpected error from mutateMetricsCollectorVolume, expected pod: %v, got: %v", tc.WantPod, tc.Pod)

	collectorKind := getSidecarContainerName(tc.CollectorKind)
	t.Run(...
	collectorKind := getSidecarContainerName(tc.CollectorKind)

	isPrimary := isPrimaryPod(tc.podLabels, tc.primaryPodLabels)
	t.Run(...
	isPrimary := isPrimaryPod(tc.podLabels, tc.primaryPodLabels)

	mutatePodMetadata(tc.pod, tc.trial)
	t.Run(...
	mutatePodMetadata(tc.pod, tc.trial)

		Instance *experimentsv1beta1.Experiment
		ParameterAssignments []commonapiv1beta1.ParameterAssignment

Introduced error constants and replaced reflect with cmp #2289

Introduced error constants and replaced reflect with cmp #2289

Conversation

tariq-hasan commented Mar 17, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq-hasan Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq-hasan Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Apr 6, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq-hasan Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Apr 16, 2024

tariq-hasan commented Apr 16, 2024

tenzen-y commented Apr 16, 2024

tariq-hasan commented Apr 16, 2024

tenzen-y commented Apr 16, 2024

tariq-hasan commented Apr 16, 2024

tenzen-y commented Apr 16, 2024

tenzen-y commented Apr 16, 2024

tariq-hasan commented Apr 16, 2024

tenzen-y commented Apr 16, 2024 • edited Loading

tariq-hasan commented Apr 16, 2024

tenzen-y commented Apr 16, 2024

tariq-hasan commented Apr 16, 2024

tenzen-y commented Apr 16, 2024

tariq-hasan commented Apr 16, 2024

tariq-hasan commented Jun 14, 2024 • edited Loading

tariq-hasan commented Jun 25, 2024 • edited Loading

tariq-hasan commented Jun 29, 2024 • edited Loading

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq-hasan Apr 5, 2024 •

edited

Loading

tariq-hasan Apr 11, 2024 •

edited

Loading

tariq-hasan Apr 12, 2024 •

edited

Loading

tenzen-y commented Apr 16, 2024 •

edited

Loading

tariq-hasan commented Jun 14, 2024 •

edited

Loading

tariq-hasan commented Jun 25, 2024 •

edited

Loading

tariq-hasan commented Jun 29, 2024 •

edited

Loading

tariq-hasan Aug 18, 2024 •

edited

Loading