Add unit test for scalable node group controller

[prateekgogia]

Description of changes:
Add unit test for scalable node group controller

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[ellistarn]

IMO, all this logic should go in a before each

[prateekgogia]

There is some duplication I removed that from here. Kept reseting nodeGroupObject, to be specific that we are reseting the nodeGroupObject after every run. Let me know, if you still think it might be better moving to BeforeEach func.

[ellistarn]

+1

[ellistarn]

Consider more idiomatic test naming with "It".

e.g. "it should scale up nodes", where the test name is "should scale up nodes".

[ellistarn]

I'm not sure I see the difference between Basic and Advanced tests, they share all Before/After hooks and neither context has variables. Does it make sense to just collapse them?

[prateekgogia]

Yes, when I started I though I will be adding some complex test but it didn't make sense while writing tests. So yes, I will combine them.

[ellistarn]

fake.fakeError stutters. Consider fake.RetriableError

[ellistarn]

I was gonna suggest retriable, but it looks like this is a fairly deep rabbit hole. I think retryable is actually correct -- go figured. Either way, A(ble) shouldn't be capitalized.

[ellistarn]

I'm not sure of the benefit here... Looks like you're creating a singleton pattern for nodegroup such that we always return the same object?

[prateekgogia]

Yes, trying to reuse the same nodegroup object because when controller.reconcile calls c.CloudProvider.NodeGroupFor it returns a new nodegroup object here which cannot be referenced in tests and can't verify if the replicas set are correct since the object changes.

[ellistarn]

Let's figure out a better pattern than casting.

[ellistarn]

I'm having a little trouble following the flow of the BeforeEach, Test, AfterEach logic. It looks like some logic is duplicated (i.e. sng.Spec.Replicas being set).

When I write these, I typically try to do everything in a BeforeEach loop, unless my test has mutated something that needs to be cleaned up after all of the tests.

I also think it might be cleaner to modify the fields of the CloudProvider factory, rather than using a singleton pattern on nodegroup and modifying that object directly. I think all you'd have to do is wire through the behavior of NodeGroupStabilized and NodeGroupStabilizedMessage to the CloudProvider factory, and then before each test, you can just do something like

factory.NodeGroupStabilized = false 
factory.NodeGroupStabilizedMessage = "still trying"

Then when the factory creates the object, it will inject the behavior into the node group and should clean up most of your singleton/casting logic.

Let me know what you think to this approach.

[ellistarn]

mind adding a comment here about the race condition? Just to warn someone who might try to simplify it

fakeCloudProvider.NodeReplicas[sng.Spec.ID] = ptr.Int32(*sng.Spec.Replicas) # Create a new pointer to avoid races with the controller

[ellistarn]

IMO we should just configure this explicitly in each test.

[prateekgogia]

The reason I am setting this here is, because I want to avoid setting sng.Spec.Replicas to default value 0, we might miss some cases where values are not set and we just test the default. May be, it will be better if I rename it to defaultReplicaCount?

[ellistarn]

Sounds good!

[ellistarn]

Thought variable rename isn't necessary. I think it's good as is.

[ellistarn]

I'd either make this a file level constant, or probably cleaner to just inline.

[ellistarn]

I don't understand why this is necessary.

[prateekgogia]

Yeah, I can probably get away with it.

[prateekgogia]

@ellistarn I addressed all the comments, PTAL.

[ellistarn]

if I understand correctly, sng.Spec.ID is nil here, so this line is just muddying the water.

[prateekgogia]

sng.Spec.ID is an empty string.
This is required here, since, in my tests also sng.Spec.ID is always an empty string and NodeReplicas[""] is a Nil value if I don't set it which causes nodegroup.Replicas to be also Nil.

[ellistarn]

Nice work. Ready to approve after this around.

[JacobGabrielson]

Looks good overall, just a few comments to add

[ellistarn]

This is never reassigned, so consider making this

defaultReplicas := 3
or
defaultRepliacs := ptr.Int32(3)

I'd avoid relying on ptr if you can just use native go.

[prateekgogia]

What's the difference in these two declarations?

var defaultReplicas  = ptr.Int32(3)
defaultRepliacs := ptr.Int32(3)

IIUC, latter is just a short method of doing what we are doing with var statement, am I missing something?

[ellistarn]

From the go docs The := syntax is shorthand for declaring and initializing a variable, e.g. for var f string = "apple" in this case.. Fine either way. I tend to use the shorthand.

[ellistarn]

Consider embedding a fake error message into the fake.Factory implementation so that tests don't need to configure it.

Maybe even make a public const that tests can reference

const ErrorMessage = "fake factory failed"

[prateekgogia]

Are you suggesting doing something like this-

const (
	ErrMessage = ""
)

func NewFactory(options cloudprovider.Options) *Factory {
	return &Factory{
		NodeReplicas:     make(map[string]*int32),
		NodeGroupStable:  true,
		NodeGroupMessage: ErrMessage,
	}
}

and in tests we will be doing this -

fake.ErrMessage = dummyMessage

I am not sure how does this help?

[ellistarn]

In the test we wouldn't need to do fake.ErrMessage = dummyMessage. You would only return the ErrMessage if NodeGroupStable is false. Either way works, but the former results in less test code.