Unit test: Verify hc feature-gate disable after enable scenario #299

vikaschoudhary16 · 2019-04-25T14:32:39Z

This unit test verifies that if hc feature-gate is disabled after enabling it, hc-controller gets removed.

ingvagabund · 2019-04-25T14:36:52Z

pkg/operator/operator_test.go

 			},
 		},
 		expectedMachineHealthCheckController: true,
+	}, {


How does this verify the feature is disable after it's enabled? The operator is initialized from scratch for every test case.

indeed, this is duplicating the tc https://github.com/openshift/machine-api-operator/pull/299/files#diff-d5d9cf6e8104e5231787b5cc89fb7b52R198

michaelgugino

/lgtm

michaelgugino · 2019-04-25T14:39:48Z

/lgtm cancel

ingvagabund · 2019-04-26T12:05:17Z

pkg/operator/operator.go

+	}, nil
+}
+
+func (optr *Operator) getImages() (string, string, error) {


No need to implement and mock getImages(). You can create Configv1.Infrastructure object and have the fake client return it.

ingvagabund · 2019-04-26T12:06:56Z

vendor/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go

 		}
 		ensureContainer(modified, existingCurr, required)
 	}
+	var requiredContainers []corev1.Container


How is this change related to the unit tests?

openshift/cluster-version-operator#178

ingvagabund · 2019-04-26T12:10:56Z

pkg/operator/operator.go

-	syncHandler func(ic string) error
+	syncHandler           func(ic string) error
+	imageGetterFunc       imageGetterFuncType
+	statusProgressingFunc func() error


No need to mock optr.statusProgressing given you can have fake clients return expected objects. Please, give fake client osconfigv1.ClusterOperator object with pre-populated testing values.

Implemented here: #304

enxebre · 2019-05-14T10:27:30Z

@vikaschoudhary16 needs rebase

vikaschoudhary16 · 2019-06-28T05:24:43Z

This PR still needs https://github.com/openshift/cluster-version-operator/pull/216/files

vikaschoudhary16 · 2019-07-01T07:51:09Z

/test e2e-aws

ingvagabund · 2019-07-01T10:31:49Z

pkg/operator/operator_test.go

-			Status: v1.InfrastructureStatus{
-				Platform: v1.AWSPlatformType,
-			},
+		err := initFeatureGateInformer([]runtime.Object{tc.featureGate}, optr)


This is still wrong. All tests in the list (tests) are supposed to assume clean/vanilla environment. Initializing fg informer only once leaves previous fg object in the cache. Which might (or might not) influence result of each test in the last. If you want to assume dependency between fg objects, you need to create separate unit test for it.

ingvagabund · 2019-07-01T11:13:22Z

pkg/operator/operator_test.go

