add test for AddPVCToDeploymentConfig #551

syamgk · 2018-06-28T09:46:47Z

add test for AddPVCToDeploymentConfig

+ fixing the format mismatch

syamgk · 2018-06-29T09:44:56Z

as of now, this is failing on panic error for

- Test case : dc without container
 -Test case : dc without Template

merge #554 which will add validation for pointers before dereferencing and validate by rerunning tests and then merge

cdrage

This LGTM after #554 is merged.

cdrage · 2018-07-10T18:27:42Z

Needs re-basing, looks like a recent merge broke things.

syamgk · 2018-07-19T10:40:50Z

rebased

cdrage

LGTM

cdrage · 2018-07-19T16:13:23Z

pkg/occlient/occlient_test.go

+		t.Run(tt.name, func(t *testing.T) {
+			fakeClient, fakeClientset := FakeNew()
+
+			fakeClientset.AppClientset.PrependReactor("update", "deploymentconfigs", func(action ktesting.Action) (bool, runtime.Object, error) {


mik-dass · 2018-07-25T12:35:13Z

pkg/occlient/occlient_test.go

+			fakeClient, fakeClientset := FakeNew()
+
+			fakeClientset.AppClientset.PrependReactor("update", "deploymentconfigs", func(action ktesting.Action) (bool, runtime.Object, error) {
+				return true, nil, nil


We should return the updated dc instead of nil and verify that the dc was updated with the proper values as done in

odo/pkg/occlient/occlient_test.go

Line 1337 in e8e223b

updatedDc := fkclientset.BuildClientset.Actions()[1].(ktesting.UpdateAction).GetObject().(*buildv1.BuildConfig)

Also we should check that update was called on the correct dc by verifying against the name of the dc

odo/pkg/occlient/occlient_test.go

Lines 1323 to 1326 in e8e223b

buildConfig := action.(ktesting.UpdateAction).GetObject().(*buildv1.BuildConfig)

if buildConfig.Name != tt.buildConfigName {

return true, nil, fmt.Errorf("'update' was called with wrong buildConfig name")

}

I agree that it is good to validate the values with which the function is being called but in this specific case there is a random string generation involved here https://github.com/redhat-developer/odo/blob/master/pkg/occlient/occlient.go#L1333
so it won't be feasible.
will add validation for the number of action performed

@syamgk The random string generation is only for the volumeName and we can leave that or check that if the volumeName is generated. In the returned DC, we should test the other parts, like if the pvcClaimName is the same as the one passed to the function and also the volumeMount Path is the same as the path passed to the function.
Also we should check if the update reactor was called with the correct dc name passed to the function because we might end up calling update on the wrong dc.

added validation for the presence of mounts and with passing volume names & rebased

now ready for review

mik-dass · 2018-07-27T07:02:24Z

pkg/occlient/occlient_test.go

+			fakeClient, fakeClientset := FakeNew()
+
+			fakeClientset.AppsClientset.PrependReactor("update", "deploymentconfigs", func(action ktesting.Action) (bool, runtime.Object, error) {
+				return true, nil, nil


Also we should check if the update reactor was called with the correct dc name passed to the function like

odo/pkg/occlient/occlient_test.go

Lines 1323 to 1326 in e8e223b

buildConfig := action.(ktesting.UpdateAction).GetObject().(*buildv1.BuildConfig)

if buildConfig.Name != tt.buildConfigName {

return true, nil, fmt.Errorf("'update' was called with wrong buildConfig name")

}

because we might end up calling update on the wrong dc.

mik-dass · 2018-07-27T07:30:30Z

pkg/occlient/occlient_test.go

+				}
+				if found == false {
+					t.Errorf("expected Volume mount path %v not found", tt.args.path)
+				}


We should also have a test for this part of the code i.e the volumes, check the volume name as before and that the claimName is the same as desired

odo/pkg/occlient/occlient.go

Lines 1339 to 1346 in 4cd7e79

dc.Spec.Template.Spec.Volumes = append(dc.Spec.Template.Spec.Volumes, corev1.Volume{

Name: volumeName,

VolumeSource: corev1.VolumeSource{

PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{

ClaimName: pvc,

},

},

})

added validation for this

mik-dass

LGTM :)

cdrage · 2018-07-27T13:23:46Z

pkg/occlient/occlient_test.go

+		wantErr bool
+	}{
+		{
+			name: "Test case : valid dc",


Maybe add "Test case 1". Add numbers to each test case name

cdrage · 2018-07-27T13:24:03Z

pkg/occlient/occlient_test.go

+						},
+					},
+				},
+				pvc:  "testglocks volume",


glocks? Not too sure what this name means.

cdrage · 2018-07-27T13:24:21Z

pkg/occlient/occlient_test.go

+						},
+					},
+				},
+				pvc:  "my xx voulme",


better name maybe :). test-volume?

