Use test builder. #1871

timoreimann · 2017-07-17T22:48:06Z

This change introduces the builder pattern to the Marathon unit tests in order to simplify and reduce the amount of testing boilerplate.

Additional changes:

Add missing unit tests.
Make all tests look consistent.
Use dedicated type for task states for increased type safety.
Remove obsoleted getApplication function.

Note: I pushed the branch to the containous repo because I intend to submit another PR based onto this one.

@containous/marathon @containous/traefik PTAL.

timoreimann · 2017-07-17T22:51:08Z

/cc @marco-jantke happy to have your review too. :-)

m3co-code

Added some minor comments. PR looks great to me, uniformity FTW :D

BTW the validation make target is currently still failing.

m3co-code · 2017-07-18T05:35:33Z

provider/marathon/marathon_test.go

-			},
+				}),
+			),
+


Unnecessary newline.

Argh, damn you unintentional blank lines!

Removed. :-)

m3co-code · 2017-07-18T05:45:06Z

provider/marathon/marathon_test.go

-				Ports: []int{},
-			},
+			desc: "port definition available",
+			application: application(


Shouldn't you use the ipAddrPerTask here to simulate the same behaviour or did you remove it by intend?

I did remove it intentionally for two reasons (should have commented this in the first place, sorry):

My original thinking was that the IP-per-task setup would serve as a safeguard to running into an undesirable happy path due to a bug in the code (i.e., false positive). On closer inspection, this seems highly unlikely since we can't really produce the expected port result of 443 without configuring it somewhere, which we only do for the test case at hand that we intend to cover.

Configuring both port definitions and IPs-per-task should actually not be allowed: go-marathon explicitly clears the former when setting the latter because the Marathon API would otherwise reject our API request. In that sense, it makes even less sense to set up both in the test (though I wouldn't relax the production code logic because of that because you never know how the Marathon API may evolve over time).

m3co-code · 2017-07-18T05:51:59Z

provider/marathon/marathon_test.go

+			provider := &Provider{}
+			actual := provider.getPassHostHeader(c.application)
+			if actual != c.expected {
+				t.Fatalf("actual %q, expected %q", actual, c.expected)


I think t.Errorf be sufficient here.

Here and almost everywhere else -- updated.

m3co-code · 2017-07-18T05:56:06Z

provider/marathon/marathon_test.go

+		{
+			desc:        "label missing",
+			application: application(),
+			expected:    "NetworkErrorRatio() > 1",


Just out of curiosity. This means there is always a CircuitBreaker in place for Backends provided by Marathon, but the expression should never evaluate to true? Is this intended? It looks like it would only add one middleware which can not have any effect.

This part is somewhat confusing: While we do return a default value, we only include circuit-breaker settings when the label is actually set.

It might make sense to change the default return value to an empty string to improve clarity. Let's consider that for a future PR.

m3co-code · 2017-07-18T06:05:30Z

provider/marathon/marathon_test.go

-		want  string
+	cases := []struct {
+		desc     string
+		value    *string


AFAICS its not necessary anymore to use *string as we only dereference the pointer when passing it to AddLabel. Or do I miss something?

You're right: There's still one test left where we want to distinguish set/unset/omitted, but not this one.

Simplified.

m3co-code · 2017-07-18T06:06:34Z

provider/marathon/marathon_test.go

-		want  string
+	cases := []struct {
+		desc     string
+		value    *string


Same as previous comment.

Simplified too.

timoreimann · 2017-07-18T08:27:05Z

@marco-jantke all comments addressed.

m3co-code

Thanks for addressing. LGTM 👍

ldez · 2017-07-18T08:38:43Z

@timoreimann could you fix the linting?

Errors from golint:
provider/marathon/marathon.go:31:6: exported type TaskState should have comment or be unexported

timoreimann · 2017-07-18T09:12:13Z

@ldez should be good now.

ldez

Could you do the same approach with backends and frontends:

expectedFrontends: map[string]*types.Frontend{
	"frontend-app": {
		Backend: "backend-app",
		Routes: map[string]types.Route{
			"route-host-app": {
				Rule: "Host:app.docker.localhost",
			},
		},
	},
},
expectedBackends: map[string]*types.Backend{
	"backend-app": {
		Servers: map[string]types.Server{
			"server-task": {
				URL:    "http://localhost:80",
				Weight: 0,
			},
		},
		CircuitBreaker: nil,
	},
},

ldez · 2017-07-18T12:32:12Z

provider/marathon/marathon_test.go

 			}
 		})
 	}
 }
+
+func stringp(s string) *string {


maybe move this into testhelpers like Intp?

Good idea, done.

ldez · 2017-07-18T12:35:53Z

provider/marathon/marathon_test.go

-			application: marathon.Application{
-				Labels: &map[string]string{}},
+			desc:        "label missing",
+			application: application(),


To avoid the ambiguity of names, I rather name builders like anApplication(...) and if necessary options like withLabels(...)

In general, I'd prefer not to use longer names in order to keep things concise since builder functions are used very frequently. IMHO, we shouldn't have extra verbs on builder operations.

anApplication looks quite weird to my eyes. :) How about createApp and createTask instead?

create prefix is not fluent syntax 😉 (builders must be fluent, I think)
@vdemeester do that like me http://vincent.demeester.fr/posts/2017-01-01-go-testing-functionnal-builders/

But in this case, I prefer you don't change anything instead of use a create prefix.

Okay, now I'm confused: The blog post uses a/an and with prefixes. However, our Docker builder comes with just two with functions and otherwise uses no prefixes. ¯_(ツ)_/¯

@vdemeester: can you help me out here? :)

@vdemeester's out-of-band feedback is that he more often than not drops the with prefix in his builders. :-)

So I just added the create prefix. Not sure if it violates fluent builder syntaxes. I did this a lot back in my Java days; maybe that can be considered a sin of the past. ;)

ldez · 2017-07-18T12:41:35Z

provider/marathon/builder_test.go

+	}
+}
+
+func labels(l map[string]string) func(*marathon.Application) {


I don't really show the need to use a map here.
Why not use a simple function like this:

func label(key, value string) { return func(app *marathon.Application) { app.AddLabel(k, v) } }

I was leaning towards such an implementation too at some point, so I made the change. 👍

timoreimann · 2017-07-18T13:58:37Z

Re: backend/frontend refactoring: I'd like to leave that for a separate PR as this one is already fairly big. Plus, we need to "builderize" the configuration in numerous places (server, kubernetes, etc.), so I'd rather do it in one focused wash (which will also make it easier to see how we need to design that builder).

vdemeester

Few comments but overall great work !
LGTM 🦁

vdemeester · 2017-07-19T08:43:25Z

testhelpers/helpers.go

+func Stringp(s string) *string {
+	return &s
+}
+


one \n too much 😝

vdemeester · 2017-07-19T08:47:51Z

provider/marathon/marathon_test.go

-				},
-			},
+			desc:        "simple application",
+			application: application(appPorts(80)),


You seem to use appPorts(80) a lot, maybe it should be a default value in the application (and setting it only when you actually care about the value in your test) 👼

I should already only be setting it when I actually need it. (There are a number of application()-only invocations where I don't pass the appPorts builder function.)

I thought about setting default ports on the app and task but dropped the idea eventually: We have a lot of different cases where we sometimes need just the Marathon port and sometimes the task port; sometimes they need to be equal and sometimes they don't.

So if I added a default, I'd also need to override it explicitly sometimes. In the end, this was too intransparent for me, so I went down the slightly more explicit and but also more clear route.

vdemeester · 2017-07-19T08:48:29Z

provider/marathon/marathon_test.go

+				label(types.LabelBackendLoadbalancerMethod, "drr"),
+				label(types.LabelBackendCircuitbreakerExpression, "NetworkErrorRatio() > 0.5"),
+			),
+			task: localhostTask(taskPorts(80)),


Same for taskPorts

See above. :)

timoreimann · 2017-07-19T10:37:10Z

Rebased and addressed all comments for now.

PTAL again.

ldez

LGTM

juliens

LGTM

ldez · 2017-07-21T14:22:43Z

provider/marathon/builder_test.go

+
+// Functions related to building applications.
+
+func createApplication(ops ...func(*marathon.Application)) marathon.Application {


The conventional way in Java it's a build prefix.

https://docs.oracle.com/javase/8/docs/api/java/util/Calendar.Builder.html

https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.Builder.html

https://github.com/spring-projects/spring-framework/blob/bc14c5ba83e1f211628456bbccce7b2531aac58c/spring-test/src/test/java/org/springframework/test/context/support/BootstrapTestUtilsMergedConfigTests.java

https://github.com/spring-projects/spring-framework/blob/7018804e8451fa935e8715754de43a3ef62fa5c8/spring-webflux/src/test/java/org/springframework/web/reactive/function/server/RequestPredicatesTests.java

I prefer you simply remove the create prefix.

ldez

I approve but I don't agree with the create prefix.

This change introduces the builder pattern to the Marathon unit tests in order to simplify and reduce the amount of testing boilerplate. Additional changes: - Add missing unit tests. - Make all tests look consistent. - Use dedicated type for task states for increased type safety. - Remove obsoleted getApplication function.

timoreimann · 2017-07-21T16:41:04Z

I only added the prefix because I understood you asked for it. Will happily remove it again in another PR. :-)

timoreimann added area/provider/marathon kind/enhancement a new or improved feature. labels Jul 17, 2017

timoreimann requested review from juliens, ldez and emilevauge July 17, 2017 22:48

m3co-code reviewed Jul 18, 2017

View reviewed changes

ldez added status/2-needs-review contributor/waiting-for-corrections labels Jul 18, 2017

m3co-code approved these changes Jul 18, 2017

View reviewed changes

ldez suggested changes Jul 18, 2017

View reviewed changes

vdemeester approved these changes Jul 19, 2017

View reviewed changes

timoreimann force-pushed the marathon-test-builder branch from 94f762e to 62eaa4d Compare July 19, 2017 10:36

ldez approved these changes Jul 19, 2017

View reviewed changes

ldez removed the contributor/waiting-for-corrections label Jul 19, 2017

ldez modified the milestone: 1.4 Jul 19, 2017

timoreimann mentioned this pull request Jul 20, 2017

Assign filtered tasks to apps contained in slice. #1881

Merged

timoreimann force-pushed the marathon-test-builder branch 2 times, most recently from 30e50ae to 6dcc278 Compare July 20, 2017 19:07

timoreimann mentioned this pull request Jul 20, 2017

Add support for readiness checks. #1883

Merged

juliens approved these changes Jul 21, 2017

View reviewed changes

ldez added status/3-needs-merge status/2-needs-review and removed status/2-needs-review labels Jul 21, 2017

ldez removed the status/3-needs-merge label Jul 21, 2017

ldez suggested changes Jul 21, 2017

View reviewed changes

ldez approved these changes Jul 21, 2017

View reviewed changes

ldez force-pushed the marathon-test-builder branch from 6dcc278 to 4546613 Compare July 21, 2017 14:57

ldez added status/3-needs-merge and removed status/2-needs-review labels Jul 21, 2017

ldez merged commit 8cb4459 into master Jul 21, 2017

ldez removed the status/3-needs-merge label Jul 21, 2017

ldez deleted the marathon-test-builder branch July 22, 2017 15:05


		// Functions related to building applications.

		func createApplication(ops ...func(*marathon.Application)) marathon.Application {

Use test builder. #1871

Use test builder. #1871

Conversation

timoreimann commented Jul 17, 2017 • edited Loading

timoreimann commented Jul 17, 2017

m3co-code left a 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

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

timoreimann commented Jul 18, 2017

m3co-code left a comment

Choose a reason for hiding this comment

ldez commented Jul 18, 2017

timoreimann commented Jul 18, 2017

ldez left a 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

ldez Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented Jul 18, 2017

vdemeester left a 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

timoreimann commented Jul 19, 2017

ldez left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez left a comment • edited Loading

Choose a reason for hiding this comment

timoreimann commented Jul 21, 2017

timoreimann commented Jul 17, 2017 •

edited

Loading

ldez Jul 18, 2017 •

edited

Loading

timoreimann Jul 19, 2017 •

edited

Loading

ldez left a comment •

edited

Loading