✨ Add mock client and 2 tests for networking package #935

macaptain · 2021-07-12T14:36:54Z

Refactors the Service in the networking package to use a client with CRUD functions for manipulating OpenStack resources. This interface is combined with mockgen code generation to make the package unit testable.

Two tests are added to illustrate how writing tests for the package will work. The list / filter networks and subnets functions are also slightly simplified.

What this PR does / why we need it:

A refactor of the code to allow unit tests to be written for code that uses gophercloud to interface with OpenStack.

Which issue(s) this PR fixes:
Goes some way to fixing #843.

Special notes for your reviewer:

If you like this change, my plan is to move some of the 'ports' logic to the networking package and write unit tests for it, and then apply the same changes, adding mock clients for the Service in the compute package.

I tried a few things out for making the code unit testable, including using gomega's ghttp server for mocking (too much JSON), or trying to write smaller interfaces (ended up being a lot of boiler plate and manual writing of mocks), and ended up settling on this. It's a similar pattern to what's used in cluster-api-provider-aws (see instance_test.go and the generated mocks: https://github.com/kubernetes-sigs/cluster-api-provider-aws/tree/main/pkg/cloud/services/ec2), as well as using the gomega library for matching (as recommended in the Cluster API book section on Testing.)

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

k8s-ci-robot · 2021-07-12T14:37:02Z

Hi @macaptain. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jichenjc · 2021-07-13T01:37:39Z

/ok-to-test

lentzi90

This looks great!

macaptain · 2021-07-16T08:16:48Z

This PR is two commits to make it a bit easier to review. After rebasing for the metrics changes in #932, I decided to move other metrics into the Client implementation as well for consistency in a second commit. I can squash the commits before merging if desired.

jichenjc · 2021-07-19T00:54:38Z

/lgtm

I think it's great PR , @hidekazuna can you take a look so that we can merge it soon (to avoid various conflict)
and if any followup needed we can anyway do it later on

jichenjc · 2021-07-19T00:56:03Z

pkg/cloud/services/networking/client.go

+	return ports.Create(c.serviceClient, opts).Extract()
+}
+
+func (c networkClient) DeletePort(id string) error {


I'd suggest we consider add metrics for those kind of API call as well , but no need to be in this PR as this is a pure refactory

pkg/cloud/services/networking/client.go

pkg/cloud/services/networking/mock_client/client.go

jichenjc · 2021-07-22T01:56:00Z

/lgtm

hidekazuna · 2021-07-22T06:29:26Z

Can we update like following?

networking/
├── client.go
├── floatingip.go
├── floatingip_test.go
├── mock_networking
│ ├── client.go -> rename to client_mock.go
│ ├── doc.go
│ └── mock_client -> delete.
│ └── client.go -> delete.
.......

Because CAPZ is the following.

subnets/
├── client.go
├── mock_subnets
│ ├── client_mock.go
│ ├── doc.go
│ └── subnets_mock.go
├── subnets.go
└── subnets_test.go

Refactors the Service in the networking package to use a client with CRUD functions for manipulating OpenStack resources. This interface is combined with mockgen code generation to make the package unit testable. Two tests are added to illustrate how writing tests for the package will work. Metrics recording is also refactored to Client. It makes sense to take the metrics at the site where the OpenStack call is made, and also tidies up the 'business logic' functions when metrics recording is the responsibility of the network client implementation.

macaptain · 2021-07-22T06:49:23Z

Can we update like following?

Yes, of course. I've updated the folder structure and filenames to match. The mock package is called mock_networking and the generated file is client_mock.go, and mock_client and mock_client.go have been removed.

jichenjc · 2021-07-22T06:55:12Z

/lgtm

hidekazuna · 2021-07-22T11:28:35Z

@macaptain Thanks updating.

/approve

k8s-ci-robot · 2021-07-22T11:28:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, lentzi90, macaptain

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~~ [hidekazuna]

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

hidekazuna · 2021-07-22T11:29:30Z

/hold cancel

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 12, 2021

k8s-ci-robot requested review from jichenjc and sbueringer July 12, 2021 14:36

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 12, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 13, 2021

macaptain force-pushed the feat-add-unit-tests branch from 1d35770 to ca8ccdb Compare July 13, 2021 06:44

lentzi90 approved these changes Jul 13, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021

Xenwar mentioned this pull request Jul 15, 2021

✨ Allow Trunk configuration at a Port level #934

Merged

macaptain force-pushed the feat-add-unit-tests branch from ca8ccdb to e10d1bf Compare July 16, 2021 06:42

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2021

k8s-ci-robot assigned jichenjc Jul 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2021

jichenjc reviewed Jul 19, 2021

View reviewed changes

hidekazuna reviewed Jul 21, 2021

View reviewed changes

pkg/cloud/services/networking/client.go Outdated Show resolved Hide resolved

pkg/cloud/services/networking/mock_client/client.go Outdated Show resolved Hide resolved

macaptain force-pushed the feat-add-unit-tests branch from 5ca1033 to 300cf9c Compare July 21, 2021 12:11

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2021

macaptain force-pushed the feat-add-unit-tests branch from 300cf9c to efe8353 Compare July 22, 2021 06:47

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021

k8s-ci-robot merged commit da1c282 into kubernetes-sigs:master Jul 22, 2021

hidekazuna mentioned this pull request Jul 22, 2021

🏃 Refactor: do not use networkclient in compute package #883

Closed

3 tasks

macaptain deleted the feat-add-unit-tests branch July 22, 2021 12:37

macaptain mentioned this pull request Jul 22, 2021

🏃 Refactor all network client calls into networking package #950

Merged

1 task

This was referenced Sep 13, 2021

Regression: Specifying subnet filter without network fails when it previously did not #993

Closed

🐛 Don't make unnecessary REST API calls in getServerNetworks #994

Merged

macaptain mentioned this pull request Sep 15, 2021

Prometheus metrics when making API calls are inconsistent #997

Closed

macaptain mentioned this pull request Oct 6, 2021

REQUEST: New membership for macaptain kubernetes/org#3030

Closed

7 tasks

macaptain mentioned this pull request Nov 1, 2021

Improve unit test coverage #843

Closed

This was referenced Jan 24, 2022

✨Service interface load balancer #1119

Merged

Create a service interface for package pkg/cloud/services/loadbalancer #1120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add mock client and 2 tests for networking package #935

✨ Add mock client and 2 tests for networking package #935

macaptain commented Jul 12, 2021 •

edited

Loading

k8s-ci-robot commented Jul 12, 2021

jichenjc commented Jul 13, 2021

lentzi90 left a comment

macaptain commented Jul 16, 2021

jichenjc commented Jul 19, 2021

jichenjc Jul 19, 2021

jichenjc commented Jul 22, 2021

hidekazuna commented Jul 22, 2021

macaptain commented Jul 22, 2021

jichenjc commented Jul 22, 2021

hidekazuna commented Jul 22, 2021

k8s-ci-robot commented Jul 22, 2021

hidekazuna commented Jul 22, 2021

✨ Add mock client and 2 tests for networking package #935

✨ Add mock client and 2 tests for networking package #935

Conversation

macaptain commented Jul 12, 2021 • edited Loading

k8s-ci-robot commented Jul 12, 2021

jichenjc commented Jul 13, 2021

lentzi90 left a comment

Choose a reason for hiding this comment

macaptain commented Jul 16, 2021

jichenjc commented Jul 19, 2021

jichenjc Jul 19, 2021

Choose a reason for hiding this comment

jichenjc commented Jul 22, 2021

hidekazuna commented Jul 22, 2021

macaptain commented Jul 22, 2021

jichenjc commented Jul 22, 2021

hidekazuna commented Jul 22, 2021

k8s-ci-robot commented Jul 22, 2021

hidekazuna commented Jul 22, 2021

macaptain commented Jul 12, 2021 •

edited

Loading