Add Kotlin DSL for working with MockMvc #1951

checketts · 2018-09-06T13:06:24Z

This DSL make working with MockMvc much cleaner.

Example:

	@Test
	fun json() {
		mockMvc.performGet("/person/{name}", "Lee") {    // 1
			builder { accept(MediaType.APPLICATION_JSON) } // 2
			printRequestAndResponse() // 3
			expect { // 4
				status { isOk }
				content { contentType("application/json;charset=UTF-8") }
				jsonPath("$.name") { value("Lee") }
				json("""{"someBoolean": false}""", strict = false)
			}
		}
	}

Highlights:

Extension methods to mockMvc to start dsl
Builder block for configuring request
Useful equivalent to andDo(print()) that avoids static imports
Expect block that highlights response expectations

This code is based on the work of @petrbalat and myself in https://github.com/petrbalat/kd4smt

checketts · 2018-09-13T19:58:35Z

Can I get any feedback in regards to this PR? No rush, just curious if I should improve parts.

sdeleuze · 2019-02-19T13:32:13Z

Thanks for this pull request, please find bellow my proposal inspired from yours:

@Test
fun json() {
	mockMvc.perform(HttpMethod.GET, "/person/{name}", "Lee") {
		accept = MediaType.APPLICATION_JSON
		print()
		expect {
			status.isOk()
			content.contentType("application/json;charset=UTF-8")
			jsonPath("\$.name").value("Lee")
			json("""{"someBoolean": false}""", strict = false)
		}
	}
}

Any thoughts?

checketts · 2019-02-19T15:19:55Z

Thanks for the reply!

I like it. The main differences I see:

No builder block: this mean all configuration will need to happen in the root. This should be fine, I think there was a reason we needed the builder block, but if we can code around them, removing an extra layer would be great.
Using print() instead of printRequestAndResponse(): Kotlin has a native print() function that writes to the console so without the Java andDo surrounding syntax it looks like a print to console out. Also as new developers have come on board and they had wondered 'How can I print the request and response in this test?', the longer name has jumped out at them and made it simple for them to recognize. I can change it if you want, but it does feel like it makes the code less clear.
How to support andDo blocks as needed for Spring RestDocs? Just leave them as a top level method as they currently are? (I'm glad discussion is happening on Create Kotlin DSL for Spring RestDocs spring-restdocs#547 at the same time)
Avoiding blocks in expectclause: I like the idea of removing the blocks however it will involve a fair amount of code. For example status { isOk() } simply makes the receiver a StatusResultMatchers that already exists in Spring. I'll do a spike to see if that change will be very difficult.

So I'll begin researching how difficult points 1 and 4 are to accomplish. I want to say there were technical limitations that made the original approach needed, but I'll investigate to be certain.

checketts · 2019-02-19T16:31:17Z

@sdeleuze One discoverability caveat we ran into is when using perfom(HttpMethod.GET) versus performGet() is that the one with the HttpMethod.GET relies on the user to do an additional static import instead of being able to auto-complete the method. Since there are only 6 verbs, moving them to the top level helps with the auto-complete without creating too much work.

checketts · 2019-02-19T16:46:07Z

jsonPath("\$.name").value("Lee") The escape (\$) is not needed when followed immediately by a dot.

sdeleuze · 2019-02-19T16:53:40Z

I think I would keep print()
andDo does not feel very idiomatic in Kotlin DSL, do is a reserved keyword, so I would maybe suggest handle { ... }
Good point, we can keep status { isOk() }

sdeleuze · 2019-02-19T16:58:25Z

For perform(HttpMethod.GET) versus performGet(), I tend to think the IDE will do a pretty good job proposing the various HttpMethod enum constants and that the static import is needed only if you want perfom(GET), so I am not sure adding the performXxx() is really needed.

checketts · 2019-02-19T18:36:05Z

I've pushed updates to incorporate your comments. Here is its current state:

@Test
fun json() {
    mockMvc.perform(HttpMethod.GET,"/person/{name}", "Lee") {
      builder { accept(MediaType.APPLICATION_JSON) }
      print()
      expect {
        status { isOk }
        content { contentType("application/json;charset=UTF-8") }
        jsonPath("$.name") { value("Lee") }
        json("""{"someBoolean": false}""", strict = false)
      }
    }
 }

I've had to keep the builder since it creates a receiver block for the MockHttpServletRequestBuilder. Any recommendations or a different name? request or requestBuilder perhaps?

checketts · 2019-02-19T18:36:45Z

spring-test/src/test/kotlin/org/springframework/test/web/servlet/MockMvcDslTests.kt

+	}
+
+	@Test
+	fun json() {


Here is the passing test that demonstrates the DSL

checketts · 2019-02-19T18:47:19Z

spring-test/src/test/kotlin/org/springframework/test/web/servlet/MockMvcDslTests.kt

+			builder { accept(MediaType.APPLICATION_XML) }
+
+			andDo(MockMvcResultHandlers.print())
+			andExpect(status().isOk)


I left andDo and andExpect as methods that allow the user to transition to the new syntax. They can use the syntax they are familiar with as well as include their existing matchers.

By changing to print() for printing the request and response then it forces the user to qualify MockMvcResultHandlers.print(), but I think that is fine.

I haven't switched andDo to handle yet. I think we need some better examples before changing that. handle(document()) for RestDocs for example feels weird.

sdeleuze · 2019-02-20T15:01:31Z

Thanks I will take it from here.

sdeleuze · 2019-02-22T12:30:26Z

After more thoughts, I have rewritten the DSL entirely and ended-up with that more idiomatic DSL which is still pretty close to MockMvc spirit with a router { } style DSL:

mockMvc {
	GET("/person/{name}", "Lee") {
		print()
		request {
			secure = true
			headers {
				accept = listOf(MediaType.APPLICATION_JSON)
			}
			principal = Principal { "foo" }
		}
		expect {
			status { isOk }
			content { contentType("application/json;charset=UTF-8") }
			jsonPath("$.name") { value("Lee") }
			content { json("""{"someBoolean": false}""", false) }
		}
	}
	POST("/person") {
		request {
			content = """{ "name": "foo" }"""
			headers {
				accept = listOf(MediaType.APPLICATION_JSON)
				contentType = MediaType.APPLICATION_JSON
			}
		}
		expect {
			status {
				isCreated
			}
		}
	}
}

See this commit for more details. Any thoughts @checketts @jnizet @gzoritchak?

sdeleuze · 2019-02-22T12:35:54Z

Note it is possible to remove the request { } block if we want. Not sure.

checketts · 2019-02-22T13:14:17Z

It looks good! The only questions I have are around extensiblility.

What is the equivalent of with(user(myUser)) in the request builder? I use that extensively to test code that relies on logged in user details
In MockHttpServletRequestDsl(private val builder: MockHttpServletRequestBuilder) could we make the builder public so I can add my own extension functions (or expose the builder some way)?

In my own code I've added a couple helpers like json where I set the content type to json and pass in a string (setting the content) all at once. I realize that is a specialized usecase, so that is why I had left it out of the DSL, but allowed for the extensiblity in my own tests.
We also might want to consider making MockMvcPerformDsl more extensible as well. For example, how would RestDocs' andDo(document()) hook in? Perhaps we just need methods that allow the user to add their own resultHandler/resultMatcher?
MockMvcDsl do we want to add a perform method so a user can pass in the method as a variable?
In one test (master...sdeleuze:kotlin-mockmvc#diff-1950a1703b6b3d8d2b5110324ebbe4e2R124) you nested the print and expect, was that intentional? Do we want to add @DslMarker to discourage that?

jnizet · 2019-02-22T13:32:47Z

Here are my thoughts on this example alone. I'll comment on the code after, as comments to the commit.

accept = listOf(MediaType.APPLICATION_JSON). I find this a bit too verbose. In most of the cases, in my experience, we pass a single value for this header. So maybe a fun accept(vararg mediaType: MediaType) would be nicer: accept(MediaType.APPLICATION_JSON) in the common case, and if we want more: accept(MediaType.APPLICATION_JSON, MediaType.TEXT_HTML)
I find it strange to have jsonPath("$.name") { value("Lee") } outside of the content { } block, since the jsonPath expectation is about the content.
The contentType() expectation, on the other hand, is not about the content, but about the Content-Type header. So I wouldn't expect it inside a content { } block. If it must be in a block, I would expect it inside a headers block.
I'm not sure I like the all-uppercase GET and POST methods. They violate the standard naming conventions, and are inconsistent with the Java DSL which uses get and post.

sdeleuze · 2019-02-22T13:45:55Z

@checketts I have updated my branch in order to take in account your feedback:

I added MockHttpServletRequestDsl#with
I made the DSL classes open instead
I added MockMvcPerformDsl#handler + DSL class is open now so you can use extensions on them
I added MockMvcDsl#request and MockMvcDsl#multipart
Food catch, the DSL is now using @DslMarker

sdeleuze · 2019-02-22T14:12:45Z

@jnizet Please find my feedback:

I have bring back accept and contentType shortcuts.
For jsonPath expectation I think I just follows the Java DSL
For contentType expectation I think I just follows the Java DSL
Maybe better to stay consistent with the Java DSL indead.

I have also removed the request { } that is a kind of artificial one.

Latest version:

mockMvc {
	get("/person/{name}", "Lee") {
		print()
		secure = true
		accept = MediaType.APPLICATION_JSON
		headers {
			contentLanguage = Locale.FRANCE
		}
		principal = Principal { "foo" }
		expect {
			status { isOk }
			content { contentType("application/json;charset=UTF-8") }
			jsonPath("$.name") { value("Lee") }
			content { json("""{"someBoolean": false}""", false) }
		}
	}
	post("/person") {
		content = """{ "name": "foo" }"""
		headers {
			accept = listOf(MediaType.APPLICATION_JSON)
			contentType = MediaType.APPLICATION_JSON
		}
		expect {
			status {
				isCreated
			}
		}
	}
}

I like it :-)

checketts · 2019-02-22T14:22:57Z

Yeah it looks great! The only question I have left is: 'How can a user add a custom matcher?' (extending the expect block)

sdeleuze · 2019-02-22T17:31:41Z

@checketts Thanks, but I think there we need to stay closer to MockMvc lifecycle, I think I have been too far in mixing various parts of the DSL.

Here is my latest proposal which seems to me more consistent. The number of optional parameter is important but with named parameter it seems doable and this is more close to MockMvc spirit and lifecycle.

mockMvc {
	request(GET, "/person/Lee",
			secure = true,
			accept = MediaType.APPLICATION_JSON,
			headers = { contentLanguage = Locale.FRANCE },
			principal = Principal { "foo" }) {
		status { isOk }
		print()
		content { contentType("application/json;charset=UTF-8") }
		jsonPath("$.name") { value("Lee") }
		content { json("""{"someBoolean": false}""", false) }
	}

	request(POST, "/person",
			content = """{ "name": "foo" }""",
			headers = {
				accept = listOf(MediaType.APPLICATION_JSON)
				contentType = MediaType.APPLICATION_JSON
			}) {
		status { isCreated }
	}
}

I have pushed the updated implementation on my branch. Any thoughts?

jnizet · 2019-02-22T18:11:08Z

Hmm, I'm not a big fan, especially since the varargs for url variables is missing here, and is quite important, IMHO.

Why not just chain calls, just the way we do it with list.map { ... }.filter { ... }?

The DSL could look like the following, which is actually closer to your original proposal:

mockMvc {
	get("/persons/{name}", "Lee") { // urlTemplate, vararg variables
		secure = true,
		accept(MediaType.APPLICATION_JSON) // vararg, since we might want to pass several media types
		headers {
			contentLanguage = Locale.FRANCE
		}
		principal = Principal { "foo" }
	}.then {
		status { isOk }
		print()
		content { 
			contentType("application/json;charset=UTF-8") 
		}
		jsonPath("$.name") { value("Lee") }
		content {
			json("""{"someBoolean": false}""", false) 
		}
	}
}

This is actually closer to the Java DSL, while still being very Kotlinesque, IMHO. It allows doing everything you want to create the request. Since the get()/post()/put(), etc. functions take some RequestBuilder.() -> Unit extension function, we can extend the DSL by creating helper extension functions to specify several properties at once. And the ActionsBuilder.() -> Unit passed to then() has the MockMvc ResultActions available to simply execute all the result matchers and result handlers sequentially.

I also like how the two blocks act on the two main entities of a REST call: the request, and the response.

What do you think?

sdeleuze · 2019-02-22T18:24:16Z

Yeah, I was hesitating between both approaches, I will update my branch accordingly. Not sure about then, we can maybe keep expect even if we integrate some handlers to the mix ?

jnizet · 2019-02-22T18:35:30Z

Yes, I chose then() because it was more neutral. Maybe and()`? expect() would be a bit strange indeed, since print(), document(), log(), etc. are not expectations.

checketts · 2019-02-22T18:54:54Z

I'm not sure I understand the need to separate the request/response. Is it just to keep the 2 separate visually? I thought the reworking was to preserve the call/expect ordering that @jnizet had noted.

If we stayed with the request block @sdeleuze had first mentioned, we would have the separation:

		request {
			secure = true
			headers {
				accept = listOf(MediaType.APPLICATION_JSON)
			}
			principal = Principal { "foo" }
		}

sdeleuze · 2019-02-22T19:06:00Z

Latest iteration closer to original MockMvc Java DSL but still idiomatic Kotlin:

mockMvc.get("/person/{name}", "Lee") {
	secure = true
	accept = MediaType.APPLICATION_JSON
	headers { contentLanguage = Locale.FRANCE }
	principal = Principal { "foo" }
} andExpect {
	status { isOk }
	content { contentType("application/json;charset=UTF-8") }
	jsonPath("$.name") { value("Lee") }
	content { json("""{"someBoolean": false}""", false) }
} andDo {
	print()
}

mockMvc.post("/person") {
	content = """{ "name": "foo" }"""
	headers {
		accept = listOf(MediaType.APPLICATION_JSON)
		contentType = MediaType.APPLICATION_JSON
	}
} andExpect {
	status { isCreated }
}

checketts · 2019-02-22T19:09:58Z

This is much better than the one with all the optional arguments.

If I don't need to customize the request then a minimal one would be:

mockMvc.get("/person/{name}", "Lee") andExpect {
	status { isOk }
}

jnizet · 2019-02-22T19:25:50Z

I like this last one a lot (except for accept = MediaType.APPLICATION_JSONwhich I would still replace by accept() with a vararg). 👏
andExpect and andDo both return the same type, right? So we can chain them in any order?

jnizet · 2019-02-22T19:43:28Z

I had a look at the code, and it confirms that this last version is the right one to me: it looks very Kotlinesque and the code makes it clear that it's actually a thin wrapper around the Java DSL, making the transition between both intuitive and natural. 👍

I'm not sure I understand why the classes are open, though.

sdeleuze · 2019-02-22T19:46:40Z

Glad you like it @checketts @jnizet :-)

Yes andExpect and andDo return the same type. I will have a look to accept but I kind like the property syntax for a single one which is pretty common, while headers { accept(listOf(...)) } allows to set multiple one if needed.

Classes are open to allow users to add extensions.

jnizet · 2019-02-22T19:56:28Z

Classes are open to allow users to add extensions.

A class doesn't need to be open to add extensions to it: you can add extensions to String, Int, whatever. It only needs to be open if you want to create a subclass. But the DSL (ResultActionsWrapper, for example) won't ever create an instance of a subclass (it calls MockMvcResultMatchersDsl(actions)) , so I don't see why it would be necessary.

sdeleuze · 2019-02-22T20:28:42Z

What you describe is true only for Java classes, Kotlin classes need to be open to allow users to add extensions on them.

checketts · 2019-02-22T20:29:53Z

Please add default values for the dsl: MockHttpServletRequestDsl.() -> Unit parameter.

Otherwise the minimal usecase requires you to customize the response, even when it isn't needed:

mockMvc.get("/person/{name}", "Lee") {} andExpect { //<- note the extra curlies before the andExpect 
	status { isOk }
}

checketts · 2019-02-22T21:04:17Z

The latest is definitely the best. Some of the extensions I had tried before weren't possible, and are possible again.

Here is the comparison I've been working against: https://github.com/sdeleuze/spring-framework/pull/1/files

Another finding: the handle method is hard to use as-is due to the kotlin lambda not being matched to the ResultHandler, I propose a top level andHandle addition to ResultActionsWrapper, then we can just make the receiver MvcResult directly:

	infix fun andHandle(handler:MvcResult.()-> Unit): ResultActionsWrapper {
		actions.andDo { it.handler() }
		return this
	}

checketts · 2019-02-22T21:21:20Z

@jnizet Here is an example of extending the expects DSL: https://github.com/sdeleuze/spring-framework/pull/1/files#diff-525cfe15474d3e20b0902649bd2e8695R31 which in this case required it to be open

And here is where it is used: https://github.com/sdeleuze/spring-framework/pull/1/files#diff-525cfe15474d3e20b0902649bd2e8695R53

	@Test
	fun `hello json`() {
		val name = "Petr"
		mockMvc.get("/hello/$name").andExpectCustom(::ClintMatchers) {
			"$.surname" jsonPathIs name //JsonPath
			"surname" jsonPath { value("Petr") }
			model<HelloPostDto>("helloPostDto") {
				assertEquals("Balat", surname)
			}
		}
	}

I'm open to feedback on if this approach is even useful. I was mainly exploring if it was open enough to support these goals.

jnizet · 2019-02-22T23:12:00Z

@sdeleuze please check this link. It shows that a class doesn't need to be open to accept extension functions. They're just disguised static helper method taking the receiver as argument. Am I missing something?

checketts · 2019-02-23T04:13:29Z

Here is a better approach: if we make actions public in ResultActionsWrapper then it allows the custom expect block as well as andDocument which restdocs would want:

See: https://github.com/sdeleuze/spring-framework/pull/1/files#diff-525cfe15474d3e20b0902649bd2e8695R54

	fun ResultActionsWrapper.andDocument(identifier: String, configure: DocumentationScope.() -> Unit): ResultActionsWrapper {
		actions.andDo(DocumentationScope(identifier).apply(configure).document())
		return this
	}

sdeleuze · 2019-02-23T07:43:30Z

@jnizet My mistake, I thought it was required.

Thanks for your help, I will be off during my week of vacation. I will check extensibility with you, document and add more tests when I will be back.

jnizet · 2019-02-23T07:51:17Z

Meanwile, I've polished a few things in the restdocs DSL, which is now fully documented, tested with 100% coverage, and with a DSL for preprocessors too. You can check it out at https://github.com/Ninja-Squad/spring-rest-docs-kotlin. I'm pretty happy with the result.

With the handle() method, adding an andDocument() extension function which will call andDo and delegate to handle() to add the documentation handler shouldn't be a problem.

sdeleuze · 2019-03-04T10:17:00Z

I have the possibility to avoid ResultActionsWrapper by using infix extensions like infix fun ResultActions.andExpect(dsl: MockMvcResultMatchersDsl.() -> Unit): ResultActions.

On one side, I like the fact we avoid an artificial wrapper that can limit extensibility and make the DSL available in every ResultActions. But it may confuse people since mockMvc.get(...) { ... }.andExpect { ... } won't provide the Kotlin DSL, only mockMvc.get(...) { ... } andExpect { ... } will.

Any thoughts @jnizet @checketts?

jnizet · 2019-03-04T10:39:43Z

The confusion between .andExpect and andExpect is a deal-breaker to me. I don't really mind the ResultActionsWrapper class existence, but a better name would be nice.

checketts · 2019-03-04T13:09:44Z

Another strike I noticed is that you can't chain a non-infix after an infix. For example if I created an andDocument("iHaveAParameterSoICan'tBeInfix") { } block then the compiler wouldn't let me add it after calling andDo or andExpect using their infix forms.

This commit introduces a `MockMvc` Kotlin DSL via a set of extensions like `MockMvc.get` or `MockMvc.request` that provide a Kotlin idiomatic and easily discoverable API while staying close to the original Java API design. Closes spring-projectsgh-1951

jnizet · 2019-03-04T17:20:45Z

🎉

The infix form limits the extensibility of the API and prevents calling `andReturn()`. See spring-projectsgh-1951

See spring-projectsgh-1951

This was referenced Sep 6, 2018

Ability to contribute to SpringTest? petrbalat/kd4smt#7

Closed

Create Kotlin DSL for Spring RestDocs spring-projects/spring-restdocs#547

Open

jhoeller assigned sdeleuze Sep 13, 2018

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 24, 2019

sdeleuze added in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 16, 2019

sdeleuze added this to the 5.2 M1 milestone Feb 16, 2019

checketts force-pushed the master branch from ba025ef to 5b2c9c8 Compare February 19, 2019 18:20

checketts commented Feb 19, 2019

View reviewed changes

Add Kotlin DSL for working with MockMvc

f7aeb9d

checketts force-pushed the master branch from 5b2c9c8 to f7aeb9d Compare February 19, 2019 18:37

checketts commented Feb 19, 2019

View reviewed changes

sdeleuze closed this in 0c33228 Mar 4, 2019

sdeleuze added a commit to sdeleuze/spring-framework that referenced this pull request Mar 4, 2019

Remove infix qualifier from ResultActionsDsl methods

9e873af

The infix form limits the extensibility of the API and prevents calling `andReturn()`. See spring-projectsgh-1951

sdeleuze added a commit to sdeleuze/spring-framework that referenced this pull request Mar 5, 2019

Add missing MockMvc.head Kotlin extension

412c673

See spring-projectsgh-1951

Add Kotlin DSL for working with MockMvc #1951

Add Kotlin DSL for working with MockMvc #1951

Conversation

checketts commented Sep 6, 2018 • edited Loading

checketts commented Sep 13, 2018

sdeleuze commented Feb 19, 2019

checketts commented Feb 19, 2019 • edited Loading

checketts commented Feb 19, 2019 • edited Loading

checketts commented Feb 19, 2019 • edited Loading

sdeleuze commented Feb 19, 2019

sdeleuze commented Feb 19, 2019 • edited Loading

checketts commented Feb 19, 2019

checketts Feb 19, 2019

Choose a reason for hiding this comment

checketts Feb 19, 2019

Choose a reason for hiding this comment

sdeleuze commented Feb 20, 2019

sdeleuze commented Feb 22, 2019

sdeleuze commented Feb 22, 2019

checketts commented Feb 22, 2019 • edited Loading

jnizet commented Feb 22, 2019

sdeleuze commented Feb 22, 2019

sdeleuze commented Feb 22, 2019

checketts commented Feb 22, 2019

sdeleuze commented Feb 22, 2019

jnizet commented Feb 22, 2019 • edited Loading

sdeleuze commented Feb 22, 2019

jnizet commented Feb 22, 2019

checketts commented Feb 22, 2019 • edited Loading

sdeleuze commented Feb 22, 2019 • edited Loading

checketts commented Feb 22, 2019 • edited Loading

jnizet commented Feb 22, 2019

jnizet commented Feb 22, 2019

sdeleuze commented Feb 22, 2019

jnizet commented Feb 22, 2019

sdeleuze commented Feb 22, 2019

checketts commented Feb 22, 2019

checketts commented Feb 22, 2019 • edited Loading

checketts commented Feb 22, 2019

jnizet commented Feb 22, 2019 • edited Loading

checketts commented Feb 23, 2019

sdeleuze commented Feb 23, 2019

jnizet commented Feb 23, 2019

sdeleuze commented Mar 4, 2019

jnizet commented Mar 4, 2019

checketts commented Mar 4, 2019

jnizet commented Mar 4, 2019

checketts commented Sep 6, 2018 •

edited

Loading

checketts commented Feb 19, 2019 •

edited

Loading

checketts commented Feb 19, 2019 •

edited

Loading

checketts commented Feb 19, 2019 •

edited

Loading

sdeleuze commented Feb 19, 2019 •

edited

Loading

checketts commented Feb 22, 2019 •

edited

Loading

jnizet commented Feb 22, 2019 •

edited

Loading

checketts commented Feb 22, 2019 •

edited

Loading

sdeleuze commented Feb 22, 2019 •

edited

Loading

checketts commented Feb 22, 2019 •

edited

Loading

checketts commented Feb 22, 2019 •

edited

Loading

jnizet commented Feb 22, 2019 •

edited

Loading