Test refactoring #153

frodopwns · 2019-08-28T15:44:15Z

What this PR does / why we need it:

This PR rips out ginkgo for controller integration testing. My experience with Ginkgo has largely been poor and at this point, I believe we can get by without it.

Go Test is simpler to understand and write in. The BDD syntax (Describe, It syntax) doesn't add much value when compared to normal comments/logs
Ability to add build tags so we can select what tests to run
Output is similar to this if we enable ginkgo.verbose or debug, but we can control the output using Go Test better

The tests can all be run in parallel.

Run via

go test -parallel 3 -v -tags all ./controllers/...

or

makke test

If you are unsure whether all tests passed: echo $? to get the exit code

* feat: implement keyvault controller * Ace's KV changes with updates * Added an event for the final successful provisioning * Updated changes based on the PR comments * removing unwanted file * making resource group name the one in the keyvault yaml Co-authored-by: Ace Eldeib <alexeldeib@gmail.com>

This reverts commit 594c5cd.

…azure-service-operator into test-refactors

melonrush13 · 2019-08-28T21:21:08Z

controllers/resourcegroup_controller_test.go

+	Expect(apierrors.IsInvalid(err)).To(Equal(false))
+	Expect(err).NotTo(HaveOccurred())
+
+	// prep query gor Get


Suggested change

// prep query gor Get

// prep query for Get

frodopwns · 2019-08-29T00:05:12Z

note: the output needs some work still for sure

frodopwns · 2019-08-29T00:07:28Z

Part of the goal here is to be able to debug individual tests from within the IDE. I think the build flags will need to be removed for that.

szoio · 2019-09-02T02:38:10Z

Hi @frodopwns

Some comments/questions/observations:

Go Test is simpler to understand and write in. The BDD syntax (Describe, It syntax) doesn't add much value when compared to normal comments/logs

Totally agree with this - can't say I ever quite drank the BDD kool-aid myself

The tests can all be run in parallel.

Can/will it accommodate synchronized shared setup and teardown?

Part of the goal here is to be able to debug individual tests from within the IDE. I think the build flags will need to be removed for that.

I've been using focus specs FDescribe etc to do this, definitely it's not ideal to have to edit the code to do this.

My experience with Ginkgo has largely been poor

So far my experience has been pretty average. I have been able to improve it a little with this PR to improve the existing tests (#172).
Not suggesting not to get rid of Ginkgo, but some small changes have improved the ginkgo experience quite a bit.

In fact happy to get rid of Ginkgo and move to go test, provided one can do common test setup, run several tests in parallel, then do common teardown.

szoio · 2019-09-02T02:48:26Z

controllers/eventhub_controller_test.go

+	}, tcfg.Timeout, tcfg.Poll,
+	).Should(BeTrue())
+
+}


Secret test been removed?
(maybe they should be tested separately anyway)

This PR shouldn't be considered as ready to merge so much as a proposal to abandon Ginkkgo.

szoio · 2019-09-02T06:23:54Z

controllers/consumergroup_controller_test.go

