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 doc #612

Merged
merged 4 commits into from
Sep 10, 2018
Merged

Add unit test doc #612

merged 4 commits into from
Sep 10, 2018

Conversation

syamgk
Copy link
Member

@syamgk syamgk commented Aug 2, 2018

Add Unit test documentation to deployment.md

@syamgk syamgk added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. do not review labels Aug 2, 2018
@syamgk syamgk force-pushed the utestDoc branch 2 times, most recently from 817f1b8 to 0336400 Compare August 3, 2018 10:44
@syamgk syamgk removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Aug 7, 2018
#### Unit test for functions consuming client-go functions

We started writing unit-tests for the functions which are making API calls with client-go library
by using package fake[ ref: https://godoc.org/k8s.io/client-go/kubernetes/fake ].
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put links in markdown format instead?

We started writing unit-tests for the functions which are making API calls with client-go library
by using package fake[ ref: https://godoc.org/k8s.io/client-go/kubernetes/fake ].

There are few techniques we are using for mocking the api calls,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the tense for the entire document should be simple present tense.

by using package fake[ ref: https://godoc.org/k8s.io/client-go/kubernetes/fake ].

There are few techniques we are using for mocking the api calls,
basically mocking the actual api calls with functions defined in
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove basically

from there and using https://golang.org/pkg/testing/ for tests.


##### How to write unit tests having API calls in a nutshell?
Copy link
Contributor

Choose a reason for hiding this comment

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

Just "Writing unit tests" should be fine ...


##### How to write unit tests having API calls in a nutshell?

- Identify the API calls being made by the function during the execution
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have example code for each of these points.

Also, bullet/number them.

adding reactors for the relevent actions is the way to go with this situation.

for example take a look at `RemoveVolumeFromDeploymentConfig`
https://github.com/redhat-developer/odo/blob/master/pkg/occlient/occlient.go#L1413
Copy link
Contributor

Choose a reason for hiding this comment

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

Use [](url)

Copy link
Member Author

@syamgk syamgk Aug 24, 2018

Choose a reason for hiding this comment

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

thought explicit will be better but will go with hyperlink

}
```

so for this we can add a reactor like below,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of using RemoveVolumeFromDeploymentConfig which internally calls GetDeploymentConfigFromName, we should have examples of functions which directly make create, list and get calls.

return true, tt.dcBefore, nil
})
```

Copy link
Contributor

Choose a reason for hiding this comment

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

We need more description for this section explaining the concepts like reactor, actions, verb in more detail. We could work together on this.

Add unit test documentation into development.md
@surajnarwade surajnarwade added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 24, 2018
@syamgk syamgk removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 24, 2018
@cdrage
Copy link
Member

cdrage commented Aug 30, 2018

What'st he status of this @syamgk ?

@syamgk
Copy link
Member Author

syamgk commented Sep 5, 2018

@cdrage incorporated the comments and ready for next round of review

#### Unit test for functions consuming client-go functions

Unit-tests for the functions making API calls with client-go library
can be written by using package (fake)[https://godoc.org/k8s.io/client-go/kubernetes/fake].
Copy link
Contributor

Choose a reason for hiding this comment

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

can be written by using package (fake)[https://godoc.org/k8s.io/client-go/kubernetes/fake].

The way to mock the api calls is by mocking the actual api calls with functions defined in
(client-go/testing)[https://godoc.org/k8s.io/client-go/testing]
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

}
```

Looking at the function body and identify how many API calls it is making while execution.
Copy link
Contributor

@anmolbabu anmolbabu Sep 5, 2018

Choose a reason for hiding this comment

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

It can be observed that, the function CreateRoute above is making 1 api call as in the line below:

r, err := c.routeClient.Routes(c.namespace).Create(route)

r, err := c.routeClient.Routes(c.namespace).Create(route)
```

for example,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be ok to shorten the description as under:

From the API call extract above, it is clear that the routeClient needs to be mocked.
The mock routeClient can be obtained using pkg/occlient/fakeclient.go#FakeNew function.

It is done as under:

code here...

Note:
The FakeNew function contains mock implementations of all client sets currently in use by Odo
If the mock implementation of a client set that you are using does not exist, please add it as under:

fkclientset.RouteClientset = fakeRouteClientset.NewSimpleClientset()
client.routeClient = fkclientset.RouteClientset.Route()

and the number of actions performed on routeclientset.


Then after making that function call inside the test function,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideal list of items to validate in the unit test function are:

  • Number of actions performed or number of api calls made using the client set
    ex : len(fkclientset.RouteClientset.Actions())
  • Validate the return values of the function being tested
    ex: here

and values are being validated later.
Ref. (here)[https://github.com/redhat-developer/odo/pull/456/files#diff-54c1e3725d2cfb565cbd1cfdb02bd792R63]

For the API calls which are returning objects that are later being processed inside the function body,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this above I feel it would be nice if we could group all mocking together, all validations together etc..

and would it be nice to rephrase it as under:

Mock the api call return values using reactor functions here

For the API calls which are returning objects that are later being processed inside the function body,
adding reactors for the relevent actions is also needed.

for example take a look at `GetDeploymentConfigsFromSelector`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to convert all(as much as possible) all github links to markdown links and not expose the github urls(Github urls seem very ugly to me from the perspective of a reader :) but yes this for a special reader -- A DEVELOPER :P but still I feel it would be nice to avoid it)

Copy link
Contributor

@anmolbabu anmolbabu Sep 5, 2018

Choose a reason for hiding this comment

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

If possible can we pick one single example that covers all aspects of testing and only consider that throughout the doc?
Not a very crucial thing to be done, but I feel it would avoid context switches in reader's mind


More examples can be found in https://github.com/redhat-developer/odo/blob/master/pkg/occlient/occlient_test.go

- Reactor is an interface to allow the composition of reaction functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

May be call this section as Terminologies or something?

@ashetty1
Copy link
Contributor

ashetty1 commented Sep 5, 2018

Unit tests

Introduction

Unit-tests for ODO functions are written using package fake. This allows us to create a fake client, and then mock the API calls defined under OpenShift client-go and k8s client-go.

The tests are written in golang using (pkg/testing)[https://golang.org/pkg/testing/] package.

Writing unit tests
  1. Identify the APIs used by the function to be tested.

  2. Initialise the fake client along with the relevant clientsets.

  3. In the case of functions fetching or creating new objects through the APIs, add a reactor interface returning fake objects.

  4. Verify the objects returned

Initialising fake client and creating fake objects

Let us understand the initialisation of fake clients and thereafter creation of fake objects with an example.

The function GetImageStreams in pkg/occlient.go fetches imagestream objects through the API:

func (c *Client) GetImageStreams(namespace string) ([]imagev1.ImageStream, error) {
        imageStreamList, err := c.imageClient.ImageStreams(namespace).List(metav1.ListOptions{})
        if err != nil {
                return nil, errors.Wrap(err, "unable to list imagestreams")
        }
        return imageStreamList.Items, nil
}

  1. For writing the tests, we start by initialising the fake client using the function FakeNew() which initialises the image clientset harnessed by GetImageStreams funtion:

    client, fkclientset := FakeNew()

  2. In the GetImageStreams funtions, the list of imagestreams is fetched through the API. While using fake client, this list can be emulated using a PrependReactor interface:

     fkclientset.ImageClientset.PrependReactor("list", "imagestreams", func(action ktesting.Action) (bool, runtime.Object, error) {
         	return true, fakeImageStreams(tt.args.name, tt.args.namespace), nil
         })
    

    The PrependReactor expects resource and verb to be passed as arguments. We can get this information by looking into List function for fake imagestream:

    func (c *FakeImageStreams) List(opts v1.ListOptions) (result *image_v1.ImageStreamList, err error) {
        	obj, err := c.Fake.Invokes(testing.NewListAction(imagestreamsResource, imagestreamsKind, c.ns, opts), &image_v1.ImageStreamList{})
    	...
        }
        
    func NewListAction(resource schema.GroupVersionResource, kind schema.GroupVersionKind, namespace string, opts interface{}) ListActionImpl {
        	action := ListActionImpl{}
        	action.Verb = "list"
        	action.Resource = resource
        	action.Kind = kind
        	action.Namespace = namespace
        	labelSelector, fieldSelector, _ := ExtractFromListOptions(opts)
        	action.ListRestrictions = ListRestrictions{labelSelector, fieldSelector}
    
        	return action
    }
    
    
    

The List function internally calls NewListAction defined in k8s.io/client-go/testing/actions.go. From these functions, we see that the resource and verbto be passed into the PrependReactor interface are imagestreams and list respectively.

You can see the entire test function TestGetImageStream in pkg/occlient/occlient_test.go

@ashetty1
Copy link
Contributor

ashetty1 commented Sep 5, 2018

@syamgk see if you can use the blurb for this PR.

@cdrage you review would be valuable :)

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.

ODO needs to be Odo.


##### Introduction

Unit-tests for ODO functions are written using package [fake](https://godoc.org/k8s.io/client-go/kubernetes/fake). This allows us to create a fake client, and then mock the API calls defined under [OpenShift client-go](https://github.com/openshift/client-go) and [k8s client-go](https://godoc.org/k8s.io/client-go).
Copy link
Member

Choose a reason for hiding this comment

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

ODO needs to be Odo.


Unit-tests for ODO functions are written using package [fake](https://godoc.org/k8s.io/client-go/kubernetes/fake). This allows us to create a fake client, and then mock the API calls defined under [OpenShift client-go](https://github.com/openshift/client-go) and [k8s client-go](https://godoc.org/k8s.io/client-go).

The tests are written in golang using [pkg/testing](https://golang.org/pkg/testing/) package.
Copy link
Member

Choose a reason for hiding this comment

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

using the


##### Initialising fake client and creating fake objects

Let us understand the initialisation of fake clients and thereafter creation of fake objects with an example.
Copy link
Member

Choose a reason for hiding this comment

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

therefore the creation of fake objects with an example.


The function `GetImageStreams` in [pkg/occlient.go](https://github.com/redhat-developer/odo/blob/master/pkg/occlient/occlient.go) fetches imagestream objects through the API:

```
Copy link
Member

Choose a reason for hiding this comment

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

you need to make it:

```go

wherever there is go code to be shown.


2. In the `GetImageStreams` funtions, the list of imagestreams is fetched through the API. While using fake client, this list can be emulated using a [`PrependReactor`](https://github.com/kubernetes/client-go/blob/master/testing/fake.go) interface:

```
Copy link
Member

Choose a reason for hiding this comment

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

```go

})
```

The `PrependReactor` expects `resource` and `verb` to be passed as arguments. We can get this information by looking into [`List` function for fake imagestream](https://github.com/openshift/client-go/blob/master/image/clientset/versioned/typed/image/v1/fake/fake_imagestream.go):
Copy link
Member

Choose a reason for hiding this comment

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

passed in as arguments

})
```

The `PrependReactor` expects `resource` and `verb` to be passed as arguments. We can get this information by looking into [`List` function for fake imagestream](https://github.com/openshift/client-go/blob/master/image/clientset/versioned/typed/image/v1/fake/fake_imagestream.go):
Copy link
Member

Choose a reason for hiding this comment

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

by looking at the

The `PrependReactor` expects `resource` and `verb` to be passed as arguments. We can get this information by looking into [`List` function for fake imagestream](https://github.com/openshift/client-go/blob/master/image/clientset/versioned/typed/image/v1/fake/fake_imagestream.go):


```
Copy link
Member

Choose a reason for hiding this comment

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

```go

action.ListRestrictions = ListRestrictions{labelSelector, fieldSelector}

return action
}
Copy link
Member

Choose a reason for hiding this comment

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

you forgot to end this with 3 `

}


The `List` function internally calls `NewListAction` defined in [k8s.io/client-go/testing/actions.go](https://github.com/kubernetes/client-go/blob/master/testing/actions.go). From these functions, we see that the `resource` and `verb`to be passed into the `PrependReactor` interface are `imagestreams` and `list` respectively.
Copy link
Member

Choose a reason for hiding this comment

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

space after verb

Copy link
Member

Choose a reason for hiding this comment

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

interface are imagestreams and list functions.

@ashetty1 ashetty1 mentioned this pull request Sep 10, 2018
@cdrage
Copy link
Member

cdrage commented Sep 10, 2018

This LGTM 👍 I'm going to merge this in. (only 1 ACK needed for docs)

@cdrage cdrage merged commit b2c66a1 into redhat-developer:master Sep 10, 2018
mik-dass pushed a commit to mik-dass/odo that referenced this pull request Oct 17, 2018
* Add unit test doc into development.md

Add unit test documentation into development.md

* incorporated changes as per anush's review

* patched with anush's modification

* incorporated cdrage's comments
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.

None yet

5 participants