Add unit test for machine controller reconcile #650

enxebre · 2018-12-20T17:55:32Z

What this PR does / why we need it:
Add uni test for machine controller reconcile

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

k8s-ci-robot · 2018-12-20T17:55:45Z

Hi @enxebre. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

davidewatson · 2018-12-20T22:06:52Z

/lgtm

davidewatson · 2018-12-20T22:19:52Z

@enxebre: This appears to conflict with an existing tests:

W1220 22:09:24.921] pkg/controller/machine/machine_controller_test.go:38:23: TestReconcile redeclared in this block
W1220 22:09:24.922] 	previous declaration at pkg/controller/machine/controller_test.go:31:23

roberthbailey · 2018-12-21T08:17:17Z

/ok-to-test

roberthbailey

The existing function is at https://github.com/kubernetes-sigs/cluster-api/blob/master/pkg/controller/machine/machine_controller_test.go#L38

roberthbailey · 2018-12-21T08:18:07Z

pkg/controller/machine/controller.go

+			},
+		},
+	}
+	err := c.Client.List(ctx, listOptions, &clusterList)


nit: This should be cuddled, e.g. if err := c.Client.List(...); err != nil { ... }

enxebre · 2018-12-21T08:35:58Z

@roberthbailey I changed the unit introduced here to TestReconcileRequest please let me know if anyone has a better suggestion

detiber · 2018-12-21T15:06:02Z

/lgtm

vincepri

/lgtm

vincepri · 2018-12-21T16:00:06Z

/assign @roberthbailey @justinsb

ingvagabund · 2018-12-24T15:23:33Z

pkg/controller/machine/controller_test.go

+)
+
+func TestReconcileRequest(t *testing.T) {
+	m1 := v1alpha1.Machine{


Please, use at least three letters long identifiers:

s/m1/machine1/

s/m2/machine2/

s/m3/machine3/

s/c/cluster/

s/cl/clusterList/

E.g. c identifies are very hard to find (as other identifiers contain the c letter), m1 (m, one) tends to look like (m, l), the same for cl (c, l) as (c, one).

ingvagabund · 2018-12-24T15:26:12Z

pkg/controller/machine/controller_test.go

+			DeletionTimestamp: &time,
+		},
+	}
+	c := v1alpha1.Cluster{


Not sure if it is used only once. Though, if so, it can be embedded into ClusterList.

ingvagabund · 2018-12-24T15:27:03Z

pkg/controller/machine/controller_test.go

+	}
+	testCases := []struct {
+		request     reconcile.Request
+		ExistsValue bool


Does the field need to be exported? existsValue will do as well otherwise.

ingvagabund · 2018-12-24T15:31:32Z

pkg/controller/machine/controller_test.go

+	}
+
+	for _, tc := range testCases {
+		a := newTestActuator()


s/a/act, s/a/actuator or s/a/tesActuator

ingvagabund · 2018-12-28T14:13:52Z

Can you also extend testactuator.go with:

var _ Actuator = &TestActuator{}

? So the fact that TestActuator implements Actuator interface is checked during compilation.

roberthbailey · 2019-01-02T17:03:22Z

It looks like there are a couple of outstanding comments from @ingvagabund. Can you address / resolve those and then we can merge this.

enxebre · 2019-01-03T09:43:00Z

/test pull-cluster-api-test

ingvagabund · 2019-01-04T11:43:41Z

/lgtm

roberthbailey · 2019-01-08T20:08:53Z

/approve

k8s-ci-robot · 2019-01-08T20:09:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre, roberthbailey

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:

~~OWNERS~~ [roberthbailey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Update CAPBK to v0.1.5

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 20, 2018

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 20, 2018

k8s-ci-robot requested review from krisnova and roberthbailey December 20, 2018 17:55

k8s-ci-robot assigned davidewatson Dec 20, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2018

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 21, 2018

roberthbailey reviewed Dec 21, 2018

View reviewed changes

enxebre force-pushed the machine-reconcile-test branch from e874214 to 9efd834 Compare December 21, 2018 08:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2018

k8s-ci-robot assigned detiber Dec 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2018

enxebre force-pushed the machine-reconcile-test branch from 9efd834 to 6ec3bdd Compare December 21, 2018 15:55

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2018

vincepri approved these changes Dec 21, 2018

View reviewed changes

k8s-ci-robot assigned vincepri Dec 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2018

k8s-ci-robot assigned justinsb and roberthbailey Dec 21, 2018

ingvagabund reviewed Dec 24, 2018

View reviewed changes

enxebre force-pushed the machine-reconcile-test branch from 6ec3bdd to 63b02d9 Compare January 2, 2019 11:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2019

enxebre force-pushed the machine-reconcile-test branch from 63b02d9 to 16031bd Compare January 3, 2019 09:03

Add unit test for machine controller reconcile

abc48d6

enxebre force-pushed the machine-reconcile-test branch from 16031bd to abc48d6 Compare January 3, 2019 10:03

k8s-ci-robot assigned ingvagabund Jan 4, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2019

k8s-ci-robot merged commit 59a129b into kubernetes-sigs:master Jan 8, 2019

jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020

Merge pull request kubernetes-sigs#650 from andrewsykim/cabpk-0.1.5

2d56142

Update CAPBK to v0.1.5

enxebre mentioned this pull request Feb 5, 2020

REQUEST: New membership for enxebre kubernetes/org#1614

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit test for machine controller reconcile #650

Add unit test for machine controller reconcile #650

enxebre commented Dec 20, 2018

k8s-ci-robot commented Dec 20, 2018

davidewatson commented Dec 20, 2018

davidewatson commented Dec 20, 2018

roberthbailey commented Dec 21, 2018

roberthbailey left a comment

roberthbailey Dec 21, 2018

enxebre commented Dec 21, 2018 •

edited

Loading

detiber commented Dec 21, 2018

vincepri left a comment

vincepri commented Dec 21, 2018

ingvagabund Dec 24, 2018

ingvagabund Dec 24, 2018

ingvagabund Dec 24, 2018

ingvagabund Dec 24, 2018

ingvagabund commented Dec 28, 2018

roberthbailey commented Jan 2, 2019

enxebre commented Jan 3, 2019

ingvagabund commented Jan 4, 2019

roberthbailey commented Jan 8, 2019

k8s-ci-robot commented Jan 8, 2019

Add unit test for machine controller reconcile #650

Add unit test for machine controller reconcile #650

Conversation

enxebre commented Dec 20, 2018

k8s-ci-robot commented Dec 20, 2018

davidewatson commented Dec 20, 2018

davidewatson commented Dec 20, 2018

roberthbailey commented Dec 21, 2018

roberthbailey left a comment

Choose a reason for hiding this comment

roberthbailey Dec 21, 2018

Choose a reason for hiding this comment

enxebre commented Dec 21, 2018 • edited Loading

detiber commented Dec 21, 2018

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Dec 21, 2018

ingvagabund Dec 24, 2018

Choose a reason for hiding this comment

ingvagabund Dec 24, 2018

Choose a reason for hiding this comment

ingvagabund Dec 24, 2018

Choose a reason for hiding this comment

ingvagabund Dec 24, 2018

Choose a reason for hiding this comment

ingvagabund commented Dec 28, 2018

roberthbailey commented Jan 2, 2019

enxebre commented Jan 3, 2019

ingvagabund commented Jan 4, 2019

roberthbailey commented Jan 8, 2019

k8s-ci-robot commented Jan 8, 2019

enxebre commented Dec 21, 2018 •

edited

Loading