+
+	consumerGroupNamespacedName := types.NamespacedName{Name: consumerGroupName, Namespace: "default"}
+
+	Eventually(func() bool {


Do we want to carry on using the Gomega assertions?
some other assertion lib?

Azadehkhojandi · 2019-09-04T06:17:31Z

api/v1/eventhub_types.go

@@ -115,3 +116,25 @@ func (eventhub *Eventhub) AddFinalizer(finalizerName string) {
 func (eventhub *Eventhub) RemoveFinalizer(finalizerName string) {
 	eventhub.ObjectMeta.Finalizers = helpers.RemoveString(eventhub.ObjectMeta.Finalizers, finalizerName)
 }
+
+func NewTestEventhub(cfg TestConfig, namespace string) *Eventhub {


For Simplicity Please move this function to a separate file that contains all the initializations for the test.

Azadehkhojandi · 2019-09-04T06:18:07Z

api/v1/eventhubnamespace_types.go

@@ -99,3 +99,16 @@ func (eventhubNamespace *EventhubNamespace) AddFinalizer(finalizerName string) {
 func (eventhubNamespace *EventhubNamespace) RemoveFinalizer(finalizerName string) {
 	eventhubNamespace.ObjectMeta.Finalizers = helpers.RemoveString(eventhubNamespace.ObjectMeta.Finalizers, finalizerName)
 }
+
+func NewTestEventhubNamespace(cfg TestConfig) *EventhubNamespace {


For Simplicity Please move this function to a separate file that contains all the initializations for the test

Azadehkhojandi · 2019-09-04T06:18:15Z

api/v1/resourcegroup_types.go

@@ -83,3 +83,15 @@ func (resourceGroup *ResourceGroup) AddFinalizer(finalizerName string) {
 func (resourceGroup *ResourceGroup) RemoveFinalizer(finalizerName string) {
 	resourceGroup.ObjectMeta.Finalizers = helpers.RemoveString(resourceGroup.ObjectMeta.Finalizers, finalizerName)
 }
+
+func NewTestResourceGroup(cfg TestConfig) *ResourceGroup {


For Simplicity Please move this function to a separate file that contains all the initializations for the test

Azadehkhojandi · 2019-09-04T06:56:33Z

I don’t have any preference in terms of a testing framework the only comment I would add there are some drawbacks if branch out from kubebuilder test framework

We will lose the auto-generated test codes when we add new Resource. @erin Corson have you checked if kubebuilder create api --group xxx --version xxx --kind xxx works after your changes?
.@erin Corson have you benchmarked the two approaches and do you have a duration of each approach?
We might not be able to benefit from the community contribution towards tests in kubebuilder repo.
It might affect onboarding and the community contribution at this point whoever read kubebuilder book can jump in but with the new framework we need to add documentation and guideline

frodopwns · 2019-09-04T15:47:01Z

The tests can all be run in parallel.

Can/will it accommodate synchronized shared setup and teardown?

Currently it allows a Global setup/teardown. It happens before parallelization starts, so I'm assuming it is "synchronized".
edit - There does seem to be errors at the end of the integration tests I think from the tests and k8smanager not ending at the same time.

Part of the goal here is to be able to debug individual tests from within the IDE. I think the build flags will need to be removed for that.

I've been using focus specs FDescribe etc to do this, definitely it's not ideal to have to edit the code to do this.

I've never heard of this which immediately makes me not like it. Normal go tests can be isolated simply from the VSCode IDE code lenses.

My experience with Ginkgo has largely been poor

So far my experience has been pretty average. I have been able to improve it a little with this PR to improve the existing tests (#172).
Not suggesting not to get rid of Ginkgo, but some small changes have improved the ginkgo experience quite a bit.

In fact happy to get rid of Ginkgo and move to go test, provided one can do common test setup, run several tests in parallel, then do common teardown.

This is possible and exists in this PR. I'm not dead set on making this change. For the most part I wanted to draw attention to the state of the tests which were hard to run and harder to debug. Which is the opposite of how we want the tests.

frodopwns · 2019-09-04T23:45:26Z

I don’t have any preference in terms of a testing framework the only comment I would add there are some drawbacks if branch out from kubebuilder test framework

We will lose the auto-generated test codes when we add new Resource. @erin Corson have you checked if kubebuilder create api --group xxx --version xxx --kind xxx works after your changes?

The only issue preventing kubebuilder create from working is that I removed the Gomega import from suite_test.go. So the go vet call at the end of the generation causes a failure. Putting the gomega import back in causes the generation to work fine.

.@erin Corson have you benchmarked the two approaches and do you have a duration of each approach?

The tests in master aren't currently being run in parallel (I think?), so I didn't compare them. I would expect performance not to change otherwise.

We might not be able to benefit from the community contribution towards tests in kubebuilder repo.

I'm not aware of anything coming down the line that would be useful to us wrt testing.

It might affect onboarding and the community contribution at this point whoever read kubebuilder book can jump in but with the new framework we need to add documentation and guideline

Having more go-like testing should be a boon to new devs on the project as it would match their expectation more closely.

frodopwns · 2019-09-05T15:27:46Z

I think I have a new proposal. The more I think about it, the more I think the controller tests in /controllers should be mocking the Azure bit in some way. Maybe that means skipping the call to Azure, maybe that means calling some fake service. It should check that the statuses get updated properly and that the correct actions would be taken if it were real.

The end-to-end integration tests should be run against a live AKS cluster. I'd like to see this take the form of actual applications that get deployed using the operator. Some apps like this already exist in the Azure world.

eg. https://github.com/Azure-Samples/azure-voting-app-redis

frodopwns · 2019-09-06T01:06:50Z

closing this PR...discussed with stakeholders....

new plan is to make improvements to the current ginkgo usage and slowly move toward mocked azure interaction in the /controllers tests...e2e tests will be moved to z different directory

- No change in CI, but this makes it easier to catch missing headers in the generator files before opening PRs.

jananivMS and others added 14 commits August 20, 2019 17:38

Merge branch 'master' of github.com:Azure/azure-service-operator

40858fc

Merge branch 'master' of github.com:Azure/azure-service-operator

b86acf8

Merge branch 'master' of github.com:Azure/azure-service-operator

85c6a19

Merge branch 'master' of github.com:Azure/azure-service-operator

a4d4754

Merge branch 'master' of github.com:Azure/azure-service-operator

c460bf4

Revert "fix tests (#140)"

87976c3

This reverts commit 594c5cd.

Merge branch 'master' of github.com:Azure/azure-service-operator

dbb238b

Merge branch 'revert-140-feat/FixFailingTestsEH' of github.com:Azure/…

453b2d0

…azure-service-operator into test-refactors

refacttoring tests tto use go test

f08373d

using go test for aall controller ttests

49fb8c4

handle merge

8576774

small updates

a587c65

contrived chaange to kkick off testts

02c4b72

melonrush13 reviewed Aug 28, 2019

View reviewed changes

frodopwns requested review from Azadehkhojandi and priyakumarank August 29, 2019 00:04

Azadehkhojandi assigned szoio Sep 1, 2019

szoio reviewed Sep 3, 2019

View reviewed changes

Azadehkhojandi reviewed Sep 4, 2019

View reviewed changes

frodopwns closed this Sep 6, 2019

frodopwns unassigned szoio Jan 16, 2020

Porges pushed a commit that referenced this pull request May 11, 2021

Perform header check in both makefiles (#153)

e255d6a

- No change in CI, but this makes it easier to catch missing headers in the generator files before opening PRs.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test refactoring #153

Test refactoring #153

frodopwns commented Aug 28, 2019 •

edited

Loading

melonrush13 Aug 28, 2019

frodopwns commented Aug 29, 2019

frodopwns commented Aug 29, 2019

szoio commented Sep 2, 2019 •

edited

Loading

szoio Sep 2, 2019

frodopwns Sep 4, 2019

szoio Sep 2, 2019

Azadehkhojandi Sep 4, 2019

Azadehkhojandi Sep 4, 2019

Azadehkhojandi Sep 4, 2019

Azadehkhojandi commented Sep 4, 2019

frodopwns commented Sep 4, 2019 •

edited

Loading

frodopwns commented Sep 4, 2019

frodopwns commented Sep 5, 2019

frodopwns commented Sep 6, 2019


		consumerGroupNamespacedName := types.NamespacedName{Name: consumerGroupName, Namespace: "default"}

		Eventually(func() bool {

Test refactoring #153

Test refactoring #153

Conversation

frodopwns commented Aug 28, 2019 • edited Loading

melonrush13 Aug 28, 2019

Choose a reason for hiding this comment

frodopwns commented Aug 29, 2019

frodopwns commented Aug 29, 2019

szoio commented Sep 2, 2019 • edited Loading

szoio Sep 2, 2019

Choose a reason for hiding this comment

frodopwns Sep 4, 2019

Choose a reason for hiding this comment

szoio Sep 2, 2019

Choose a reason for hiding this comment

Azadehkhojandi Sep 4, 2019

Choose a reason for hiding this comment

Azadehkhojandi Sep 4, 2019

Choose a reason for hiding this comment

Azadehkhojandi Sep 4, 2019

Choose a reason for hiding this comment

Azadehkhojandi commented Sep 4, 2019

frodopwns commented Sep 4, 2019 • edited Loading

frodopwns commented Sep 4, 2019

frodopwns commented Sep 5, 2019

frodopwns commented Sep 6, 2019

frodopwns commented Aug 28, 2019 •

edited

Loading

szoio commented Sep 2, 2019 •

edited

Loading

frodopwns commented Sep 4, 2019 •

edited

Loading