Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow grouping assertions in MvcResultAssert #32642

Closed
odrotbohm opened this issue Apr 16, 2024 · 5 comments
Closed

Allow grouping assertions in MvcResultAssert #32642

odrotbohm opened this issue Apr 16, 2024 · 5 comments
Labels
in: test Issues in the test module status: declined A suggestion or change that we don't feel we should currently apply

Comments

@odrotbohm
Copy link
Member

Standard MockMvc allows setting up expectations targeting different aspects of the response through its ….andExpect(…) method. On MvcResultAssert, once you move into a particular assert, for example ….contentType(), there's no way to get back to MvcResultAssert, to define constraints on another aspect of the response.

Do you think there could be an ….and() method that would navigate back to MvcResultAssert and allow me to write something like this?

assertThat(mvc.perform(get("/")))
  .hasStatus(HttpStatus.OK);
  .contentType().isCompatibleWith(…).and() // trailing and() navigates back to MvcResultAssert
  .body().jsonPath().extractingPath(…).isNotNull();

The current workaround is assigning MvcResultAssert to a variable and calling those methods on it, which adds a bit of ceremony.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 16, 2024
@jhoeller jhoeller added the in: test Issues in the test module label Apr 16, 2024
@snicoll
Copy link
Member

snicoll commented Apr 16, 2024

The way to do this with AssertJ is via satisfies that doesn’t require any assignment. We explicitly avoid that use of and as it’s not idiomatic with AssertJ and we’re following their lead. What is the problem with that approach?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Apr 16, 2024
@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Apr 23, 2024
@odrotbohm
Copy link
Member Author

odrotbohm commented Apr 23, 2024

What is the problem with that approach?

Boilerplate code that's not needed when using the established MockMvc approach.

I don't want to repeat the discussion we already had on #32643. I'd just, for the record, would like to leave that being able to define expectations on any aspect of the response in any order has been idiomatic with MockMvc. With the new API, users expecting that flexibility might be caught by surprise, as they're left in a dead end once they've moved down one path of the assertion chain (e.g., body assertions).

I've found the discoverability of domain-specific assertions (the methods specific to responses and response aspects) to be quite hidden between all the generic assertion methods (satisfies). That's something we cannot fix, as it's the way that AssertJ works. But it's one more reason to avoid additionally making these methods harder to discover. “Oh, I can get access to other assertions, if I just assign this original thing to a variable and then call different methods on it.” is a realization you actually have to come to in the first place, especially if you're trained to think in the fluent MockMvc style that has been around for ages.

Just to make sure I don't appear obnoxious about this: I think we're dealing with two separate idiomacies here. One being the established MockMvc programming model that allowed users to flexibly and fluently trigger assertions on different aspects. The other being the way that AssertJ assertions being designed usually. There are two reasons that I think we should favor the former over the latter:

  1. Users' expectations are more driven by things they have (had) available (the methods on MockMvc) than they're by things they did not have available (additional methods on the assertion).
  2. The consequences of “violating” the former idiomacy will cause users trouble to formulating the assertions they're used to defining. At least initially. If they've overcome that discoverability hurdle, they still need to write more cumbersome code. I do not yet understand the negative consequences of “violating” the latter idiomacy, especially not for our users.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Apr 23, 2024
@snicoll
Copy link
Member

snicoll commented Apr 24, 2024

I think we're coming from two different camps and it's a question of whether we could please both camps by offering both approaches but there's more code to maintain and that'd force users to figure out which they should use.

For me, if I'm choosing to use MvcResultAssert that's because I like AssertJ and the style of assertions that its opinions promote. So I would much prefer MvcResultAssert to follow AssertJ conventions rather then MockMvc conventions.

“Oh, I can get access to other assertions, if I just assign this original thing to a variable and then call different methods on it.”

IMO, that's the established approach with AssertJ and it feels natural to me as an experienced user of AssertJ.

especially if you're trained to think in the fluent MockMvc style that has been around for ages

I don't think this is the target audience. I expect MvcResultAssert to appeal to those who know and love AssertJ and have been looking forward to getting rid of MockMvc.

Repeating myself, assigning the result with multiple assertThat or use satisfies if you don't want to assign the result is what AssertJ users are doing. I like the natural grouping that you get with this sort of approach rather than chaining absolutely everything. I find the tests easier to read when there's some separation of the assertions into different areas. So you'd have one statement for the status, one for the content type, one for the body, and so on. You then get natural line breaks whereas if it's all one mass of chained methods it's easy to lose track of exactly what's being asserted.

On a final note, Spring Security is moving away from and() in its DSL and I think that's a good thing. Consistency across the portfolio would also be nice here.

With all that said, MvcResultAssert is a bit special as it encompasses a lot of different concepts so I would like to explore other options to "group" things at the root level. I'll requalify this issue accordingly.

@snicoll snicoll changed the title Allow chaining assertions of different kinds back into MvcResultAssert Allow grouping assertions in MvcResultAssert Apr 24, 2024
@snicoll
Copy link
Member

snicoll commented May 8, 2024

So we've spent quite a bit of time in #32712 and that led to a bit of more first-level type on the root assert but nothing that would change significantly what we have. We'll continue iterating but I want to emphasis on what what said previously and our wish to follow AssertJ conventions.

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale May 8, 2024
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants