-
Notifications
You must be signed in to change notification settings - Fork 38.3k
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
Rename AssertableMockMvc to MvcTester and review assertions structure #32712
Comments
We've made quite a bit of progress here, focusing on the renaming bit of it. We came to the following proposal:
Irrespective of this issue, we've reviewed of the API and found several shortcuts to be desirable, as well as avoid a double navigation for the body. This proposal is available at https://github.com/snicoll/spring-framework/tree/mvctester-review |
This commit merges the JSONCompare and JsonPath support in a single assert object. The rationale for the previous situation was that JsonPath is optional but merging the assertions methods do not make it mandatory as the usage if JsonPath is triggered only if it is actually used. See gh-32712
This commit removes ResponseBodyAssert and rather offers first-class access support for the response body at the root level using bodyText(), bodyJson(), and body(). This avoids a double navigation to assert the response body. See gh-32712
This commit renames AssertableMockMvc to MockMvcTester as the former was too mouthful, and we are looking for a naming pattern that can be applied consistently to test utilities that rely on AssertJ. Rather than AssertableMvcResult, we now use MvcTestResult and it no longer extends from MvcResult itself. That avoids having existing code that is migrated to the new API whilst assigning the result to MvcResult and get a bad DevXP as that doesn't bring any of the AssertJ support. See gh-32712
I've applied those upgrades to the sample project I had on M1. As can be seen in this commit, this indeed improves the code to be written in some places. That said, it actually introduces more boilerplate in many places, as there's now a If you look at the diff overall, it really shows that the previously concise declaration (one fluent statement verifying and producing a result) now has to be scattered into assignment, assertions, and the very same two-levels-deep lookup. |
Thanks for trying the update. The missing
That's not an equivalent of what you had before. What you had before is: .body().jsonPath().extractingPath("$.status").isEqualTo("Delivered"); The equivalent after this issue is: .bodyJson().extractingPath("$.status").isEqualTo("Delivered"); I guess it looks more complicated as you're trying to build a single assertion statement irrespective of how complicated it may or may not be. This issue didn't mean to address that I am afraid (as we've established several times).
Assignment and assertions are to be expected, that's just how AssertJ works and I still have this pattern of returning a new assert object based on a function on my radar. That would allow you to chain multiple invocations where the next request can be triggered based on the response of the previous one. We've tried several API changes based on your tests but couldn't find anything that we find satisfactory so far. |
When we perform a request with MockMvc, we get a
MvcResult
that contains all the elements we need to assert that the processing of the request went well, such as:AssertableMvcResult
expands on the latter by providing the unresolved exception as well, rather than throwing a checked exception asMockMvc
does.This class can then be fed to AssertJ and we can then navigate on various pieces of the result:
There are two things we'd like to explore here:
AssertableMvcResult
is a bit off putting so we should consider renaming itMvcResult
than extendingMvcResult
. This may allow us to navigate to more structured part of the result, such as:paging @philwebb and @simonbasle who helped me brainstorm on this.
The text was updated successfully, but these errors were encountered: