Writing unit tests using Gomock #4110

prietyc123 · 2020-10-11T04:41:46Z

What type of PR is this?

/kind feature

What does does this PR do / why we need it:

This PR is for deciding which mocking framework we should use in odo. It includes mocks for interfaces, nested interfaces and example of unit tests using gomock and testify, so that team can decide their area of comfortability in writing tests. I will add the documentation once we decide the framework.

Which issue(s) this PR fixes:

Fixes #4097

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:
Unit tests should pass

prietyc123 · 2020-10-11T04:43:37Z

pkg/component/component_test.go

@@ -29,6 +31,281 @@ import (
 	. "github.com/openshift/odo/pkg/config"
 )

+// func TestGomockGetComponentFrom(t *testing.T) {


I have commented out TestGomockGetComponentFrom test because currently facing some issues while retrieving the returned value in gomock framework

prietyc123 · 2020-10-11T04:45:28Z

pkg/component/component_test.go

+					ComponentSettings: tt.cmpSetting,
+				}
+
+				cmpN := (reflect.ValueOf((mockLocalConfig.On("GetName").Return(envInfo.ComponentSettings.Name)).ReturnArguments[0])).Interface().(string)


Will do cleanup and modify the variable name after the final decision on the tests. This is just a kind of unit test example using testify.

prietyc123 · 2020-10-11T04:51:21Z

pkg/devfile/adapters/common/mocks/ComponentAdapter.go

@@ -0,0 +1,191 @@
+// Code generated by mockery v1.0.0. DO NOT EDIT.
+
+package mocks


Generated mocks for nested interface too using mockery. I found ease of generating mocks for such kind of interfaces using mockery. However I will try to add mocks for such scenario with gomock as well which is little challenging and still working on that.

How to generator mocks using mockery

$ mockery -dir=./pkg/devfile/adapters/common/ -name "ComponentAdapter|StorageAdapter" -output ./pkg/devfile/adapters/common/mocks/

prietyc123 · 2020-10-11T04:53:04Z

pkg/envinfo/LocalConfigProviderMocks.go

+// Source: ./pkg/envinfo/envinfo.go
+
+// Package envinfo is a generated GoMock package.
+package envinfo


Mocks generated for
https://github.com/openshift/odo/blob/a28e37838c5cbaa396f6675fe90a9986168165f5/pkg/envinfo/envinfo.go#L90-L97
using gomock mockgen

$ mockgen -source=./pkg/envinfo/envinfo.go -destination=./pkg/envinfo/LocalConfigProviderMocks.go -package=envinfo LocalConfigProvider

prietyc123 · 2020-10-11T04:53:32Z

pkg/envinfo/mocks/LocalConfigProvider.go

@@ -0,0 +1,99 @@
+// Code generated by mockery v1.0.0. DO NOT EDIT.
+
+package mocks


Mocks generated for
https://github.com/openshift/odo/blob/a28e37838c5cbaa396f6675fe90a9986168165f5/pkg/envinfo/envinfo.go#L90-L97
using testify mockery

$ mockery -dir=./pkg/envinfo/ -name LocalConfigProvider -output ./pkg/envinfo/mocks/

codecov · 2020-10-11T13:16:17Z

Codecov Report

Merging #4110 into master will decrease coverage by 0.12%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #4110      +/-   ##
==========================================
- Coverage   42.28%   42.15%   -0.13%     
==========================================
  Files         154      155       +1     
  Lines       13074    13114      +40     
==========================================
  Hits         5528     5528              
- Misses       6954     6994      +40     
  Partials      592      592

Impacted Files	Coverage Δ
pkg/envinfo/LocalConfigProviderMocks.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b33adbc...0fd0d9e. Read the comment docs.

prietyc123 · 2020-10-12T08:22:03Z

For gomock comparison I am going through hard time to get it, However using mockery i have completely implemented the acceptance criteria.

Issues with gomock:

Not able to get the returned value to use it further in the tests for example envFileExists https://github.com/openshift/odo/pull/4110/files#diff-e409fdb53e039c31ec05cbb2dab7703bR91-R98 . Similar issue is already there return value depend on input value golang/mock#34 and I tried the suggested sol too.
With embedded interface gomock with mockgen has already ongoing issues mockgen can't generate interfaces that embed them from a separate file shutej/gomock#16, However we can generate such mocks using mockery easily see - https://github.com/openshift/odo/pull/4110/files#diff-1391632fe1305a3ec6e1592e4c1d7ac9
One more difficulties I found with gomock while working on it, is error message while failure/debugging. TBH it was really tough for me to get the error message using gomock, however testify with mockery is simplest and user friendly.

@girishramnani @kadel I have fulfilled the acceptance criteria for issue #4097 using mockery and facing lots of challenges with gomock mockgen. Trying to fix the unit test with gomock and overcome its challenges with embedded interface. I will update the progress going forward.

prietyc123 · 2020-10-13T11:52:36Z

Observation and challenges faced to generate mocking using gomock/testify

Pre-requisite:

Should have Interfaces across the code

Embedded interface mocking:

Gomock
- There are two modes for mocking using mockgen (source and reflect), However we use reflect mode for embedded
  interface. For example mock generated for https://github.com/openshift/odo/blob/master/pkg/devfile/adapters/common/interface.go

$ mockgen -destination=./pkg/devfile/adapters/common/mock/InterfaceMock.go github.com/openshift/odo/pkg/devfile/adapters/common ComponentAdapter,StorageAdapter

Generated mock - https://github.com/openshift/odo/pull/4110/files#diff-21de83616946ca64be5a4b32b6c372d1ed5d72a8d770c10a8b995b8f0baf1555

Testify
- It has general way of mocking interface or embedded interface. Examples for generating mock such scenario https://github.com/openshift/odo/pull/4110/files#diff-12016d666198a0bca70c3c8fb35897e9fe8bd468766eae230781b6990980dc70 and https://github.com/openshift/odo/pull/4110/files#diff-cfc79cda65f2d5696f28d7a2d5a444484dd6239e0bf616c1546ac5425348e983

$ mockery -dir=./pkg/devfile/adapters/common/ -name "ComponentAdapter|StorageAdapter" -output ./pkg/devfile/adapters/common/mocks/

Writing unit tests

Gomock
- Errors out at compile time when we try to mock invalid functions.
- Almost same approach of writing unit test. Refer: https://github.com/openshift/odo/pull/4110/files#diff-18b00cb22cbf956666faa888c1a4b2977dfbafb20a3712c2301405d1b4527307R35-R145
- Generate mocks using

$ mockgen -source=./pkg/envinfo/envinfo.go -destination=./pkg/envinfo/LocalConfigProviderMocks.go -package=envinfo LocalConfigProvider

Testify
- Do not errors out at compile time for invalid functions.
- Refer: https://github.com/openshift/odo/pull/4110/files#diff-18b00cb22cbf956666faa888c1a4b2977dfbafb20a3712c2301405d1b4527307R147-R257
- Generate mocks using

$ mockery -dir=./pkg/envinfo/ -name LocalConfigProvider -output ./pkg/envinfo/mocks/

Thanks @mik-dass for providing useful feedback on the unit tests part over bjn. I have updated the unit tests as per our discussion. It would be great if you review once and let me know your thoughts.

@kadel @girishramnani To the best of my knowledge I have jotted down all the pros and cons for both the mocking frameworks. Could you please let me know your thoughts on deciding factor.

prietyc123 · 2020-10-14T03:57:17Z

platform issue

/retest

prietyc123 · 2020-10-14T07:14:49Z

Already we have an issue for

[odo] I1014 06:25:32.544747   73002 deployments.go:146] Waiting for deployment spec update to be observed...
[odo]  ✗  Waiting for component to start [5m]
[odo]  ✗  Failed to start component with name xabhim. Error: Failed to create the component: error while waiting for deployment rollout: timeout while waiting for xabhim deployment roll out

#3256

prietyc123 · 2020-10-14T07:14:58Z

/retest

prietyc123 · 2020-10-19T04:53:28Z

platform issue

/retest

prietyc123 · 2020-10-19T08:42:33Z

/retest

amitkrout · 2020-10-19T12:57:34Z

/test unit

prietyc123 · 2020-10-20T06:03:07Z

/retest

girishramnani · 2020-10-20T12:26:05Z

Final concensus is to use gomock and mockgen

prietyc123 · 2020-10-29T10:32:24Z

/retest

…gomock and testify

… interface

prietyc123 · 2020-11-03T11:12:19Z

/retest

dharmit

Code /lgtm.

However, I would have preferred having comments as well.

dharmit · 2020-11-03T12:01:11Z

I will add the documentation once we decide the framework.

I've opened #4186 so that we don't miss this aspect.

girishramnani · 2020-11-03T12:25:37Z

/approve

openshift-ci-robot · 2020-11-03T12:26:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [girishramnani]

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

girishramnani · 2020-11-04T07:06:35Z

the lgtm didn't kick in adding again 🙂
/lgtm

* Adding unit tests and mocks for interface and nested interface using gomock and testify * Add testify latest verion in go mod * Added vendor for dependencies * Added examples with both gomock and testify for interface and embeded interface * removed testify tests and mocks

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Oct 11, 2020

openshift-ci-robot requested review from dharmit and girishramnani October 11, 2020 04:42

prietyc123 commented Oct 11, 2020

View reviewed changes

prietyc123 force-pushed the POCOnGomockAndTestifyMockingFramework branch from e03077b to 8ef2a53 Compare October 11, 2020 05:09

prietyc123 force-pushed the POCOnGomockAndTestifyMockingFramework branch 2 times, most recently from 4e0e2c5 to 446b5c4 Compare October 13, 2020 11:46

prietyc123 force-pushed the POCOnGomockAndTestifyMockingFramework branch from 446b5c4 to 34ec602 Compare October 13, 2020 15:44

prietyc123 changed the title ~~[WIP]POC on writing unit tests using Gomock and Testify~~ Writing unit tests using Gomock and Testify Oct 14, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Oct 14, 2020

prietyc123 force-pushed the POCOnGomockAndTestifyMockingFramework branch from 34ec602 to d9325b1 Compare October 19, 2020 04:40

prietyc123 force-pushed the POCOnGomockAndTestifyMockingFramework branch from d61ba45 to 4a16805 Compare October 20, 2020 13:35

prietyc123 changed the title ~~Writing unit tests using Gomock and Testify~~ Writing unit tests using Gomock Oct 20, 2020

prietyc123 force-pushed the POCOnGomockAndTestifyMockingFramework branch from 4a16805 to a317653 Compare October 21, 2020 17:36

prietyc123 force-pushed the POCOnGomockAndTestifyMockingFramework branch 2 times, most recently from 732a53c to 5335c56 Compare October 28, 2020 09:52

prietyc123 added 5 commits November 3, 2020 11:55

Adding unit tests and mocks for interface and nested interface using …

b39d034

…gomock and testify

Add testify latest verion in go mod

b4c19da

Added vendor for dependencies

767384c

Added examples with both gomock and testify for interface and embeded…

66711d7

… interface

removed testify tests and mocks

0fd0d9e

prietyc123 force-pushed the POCOnGomockAndTestifyMockingFramework branch from 5335c56 to 0fd0d9e Compare November 3, 2020 06:29

dharmit reviewed Nov 3, 2020

View reviewed changes

dharmit mentioned this pull request Nov 3, 2020

Document guidelines for writing unit tests using GoMock #4186

Closed

4 tasks

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Nov 3, 2020

openshift-ci-robot assigned girishramnani Nov 4, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 4, 2020

openshift-merge-robot merged commit 55c4c5a into redhat-developer:master Nov 4, 2020

prietyc123 mentioned this pull request Feb 9, 2021

Mocking framework for unit test #4076

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Writing unit tests using Gomock #4110

Writing unit tests using Gomock #4110

prietyc123 commented Oct 11, 2020

prietyc123 Oct 11, 2020

prietyc123 Oct 11, 2020 •

edited

Loading

prietyc123 Oct 11, 2020 •

edited

Loading

prietyc123 Oct 11, 2020 •

edited

Loading

prietyc123 Oct 11, 2020 •

edited

Loading

codecov bot commented Oct 11, 2020 •

edited

Loading

prietyc123 commented Oct 12, 2020 •

edited

Loading

prietyc123 commented Oct 13, 2020 •

edited

Loading

prietyc123 commented Oct 14, 2020

prietyc123 commented Oct 14, 2020

prietyc123 commented Oct 14, 2020

prietyc123 commented Oct 19, 2020

prietyc123 commented Oct 19, 2020

amitkrout commented Oct 19, 2020

prietyc123 commented Oct 20, 2020

girishramnani commented Oct 20, 2020

prietyc123 commented Oct 29, 2020

prietyc123 commented Nov 3, 2020

dharmit left a comment

dharmit commented Nov 3, 2020

girishramnani commented Nov 3, 2020

openshift-ci-robot commented Nov 3, 2020

girishramnani commented Nov 4, 2020

		@@ -0,0 +1,191 @@
		// Code generated by mockery v1.0.0. DO NOT EDIT.

		package mocks

		@@ -0,0 +1,99 @@
		// Code generated by mockery v1.0.0. DO NOT EDIT.

		package mocks

Writing unit tests using Gomock #4110

Writing unit tests using Gomock #4110

Conversation

prietyc123 commented Oct 11, 2020

prietyc123 Oct 11, 2020

Choose a reason for hiding this comment

prietyc123 Oct 11, 2020 • edited Loading

Choose a reason for hiding this comment

prietyc123 Oct 11, 2020 • edited Loading

Choose a reason for hiding this comment

prietyc123 Oct 11, 2020 • edited Loading

Choose a reason for hiding this comment

prietyc123 Oct 11, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2020 • edited Loading

Codecov Report

prietyc123 commented Oct 12, 2020 • edited Loading

prietyc123 commented Oct 13, 2020 • edited Loading

prietyc123 commented Oct 14, 2020

prietyc123 commented Oct 14, 2020

prietyc123 commented Oct 14, 2020

prietyc123 commented Oct 19, 2020

prietyc123 commented Oct 19, 2020

amitkrout commented Oct 19, 2020

prietyc123 commented Oct 20, 2020

girishramnani commented Oct 20, 2020

prietyc123 commented Oct 29, 2020

prietyc123 commented Nov 3, 2020

dharmit left a comment

Choose a reason for hiding this comment

dharmit commented Nov 3, 2020

girishramnani commented Nov 3, 2020

openshift-ci-robot commented Nov 3, 2020

girishramnani commented Nov 4, 2020

prietyc123 Oct 11, 2020 •

edited

Loading

prietyc123 Oct 11, 2020 •

edited

Loading

prietyc123 Oct 11, 2020 •

edited

Loading

prietyc123 Oct 11, 2020 •

edited

Loading

codecov bot commented Oct 11, 2020 •

edited

Loading

prietyc123 commented Oct 12, 2020 •

edited

Loading

prietyc123 commented Oct 13, 2020 •

edited

Loading