Refactor GCEClient: wrap compute.Service in an interface for mocking … #73
Conversation
k8s-ci-robot
added
size/L
|
/test all |
|
/retest |
|
@spew: you can't request testing unless you are a kubernetes or kubernetes-sigs member. In response to this:
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. |
|
/retest |
| } | ||
|
|
||
| func NewComputeService(client *http.Client) (*ComputeService, error) { | ||
| return newComputeService(client) |
There was a problem hiding this comment.
You should just move the body of newComputeService here.
There was a problem hiding this comment.
Removed, this was an artifact of the previous interface approach.
There was a problem hiding this comment.
Note this is another good thing about not locating the interface in computeservice.go.
| } | ||
| mux.Handle("/compute/v1/projects/projectName/global/images/imageName", handler(nil, &responseImage)) | ||
| image, err := client.ImagesGet("projectName", "imageName") | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
If you're going to use an assertion library, at least provide useful failure messages. This is hardly helpful:
func TestFoo(t *testing.T) {
x := 1
y := 2
assert.Equal(t, x, y)
} go test
--- FAIL: TestFoo (0.00s)
Error Trace:
Error: Not equal: 1 (expected)
!= 2 (actual)Now for a more opinionated comment, please don't use assertion libraries if you can. This a request, not a mandate. I would state my reasons, but the Go team sums them up pretty neatly: https://golang.org/doc/faq#testing_framework.
There was a problem hiding this comment.
I think the go testing library does support letting the test finish and then all failed assertions will print which is one of the main points talked about in that FAQ. I sort of understand the anti-assertion library sentiment. I think one good thing about using the library is that the error messages are consistent and easy to scan & read. However, as you pointed out, that's not very useful if the failure messages aren't populated in which case there is a bit of a custom message anyway.
I was looking through some of the k8s.io libraries and it seems that there is a mix of using the assert library and implementing the checks. I'll go with what you suggested and rework these tests to not use assert since it seems that is somewhat the norm.
|
Updated with the following changes:
|
|
Been out -- rebased on top of the latest commits so this can be reviewed again. |
| "net/url" | ||
| ) | ||
|
|
||
| type ComputeService struct { |
There was a problem hiding this comment.
Could you provide documentation for all the public types?
There was a problem hiding this comment.
Updating with documentation.
There was a problem hiding this comment.
I've updated with some minimal documentation that I thought was appropriate, happy to change or add more if you have something different in mind.
| @@ -0,0 +1,156 @@ | |||
| package clients_test | |||
There was a problem hiding this comment.
Are we testing our code here? Or are we testing the GCE client code?
There was a problem hiding this comment.
It is possible that we call the wrong underlying GCE client method or that we do not properly hit the specified endpoint. We are testing that each ComputeService method calls the correct underlying GCE client method at the endpoint that we specified when creating the ComputeService.
…GCP compute This change creates a ComputeService implementation which has a runtime implementation that wraps the compute.Service. The MachineActuator is changed to make use of the ComputeService through a new interface named GCEClientComputeService. This will enable creating tests that mock GCP Compute Service calls to control MachineActuator behavior.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: krousey, spew 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
….2-cluster-api-0.1.0 Rebase openshift 4.2 cluster api 0.1.0
…GCP compute
This change creates a ComputeService implementation which has a runtime
implementation that wraps the compute.Service. The MachineActuator is
changed to make use of the ComputeService through a new interface named
GCEClientComputeService. This will enable creating tests that mock GCP Compute
Service calls to control MachineActuator behavior.
What this PR does / why we need it:
It is difficult to test changes that interact with GCP without manually running integration tests. This change will enable developers to write unit tests that mock GCP and test that their code correctly handles various scenarios.
Release note:
@kubernetes/kube-deploy-reviewers