Unit test coverage for DeleteRepo and upsertRepo functions in repository reconciler#426
Conversation
|
/assign @vjayaramrh @rravindran123 @efiacor @lapentad Unit test coverage for a small function as a starting point. |
| return r.giteaClient | ||
| } | ||
|
|
||
| func (r *gc) GetMyUserInfo() (*gitea.User, *gitea.Response, error) { |
There was a problem hiding this comment.
What would be the goal for adding this method? If one invokes the Get() method, they all already have access to the client and could invoke all the functions the client object offers, right? Would be adding other functions that this client supports as methods as well in the future? THanks
There was a problem hiding this comment.
I have to introduce this function and the DeleteRepo function in order to make mocking possible in the reconciler. I could not write a mock in the reconciler because the reconciler code used the Get() function to get a reference to the implementation of the interface and then called the implementaiton directly, therefore short circuiting the interface.
What is done here in the PR is actually only the first step of what we need to do to refactor the reconciler so that we can use repositories other than gitea. The reconciler should only call functions on the interface and then there can be different implementations of that interface for Gitea and Git.
In fact, perhaps we should remove the Get() method from the interface altogether to prevent direct access to the implementation in users of the interface.
There was a problem hiding this comment.
So, it sounds like this refactoring is aiming to have a single reconciler for various Git providers. This is the same pattern and concerns as what we discussed last week (see #421 (comment)).
There was a problem hiding this comment.
Actually having different reconcilers for different repo providers would be a good approach. In a particular installation you can configure the system to use whichever one you want to run.
There was a problem hiding this comment.
After yesterday's SIG-Automation meeting, we will discuss the restructuring/refactoring separately. Can we review this PR as a pure Unit Test review? The refactoring is done just to aid unit testing.
| return r.giteaClient.GetMyUserInfo() | ||
| } | ||
|
|
||
| func (r *gc) DeleteRepo(owner string, repo string) (*gitea.Response, error) { |
…ix/nephio into coverage-repo-reconciler-deleteRepo
liamfallon
changed the title
There was a problem hiding this comment.
/lgtm
@liamfallon overall, the approach looks good to me,
We may also want to do a POC using https://pkg.go.dev/github.com/stretchr/testify/mock to generate mocks and see if it could be better. I have used it and found it useful to create mocks and write unit tests. We may have to come up with training material to make it easy for community members to adopt this if we think it is good.
Also, does it make sense to rename the directory from repository to gitea-repository as the code is specific to gitea?
Yes, we should also try to do a POC to try the mock generation. It probably would be a good idea to rename this to gitea-repository but maybe let's discuss this in the community and take it in another PR |
|
Can I have some more reviews on this PR please? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: efiacor, johnbelamaric, vjayaramrh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
No description provided.