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.

Sign up for GitHub

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

Jump to bottom

Move testclient to a separate package #18229

Merged
merged 1 commit into from
Dec 10, 2015
Merged

Move testclient to a separate package #18229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
206 changes: 0 additions & 206 deletions pkg/client/unversioned/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,222 +20,16 @@ import (
"encoding/json"
"net/http"
"net/http/httptest"
"net/url"
"path"
"reflect"
"strings"
"testing"

"github.com/emicklei/go-restful/swagger"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/testapi"
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/fields"
"k8s.io/kubernetes/pkg/labels"
"k8s.io/kubernetes/pkg/runtime"
"k8s.io/kubernetes/pkg/util"
"k8s.io/kubernetes/pkg/version"
)

const nameRequiredError = "resource name may not be empty"

type testRequest struct {
Method string
Path string
Header string
Query url.Values
Body runtime.Object
RawBody *string
}

type Response struct {
StatusCode int
Body runtime.Object
RawBody *string
}

type testClient struct {
*Client
Request testRequest
Response Response
Error bool
Created bool
server *httptest.Server
handler *util.FakeHandler
// For query args, an optional function to validate the contents
// useful when the contents can change but still be correct.
// Maps from query arg key to validator.
// If no validator is present, string equality is used.
QueryValidator map[string]func(string, string) bool
}

func (c *testClient) Setup(t *testing.T) *testClient {
c.handler = &util.FakeHandler{
StatusCode: c.Response.StatusCode,
}
if responseBody := body(t, c.Response.Body, c.Response.RawBody); responseBody != nil {
c.handler.ResponseBody = *responseBody
}
c.server = httptest.NewServer(c.handler)
if c.Client == nil {
c.Client = NewOrDie(&Config{
Host: c.server.URL,
GroupVersion: testapi.Default.GroupVersion(),
})

// TODO: caesarxuchao: hacky way to specify version of Experimental client.
// We will fix this by supporting multiple group versions in Config
c.ExtensionsClient = NewExtensionsOrDie(&Config{
Host: c.server.URL,
GroupVersion: testapi.Extensions.GroupVersion(),
})
}
c.QueryValidator = map[string]func(string, string) bool{}
return c
}

func (c *testClient) Validate(t *testing.T, received runtime.Object, err error) {
c.ValidateCommon(t, err)

if c.Response.Body != nil && !api.Semantic.DeepDerivative(c.Response.Body, received) {
t.Errorf("bad response for request %#v: expected %#v, got %#v", c.Request, c.Response.Body, received)
}
}

func (c *testClient) ValidateRaw(t *testing.T, received []byte, err error) {
c.ValidateCommon(t, err)

if c.Response.Body != nil && !reflect.DeepEqual(c.Response.Body, received) {
t.Errorf("bad response for request %#v: expected %#v, got %#v", c.Request, c.Response.Body, received)
}
}

func (c *testClient) ValidateCommon(t *testing.T, err error) {
defer c.server.Close()

if c.Error {
if err == nil {
t.Errorf("error expected for %#v, got none", c.Request)
}
return
}
if err != nil {
t.Errorf("no error expected for %#v, got: %v", c.Request, err)
}

if c.handler.RequestReceived == nil {
t.Errorf("handler had an empty request, %#v", c)
return
}

requestBody := body(t, c.Request.Body, c.Request.RawBody)
actualQuery := c.handler.RequestReceived.URL.Query()
t.Logf("got query: %v", actualQuery)
t.Logf("path: %v", c.Request.Path)
// We check the query manually, so blank it out so that FakeHandler.ValidateRequest
// won't check it.
c.handler.RequestReceived.URL.RawQuery = ""
c.handler.ValidateRequest(t, path.Join(c.Request.Path), c.Request.Method, requestBody)
for key, values := range c.Request.Query {
validator, ok := c.QueryValidator[key]
if !ok {
switch key {
case unversioned.LabelSelectorQueryParam(testapi.Default.GroupVersion().String()):
validator = validateLabels
case unversioned.FieldSelectorQueryParam(testapi.Default.GroupVersion().String()):
validator = validateFields
default:
validator = func(a, b string) bool { return a == b }
}
}
observed := actualQuery.Get(key)
wanted := strings.Join(values, "")
if !validator(wanted, observed) {
t.Errorf("Unexpected query arg for key: %s. Expected %s, Received %s", key, wanted, observed)
}
}
if c.Request.Header != "" {
if c.handler.RequestReceived.Header.Get(c.Request.Header) == "" {
t.Errorf("header %q not found in request %#v", c.Request.Header, c.handler.RequestReceived)
}
}

if expected, received := requestBody, c.handler.RequestBody; expected != nil && *expected != received {
t.Errorf("bad body for request %#v: expected %s, got %s", c.Request, *expected, received)
}
}

// buildResourcePath is a convenience function for knowing if a namespace should be in a path param or not
func buildResourcePath(namespace, resource string) string {
if len(namespace) > 0 {
return path.Join("namespaces", namespace, resource)
}
return resource
}

// buildQueryValues is a convenience function for knowing if a namespace should be in a query param or not
func buildQueryValues(query url.Values) url.Values {
v := url.Values{}
if query != nil {
for key, values := range query {
for _, value := range values {
v.Add(key, value)
}
}
}
return v
}

func validateLabels(a, b string) bool {
sA, eA := labels.Parse(a)
if eA != nil {
return false
}
sB, eB := labels.Parse(b)
if eB != nil {
return false
}
return sA.String() == sB.String()
}

func validateFields(a, b string) bool {
sA, _ := fields.ParseSelector(a)
sB, _ := fields.ParseSelector(b)
return sA.String() == sB.String()
}

func body(t *testing.T, obj runtime.Object, raw *string) *string {
if obj != nil {
fqKind, err := api.Scheme.ObjectKind(obj)
if err != nil {
t.Errorf("unexpected encoding error: %v", err)
}
// TODO: caesarxuchao: we should detect which group an object belongs to
// by using the version returned by Schem.ObjectVersionAndKind() once we
// split the schemes for internal objects.
// TODO: caesarxuchao: we should add a map from kind to group in Scheme.
var bs []byte
if api.Scheme.Recognizes(testapi.Default.GroupVersion().WithKind(fqKind.Kind)) {
bs, err = testapi.Default.Codec().Encode(obj)
if err != nil {
t.Errorf("unexpected encoding error: %v", err)
}
} else if api.Scheme.Recognizes(testapi.Extensions.GroupVersion().WithKind(fqKind.Kind)) {
bs, err = testapi.Extensions.Codec().Encode(obj)
if err != nil {
t.Errorf("unexpected encoding error: %v", err)
}
} else {
t.Errorf("unexpected kind: %v", fqKind.Kind)
}
body := string(bs)
return &body
}
return raw
}

func TestGetServerVersion(t *testing.T) {
expect := version.Info{
Major: "foo",
Expand Down
49 changes: 27 additions & 22 deletions pkg/client/unversioned/daemon_sets_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package unversioned
package unversioned_test
Copy link
Contributor

Choose a reason for hiding this comment

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

unversionedTest?

We do not use underscores in go.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the blackbox testing feature. https://splice.com/blog/lesser-known-features-go-test/

And for the package name, I think the convention in this project is using under score.

Copy link
Contributor

Choose a reason for hiding this comment

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

And for the package name, I think the convention in this project is using under score.

Where is the convention documented. Is there any other pkgs has underscore in names in k8s?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it probably is a Go convention. If you git grep "^package .*_.*" in the repo you will find many examples.

A black-box test example: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/deep_copy_test.go#L17

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. The pkg naming convention is not using underscore in go.
Please check https://golang.org/doc/effective_go.html#package-names.

I cannot see why you add the _test prefix. Test files that declare a package with the suffix "_test" will be compiled as a separate package, and then linked and run with the main test binary. The main reason we want to put it into a different pkg is to avoid circle import I think.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I can't seem to find the official go documentation of this feature, but here is a decent explanation, scroll down to "black-box test package" https://splice.com/blog/lesser-known-features-go-test/

Copy link
Contributor

Choose a reason for hiding this comment

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

@lavalamp OK. I did not notice there is an import cycle by my eye. :( The unversion pkg is named to client.

Sorry, I can't seem to find the official go documentation of this feature, but here is a decent explanation, scroll down to "black-box test package" https://splice.com/blog/lesser-known-features-go-test/

The official doc is at https://golang.org/cmd/go/#hdr-Test_packages.

It is the same as I mentioned 3 replies above.

Copy link
Member

Choose a reason for hiding this comment

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

Ah gotcha, very sorry, clearly I did not read the above very carefully at all :)

btw, I believe your suggestion of "unversionedTest" will not compile, since there are other files in package "unversioned" in the directory.

But ordinarily, I agree that we wouldn't use "_" in a package name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to emphasize @xiang90's point: https://golang.org/doc/effective_go.html#package-names: "there should be no need for underscores or mixedCaps". though this rule is not strictly followed in our codebase.

"_test" is a special case. And yes, we need to use it to break cyclic imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, I believe your suggestion of "unversionedTest" will not compile,

Yea... I was wrong for the beginning. Did not take a very close look at the beginning.


import (
. "k8s.io/kubernetes/pkg/client/unversioned"
"k8s.io/kubernetes/pkg/client/unversioned/testclient/simple"
)

import (
"testing"
Expand All @@ -31,12 +36,12 @@ func getDSResourceName() string {

func TestListDaemonSets(t *testing.T) {
ns := api.NamespaceAll
c := &testClient{
Request: testRequest{
c := &simple.Client{
Request: simple.Request{
Method: "GET",
Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, ""),
},
Response: Response{StatusCode: 200,
Response: simple.Response{StatusCode: 200,
Body: &extensions.DaemonSetList{
Items: []extensions.DaemonSet{
{
Expand All @@ -62,9 +67,9 @@ func TestListDaemonSets(t *testing.T) {

func TestGetDaemonSet(t *testing.T) {
ns := api.NamespaceDefault
c := &testClient{
Request: testRequest{Method: "GET", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, "foo"), Query: buildQueryValues(nil)},
Response: Response{
c := &simple.Client{
Request: simple.Request{Method: "GET", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, "foo"), Query: simple.BuildQueryValues(nil)},
Response: simple.Response{
StatusCode: 200,
Body: &extensions.DaemonSet{
ObjectMeta: api.ObjectMeta{
Expand All @@ -86,10 +91,10 @@ func TestGetDaemonSet(t *testing.T) {

func TestGetDaemonSetWithNoName(t *testing.T) {
ns := api.NamespaceDefault
c := &testClient{Error: true}
c := &simple.Client{Error: true}
receivedPod, err := c.Setup(t).Extensions().DaemonSets(ns).Get("")
if (err != nil) && (err.Error() != nameRequiredError) {
t.Errorf("Expected error: %v, but got %v", nameRequiredError, err)
if (err != nil) && (err.Error() != simple.NameRequiredError) {
t.Errorf("Expected error: %v, but got %v", simple.NameRequiredError, err)
}

c.Validate(t, receivedPod, err)
Expand All @@ -100,9 +105,9 @@ func TestUpdateDaemonSet(t *testing.T) {
requestDaemonSet := &extensions.DaemonSet{
ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"},
}
c := &testClient{
Request: testRequest{Method: "PUT", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, "foo"), Query: buildQueryValues(nil)},
Response: Response{
c := &simple.Client{
Request: simple.Request{Method: "PUT", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, "foo"), Query: simple.BuildQueryValues(nil)},
Response: simple.Response{
StatusCode: 200,
Body: &extensions.DaemonSet{
ObjectMeta: api.ObjectMeta{
Expand All @@ -127,9 +132,9 @@ func TestUpdateDaemonSetUpdateStatus(t *testing.T) {
requestDaemonSet := &extensions.DaemonSet{
ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"},
}
c := &testClient{
Request: testRequest{Method: "PUT", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, "foo") + "/status", Query: buildQueryValues(nil)},
Response: Response{
c := &simple.Client{
Request: simple.Request{Method: "PUT", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, "foo") + "/status", Query: simple.BuildQueryValues(nil)},
Response: simple.Response{
StatusCode: 200,
Body: &extensions.DaemonSet{
ObjectMeta: api.ObjectMeta{
Expand All @@ -152,9 +157,9 @@ func TestUpdateDaemonSetUpdateStatus(t *testing.T) {

func TestDeleteDaemon(t *testing.T) {
ns := api.NamespaceDefault
c := &testClient{
Request: testRequest{Method: "DELETE", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, "foo"), Query: buildQueryValues(nil)},
Response: Response{StatusCode: 200},
c := &simple.Client{
Request: simple.Request{Method: "DELETE", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, "foo"), Query: simple.BuildQueryValues(nil)},
Response: simple.Response{StatusCode: 200},
}
err := c.Setup(t).Extensions().DaemonSets(ns).Delete("foo")
c.Validate(t, nil, err)
Expand All @@ -165,9 +170,9 @@ func TestCreateDaemonSet(t *testing.T) {
requestDaemonSet := &extensions.DaemonSet{
ObjectMeta: api.ObjectMeta{Name: "foo"},
}
c := &testClient{
Request: testRequest{Method: "POST", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, ""), Body: requestDaemonSet, Query: buildQueryValues(nil)},
Response: Response{
c := &simple.Client{
Request: simple.Request{Method: "POST", Path: testapi.Extensions.ResourcePath(getDSResourceName(), ns, ""), Body: requestDaemonSet, Query: simple.BuildQueryValues(nil)},
Response: simple.Response{
StatusCode: 200,
Body: &extensions.DaemonSet{
ObjectMeta: api.ObjectMeta{
Expand Down