test: add VMSS client interfaces and mocks to make upgrade testable #590
Conversation
Codecov Report
@@ Coverage Diff @@
## master #590 +/- ##
==========================================
- Coverage 56.7% 56.68% -0.02%
==========================================
Files 91 91
Lines 13898 13902 +4
==========================================
Hits 7881 7881
- Misses 5352 5355 +3
- Partials 665 666 +1 |
serbrech
force-pushed
the
vmss-interface-mocks
branch
from
d2f5005
to
4175ba0
Compare
| @@ -36,13 +36,15 @@ func (az *AzureClient) DeleteVirtualMachine(ctx context.Context, resourceGroup, | |||
| } | |||
|
|
|||
| // ListVirtualMachineScaleSets returns (the first page of) the vmss resources in the specified resource group. | |||
| func (az *AzureClient) ListVirtualMachineScaleSets(ctx context.Context, resourceGroup string) (compute.VirtualMachineScaleSetListResultPage, error) { | |||
| return az.virtualMachineScaleSetsClient.List(ctx, resourceGroup) | |||
| func (az *AzureClient) ListVirtualMachineScaleSets(ctx context.Context, resourceGroup string) (VirtualMachineScaleSetListResultPage, error) { | |||
There was a problem hiding this comment.
FYI this will require a change to AKS implementation, as it consumes this client+method.
There was a problem hiding this comment.
It shouldn't cause a problem as the interface is 1-1 with the compute type, and all other methods currently depend on these interfaces. If we're lucky, the import will already be present.
Otherwise, good reminder to always depend on our interface types and never on the compute types...
| } | ||
|
|
||
| // ListVirtualMachineScaleSetVMs returns the list of VMs per VMSS | ||
| func (az *AzureClient) ListVirtualMachineScaleSetVMs(ctx context.Context, resourceGroup, virtualMachineScaleSet string) (compute.VirtualMachineScaleSetVMListResultPage, error) { | ||
| return az.virtualMachineScaleSetVMsClient.List(ctx, resourceGroup, virtualMachineScaleSet, "", "", "") | ||
| func (az *AzureClient) ListVirtualMachineScaleSetVMs(ctx context.Context, resourceGroup, virtualMachineScaleSet string) (VirtualMachineScaleSetVMListResultPage, error) { |
| } | ||
|
|
||
| //MakeFakeVirtualMachine returns a fake compute.VirtualMachine | ||
| func (mc *MockAKSEngineClient) MakeFakeVirtualMachine() compute.VirtualMachine { |
There was a problem hiding this comment.
nit: since this is already a mock function, using "fake" in the name is redundant. Also slight preference for mock over fake in comment wording.
There was a problem hiding this comment.
as I already commented before, the mockClient stubs functions like CreateVirtualMachine that are real function on the service. This is a helper builder function that return a default instance of a VirtualMachine. the Fake part of the name is to distinguish it from the functions exposed by the actual service that we mock
There was a problem hiding this comment.
So you're saying existing MockAKSEngineClient methods (e.g., ListVirtualMachineScaleSets) represent mock equivalents of identically names methods in the real client libraries?
If that's the case, I'd argue we don't actually want to be adding these "fake" methods onto pre-existing mock libraries that fit that description, and better to create static functions that access the mock objects in order to return an appropriate fake, single-use object for testing.
I don't want to block progress, but something to think about as the mock + fake semantics are arguably a bit confusing.
There was a problem hiding this comment.
yes, I extracted this for reusability as it was inlined in the mockclient code and duplicated. I first wanted it standalone, but it relied on other parameters from the mockClient and so, I decided to keep it there for simplicity. it is used by the client to generate the faked results, and it can be used from the tests to build dummy objects.
|
s/Vmss/VMSS in the title/commit message for release notes |
|
lgtm besides some minor comments on wording/formatting |
|
back-compat tests passed |
serbrech
force-pushed
the
vmss-interface-mocks
branch
from
4175ba0
to
87ebb3f
Compare
squashed the 2 commits and adjusted the comment |
serbrech
force-pushed
the
vmss-interface-mocks
branch
from
87ebb3f
to
bf359d0
Compare
CecileRobertMichon
changed the title
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, serbrech 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 |
Reason for Change:
This is extracting the vmss interfaces and mocks from the
upgrade --force PRto reduce its footprint.One test is carried over, which I just marked as skipped here as it lacks the implementation to pass