Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add unit test for CreatePVC #499

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

syamgk
Copy link
Member

@syamgk syamgk commented Jun 1, 2018

add unit-test for CreatePVC

@syamgk syamgk added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 1, 2018
@syamgk syamgk force-pushed the createPVC-utest branch from dae7e82 to 08995ac Compare June 1, 2018 12:13
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few changes requested

// Checks for return values in positive cases
if err == nil {
createdPVC := fkclientset.Kubernetes.Actions()[0].(ktesting.CreateAction).GetObject().(*corev1.PersistentVolumeClaim)
// fmt.Println(createdPVC)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if !reflect.DeepEqual(createdPVC.Labels, tt.labels) {
t.Errorf("labels in created route is not matching expected labels, expected: %v, got: %v", tt.labels, createdPVC.Labels)
}
// name and size that route is pointg to should match
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointg -> pointing

// name and size that route is pointg to should match
// if createdPVC.Size != tt.size {
// t.Errorf("route is not matching to expected service name, expected: %s, got %s", tt.size, createdPVC.Size)
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove these lines of code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry this is a WIP and i still have to add checks for the size of createdPVC

@cdrage
Copy link
Member

cdrage commented Jun 4, 2018

@syamgk You need to add a description to all your commit messages as well! It helps when generating a changelog.

@syamgk syamgk force-pushed the createPVC-utest branch from 08995ac to a81988c Compare June 5, 2018 12:23
@syamgk syamgk removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 5, 2018
@syamgk syamgk force-pushed the createPVC-utest branch from a81988c to a84b192 Compare June 5, 2018 17:18
// Checks for error in positive cases
if !tt.wantErr == (err != nil) {
t.Errorf(" client.CreatePVC(name, size, labels) unexpected error %v, wantErr %v", err, tt.wantErr)
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to call return here.

// Checks for return values in positive cases
if err == nil {
createdPVC := fkclientset.Kubernetes.Actions()[0].(ktesting.CreateAction).GetObject().(*corev1.PersistentVolumeClaim)
quantity, _ := resource.ParseQuantity(tt.size)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors shouldn't be ignored

labels map[string]string
wantErr bool
}{
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should also be some tests that result in error. For example invalid size value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added one error case

add unit test for CreatePVC function
@syamgk syamgk force-pushed the createPVC-utest branch from a84b192 to 39316dc Compare June 6, 2018 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants