Skip to content
New issue

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

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

Already on GitHub? # to your account

Add new test for deepcopy #51

Merged
merged 1 commit into from
May 31, 2017
Merged

Conversation

spzala
Copy link
Member

@spzala spzala commented Apr 30, 2017

Add new test convering servicecatalog types related to changes made under
#46

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 30, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@spzala
Copy link
Member Author

spzala commented Apr 30, 2017

/cc @thockin - Hi Tim, does this make sense in regards to discussion we had in #46 Thanks!!

@thockin
Copy link
Member

thockin commented Apr 30, 2017

I appreciate the followup. Can we narrow this down and rename it to reflect the type that is being demonstrated. Specifically, a struct with a single member field that triggers the rendered code - that way each output test is trivial to look at and decide if changes are correct. It might mean 3 or 4 individual new dirs..

@spzala
Copy link
Member Author

spzala commented Apr 30, 2017

@thockin - yrw and sure, that makes sense. I will update the changes. Thanks!!

@spzala spzala force-pushed the newtestifelselint branch 2 times, most recently from 4c78c60 to 331d9bf Compare May 2, 2017 18:34
@spzala
Copy link
Member Author

spzala commented May 2, 2017

@thockin - Hi Tim, as you could tell I am new to Go, but have tried to identify new types and added tests for them. Hope that makes sense. Thanks!!

}

type Ttest struct {
i []Inter
Copy link
Member

Choose a reason for hiding this comment

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

I think this has to be named I - non-private name.

// This is a test package.
package interfaces

type Inter interface {
Copy link
Member

Choose a reason for hiding this comment

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

What does Inter mean?

in, out := &in.Types, &out.Types
*out = make(map[string]*Ttest)
for key, val := range *in {
newVal, err := c.DeepCopy(&val)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we expect this to hit the Cloner path?

@spzala spzala force-pushed the newtestifelselint branch from 331d9bf to aa0cba9 Compare May 23, 2017 21:15
Add new tests convering types related to changes made under
kubernetes#46
@spzala spzala force-pushed the newtestifelselint branch from aa0cba9 to a505af0 Compare May 23, 2017 21:17
@thockin
Copy link
Member

thockin commented May 31, 2017

LGTM!

@thockin thockin merged commit 5c4c3e9 into kubernetes:master May 31, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants