use functional options pattern in tests #13242

k15r · 2022-02-01T16:10:32Z

Description

Changes proposed in this pull request:

more consistent way to setup resource in our test

Related issue(s)

netlify · 2022-02-01T16:16:26Z

✔️ 🥰 Documentation preview ready! 🥰

🔨 Explore the source changes: 0b985bf

🔍 Inspect the deploy log: https://app.netlify.com/sites/kyma-project-docs-preview/deploys/61f95d3656081b00070edeed

😎 Browse the preview: https://deploy-preview-13242--kyma-project-docs-preview.netlify.app

…ator-from-tests

…l-options

…ions

nachtmaar · 2022-02-02T07:04:32Z

components/eventing-controller/testing/test_helpers.go

+}
+
+// WithServiceWithPathAsSink sets a kubernetes service as the sink
+func WithServiceWithPathAsSink(svc *corev1.Service, path string) SubscriptionOpt {


nit:I am thinking whether we can avoid double With 🤔 Since this is nothing else than WithSinkURL but with input from the svc and path, what about the following name: WithSinkURLFromSvcAndPath ?
It is a bit long, but by autocompleting I can immediately see that it is similar to WithSinkURL

If you like the suggestion we can also use it in WithServiceAsSink => WithSinkURLFromSvc. If you don't, then just keep it as it is :)

nachtmaar · 2022-02-02T07:27:19Z

components/eventing-controller/testing/test_helpers.go

-	}()
+func WithBinaryContentMode() ProtoOpt {
+	return func(p *eventingv1alpha1.ProtocolSettings) {
+		p.ContentMode = func() *string {


Don't we have a utility function to create a string pointer ?
I found utils.BoolPtr and utils.Int32Ptr here:https://github.com/nachtmaar/kyma/blob/13029-retry-fail-publish/components/eventing-controller/utils/utils.go#L56
We could add utils.StringPtr as well :)

nachtmaar · 2022-02-02T07:35:08Z

components/eventing-controller/testing/test_helpers.go

-		Port:       &port,
-		Host:       &host,
-		IsExternal: &isExternal,
+func WithService(host, svcName string) APIRuleOption {


👍 for returning a APIRuleOption instead of setting the variables directly.

nachtmaar · 2022-02-02T08:03:36Z

components/eventing-controller/testing/test_helpers.go

-	return func(s *eventingv1alpha1.Subscription) {
-		s.Spec.Sink = sinkURL
-	}
+func WithOrderCreatedFilter() SubscriptionOpt {


Out of curiosity for our style guidelines: Do you prefer WithOrderCreatedFilter over WithFilterOrderCreated and if yes why ? :)

Good question:
WithOrderCreatedFilter: it is a OrderCreatedFilter, but a bit harder to spot in the IDEs completion popup
WithFilterOrderCreated: it sounds a bit off, but really easy to spot in the IDE

I don't have any preference. If you want me to change it i will do this.

WithFilterOrderCreated sounds like we are filtering some OrderCreated items. Putting Filter at the end of the words makes --in my option-- clear, that this handles some kind of a Filter object.

I personally like WithFilterOrderCreated more because it enables easier auto completion, but I agree that WithOrderCreatedFilter sounds better. No need to change for me.

components/eventing-controller/testing/test_helpers.go

nachtmaar · 2022-02-02T08:18:19Z

components/eventing-controller/testing/test_helpers.go

 	return &eventingv1alpha1.EventingBackend{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
-			Namespace: ns,
+			Namespace: namespace,


(optional): Out of curiosity: Do you have a rule in your mind when you shorten or lengthen a variable name ?

E.g. you shortened apiRule to r and lengthened ns to namespace.

good point, will make it more consistent

nachtmaar · 2022-02-02T08:22:33Z

components/eventing-controller/controllers/subscription/beb/reconciler_test.go

-			optWebhook := reconcilertesting.WithWebhookAuthForBEB
-			subscription := reconcilertesting.NewSubscription(subscriptionName, namespaceName, optFilter, optWebhook)
-			reconcilertesting.WithValidSink(namespaceName, subscriberSvc.Name, subscription)
+			subscription := reconcilertesting.NewSubscription(subscriptionName, namespaceName,


Much cleaner and easier to read IMHO 👍

nachtmaar · 2022-02-02T08:51:20Z

components/eventing-controller/controllers/subscription/beb/reconciler.go

@@ -98,8 +98,9 @@ func NewReconciler(ctx context.Context, client client.Client, applicationLister
 // +kubebuilder:rbac:groups=gateway.kyma-project.io,resources=apirules,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:printcolumn:name="Ready",type=bool,JSONPath=`.status.Ready`

-// TODO: Optimize number of reconciliation calls in eventing-controller #9766: https://github.com/kyma-project/kyma/issues/9766
+// Reconcile sets up


I think there is something missing here :-D

Co-authored-by: Nils Seip <github@nilstschmidt.de>

friedrichwilken and others added 2 commits January 28, 2022 21:02

get rid of the redundant defaultValidator

d0de8d1

use functional options pattern in tests

45bd666

k15r requested review from friedrichwilken, lilitgh, marcobebway, mfaizanse, nachtmaar and pxsalehi as code owners February 1, 2022 16:10

k15r requested a review from a user February 1, 2022 16:10

k15r requested a review from vpaskar as a code owner February 1, 2022 16:10

k15r added the area/eventing Issues or PRs related to eventing label Feb 1, 2022

kyma-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 1, 2022

k15r and others added 6 commits February 1, 2022 17:17

remove unnecessary line

0b985bf

add opt pattern to NATS reconciler and NATS test reconciler

ff99fd6

Merge remote-tracking branch 'upstream/main' into remove-defaultValid…

44bc5a9

…ator-from-tests

refactor tests

e3168ce

cleanup test function

6b17e11

Merge branch 'remove-defaultValidator-from-tests' into test-functiona…

cdcd51d

…l-options

k15r force-pushed the test-functional-options branch from 1bd9c8d to cdcd51d Compare February 1, 2022 20:18

export externaly used type

a84f76f

friedrichwilken self-assigned this Feb 1, 2022

nachtmaar self-assigned this Feb 2, 2022

k15r added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2022

k15r added 2 commits February 2, 2022 09:31

Merge remote-tracking branch 'upstream/main' into test-functional-opt…

e1f0ae5

…ions

cleanup merge

ffc3fb3

k15r requested review from alexandra-simeonova, grego952, klaudiagrz and majakurcius as code owners February 2, 2022 08:42

k15r requested review from mmitoraj and NHingerl as code owners February 2, 2022 08:42

nachtmaar reviewed Feb 2, 2022

View reviewed changes

k15r force-pushed the test-functional-options branch from b64d770 to ffc3fb3 Compare February 2, 2022 09:10

k15r and others added 2 commits February 2, 2022 10:29

Update components/eventing-controller/testing/test_helpers.go

68bb27e

Co-authored-by: Nils Seip <github@nilstschmidt.de>

implement review comments

03670a3

nachtmaar previously approved these changes Feb 2, 2022

View reviewed changes

kyma-bot added the lgtm Looks good to me! label Feb 2, 2022

remove unused function (linting issue)

94c4b1f

k15r dismissed nachtmaar’s stale review via 94c4b1f February 2, 2022 10:35

kyma-bot removed the lgtm Looks good to me! label Feb 2, 2022

k15r removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2022

nachtmaar approved these changes Feb 2, 2022

View reviewed changes

kyma-bot added the lgtm Looks good to me! label Feb 2, 2022

kyma-bot merged commit fbc8561 into kyma-project:main Feb 2, 2022

nachtmaar mentioned this pull request Feb 11, 2022

Define test implementation guidelines [5d] #12820

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use functional options pattern in tests #13242

use functional options pattern in tests #13242

k15r commented Feb 1, 2022 •

edited

netlify bot commented Feb 1, 2022 •

edited

nachtmaar Feb 2, 2022

nachtmaar Feb 2, 2022

k15r Feb 2, 2022

nachtmaar Feb 2, 2022

k15r Feb 2, 2022

nachtmaar Feb 2, 2022

nachtmaar Feb 2, 2022

k15r Feb 2, 2022 •

edited

friedrichwilken Feb 2, 2022

nachtmaar Feb 2, 2022

nachtmaar Feb 2, 2022

nachtmaar Feb 2, 2022

k15r Feb 2, 2022

nachtmaar Feb 2, 2022

nachtmaar Feb 2, 2022

k15r Feb 2, 2022

use functional options pattern in tests #13242

use functional options pattern in tests #13242

Conversation

k15r commented Feb 1, 2022 • edited

netlify bot commented Feb 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k15r Feb 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k15r commented Feb 1, 2022 •

edited

netlify bot commented Feb 1, 2022 •

edited

k15r Feb 2, 2022 •

edited