+	stopCh := make(<-chan struct{})
+	optr := newFakeOperator(nil, []runtime.Object{infra}, stopCh)
+
+	for _, tc := range tests {


Given the unit test will always have two steps (first enable fg, then disable fg), there is no need to iterate through both tests. It's sufficient to initialize newFakeOperator as it was before, remove initFeatureGateInformer and just call optr.sync two times. After the first call validating the fg is enable, after the second call validating it is not.

remove initFeatureGateInformer

This is not possible because modifying the FeatureGate object which is passed to the fake clientset, will not work. Eventually it makes a copy of the object and therefor any changes to the object will not be reflected back in the object returned by informer in response to the listing operation.

Updated the PR to address other parts. PTAL!

diff --git a/pkg/operator/operator_test.go b/pkg/operator/operator_test.go index 289f4c9..24c6688 100644 --- a/pkg/operator/operator_test.go +++ b/pkg/operator/operator_test.go @@ -15,7 +15,6 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/informers" fakekube "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" @@ -261,10 +260,11 @@ func TestEnableDisableOperatorSyncClusterAPIControllerHealthCheckController(t *t Platform: v1.AWSPlatformType, }, } - hcFeatureGate := newFeatureGate(v1.TechPreviewNoUpgrade) stopCh := make(chan struct{}) - optr := newFakeOperator(nil, []runtime.Object{hcFeatureGate, infra}, stopCh) - optr.sync("test-key") + defer close(stopCh) + + optr := newFakeOperator(nil, []runtime.Object{newFeatureGate(v1.TechPreviewNoUpgrade), infra}, stopCh) + go optr.Run(2, stopCh) if err := wait.PollImmediate(1*time.Second, 5*time.Second, func() (bool, error) { d, err := optr.deployLister.Deployments(targetNamespace).Get(deploymentName) @@ -272,28 +272,33 @@ func TestEnableDisableOperatorSyncClusterAPIControllerHealthCheckController(t *t t.Logf("Failed to get %q deployment: %v", deploymentName, err) return false, nil } - if deploymentHasContainer(d, hcControllerName) != true { + if !deploymentHasContainer(d, hcControllerName) { t.Logf("Expected deploymentHasContainer for %q container to be true", hcControllerName) return false, nil } return true, nil }); err != nil { - t.Errorf("Failed to verify %q deployment", deploymentName) + t.Fatalf("Failed to verify %q deployment", deploymentName) } - close(stopCh) - err := initFeatureGateInformer([]runtime.Object{newFeatureGate(v1.Default)}, optr) + + t.Logf("Found %q container in %q deployment", hcControllerName, deploymentName) + + hcFeatureGate, err := optr.osClient.ConfigV1().FeatureGates().Get(MachineAPIFeatureGateName, metav1.GetOptions{}) if err != nil { - t.Errorf("Failed to set featuregate 'Default' : %v", err) + t.Fatalf("Unable to get %q feature gate: %v", MachineAPIFeatureGateName, err) + } + hcFeatureGate.Spec.FeatureSet = v1.Default + if _, err := optr.osClient.ConfigV1().FeatureGates().Update(hcFeatureGate); err != nil { + t.Fatalf("Unable to update %q feature gate and set default feature set: %v", MachineAPIFeatureGateName, err) } - optr.sync("test-key") - if err := wait.PollImmediate(1*time.Second, 5*time.Second, func() (bool, error) { + if err := wait.PollImmediate(1*time.Second, 15*time.Second, func() (bool, error) { d, err := optr.deployLister.Deployments(targetNamespace).Get(deploymentName) if err != nil { t.Logf("Failed to get %q deployment: %v", deploymentName, err) return false, nil } - if deploymentHasContainer(d, hcControllerName) != false { + if deploymentHasContainer(d, hcControllerName) { t.Logf("Expected deploymentHasContainer for %q container to be false", hcControllerName) return false, nil } @@ -303,16 +308,3 @@ func TestEnableDisableOperatorSyncClusterAPIControllerHealthCheckController(t *t } } - -func initFeatureGateInformer(featureGate []runtime.Object, op *Operator) error { - configClient := fakeos.NewSimpleClientset(featureGate...) - configSharedInformer := configinformersv1.NewSharedInformerFactoryWithOptions(configClient, 2*time.Minute) - featureGateInformer := configSharedInformer.Config().V1().FeatureGates() - op.featureGateLister = featureGateInformer.Lister() - stopCh := make(<-chan struct{}) - configSharedInformer.Start(stopCh) - if !cache.WaitForCacheSync(stopCh, featureGateInformer.Informer().HasSynced) { - return fmt.Errorf("unable to wait for cache to sync") - } - return nil -}

@vikaschoudhary16 also please improve the messages to be more human readable

updated, thanks!

frobware · 2019-07-01T11:14:42Z

pkg/operator/operator_test.go

+		optr := newFakeOperator(nil, []runtime.Object{infra}, stopCh)
+		err := initFeatureGateInformer([]runtime.Object{tc.featureGate}, optr)
+		if err != nil {
+			t.Errorf("Failed to set featuregate %v : %v", tc.featureGate, err)


Isn't this fatal?

with Errorf this test will be reported failed but others will be executed. Is not that we want?

frobware · 2019-07-02T08:55:44Z

pkg/operator/operator_test.go


 	configSharedInformer.Start(stopCh)
-	kubeNamespacedSharedInformer.Start(stopCh)
+	kubeInformerstopCh := make(<-chan struct{})


Why do we need this separate channel? Shouldn't the following line use stopCh?

Agree, please undo the change.

ingvagabund · 2019-07-02T09:52:23Z

pkg/operator/operator_test.go

 }

-func newFakeOperator(kubeObjects []runtime.Object, osObjects []runtime.Object, stopCh <-chan struct{}) *Operator {
+func newFakeOperator(kubeObjects []runtime.Object, osObjects []runtime.Object, stopCh chan struct{}) *Operator {


No need for s/<-chan/chan/.

ingvagabund · 2019-07-02T10:14:01Z

pkg/operator/operator_test.go


 	configSharedInformer.Start(stopCh)
-	kubeNamespacedSharedInformer.Start(stopCh)
+	kubeInformerstopCh := make(<-chan struct{})


Agree, please undo the change.

ingvagabund · 2019-07-02T10:23:45Z

pkg/operator/operator_test.go

 			}

-			stopCh := make(<-chan struct{})
+			stopCh := make(chan struct{})


Please undo

ingvagabund · 2019-07-02T10:23:49Z

pkg/operator/operator_test.go

 		}

-		stopCh := make(<-chan struct{})
+		stopCh := make(chan struct{})


Please undo

why should not the stop channel be closed?

undo = s/<-//. Channel only for reading is all that is needed.

ingvagabund · 2019-07-02T10:24:39Z

pkg/operator/operator_test.go

+			return false, nil
+		}
+		if !deploymentHasContainer(d, hcControllerName) {
+			t.Logf("Expected deploymentHasContainer for %q container to be true", hcControllerName)


Expected to found no %q container in %q deployment, got one.

ingvagabund · 2019-07-02T10:25:01Z

pkg/operator/operator_test.go

+		}
+		return true, nil
+	}); err != nil {
+		t.Fatalf("Failed to verify %q deployment", deploymentName)


"Failed to verify %q deployment has %q container"

ingvagabund · 2019-07-02T10:25:27Z

pkg/operator/operator_test.go

+			return false, nil
+		}
+		if deploymentHasContainer(d, hcControllerName) {
+			t.Logf("Expected deploymentHasContainer for %q container to be false", hcControllerName)


Expected to found %q container in %q deployment, found none.

ingvagabund · 2019-07-02T10:25:41Z

pkg/operator/operator_test.go

+		}
+		return true, nil
+	}); err != nil {
+		t.Errorf("Failed to verify %q deployment", deploymentName)


"Failed to verify %q deployment has no %q container"

frobware · 2019-07-02T12:46:51Z

pkg/operator/operator_test.go

 }

-func newFakeOperator(kubeObjects []runtime.Object, osObjects []runtime.Object, stopCh <-chan struct{}) *Operator {
+func newFakeOperator(kubeObjects []runtime.Object, osObjects []runtime.Object, stopCh <-chan struct{}, wq string) *Operator {


what is wq?

If this is the way to fix the flake, please open another PR for it. This is irrelevant for this PR.

will it be ok if i create a new commit for this flake fix. Actually with the changes in this PR, this flake has become almost 100% reproducible and thus will be difficult to merge this PR without this fix.

How can different name for a working queue be a fix for the flake?

How does the name, or a different name, fix the flake?

Still, it's unrelated to this PR. Please, open another one.

openshift-ci-robot · 2019-07-02T14:18:22Z

@vikaschoudhary16: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/actuator-pkg-staleness	`3bc2f43`	link	`/test actuator-pkg-staleness`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

vikaschoudhary16 · 2019-07-02T14:55:35Z

/test e2e-aws-operator

ingvagabund · 2019-07-02T16:50:57Z

/approve
/lgtm

openshift-ci-robot · 2019-07-02T16:51:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ingvagabund]

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

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 25, 2019

openshift-ci-robot requested review from bison and spangenberg April 25, 2019 14:33

ingvagabund reviewed Apr 25, 2019

View reviewed changes

michaelgugino approved these changes Apr 25, 2019

View reviewed changes

openshift-ci-robot assigned michaelgugino Apr 25, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2019

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2019

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from fe3595f to b75620e Compare April 26, 2019 09:15

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 26, 2019

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from b75620e to 8b75de8 Compare April 26, 2019 10:08

ingvagabund requested changes Apr 26, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2019

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from 8b75de8 to 3bc2f43 Compare April 29, 2019 18:01

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 29, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2019

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from 3bc2f43 to a56b76f Compare June 28, 2019 05:23

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2019

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from a56b76f to 0ad7f34 Compare July 1, 2019 04:09

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2019

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from 0ad7f34 to dd00275 Compare July 1, 2019 06:27

ingvagabund requested changes Jul 1, 2019

View reviewed changes

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from dd00275 to 5bc06f5 Compare July 1, 2019 11:06

ingvagabund requested changes Jul 1, 2019

View reviewed changes

frobware reviewed Jul 1, 2019

View reviewed changes

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from 5bc06f5 to 02ef72f Compare July 2, 2019 06:45

frobware reviewed Jul 2, 2019

View reviewed changes

ingvagabund reviewed Jul 2, 2019

View reviewed changes

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from 02ef72f to 970bc97 Compare July 2, 2019 10:11

ingvagabund requested changes Jul 2, 2019

View reviewed changes

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch 3 times, most recently from be7e3a7 to 5d14dd1 Compare July 2, 2019 11:56

frobware reviewed Jul 2, 2019

View reviewed changes

vikaschoudhary16 added 2 commits July 2, 2019 19:27

Unit test: verify feature-gate disable after enable scenario

d88f8f9

bump cvo

d45cf1b

vikaschoudhary16 force-pushed the hc-featuregate-enabledisable branch from 5d14dd1 to d45cf1b Compare July 2, 2019 14:06

openshift-ci-robot assigned ingvagabund Jul 2, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2019

openshift-merge-robot merged commit 99f8484 into openshift:master Jul 2, 2019

vikaschoudhary16 deleted the hc-featuregate-enabledisable branch July 2, 2019 17:19

Unit test: Verify hc feature-gate disable after enable scenario #299

Unit test: Verify hc feature-gate disable after enable scenario #299

Uh oh!

Conversation

vikaschoudhary16 commented Apr 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelgugino left a comment

Choose a reason for hiding this comment

Uh oh!

michaelgugino commented Apr 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre commented May 14, 2019

Uh oh!

vikaschoudhary16 commented Jun 28, 2019

Uh oh!

vikaschoudhary16 commented Jul 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingvagabund Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ingvagabund Jul 2, 2019 •

edited

Loading

openshift-ci-robot commented Jul 2, 2019 •

edited

Loading