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

Initial work on unit test for ./pkg/rest/core/fake rest client, addresses #860 #1009

Closed
wants to merge 8 commits into from

Conversation

@crmejia
Copy link
Contributor

crmejia commented Jul 6, 2017

Not finished yet. Looking for some early feedback and direction.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 6, 2017

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@arschles

This comment has been minimized.

Copy link
Contributor

arschles commented Jul 6, 2017

@crmejia great start. Do you have plans to write more tests in here?

@crmejia

This comment has been minimized.

Copy link
Contributor Author

crmejia commented Jul 6, 2017

Yes, I plan to test the other functions and methods. Just not sure how to test func watchItem or func watchList

@arschles

This comment has been minimized.

Copy link
Contributor

arschles commented Jul 7, 2017

@crmejia ok, sounds good. please ping me when you are ready for another review

f(rw, request)

if rw.getResponse().StatusCode != http.StatusOK && rw.headerSet {
t.Fatal("Expected Status OK", http.StatusOK, "got", rw.getResponse().StatusCode)

This comment has been minimized.

Copy link
@crmejia

crmejia Jul 10, 2017

Author Contributor

This test is failing rest_client_test.go:162: Expected Status OK 200 got 404
Not sure why. My current URL is /apis/servicecatalog.k8s.io/v1alpha1/namespaces/ns1/tipe1/name1

This comment has been minimized.

Copy link
@arschles

arschles Jul 12, 2017

Contributor

@crmejia this is failing because of the following code in the anonymous function that getItem returns:

ns := mux.Vars(r)["namespace"]
tipe := mux.Vars(r)["type"]
name := mux.Vars(r)["name"]

The return values of those Vars calls are empty because the keys ("namespace", "type", "name") are not registered with gorilla mux. The way to fix this is to register the route /apis/servicecatalog.k8s.io/v1alpha1/namespaces/{namespace}/{type}/{name} with a new mux router, and then do a request to the gorilla mux router instead of directly to the handler.

Please let me know if that doesn't make sense, and I'm happy to go into more detail.

@pmorie

This comment has been minimized.

Copy link
Member

pmorie commented Jul 21, 2017

@crmejia do you plan to try to get this in?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 31, 2017

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.


for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
request, err := http.NewRequest("POST", tc.url, nil)

This comment has been minimized.

Copy link
@crmejia

crmejia Aug 2, 2017

Author Contributor

I still need to put the object in the body. Not sure how yet. Would something like this work?
/Users/crismarmejia/go/src/github.com/kubernetes-incubator/service-catalog/pkg/rest/core/fake/rest_client.go:321 +0x2b8

ps. I've noticed that users of the rest_client.go are not using the router but the methods on storage directly. Should I stop testing these functions and mark as the issue as completed(and move to other things)?

crmejia and others added 8 commits Jul 6, 2017
Crismar Mejia
Crismar Mejia
Crismar Mejia
Crismar Mejia
Crismar Mejia
Crismar Mejia
@crmejia

This comment has been minimized.

Copy link
Contributor Author

crmejia commented Aug 8, 2017

I'm closing this PR to see If it fixes the issue with the cla. I signed the cla with a different address than the one I use for GitHub. I updated my GH account but the issue persists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.