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

Mock: manage headers using RequestHeaders #706

Merged
merged 7 commits into from
May 17, 2018
Merged

Mock: manage headers using RequestHeaders #706

merged 7 commits into from
May 17, 2018

Conversation

pboutes
Copy link
Contributor

@pboutes pboutes commented May 17, 2018

Add the new class RequestHeaders in order to improve management of request headers when testing.

So instead of doing this:

    Map<String, Collection<String>> headers = new HashMap<>();
    headers.put("my-header", Arrays.asList("val"));
    headers.put("my-header-2", Arrays.asList("val2", "val3"));

we can do :

    RequestHeaders headers = RequestHeaders.builder()
      .add("my-header", "val")
      .add("my-header-2", Arrays.asList("val2", "val3"))
      .build();

or

    RequestHeaders headers = RequestHeaders.builder()
      .add("my-header", "val")
      .add("my-header-2", "val2")
      .add("my-header-2", "val3")
      .build();

Moreover, when a request do not match, we will now print the expected headers instead of the numbers of headers.

Before:
Expected Request [GET /myrequest: with 2 headers and no charset], but was Request [GET /myrequest: with 0 headers and no charset]

After:
Expected Request [GET /myrequest: with my-header=[val], my-header-2=[val2,val3] headers and no charset], but was Request [GET /myrequest: with no headers and no charset]

velo
velo requested changes May 17, 2018
View reviewed changes
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

I started reviewing it, but the format changes make it hard to me to see the full scope of the changes, could you please run mvn clean install to reformat this PR?

mock/src/main/java/feign/mock/RequestKey.java Outdated
@@ -42,7 +41,7 @@ private Builder(HttpMethod method, String url) {
this.url = url;
}

public Builder headers(Map<String, Collection<String>> headers) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please keep the original headers with @Deprecated?

mock/src/test/java/feign/mock/MockClientSequentialTest.java Outdated

@RequestLine("PATCH /repos/{owner}/{repo}/contributors")
List<Contributor> patchContributors(@Param("owner") String owner, @Param("repo") String repo);

@RequestLine("POST /repos/{owner}/{repo}/contributors")
@Body("%7B\"login\":\"{login}\",\"type\":\"{type}\"%7D")
Contributor create(@Param("owner") String owner,
@Param("repo") String repo,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you changed the formatting for many classes.... could you please rollback?

running mvn clean install should do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, of course. I'll plug the eclipse formatter plugin to my IDE.

velo
velo approved these changes May 17, 2018
View reviewed changes
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

LGTM, I just would like to see this remaining format and java 6 changes reverted.

Ping me when ready to merge

mock/src/test/java/feign/mock/RequestKeyTest.java Outdated
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.Assert.assertThat;
import java.nio.charset.StandardCharsets;
Copy link
Member

Choose a reason for hiding this comment

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

Why not?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i'll undo. I've made this to launch test from IntelliJ directly, because test and source don't get the same language level.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, master is now java 8 =)

mock/src/main/java/feign/mock/MockClient.java Outdated
@@ -238,7 +236,8 @@ public Request verifyOne(HttpMethod method, String url) {

List<Request> result = requests.get(requestKey);
if (result.size() != times) {
throw new VerificationAssertionError("Wanted: '%s' to be invoked: '%s' times but got: '%s'!",
throw new VerificationAssertionError(
"Wanted: '%s' to be invoked: '%s' times but got: '%s'!",
Copy link
Member

Choose a reason for hiding this comment

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

I still see a bunch of No-Op changes...

Would be nice to reduce the number of thoose

mock/src/test/java/feign/mock/MockClientTest.java Outdated
private MockClient mockClient;

@Before
public void setup() throws IOException {
try (InputStream input = getClass().getResourceAsStream("/fixtures/contributors.json")) {
InputStream input = getClass().getResourceAsStream("/fixtures/contributors.json");
Copy link
Member

Choose a reason for hiding this comment

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

no need to undo it.

@pboutes
Copy link
Contributor Author

pboutes commented May 17, 2018

@velo Sounds good to you ?

@velo velo merged commit fb825f2 into OpenFeign:master May 17, 2018
@velo
Copy link
Member

velo commented May 17, 2018

Perfect!

@kdavisk6 kdavisk6 added this to the 10.0 milestone Aug 10, 2018
velo pushed a commit that referenced this pull request Oct 7, 2024
@pboutes @velo
Mock: manage headers using RequestHeaders (#706)
c008ac8 
* feat(feign-mock): add RequestHeaders class to manage headers

* feat(feign-mock): use google code style formatting

* feat(feign-mock): remove system.out

* feat(RequestKey): add deprecated headers builder + format code

* feat(feign-mock): format pom correctly

* feat(feign-mock): format pom correctly

* fix(feign-mock): undo some typo and no-op change
velo pushed a commit that referenced this pull request Oct 8, 2024
@pboutes @velo
Mock: manage headers using RequestHeaders (#706)
b0db2dd 
* feat(feign-mock): add RequestHeaders class to manage headers

* feat(feign-mock): use google code style formatting

* feat(feign-mock): remove system.out

* feat(RequestKey): add deprecated headers builder + format code

* feat(feign-mock): format pom correctly

* feat(feign-mock): format pom correctly

* fix(feign-mock): undo some typo and no-op change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@velo velo velo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@pboutes @velo @kdavisk6