Add mocking to Cyclops #85

vincentportella · 2024-08-09T03:10:31Z

Adding a new mocking package to help test individual phases of Cyclops in a repeatable manner. This PR only includes tests for the Pending phase and includes the required functionality for that.
Fixing an issue where if no nodegroups names are given then all nodegroups are determined to be matching.

awprice · 2024-08-09T03:17:48Z

pkg/cloudprovider/aws/builder.go

@@ -40,3 +42,11 @@ func NewCloudProvider(logger logr.Logger) (cloudprovider.CloudProvider, error) {

 	return p, nil
 }
+
+// NewMockCloudProvider returns a new mock AWS cloud provider
+func NewMockCloudProvider(autoscalingiface autoscalingiface.AutoScalingAPI, ec2iface ec2iface.EC2API) cloudprovider.CloudProvider {


Is this really returning a new mock AWS cloud provider or just a new provider that could either be the real thing or mocked depending on what is passed in?

Yeah I think this doesn't have to be NewMockCloudProvider but could just be NewCloudProvider where the two interfaces passed in are the mocks.

Good point, it's not necessarily mocking.

awprice · 2024-08-09T03:23:09Z

pkg/mock/phase_pending_test.go

Is this an appropriate location in the codebase for testing the Pending phase? I'd have thought these tests would be better located with the phase implementations themselves.

IMO the mock package should only contain things to do with creating / setting up / tearing down mocks. Tests themselves belong with the code file that implements them.

Unfortunately that leads to an import cycle so this is the best place I could think of putting it

The mock for an individual piece of functionality can live next to the code too, I've seen this happen in quite a few code bases. That avoids the import cycle. You just import generic mocking functionality from the central package in that situation.

Unfortunately that leads to an import cycle so this is the best place I could think of putting it

If this is occurring and as @mwhittington21 suggests we'll need to make the mocking package more generic and moving mocking logic to where it makes sense (probably next to the thing it is mocking)

awprice · 2024-08-09T03:27:24Z

pkg/mock/phase_pending_test.go

+					},
+				},
+				NodeNames: []string{
+					"node-0",


I guess one nit here is in this test we're assuming that when you create a test node group with newNodegroup() that nodes follow this naming scheme.

Someone modifying newNodegroup() may find they break a ton of tests if they decide to change an assumption on how test node groups are created

You could possibly remedy this by adding a node that obviously would very obviously never exist in the NodeNames field

awprice · 2024-08-09T03:30:51Z

pkg/mock/phase_pending_test.go

+	assert.NoError(t, err)
+	assert.Equal(t, v1.CycleNodeRequestPending, mockTransitioner.Cnr.Status.Phase)
+
+	// Simulate waiting for 1 day, this is more than the waiting limit


Are we assuming the waiting limit is always < 24 hours? Can we make this an input into the test, so that someone changing the wait limit doesn't break unrelated tests

awprice · 2024-08-09T03:34:15Z

pkg/test/aws.go

-}
-
-// SetDesiredCapacity mock implementation for MockAutoscalingService
-func (m MockAutoscalingService) SetDesiredCapacity(*autoscaling.SetDesiredCapacityInput) (*autoscaling.SetDesiredCapacityOutput, error) {


I notice we've not implemented functions like this in the new mock AWS - do we not need them?

I will implement functionality as needed. As I add new features in upcoming PRs I will add the functionality for the mocking to support the appropriate tests.

mwhittington21 · 2024-08-09T03:35:39Z

pkg/mock/util.go

Call this file test_helpers.go or something so more things don't pile into it over time. util.go is likely to attract more cruft.

mwhittington21 · 2024-08-09T03:38:56Z

pkg/cloudprovider/aws/builder.go

@@ -40,3 +42,11 @@ func NewCloudProvider(logger logr.Logger) (cloudprovider.CloudProvider, error) {

 	return p, nil
 }
+
+// NewMockCloudProvider returns a new mock AWS cloud provider
+func NewMockCloudProvider(autoscalingiface autoscalingiface.AutoScalingAPI, ec2iface ec2iface.EC2API) cloudprovider.CloudProvider {


Yeah I think this doesn't have to be NewMockCloudProvider but could just be NewCloudProvider where the two interfaces passed in are the mocks.

mwhittington21 · 2024-08-09T03:39:40Z

pkg/mock/phase_pending_test.go

IMO the mock package should only contain things to do with creating / setting up / tearing down mocks. Tests themselves belong with the code file that implements them.

mwhittington21 · 2024-08-09T03:41:55Z

pkg/mock/util.go

+		}
+
+		node := &Node{
+			Name:       fmt.Sprintf("node-%d", i),


If I create two NodeGroups for a test, is there a problem if the groups contain the same named nodes? With the current implementation this will happen. I suspect this is not intentional, and the node names should probably be generated such that they aren't shared between NodeGroups.

Good point, I can add the nodegroup name to the node name to help get around this.

mwhittington21 · 2024-08-09T03:42:31Z

pkg/mock/util.go

+	nodes := make([]*Node, 0)
+
+	for i := 0; i < num; i++ {
+		instanceID, err := generateRandomInstanceId()


If you're ever doing a test that wants to look for specific instance IDs you will want a way to customise this. Not sure if it's worth adding now or not.

This is more of a helper, instance IDs can be made custom when someone would want to create a node without it.

awprice · 2024-08-12T05:30:01Z

pkg/mock/client.go

+	return nil
+}
+
+func generateFakeInstances(nodes []*Node) *[]*fakeaws.Instance {


Is there a reason to returning a pointer to a slice?

awprice · 2024-08-12T05:32:11Z

pkg/cloudprovider/aws/fake/fake.go

+type Ec2 struct {
+	ec2iface.EC2API
+
+	Instances *[]*Instance


Related to my other comment on pointer to a slice - why?

It was to take into terminating instances using the ec2 instance and then describing asgs using the autoscaling instance and I want them to be in sync. I've done some testing and changed this to a map to make it easier to manage those fake instances in the future and it works as I'd like.

pkg/cloudprovider/aws/builder.go

Add mocking to Cyclops

d53da3d

vincentportella requested review from awprice, Jacobious52, cespo, mwhittington21, MinyiZ, hyang200 and vcatlassian August 9, 2024 03:10

Undo go.mod and go.sum changes

3761976

vincentportella self-assigned this Aug 9, 2024

vincentportella added 2 commits August 9, 2024 13:20

remove launchTemplates from mock client

789eb6d

remove commented code

0f34a56

awprice reviewed Aug 9, 2024

View reviewed changes

mwhittington21 reviewed Aug 9, 2024

View reviewed changes

vincentportella added 2 commits August 12, 2024 14:51

Refactor mocking to split functionality across packages

0c3a826

Move tests into transitioner package

9435eef

vincentportella requested review from awprice and mwhittington21 August 12, 2024 05:28

awprice reviewed Aug 12, 2024

View reviewed changes

Changes instances in fake aws implementation to map

4156abb

awprice reviewed Aug 12, 2024

View reviewed changes

pkg/cloudprovider/aws/builder.go Outdated Show resolved Hide resolved

Removed leftover commented code

f3f0ef4

vincentportella requested a review from awprice August 12, 2024 06:06

awprice approved these changes Aug 12, 2024

View reviewed changes

MinyiZ approved these changes Aug 12, 2024

View reviewed changes

vincentportella merged commit 81491a7 into master Aug 12, 2024
3 checks passed

vincentportella deleted the vportella/add-mocking-to-cyclops branch August 12, 2024 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mocking to Cyclops #85

Add mocking to Cyclops #85

vincentportella commented Aug 9, 2024

awprice Aug 9, 2024

mwhittington21 Aug 9, 2024

vincentportella Aug 9, 2024

awprice Aug 9, 2024

mwhittington21 Aug 9, 2024

vincentportella Aug 9, 2024

mwhittington21 Aug 9, 2024

awprice Aug 9, 2024

awprice Aug 9, 2024

awprice Aug 9, 2024

awprice Aug 9, 2024

awprice Aug 9, 2024

vincentportella Aug 9, 2024

mwhittington21 Aug 9, 2024

mwhittington21 Aug 9, 2024

mwhittington21 Aug 9, 2024

mwhittington21 Aug 9, 2024

vincentportella Aug 9, 2024

mwhittington21 Aug 9, 2024

vincentportella Aug 9, 2024 •

edited

Loading

awprice Aug 12, 2024

awprice Aug 12, 2024

vincentportella Aug 12, 2024 •

edited

Loading

Add mocking to Cyclops #85

Add mocking to Cyclops #85

Conversation

vincentportella commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentportella Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentportella Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

vincentportella Aug 9, 2024 •

edited

Loading

vincentportella Aug 12, 2024 •

edited

Loading