Support varargs for expectations in MockMvc [SPR-16417]

[spring-projects-issues]

Valeriy Zhirnov opened SPR-16417 and commented

The current way of writing multiple expect statements is kinda verbose:

mockMvc.perform(get("/person/1"))
  .andExpect(status().isOk())
  .andExpect(content().contentType(MediaType.APPLICATION_JSON))
  .andExpect(jsonPath("$.person.name").value("Jason"));

I propose vararg method that can do the same in a slightly cleaner way using default interface method:

mockMvc.perform(get("/person/1"))
  .andExpect(
    status().isOk(),
    content().contentType(MediaType.APPLICATION_JSON),
    jsonPath("$.person.name").value("Jason")
  );

the pull reuest are at github by the link in reference url

---

Affects: 5.0.3

Reference URL: #1651

Issue Links:

• Common API for MockMvc tests and for real HTTP tests #19647 MockMvc compatible API for doing real HTTP tests

Referenced from: commits c57e1af

[spring-projects-issues]

Rossen Stoyanchev commented

Thanks for raising this. It doesn't change things that much though. It's still 1 assertion per line and the typing effort is about the same with 1 static import per assertion.

It's a simple enough change but now that we have the WebTestClient (fluent API minus the static imports) on the WebFlux side, I think we should consider a similar fluent API for MockMvc that exposes the options available through MockMvcResultMatchers but without the need for static imports. So it might look more like this:

mockMvc.perform(get("/person/1")).andExpect(allOf().
    .status().isOk()
    .content().contentType(MediaType.APPLICATION_JSON)
    .jsonPath("$.person.name").value("Jason")
);

[spring-projects-issues]

Valeriy Zhirnov commented

Thanks for reply, but i thiknk this kind of api is bad and feels unnatural, and kinda goes by utlily classes antipattern,
and also article about private static methods, what you code suggest, its a form of a builder pattern, ie it suggest that underling object is big and complex, and thats bad
And way of object composition is more prefferable for this code
Here is example

mockMvc.perform(get("/person/1")).andExpect(
	new AllOf(
			new StatusIs(HttpStatus.OK),
			new ContentTypeEq(MediaType.APPLICATION_JSON),
			new JsonPathValueEq("$.person.name", "Jason"),
			new AnyOf(
					JsonPathValueEq("$.person.role", "Developer"),
					JsonPathValueEq("$.person.role", "Admin")
			),
			result -> assertEquals("X-Cookie", "Secret", result.getResponse().getCookie("X-Cookie"))
	)
);

each matcher is simple, has many constructors, and have only one public method from implemented ResultMatcher interface
and developer using this code would not need to know that are somewhere is static method that they can use, they just write new, shift+space and see all available options
so life gets easier for everyone

also also, if i want to provide my own mathcer, and use fluent style, it will be looking like this
allOf().status().isOk().mathcer(new MyEpicMatcher("Hello"))

this is terrible practice
on the other hand object composition is easily extendable, i just write my own mathcer, add immediatly can use it, without some fluent satatic method, that accepts it, and all the code will be in same style

[spring-projects-issues]

Rossen Stoyanchev commented

I don't follow what you find bad or unnatural. I am proposing neither utility classes, nor static methods. I am simply looking to match the WebTestClient added in Spring Framework 5, which avoids static methods entirely. Some example tests here.

[spring-projects-issues]

Valeriy Zhirnov commented

Sorry, was in a bit of rush, and did not make my points clear, i'll try again from the beginig

What this ticket about?

It is a minor enchantment, that is not necessary, but reduces some repetition in a code, and does not affect anything

What you suggested

You suggested to create entirely different api, based on existing code
I do not know spring internals very well, but looks like it will break some code

Provided example of changes

mockMvc.perform(get("/person/1")).andExpect(allOf().
    .status().isOk()
    .content().contentType(MediaType.APPLICATION_JSON)
    .jsonPath("$.person.name").value("Jason")
);

call to allOf() is definitely static method call

My point of view on fluent api

Here is the code using this type of api

@Test
public void entityWithConsumer() throws Exception {
     this.client.get().uri("/John")
               .exchange()
               .expectStatus().isOk()
               .expectHeader().contentType(MediaType.APPLICATION_JSON_UTF8)
               .expectBody(Person.class)
               .consumeWith(result -> assertEquals(new Person("John"), result.getResponseBody()));
}

It looks like complex builder, requires special formatting, and a lot of internal knowledge, when used
I, as a user of a library, can not extend it, or modify, to use in my special cases.
For example: i cannot easily add my own matchers, like exchange().expectStatusIsTeapot() or excange().validateOverRemoteServer()
Code reusability: from previous point, we can extrapolate that there's zero code reusability, i am bound to write same chains of method calls over and over

Example of same test but using proper object composition, for clarity i will use kotlin here with some pseudocode

// encapsulates status and content type matchers
class ValidRestResponse {
	fun match(body: ResponseBody) {
		Status(body, IsEqual(HttpStatus.OK))
		ContentType(body, IsEqual(MediaType.APPLICATION_JSON_UTF8))
	}
}

ResponseMatches(
		GetRequest(client = client, path = "/John"),
		AllOf(
			ValidRestResponse()
			Body(IsEqual(Person("John")),
			ValidateOverRemoteServer("127.0.0.1"),
			HasHeader("Secret")
		)
).match()

This code is crystal clear, no special formatting, any part of it can be used independently, any part can be combined and reused (i.e. in sample is ValidRestResponse combines some repeated matchers from the tests), i can easily provide my own matchers, decorate and reuse matchers provided by framework, everyone is happy

[spring-projects-issues]

Rossen Stoyanchev commented

The main point with the fluent WebTestClient API is that it's discover-able, and does not require knowledge of static imports. It may be complex to build but it should be easy to use. I don't see that it requires any knowledge.

As for not being extensible it's simply not true. It was specifically designed to allow alternatives such as Hamcrest or AssertJ. There are consumeWith(Consumer<...>) options built in several places. So no specialized interfaces, you can choose what you prefer to use.

I think you should spend some time using the API before criticizing it.

The change you proposed is very simple indeed and it may yet be added. In the mean time I don't see anything that should hold you back since you can create your own ResultMatcher that takes a vararg of expectations.

[spring-projects-issues]

Rossen Stoyanchev commented

Resolved via commit c57e1a.