[PoC] Use builders to make table tests more readable by dprotaso · Pull Request #1762 · knative/serving

dprotaso · 2018-07-30T21:14:47Z

This is just a proof-of-concept to source feedback on whether folks like this pattern, and whether they feel this improves readability. This secondly helps reduce the combinatorial method names.

google-prow-robot · 2018-07-30T21:15:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dprotaso
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: grantr

If they are not already assigned, you can assign the PR to them by writing /assign @grantr in a comment when ready.

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:

pkg/controller/OWNERS

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

dprotaso · 2018-07-30T21:16:48Z

/cc @mattmoor @grantr @shashwathi @tanzeeb

dprotaso · 2018-07-30T21:21:36Z

Looking at it while walking we could have higher level builder functions to remove more boilerplate

ie. AddStatusCondition

Edit: Pushed those changes

The table test having WantCreate be a type of metav1.Object actually results in more boilerplate - hence some of the calls to `Build`

knative-metrics-robot · 2018-07-31T02:10:51Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/configuration/configuration.go	94.0%	93.8%	-0.1

mattmoor

Generally 👍 I wonder about applicability beyond the table tests, or even outside of the controllers.

We have a lot of little bits and pieces that have been thrown together to produce test data (I'm super guilty of this), it'd be good to consolidate it all around this pattern, if we can. WDYT?

mattmoor · 2018-08-01T14:11:48Z

+		case builder.ConfigBuilder:
+			ls.Configuration.Items = append(ls.Configuration.Items, o.Configuration)
+		case builder.RevisionBuilder:
+			ls.Revision.Items = append(ls.Revision.Items, o.Revision)


I don't understand why we take these instead of just calling Build()?

mattmoor · 2018-08-01T14:12:34Z

+}
+
+func (bldr RevisionBuilder) WithCondition(c v1alpha1.RevisionCondition) RevisionBuilder {
+	bldr.Revision.Status.Conditions = append(bldr.Revision.Status.Conditions, c)


This could end up with duplicates

mattmoor · 2018-08-01T14:14:05Z

 	}

-	rev := resources.MakeRevision(config, buildName)
+	rev := resources.MakeRevision(config)


I think this part of the change is a separable distraction from the test change. Want to send a separate PR?

mattmoor · 2018-08-01T14:14:15Z

@@ -0,0 +1,94 @@
+package builder


missing license headers

mattmoor · 2018-08-01T14:16:16Z

+
+type ConfigBuilder struct {
+	*v1alpha1.Configuration
+}


This might better below under pkg/apis/serving/v1alpha1/testing?

mattmoor · 2018-08-01T14:16:33Z

-func makeRevStatus(rev *v1alpha1.Revision, status v1alpha1.RevisionStatus) *v1alpha1.Revision {
-	rev.Status = status
-	return rev
-}


mattmoor · 2018-08-01T14:17:11Z

+				WithCondition(v1alpha1.ConfigurationCondition{
+					Type:   v1alpha1.ConfigurationConditionReady,
+					Status: corev1.ConditionTrue,
+				}).AsUpdateAction(),


I like shedding the UpdateActionImpl wrapper :)

grantr

I like this direction! Thanks @dprotaso for making it work.

grantr · 2018-08-01T17:21:44Z

 	ls := NewListers(r.Objects)

+	for i, action := range r.WantUpdates {
+		if builder, ok := action.Object.(builder.ConfigBuilder); ok {


Seems like this could avoid the reference to ConfigBuilder by defining an interface:

type Builder interface{ Build() runtime.Object }

but I'm not sure about that return value. Would it work with cmp.Diff or semantic equality or whatever is being used to compare objects?

grantr · 2018-08-01T17:22:43Z

 		},
 		WantCreates: []metav1.Object{
-			setRevConcurrencyModel(resources.MakeRevision(cfg("validation-failure", "foo", 1234), noBuildName), "Bogus"),
+			setRevConcurrencyModel(resources.MakeRevision(cfg("validation-failure", "foo", 1234)), "Bogus"),


I definitely like the builders better than this style (setRevConcurrencyModel).

grantr · 2018-08-01T17:24:29Z

@@ -0,0 +1,62 @@
+package builder


Haven't thought about it too carefully, but would it make sense for these methods to be defined in the api package (or a subpackage of that) rather than in the controller testing package?

That was @mattmoor's suggestion here:
#1762 (comment)

evankanderson · 2018-08-01T23:40:23Z

/assign grantr mattmoor

jonjohnsonjr · 2018-08-02T18:18:55Z

LGTM as well. I wanted to use this but then I realized it wasn't merged yet 😆

dprotaso · 2018-08-22T17:17:26Z

Closing - moving work to #1924

wip

1b784ac

google-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 30, 2018

google-prow-robot requested review from grantr and mattmoor July 30, 2018 21:15

google-prow-robot requested review from shashwathi and tanzeeb July 30, 2018 21:16

google-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2018

Further abstraction

3f95d25

The table test having WantCreate be a type of metav1.Object actually results in more boilerplate - hence some of the calls to `Build`

dprotaso force-pushed the poc-test-builders branch from fe5d3d6 to 3f95d25 Compare July 31, 2018 02:08

dprotaso changed the title ~~[WIP] Use builders to make table tests more readable~~ [PoC] Use builders to make table tests more readable Aug 1, 2018

google-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2018

mattmoor reviewed Aug 1, 2018

View reviewed changes

grantr reviewed Aug 1, 2018

View reviewed changes

grantr mentioned this pull request Aug 1, 2018

Test ParametersFrom feature knative/eventing#307

Merged

google-prow-robot assigned grantr and mattmoor Aug 1, 2018

jonjohnsonjr mentioned this pull request Aug 3, 2018

Add status propagation tests for Service #1798

Merged

grantr mentioned this pull request Aug 8, 2018

Add configmap for flow controller knative/eventing#324

Merged

dprotaso mentioned this pull request Aug 22, 2018

Use the builder pattern to improve readability of configuration table testing #1924

Closed

dprotaso closed this Aug 22, 2018

grantr mentioned this pull request Dec 3, 2018

Fixture builders for Subscription reconcile tests knative/eventing#659

Merged

Conversation

dprotaso commented Jul 30, 2018

Uh oh!

google-prow-robot commented Jul 30, 2018

Uh oh!

dprotaso commented Jul 30, 2018

Uh oh!

dprotaso commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-metrics-robot commented Jul 31, 2018

Uh oh!

mattmoor left a comment

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!

grantr left a comment

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!

evankanderson commented Aug 1, 2018

Uh oh!

jonjohnsonjr commented Aug 2, 2018

Uh oh!

dprotaso commented Aug 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dprotaso commented Jul 30, 2018 •

edited

Loading