Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Unit test and builder cleanups #5465

Merged
merged 1 commit into from
Oct 22, 2021
Merged

🌱 Unit test and builder cleanups #5465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions controllers/topology/cluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ func TestClusterReconciler_reconcile(t *testing.T) {
// 1) Templates for Machine, Cluster, ControlPlane and Bootstrap.
infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(ns.Name, "inframachinetemplate").Build()
infrastructureClusterTemplate := builder.InfrastructureClusterTemplate(ns.Name, "infraclustertemplate").
// Create spec fake setting to assert that template spec is non-empty for tests.
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
controlPlaneTemplate := builder.ControlPlaneTemplate(ns.Name, "cp1").
WithInfrastructureMachineTemplate(infrastructureMachineTemplate).
Expand Down
2 changes: 0 additions & 2 deletions controllers/topology/current_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,10 @@ func TestGetCurrentState(t *testing.T) {

// InfrastructureCluster objects.
infraCluster := builder.InfrastructureCluster(metav1.NamespaceDefault, "infraOne").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()

// ControlPlane and ControlPlaneInfrastructureMachineTemplate objects.
controlPlaneInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfraTemplate").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
controlPlaneTemplateWithInfrastructureMachine := builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cpTemplateWithInfra1").
WithInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplate).
Expand Down
4 changes: 0 additions & 4 deletions controllers/topology/desired_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ var (
func TestComputeInfrastructureCluster(t *testing.T) {
// templates and ClusterClass
infrastructureClusterTemplate := builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "template1").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithInfrastructureClusterTemplate(infrastructureClusterTemplate).
Expand Down Expand Up @@ -143,7 +142,6 @@ func TestComputeControlPlaneInfrastructureMachineTemplate(t *testing.T) {
}

infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "template1").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithControlPlaneMetadata(labels, annotations).
Expand Down Expand Up @@ -215,7 +213,6 @@ func TestComputeControlPlane(t *testing.T) {
annotations := map[string]string{"a1": ""}

controlPlaneTemplate := builder.ControlPlaneTemplate(metav1.NamespaceDefault, "template1").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithControlPlaneMetadata(labels, annotations).
Expand Down Expand Up @@ -666,7 +663,6 @@ func TestComputeCluster(t *testing.T) {

func TestComputeMachineDeployment(t *testing.T) {
workerInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "linux-worker-inframachinetemplate").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
workerBootstrapTemplate := builder.BootstrapTemplate(metav1.NamespaceDefault, "linux-worker-bootstraptemplate").
Build()
Expand Down
82 changes: 25 additions & 57 deletions controllers/topology/reconcile_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ func TestReconcileCluster(t *testing.T) {

s := scope.New(tt.current)

// TODO: stop setting ResourceVersion when building objects
tt.desired.SetResourceVersion("")
s.Desired = &scope.ClusterState{Cluster: tt.desired}

r := ClusterReconciler{
Expand Down Expand Up @@ -188,8 +186,6 @@ func TestReconcileInfrastructureCluster(t *testing.T) {
s := scope.New(nil)
s.Current.InfrastructureCluster = tt.current

// TODO: stop setting ResourceVersion when building objects
tt.desired.SetResourceVersion("")
s.Desired = &scope.ClusterState{InfrastructureCluster: tt.desired}

r := ClusterReconciler{
Expand Down Expand Up @@ -266,47 +262,47 @@ func TestReconcileControlPlaneObject(t *testing.T) {
name: "Should create desired ControlPlane if the current does not exist",
class: ccWithoutControlPlaneInfrastructure,
current: nil,
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
{
name: "Fail on updating ControlPlaneObject with incompatible changes, here a different Kind for the infrastructureMachineTemplate",
class: ccWithoutControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane2, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane2.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: true,
},
{
name: "Update to ControlPlaneObject with no update to the underlying infrastructure",
class: ccWithoutControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: infrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
{
name: "Update to ControlPlaneObject with underlying infrastructure.",
class: ccWithControlPlaneInfrastructure,
current: &scope.ControlPlaneState{InfrastructureMachineTemplate: nil},
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
{
name: "Update to ControlPlaneObject with no underlying infrastructure",
class: ccWithoutControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlane1},
desired: &scope.ControlPlaneState{Object: controlPlane3},
want: &scope.ControlPlaneState{Object: controlPlane3},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy()},
wantErr: false,
},
{
name: "Preserve specific changes to the ControlPlaneObject",
class: ccWithoutControlPlaneInfrastructure,
current: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
}
Expand Down Expand Up @@ -338,13 +334,6 @@ func TestReconcileControlPlaneObject(t *testing.T) {
WithObjects(fakeObjs...).
Build()

// TODO: stop setting ResourceVersion when building objects
if tt.desired.InfrastructureMachineTemplate != nil {
tt.desired.InfrastructureMachineTemplate.SetResourceVersion("")
}
if tt.desired.Object != nil {
tt.desired.Object.SetResourceVersion("")
}
r := ClusterReconciler{
Client: fakeClient,
}
Expand Down Expand Up @@ -393,10 +382,8 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) {

// Create InfrastructureMachineTemplates for test cases
infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()
infrastructureMachineTemplate2 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra2").
WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}).
Build()

// Create the blueprint mandating controlPlaneInfrastructure.
Expand Down Expand Up @@ -443,22 +430,22 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) {
}{
{
name: "Create desired InfrastructureMachineTemplate where it doesn't exist",
current: &scope.ControlPlaneState{Object: controlPlane1},
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
wantErr: false,
},
{
name: "Update desired InfrastructureMachineTemplate connected to controlPlane",
current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: updatedInfrastructureMachineTemplate},
want: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: updatedInfrastructureMachineTemplate},
wantErr: false,
},
{
name: "Fail on updating infrastructure with incompatible changes",
current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate},
desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: incompatibleInfrastructureMachineTemplate},
current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()},
desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: incompatibleInfrastructureMachineTemplate},
wantErr: true,
},
}
Expand All @@ -481,13 +468,6 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) {
WithObjects(fakeObjs...).
Build()

// TODO: stop setting ResourceVersion when building objects
if tt.desired.InfrastructureMachineTemplate != nil {
tt.desired.InfrastructureMachineTemplate.SetResourceVersion("")
}
if tt.desired.Object != nil {
tt.desired.Object.SetResourceVersion("")
}
r := ClusterReconciler{
Client: fakeClient,
}
Expand Down Expand Up @@ -732,12 +712,6 @@ func TestReconcileMachineDeployments(t *testing.T) {
s := scope.New(builder.Cluster(metav1.NamespaceDefault, "cluster-1").Build())
s.Current.MachineDeployments = currentMachineDeploymentStates

// TODO: stop setting ResourceVersion when building objects
for _, md := range tt.desired {
md.Object.SetResourceVersion("")
md.BootstrapTemplate.SetResourceVersion("")
md.InfrastructureMachineTemplate.SetResourceVersion("")
}
s.Desired = &scope.ClusterState{MachineDeployments: toMachineDeploymentTopologyStateMap(tt.desired)}

r := ClusterReconciler{
Expand Down Expand Up @@ -779,11 +753,8 @@ func TestReconcileMachineDeployments(t *testing.T) {
}, &gotBootstrapTemplate)

g.Expect(err).ToNot(HaveOccurred())
// We don't want to compare resourceVersions as they are slightly different between the test cases
// and it's not worth the effort.
gotBootstrapTemplate.SetResourceVersion("")
wantMachineDeploymentState.BootstrapTemplate.SetResourceVersion("")
g.Expect(gotBootstrapTemplate).To(Equal(*wantMachineDeploymentState.BootstrapTemplate))

g.Expect(&gotBootstrapTemplate).To(EqualObject(wantMachineDeploymentState.BootstrapTemplate, IgnoreAutogeneratedMetadata))

// Check BootstrapTemplate rotation if there was a previous MachineDeployment/Template.
if currentMachineDeploymentState != nil && currentMachineDeploymentState.BootstrapTemplate != nil {
Expand All @@ -806,11 +777,8 @@ func TestReconcileMachineDeployments(t *testing.T) {
}, &gotInfrastructureMachineTemplate)

g.Expect(err).ToNot(HaveOccurred())
// We don't want to compare resourceVersions as they are slightly different between the test cases
// and it's not worth the effort.
gotInfrastructureMachineTemplate.SetResourceVersion("")
wantMachineDeploymentState.InfrastructureMachineTemplate.SetResourceVersion("")
g.Expect(gotInfrastructureMachineTemplate).To(Equal(*wantMachineDeploymentState.InfrastructureMachineTemplate))

g.Expect(&gotInfrastructureMachineTemplate).To(EqualObject(wantMachineDeploymentState.InfrastructureMachineTemplate, IgnoreAutogeneratedMetadata))

// Check InfrastructureMachineTemplate rotation if there was a previous MachineDeployment/Template.
if currentMachineDeploymentState != nil && currentMachineDeploymentState.InfrastructureMachineTemplate != nil {
Expand Down
49 changes: 44 additions & 5 deletions internal/builder/builders.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,14 @@ func InfrastructureMachineTemplate(namespace, name string) *InfrastructureMachin
}
}

// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object.
// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
// "spec.version": "v1.2.3",
// }.
func (i *InfrastructureMachineTemplateBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureMachineTemplateBuilder {
i.specFields = fields
return i
Expand All @@ -359,6 +366,9 @@ func (i *InfrastructureMachineTemplateBuilder) Build() *unstructured.Unstructure
obj.SetNamespace(i.namespace)
obj.SetName(i.name)

// Initialize the spec.template.spec to make the object valid in reconciliation.
killianmuldoon marked this conversation as resolved.
Show resolved Hide resolved
setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}})

setSpecFields(obj, i.specFields)
return obj
}
Expand All @@ -385,6 +395,9 @@ func (b *BootstrapTemplateBuilder) Build() *unstructured.Unstructured {
obj.SetNamespace(b.namespace)
obj.SetName(b.name)

// Initialize the spec.template.spec to make the object valid in reconciliation.
setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}})

return obj
}

Expand All @@ -403,7 +416,14 @@ func InfrastructureClusterTemplate(namespace, name string) *InfrastructureCluste
}
}

// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object.
// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
// "spec.version": "v1.2.3",
// }.
func (i *InfrastructureClusterTemplateBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureClusterTemplateBuilder {
i.specFields = fields
return i
Expand All @@ -417,6 +437,9 @@ func (i *InfrastructureClusterTemplateBuilder) Build() *unstructured.Unstructure
obj.SetNamespace(i.namespace)
obj.SetName(i.name)

// Initialize the spec.template.spec to make the object valid in reconciliation.
setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}})

setSpecFields(obj, i.specFields)

return obj
Expand All @@ -438,7 +461,14 @@ func ControlPlaneTemplate(namespace, name string) *ControlPlaneTemplateBuilder {
}
}

// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object.
// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
// "spec.version": "v1.2.3",
// }.
func (c *ControlPlaneTemplateBuilder) WithSpecFields(fields map[string]interface{}) *ControlPlaneTemplateBuilder {
c.specFields = fields
return c
Expand All @@ -458,6 +488,9 @@ func (c *ControlPlaneTemplateBuilder) Build() *unstructured.Unstructured {
obj.SetNamespace(c.namespace)
obj.SetName(c.name)

// Initialize the spec.template.spec to make the object valid in reconciliation.
setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}})

setSpecFields(obj, c.specFields)

if c.infrastructureMachineTemplate != nil {
Expand All @@ -475,7 +508,14 @@ type InfrastructureClusterBuilder struct {
specFields map[string]interface{}
}

// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object.
// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
// "spec.version": "v1.2.3",
// }.
func (i *InfrastructureClusterBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureClusterBuilder {
i.specFields = fields
return i
Expand Down Expand Up @@ -628,7 +668,6 @@ func (m *MachineDeploymentBuilder) WithGeneration(generation int64) *MachineDepl
}

// WithStatus sets the passed status object as the status of the machine deployment object.
// TODO (killianmuldoon): Revise making this method consistent with WithSpec fields in objectbuilders.
func (m *MachineDeploymentBuilder) WithStatus(status clusterv1.MachineDeploymentStatus) *MachineDeploymentBuilder {
m.status = &status
return m
Expand Down