cdrage · 2018-07-27T13:24:38Z

pkg/occlient/occlient_test.go

+						},
+					},
+				},
+				pvc:  "my xx voulme",


test-volume

cdrage · 2018-07-27T13:26:04Z

pkg/occlient/occlient_test.go

+					t.Errorf("expected 1 action in GetPVCFromName got: %v", fakeClientset.AppsClientset.Actions())
+				}
+				updatedDc := fakeClientset.AppsClientset.Actions()[0].(ktesting.UpdateAction).GetObject().(*appsv1.DeploymentConfig)
+				// creating a flag


you should try to add some spacing between separate sections of code. For example, add just one-line of space between the checks above and this section. Makes everything a bit less clustered.

cdrage · 2018-07-27T13:26:29Z

pkg/occlient/occlient_test.go

+					if tt.args.path == updatedDc.Spec.Template.Spec.Containers[0].VolumeMounts[bb].MountPath {
+						found = true
+						if !strings.Contains(updatedDc.Spec.Template.Spec.Containers[0].VolumeMounts[bb].Name, tt.args.pvc) {
+							t.Errorf("pvc name not matching with the specived value got: %v, expected %v", updatedDc.Spec.Template.Spec.Containers[0].VolumeMounts[bb].Name, tt.args.pvc)


ashetty1

LGTM

syamgk · 2018-08-01T13:32:41Z

@cdrage Pr is updated and ready for review now

cdrage

literally the smallest nitpick ever, once that's done, LGTM / merge away!

cdrage · 2018-08-01T18:59:31Z

pkg/occlient/occlient_test.go

+			})
+			err := fakeClient.AddPVCToDeploymentConfig(tt.args.dc, tt.args.pvc, tt.args.path)
+
+			//Checks for error in positive cases


add space after //

add test for AddPVCToDeploymentConfig

syamgk added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. do not review labels Jun 28, 2018

syamgk force-pushed the tstAddpvctoDc branch from bcc6f80 to 777c356 Compare June 29, 2018 08:45

syamgk removed do not review do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Jun 29, 2018

syamgk force-pushed the tstAddpvctoDc branch 4 times, most recently from aad63fd to f49a33a Compare June 29, 2018 09:41

syamgk force-pushed the tstAddpvctoDc branch from f49a33a to c3d4c10 Compare June 29, 2018 10:22

cdrage approved these changes Jul 4, 2018

View reviewed changes

cdrage added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 10, 2018

syamgk force-pushed the tstAddpvctoDc branch from c3d4c10 to b887a38 Compare July 19, 2018 10:24

syamgk removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Jul 19, 2018

cdrage approved these changes Jul 19, 2018

View reviewed changes

mik-dass reviewed Jul 25, 2018

View reviewed changes

syamgk force-pushed the tstAddpvctoDc branch from b887a38 to 12186aa Compare July 26, 2018 10:04

mik-dass added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 26, 2018

syamgk force-pushed the tstAddpvctoDc branch 2 times, most recently from eb3ef1c to adf6998 Compare July 26, 2018 16:40

syamgk removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 26, 2018

syamgk force-pushed the tstAddpvctoDc branch 2 times, most recently from 9c54d63 to 4cd7e79 Compare July 27, 2018 05:16

mik-dass reviewed Jul 27, 2018

View reviewed changes

redhat-developer deleted a comment from syamgk Jul 27, 2018

syamgk force-pushed the tstAddpvctoDc branch 3 times, most recently from 6d7ebd0 to 3f0cb6f Compare July 27, 2018 09:49

mik-dass approved these changes Jul 27, 2018

View reviewed changes

cdrage suggested changes Jul 27, 2018

View reviewed changes

syamgk force-pushed the tstAddpvctoDc branch from 3f0cb6f to 8f08e0c Compare July 31, 2018 14:02

ashetty1 approved these changes Aug 1, 2018

View reviewed changes

mik-dass added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 1, 2018

syamgk force-pushed the tstAddpvctoDc branch from 8f08e0c to 129553a Compare August 1, 2018 13:03

syamgk removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 1, 2018

cdrage approved these changes Aug 1, 2018

View reviewed changes

add test for AddPVCToDeploymentConfig

849dd31

add test for AddPVCToDeploymentConfig

syamgk force-pushed the tstAddpvctoDc branch from 129553a to 849dd31 Compare August 2, 2018 04:15

mik-dass merged commit 4fa6a37 into redhat-developer:master Aug 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add test for AddPVCToDeploymentConfig #551

add test for AddPVCToDeploymentConfig #551

syamgk commented Jun 28, 2018 •

edited

Loading

syamgk commented Jun 29, 2018 •

edited

Loading

cdrage left a comment

cdrage commented Jul 10, 2018

syamgk commented Jul 19, 2018

cdrage left a comment

cdrage Jul 19, 2018

mik-dass Jul 25, 2018

syamgk Jul 26, 2018

mik-dass Jul 26, 2018 •

edited

Loading

syamgk Jul 26, 2018

mik-dass Jul 27, 2018

syamgk Jul 27, 2018

mik-dass Jul 27, 2018

syamgk Jul 27, 2018

mik-dass left a comment

cdrage Jul 27, 2018

cdrage Jul 27, 2018

cdrage Jul 27, 2018

cdrage Jul 27, 2018

cdrage Jul 27, 2018

cdrage Jul 27, 2018

ashetty1 left a comment

syamgk commented Aug 1, 2018

cdrage left a comment

cdrage Aug 1, 2018

	buildConfig := action.(ktesting.UpdateAction).GetObject().(*buildv1.BuildConfig)
	if buildConfig.Name != tt.buildConfigName {
	return true, nil, fmt.Errorf("'update' was called with wrong buildConfig name")
	}

	dc.Spec.Template.Spec.Volumes = append(dc.Spec.Template.Spec.Volumes, corev1.Volume{
	Name: volumeName,
	VolumeSource: corev1.VolumeSource{
	PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
	ClaimName: pvc,
	},
	},
	})

add test for AddPVCToDeploymentConfig #551

add test for AddPVCToDeploymentConfig #551

Conversation

syamgk commented Jun 28, 2018 • edited Loading

syamgk commented Jun 29, 2018 • edited Loading

cdrage left a comment

Choose a reason for hiding this comment

cdrage commented Jul 10, 2018

syamgk commented Jul 19, 2018

cdrage 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

mik-dass Jul 26, 2018 • 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

mik-dass 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

ashetty1 left a comment

Choose a reason for hiding this comment

syamgk commented Aug 1, 2018

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syamgk commented Jun 28, 2018 •

edited

Loading

syamgk commented Jun 29, 2018 •

edited

Loading

mik-dass Jul 26, 2018 •

edited

Loading