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

Using gomock to mock sigs.k8s.io/controller-runtime/pkg/client If #973

Merged
merged 2 commits into from May 17, 2023

Conversation

bartam1
Copy link
Contributor

@bartam1 bartam1 commented May 16, 2023

Description

We want to use gomock everywhere in the Koperator repository and generate mocks through Makefile.
The sigs.k8s.io/controller-runtime/pkg/client Client interface was generated with mockery and it used testify mock.
When interfaces changes because updates we can re-generate mock easily with the "make mock-generate" command.

Type of Change

  • Refactor

Checklist

  • All new and existing tests pass

@bartam1 bartam1 requested a review from a team as a code owner May 16, 2023 13:22
@bartam1 bartam1 changed the title Using gomock to mocking sigs.k8s.io/controller-runtime/pkg/client If Using gomock to mock sigs.k8s.io/controller-runtime/pkg/client If May 16, 2023
Copy link
Member

@pregnor pregnor left a comment

Choose a reason for hiding this comment

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

LGTM, didn't examine the mock client line by line, but in general seems sane.

@mihaialexandrescu
Copy link
Contributor

mihaialexandrescu commented May 17, 2023

I wasn't sure if there needs to be a call to the Finish method of each new Controller we create.

I saw this saying (just like most examples I find online about gomock):

Each test should create a new Controller and invoke Finish via defer.

And gives these usage examples :

//   func TestFoo(t *testing.T) {
//     ctrl := gomock.NewController(t)
//     defer ctrl.Finish()
//     // ..
//   }
//
//   func TestBar(t *testing.T) {
//     t.Run("Sub-Test-1", st) {
//       ctrl := gomock.NewController(st)
//       defer ctrl.Finish()
//       // ..
//     })
//     t.Run("Sub-Test-2", st) {
//       ctrl := gomock.NewController(st)
//       defer ctrl.Finish()
//       // ..
//     })
//   })

And then the Finish method of a Controller indicates :

It should be invoked for each Controller.

But then the same method later added :

// New in go1.14+, if you are passing a *testing.T into NewController function you no
// longer need to call ctrl.Finish() in your test methods.

So I think we're fine without deferring Finish.

@bartam1 bartam1 merged commit 7829969 into master May 17, 2023
4 checks passed
@bartam1 bartam1 deleted the clientgo_mockgen branch May 17, 2023 08:23
ctrlaltluc added a commit to adobe/koperator that referenced this pull request Jun 8, 2023
…herrypick-from-banzai-master

* banzai/master:
  Add new CruiseControlTaskOperation to represent Status Cruise Control Operation (banzaicloud#975)
  Use permanent Slack link in README (banzaicloud#985)
  update cert-manager dependency libraries to 1.11.2 (banzaicloud#981)
  Adding test cases for kafka user issuerRef group  (banzaicloud#967)
  fix(cc): re-creation of CC metrics topic (banzaicloud#976)
  Revert "Enabling k8s-csr for PKIbackend in kafka cluster spec (banzaicloud#970)" (banzaicloud#972)
  Using gomock to mock sigs.k8s.io/controller-runtime/pkg/client If (banzaicloud#973)

# Conflicts:
#	go.mod
#	go.sum
#	pkg/resources/kafka/kafka_test.go
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

3 participants