Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

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 crmejia commented Jul 6, 2017

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

@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 6, 2017
@arschles
Copy link
Contributor

arschles commented Jul 6, 2017

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

@crmejia
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
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

pmorie commented Jul 21, 2017

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 21, 2017
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 31, 2017

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

Choose a reason for hiding this comment

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

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
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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants