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

sample code that uses mockery and testify for writing unit tests #441

Closed
wants to merge 1 commit into from
Closed

sample code that uses mockery and testify for writing unit tests #441

wants to merge 1 commit into from

Conversation

vjayaramrh
Copy link
Contributor

@vjayaramrh vjayaramrh commented Nov 15, 2023

The goal of this PR is to demonstrate usage of testify (https://pkg.go.dev/github.com/stretchr/testify) and mockery (https://github.com/vektra/mockery/tree/master) for writing unit tests.
Will be using comments in the PR to help folks come upto speed on using testify and mockery.

Steps to try out:

  • a)Pull the PR to your dev environment, b)delete the mock_GiteaClient.go file c)execute make install-mockery d)execute make generate-mocks to verify mock_GiteaClient.go is created.

References:

Mockery -> https://vektra.github.io/mockery/latest/#why-mockery
Testify -> https://pkg.go.dev/github.com/stretchr/testify
Installing Testify --> https://pkg.go.dev/github.com/stretchr/testify#readme-installation

@nephio-prow nephio-prow bot requested review from s3wong and tliron November 15, 2023 00:42
@vjayaramrh vjayaramrh requested review from liamfallon and removed request for s3wong and tliron November 15, 2023 00:42
@@ -0,0 +1,6 @@
packages:
Copy link
Contributor Author

@vjayaramrh vjayaramrh Nov 15, 2023

Choose a reason for hiding this comment

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

This file was authored following the example at https://vektra.github.io/mockery/latest/configuration/#recommended-basic-config and understanding the parameter descriptions at https://vektra.github.io/mockery/latest/configuration/#parameter-descriptions.
Note: This file has to be located in the top directory of the project folder.

@@ -0,0 +1,6 @@
packages:
github.com/nephio-project/nephio/controllers/pkg/giteaclient:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since, we want to generate mock for the GiteaClient interface, the package path to the interface has to be provided. Refer https://vektra.github.io/mockery/latest/features/#packages-configuration as well

interfaces:
GiteaClient:
config:
dir: "{{.InterfaceDir}}"
Copy link
Contributor Author

@vjayaramrh vjayaramrh Nov 15, 2023

Choose a reason for hiding this comment

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

The {{.InterfaceDir}} indicates to generate the mock file in the same directory where the interface is located. Refer https://vektra.github.io/mockery/latest/configuration/#variables

@@ -0,0 +1,423 @@
// Code generated by mockery v2.36.1. DO NOT EDIT.
Copy link
Contributor Author

@vjayaramrh vjayaramrh Nov 15, 2023

Choose a reason for hiding this comment

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

This file was generated by running the vektra/mockery container image and mounting the project base folder into the container. I ran the below podman command to generate this file from the top level of the project directory.
podman run -v ${PWD}:/src:Z -w /src vektra/mockery:latest.
On can treat this generated file as a black box and do not have to be versed with the contents of this file to write unit tests.
The file .mockery.yaml is read by mockery to generate this file.

@@ -196,3 +202,77 @@ func TestDeleteRepo(t *testing.T) {
})
}
}

func Test_reconciler_upsertRepo(t *testing.T) {
type mockHelper struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mockHelper is a helper struct to capture the details of the method to be mocked along with its parameters and return values when used in unit testing.

name: "User Info reports error",
fields: fields{resource.NewAPIPatchingApplicator(nil), nil, nil, log.FromContext(nil)},
args: args{nil, nil, &infrav1alpha1.Repository{}},
mocks: []mockHelper{
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 captures the info where we want the the GetMyUserInfo method to return an error to be able to test that flow.

@@ -196,3 +202,77 @@ func TestDeleteRepo(t *testing.T) {
})
}
}

func Test_reconciler_upsertRepo(t *testing.T) {
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 method writes tests for the upsertRepo method (already written at https://github.com/nephio-project/nephio/blob/main/controllers/pkg/reconcilers/repository/reconciler_test.go#L65) and demonstrate how this can be written using mockery and testify.

finalizer: tt.fields.finalizer,
l: tt.fields.l,
}
// The below block being setup and processing of mocks before invoking the function to be tested
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lines 260 to L271 is code to process the info in mocks struct to setup prior to actual invocation of the function to be tested.

Copy link
Contributor

@efiacor efiacor left a comment

Choose a reason for hiding this comment

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

Thanks for this @vjayaramrh
I had a go at adding the test case fro deleteRepo using your example to work from and it was straight forward enough.
I may look at trying to test the Token reconciler functions which also uses the gitea client interfaces.
My main concern would be a bit of a learning curve for people to use the framework.
Also, from having a go at testing some of the other controllers, to me, the main interface that we would need to mock is the controller-runtime client, which from most of what I have found online, the recommendation is to use envtest instead of the fake/mock.

@vjayaramrh
Copy link
Contributor Author

Thanks for this @vjayaramrh I had a go at adding the test case fro deleteRepo using your example to work from and it was straight forward enough. I may look at trying to test the Token reconciler functions which also uses the gitea client interfaces. My main concern would be a bit of a learning curve for people to use the framework. Also, from having a go at testing some of the other controllers, to me, the main interface that we would need to mock is the controller-runtime client, which from most of what I have found online, the recommendation is to use envtest instead of the fake/mock.

@efiacor Thanks for trying out, do you mind updating this PR with your commit of the test case for the deleteRepo. This PR could be the one where different folks try out using the framework and for others to see how it is done, Thanks

@efiacor
Copy link
Contributor

efiacor commented Nov 21, 2023

Thanks for this @vjayaramrh I had a go at adding the test case fro deleteRepo using your example to work from and it was straight forward enough. I may look at trying to test the Token reconciler functions which also uses the gitea client interfaces. My main concern would be a bit of a learning curve for people to use the framework. Also, from having a go at testing some of the other controllers, to me, the main interface that we would need to mock is the controller-runtime client, which from most of what I have found online, the recommendation is to use envtest instead of the fake/mock.

@efiacor Thanks for trying out, do you mind updating this PR with your commit of the test case for the deleteRepo. This PR could be the one where different folks try out using the framework and for others to see how it is done, Thanks

Hi @vjayaramrh. Not sure if I can. We push via a proxy github project - https://github.com/Nordix/nephio
I can edit via the codespaces web app here but when I try to push a commit it denies it with 403 no write access. Any suggestions?

@vjayaramrh
Copy link
Contributor Author

Thanks for this @vjayaramrh I had a go at adding the test case fro deleteRepo using your example to work from and it was straight forward enough. I may look at trying to test the Token reconciler functions which also uses the gitea client interfaces. My main concern would be a bit of a learning curve for people to use the framework. Also, from having a go at testing some of the other controllers, to me, the main interface that we would need to mock is the controller-runtime client, which from most of what I have found online, the recommendation is to use envtest instead of the fake/mock.

@efiacor Thanks for trying out, do you mind updating this PR with your commit of the test case for the deleteRepo. This PR could be the one where different folks try out using the framework and for others to see how it is done, Thanks

Hi @vjayaramrh. Not sure if I can. We push via a proxy github project - https://github.com/Nordix/nephio I can edit via the codespaces web app here but when I try to push a commit it denies it with 403 no write access. Any suggestions?

@efiacor came across the below like that might help, I belive.
https://tighten.com/insights/adding-commits-to-a-pull-request/
@electrocucaracha , Do you know of how others can make commits to a PR? Thanks

@electrocucaracha
Copy link
Member

@vjayaramrh I think the way to do it is accepting their suggestions, that creates another commit into the existing branch

Copy link
Contributor

@efiacor efiacor left a comment

Choose a reason for hiding this comment

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

Add deleteRepo test case.

func Test_reconciler_deleteRepo(t *testing.T) {
type mockHelper struct {
methodName string
argType []string
retArgList []interface{}
}
type fields struct {
APIPatchingApplicator resource.APIPatchingApplicator
giteaClient giteaclient.GiteaClient
finalizer *resource.APIFinalizer
l logr.Logger
}
type args struct {
ctx context.Context
giteaClient giteaclient.GiteaClient
cr *infrav1alpha1.Repository
}
tests := []struct {
name string
fields fields
args args
mocks []mockHelper
wantErr bool
}{
{
name: "User Info reports error",
fields: fields{resource.NewAPIPatchingApplicator(nil), nil, nil, log.FromContext(nil)},
args: args{nil, nil, &infrav1alpha1.Repository{}},
mocks: []mockHelper{
{"GetMyUserInfo", []string{}, []interface{}{nil, nil, fmt.Errorf("error getting User Information")}},
},
wantErr: true,
},
{
name: "Delete repo reports error",
fields: fields{resource.NewAPIPatchingApplicator(nil), nil, nil, log.FromContext(nil)},
args: args{nil, nil, &infrav1alpha1.Repository{Status: infrav1alpha1.RepositoryStatus{}}},
mocks: []mockHelper{
{"GetMyUserInfo", []string{}, []interface{}{&gitea.User{UserName: "gitea"}, nil, nil}},
{"DeleteRepo", []string{"string", "string"}, []interface{}{nil, fmt.Errorf("error deleting repo")}},
},
wantErr: true,
},
{
name: "Delete repo success",
fields: fields{resource.NewAPIPatchingApplicator(nil), nil, nil, log.FromContext(nil)},
args: args{nil, nil, &infrav1alpha1.Repository{Status: infrav1alpha1.RepositoryStatus{}}},
mocks: []mockHelper{
{"GetMyUserInfo", []string{}, []interface{}{&gitea.User{UserName: "gitea"}, nil, nil}},
{"DeleteRepo", []string{"string", "string"}, []interface{}{nil, nil}},
},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r := &reconciler{
APIPatchingApplicator: tt.fields.APIPatchingApplicator,
giteaClient: tt.fields.giteaClient,
finalizer: tt.fields.finalizer,
l: tt.fields.l,
}
// The below block being setup and processing of mocks before invoking the function to be tested
mockGClient := new(giteaclient.MockGiteaClient)
tt.args.giteaClient = mockGClient
tt.fields.giteaClient = mockGClient
for counter := range tt.mocks {
call := mockGClient.Mock.On(tt.mocks[counter].methodName)
for _, arg := range tt.mocks[counter].argType {
call.Arguments = append(call.Arguments, mock.AnythingOfType(arg))
}
for _, ret := range tt.mocks[counter].retArgList {
call.ReturnArguments = append(call.ReturnArguments, ret)
}
}
if err := r.deleteRepo(tt.args.ctx, tt.args.giteaClient, tt.args.cr); (err != nil) != tt.wantErr {
t.Errorf("deleteRepo() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}

Copy link
Contributor

nephio-prow bot commented Nov 27, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from henderiw by writing /assign @henderiw in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vjayaramrh
Copy link
Contributor Author

Add deleteRepo test case.

func Test_reconciler_deleteRepo(t *testing.T) { type mockHelper struct { methodName string argType []string retArgList []interface{} } type fields struct { APIPatchingApplicator resource.APIPatchingApplicator giteaClient giteaclient.GiteaClient finalizer *resource.APIFinalizer l logr.Logger } type args struct { ctx context.Context giteaClient giteaclient.GiteaClient cr *infrav1alpha1.Repository } tests := []struct { name string fields fields args args mocks []mockHelper wantErr bool }{ { name: "User Info reports error", fields: fields{resource.NewAPIPatchingApplicator(nil), nil, nil, log.FromContext(nil)}, args: args{nil, nil, &infrav1alpha1.Repository{}}, mocks: []mockHelper{ {"GetMyUserInfo", []string{}, []interface{}{nil, nil, fmt.Errorf("error getting User Information")}}, }, wantErr: true, }, { name: "Delete repo reports error", fields: fields{resource.NewAPIPatchingApplicator(nil), nil, nil, log.FromContext(nil)}, args: args{nil, nil, &infrav1alpha1.Repository{Status: infrav1alpha1.RepositoryStatus{}}}, mocks: []mockHelper{ {"GetMyUserInfo", []string{}, []interface{}{&gitea.User{UserName: "gitea"}, nil, nil}}, {"DeleteRepo", []string{"string", "string"}, []interface{}{nil, fmt.Errorf("error deleting repo")}}, }, wantErr: true, }, { name: "Delete repo success", fields: fields{resource.NewAPIPatchingApplicator(nil), nil, nil, log.FromContext(nil)}, args: args{nil, nil, &infrav1alpha1.Repository{Status: infrav1alpha1.RepositoryStatus{}}}, mocks: []mockHelper{ {"GetMyUserInfo", []string{}, []interface{}{&gitea.User{UserName: "gitea"}, nil, nil}}, {"DeleteRepo", []string{"string", "string"}, []interface{}{nil, nil}}, }, wantErr: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { r := &reconciler{ APIPatchingApplicator: tt.fields.APIPatchingApplicator, giteaClient: tt.fields.giteaClient, finalizer: tt.fields.finalizer, l: tt.fields.l, } // The below block being setup and processing of mocks before invoking the function to be tested mockGClient := new(giteaclient.MockGiteaClient) tt.args.giteaClient = mockGClient tt.fields.giteaClient = mockGClient for counter := range tt.mocks { call := mockGClient.Mock.On(tt.mocks[counter].methodName) for _, arg := range tt.mocks[counter].argType { call.Arguments = append(call.Arguments, mock.AnythingOfType(arg)) } for _, ret := range tt.mocks[counter].retArgList { call.ReturnArguments = append(call.ReturnArguments, ret) } } if err := r.deleteRepo(tt.args.ctx, tt.args.giteaClient, tt.args.cr); (err != nil) != tt.wantErr { t.Errorf("deleteRepo() error = %v, wantErr %v", err, tt.wantErr) } }) } }

@efiacor thanks, I found this article that details how you maybe able to provide suggestions as comments that could be incorporated as a commit into this PR. REfer https://haacked.com/archive/2019/06/03/suggested-changes/

@liamfallon
Copy link
Member

@vjayaramrh sorry for not looking at this earlier. This is great, I think we should go this way with our testing. Would it be an idea to turn this PR into a document in the developer docs?

@vjayaramrh
Copy link
Contributor Author

@vjayaramrh sorry for not looking at this earlier. This is great, I think we should go this way with our testing. Would it be an idea to turn this PR into a document in the developer docs?

@liamfallon Good suggestion, we should definitely collaborate on the developer doc so that multiple folks have understanding of this framework. CC: @efiacor @rravindran123

@efiacor
Copy link
Contributor

efiacor commented Dec 5, 2023

@vjayaramrh
Copy link
Contributor Author

@radoslawc I am trying to figure out how to go past the issues at a)http://prow.nephio.io/view/gs/prow-nephio-sig-release/pr-logs/pull/nephio-project_nephio/441/presubmit-nephio-golangci-lint/1732040044367056896#1:build-log.txt%3A171 b)http://prow.nephio.io/view/gs/prow-nephio-sig-release/pr-logs/pull/nephio-project_nephio/441/presubmit-nephio-go-test/1732040044299948032#1:build-log.txt%3A1738 c)http://prow.nephio.io/view/gs/prow-nephio-sig-release/pr-logs/pull/nephio-project_nephio/441/presubmit-nephio-lichen/1732040044828430336#1:build-log.txt%3A513 Would appreciate any pointers you may have

Hi @vjayaramrh I made a few tweaks on the other PR and it seems ok now. #446 FYI, I had to move the mockery.yaml inside the pkg module to get the "make" to run, or maybe I am running from the wrong dir.

@efiacor The fix in the Makefile at https://github.com/nephio-project/nephio/pull/441/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R34 was missing and hence you were probably not able to invoke the make install-mockery and make generate-mocks. It is suggested to keep the .mockery.yaml in the base directory.

Copy link
Contributor

nephio-prow bot commented Dec 18, 2023

@vjayaramrh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
presubmit-nephio-golangci-lint 6df4bdf link true /test presubmit-nephio-golangci-lint
presubmit-nephio-go-test 6df4bdf link true /test presubmit-nephio-go-test
presubmit-nephio-lichen 6df4bdf link true /test presubmit-nephio-lichen

Full PR test history. Your PR dashboard.

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.

nephio-prow bot pushed a commit that referenced this pull request Feb 12, 2024
This PR uses the approach and code described by @vjayaramrh in
#441 to replace the unit
tests for the Repository operator. This PR replaces all the unit tests
with Mockery implementations.
nephio-prow bot pushed a commit to nephio-project/docs that referenced this pull request Feb 19, 2024
This is a contributor guide describing how to use Mockery. It draws
heavily on [PR-441](nephio-project/nephio#441)
raised by @vjayaramrh

---------

Co-authored-by: Victor Morales <[email protected]>
@liamfallon
Copy link
Member

@vjayaramrh Does the unit test documentation now cover the content of this PR or did we miss anything? Can we close this PR?

@vjayaramrh
Copy link
Contributor Author

@vjayaramrh Does the unit test documentation now cover the content of this PR or did we miss anything? Can we close this PR?

@liamfallon your documentation PR covers what we need, I shall close this PR now

@vjayaramrh vjayaramrh closed this Mar 5, 2024
@liamfallon
Copy link
Member

Thanks @vjayaramrh for all your work on this.

PrimalPimmy pushed a commit to 5GSEC/nephio that referenced this pull request Aug 2, 2024
This PR uses the approach and code described by @vjayaramrh in
nephio-project#441 to replace the unit
tests for the Repository operator. This PR replaces all the unit tests
with Mockery implementations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants