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.

Sign up for GitHub

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

Jump to bottom

Refactoring for MockServerHttpRequest #25148

Closed
wants to merge 3 commits into from
Closed

Refactoring for MockServerHttpRequest #25148

wants to merge 3 commits into from

Conversation

midumitrescu
Copy link
Contributor

@midumitrescu midumitrescu commented May 28, 2020 •
edited

Created a small refactoring for gh-25109.

Changed:

  1. Field duplication
  2. Set DefaultBodyBuilder as protected, to allow children from other packages to call constructor (it seemed very constrictive to allow the call of the contructor only from the existing package)
  3. Moved the
Assert.notNull(method, "HttpMethod is required. If testing a custom HTTP method, " +
					"please use the variant that accepts a String based HTTP method.");

to DefaultBodyContructor to also catch the contructor call

public static BodyBuilder method(HttpMethod method, URI url) {
		return new DefaultBodyBuilder(method, url);
	}

to leave the main constructor easier to understand and failfast when constructing he builder.
Also, messages when calling the

MockServerHttpRequest.method(null, *any URI*)

would have failed with another kind of assertion message than

MockServerHttpRequest.method(null, urlTemplate, param1, param2)
  1. Extended the check for the string value of HttpMethod to exclude the set of HttpMethod to only whitespaces, as this, imho, makes no sense

@midumitrescu
Copy link
Contributor Author

midumitrescu commented May 28, 2020 •
edited

Hey Rosen (@rstoyanchev ),

would you please mind taking a short look if this makes any sense to you?

I could also create some small unit tests to make sure that the failure message is the same when HttpMethod null is set, but I think this is not that valuable

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 28, 2020
@sbrannen sbrannen requested a review from rstoyanchev May 28, 2020 10:29
@sbrannen sbrannen added in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) labels May 28, 2020
@rstoyanchev rstoyanchev self-assigned this May 28, 2020
rstoyanchev
rstoyanchev reviewed May 28, 2020
View reviewed changes
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a reasonable simplification and the resolve is a map lookup so it shouldn't be a concern.

...-test/src/main/java/org/springframework/mock/http/server/reactive/MockServerHttpRequest.java
URI uri, @Nullable String contextPath, HttpHeaders headers, MultiValueMap<String, HttpCookie> cookies,
@Nullable InetSocketAddress remoteAddress, @Nullable InetSocketAddress localAddress,
@Nullable SslInfo sslInfo, Publisher<? extends DataBuffer> body) {

super(uri, contextPath, headers);
Assert.isTrue(httpMethod != null || customHttpMethod != null, "HTTP method must not be null");
this.httpMethod = httpMethod;
this.customHttpMethod = customHttpMethod;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still can assert the httpMethodValue is not null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rstoyanchev I added some easy tests, testing all possible build paths.

Should I anyways add the assertion here?

@rstoyanchev rstoyanchev removed the status: waiting-for-triage An issue we've not yet triaged or decided on label May 28, 2020
@rstoyanchev rstoyanchev added this to the 5.3 M1 milestone May 28, 2020
@rstoyanchev rstoyanchev added the type: task A general task label May 28, 2020
@rstoyanchev rstoyanchev changed the title Refactoring for gh-25109 Refactoring for MockServerHttpRequest May 28, 2020
@midumitrescu
Copy link
Contributor Author

Quick question, sorry for asking this.

There is another MockServerHttpRequest in

org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest

Is that auto generated or should it also receive this functionality ?

@sbrannen
Copy link
Member

sbrannen commented May 28, 2020 •
edited

There is another MockServerHttpRequest in

You will need to manually copy the changes to that class to keep them in sync.

@midumitrescu
Add tests for spring-projectsgh-25109
dd91a7e
@midumitrescu
Keep in sync org.springframework.web.testfixture.http.server.reactive…
e59b902 
….MockServerHttpRequest with org.springframework.mock.http.server.reactive.MockServerHttpRequest for spring-projectsgh-25109
@midumitrescu
Copy link
Contributor Author

There is another MockServerHttpRequest in

You will need to manually copy the changes to that class to keep them in sync.

Done

rstoyanchev added a commit that referenced this pull request May 29, 2020
@rstoyanchev
Polishing contribution
8d44947 
See gh-25148
kenny5he pushed a commit to kenny5he/spring-framework that referenced this pull request Jun 21, 2020
@rstoyanchev @kenny5he
Polishing contribution
41736eb 
See spring-projectsgh-25148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rstoyanchev rstoyanchev Awaiting requested review from rstoyanchev

Assignees

@rstoyanchev rstoyanchev

Labels
in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) type: task A general task
Projects
None yet
Milestone
5.3 M1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@midumitrescu @sbrannen @rstoyanchev @spring-projects-issues