Fixes AwsHttpHeadersTestCase #187
Conversation
There was a problem hiding this comment.
@SherifWaly It looks very nice. I have only
2 comments about the code :)
| public void performsRequest() throws IOException { | ||
| Map<String, String> headers = new HashMap<String, String>(); | ||
| public void addsHeaders() throws IOException { | ||
| Map<String, String> headers = new HashMap<String, String>(); | ||
| headers.put("Content-type", "json"); | ||
| headers.put("HeaderName", "HeaderValue"); | ||
| AwsHttpHeaders<String> awsh = new AwsHttpHeaders<>( | ||
| new AwsHttpRequest.FakeAwsHttpRequest(), headers |
There was a problem hiding this comment.
@SherifWaly This unit test is good, but it misses a case: the one where the original request already has some headers. We should check that AwsHttpHeaders adds, but does not replace.
So, the original request here should have a header already.
You can do it like this:
AwsHttpRequest<String> fake = new AwsHttpRequest.FakeAwsHttpRequest();
fake.request().addHeader("previously-here", "value");
AwsHttpHeaders<String> awsh = new AwsHttpHeaders<>(fake, headers);and modify the asserts: received.size() should be 3, not 2, plus add an assert for the previous header, to see that it's still there.
| new AwsHttpRequest.FakeAwsHttpRequest(), | ||
| new HashMap<String, String>() | ||
| ); | ||
| assertTrue(awsh.perform().equals("performed fake request")); |
There was a problem hiding this comment.
@SherifWaly As you see in this issue, we want to start using hamcrest matchers instead of simple asserts (see there the explanation).
Can you change the asserts to matcher asserts? It's not hard at all and the gain is big. See this test class for examples.
Any questions, please ask.
|
@SherifWaly Can you please modify the PR's title and description? The title should briefly say what the changes are (e.g. Fixes AwsHttpHeadersTestCase) and in the description you should link the issue that the PR is solving (e.g. "This PR is for #177"). This is because we want traceability of changes. It's one of the reasons we make the changes on opened issues and separate branches, but if we don't link them to each other, they will eventually get lost and nobody will know what's with them :D |
|
@amihaiemil Please review the last commit after adding an extra check and using hamcrest matchers for testing :) |
|
@SherifWaly looks good :) |
|
@rultor merge pls |
@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here |
@amihaiemil Done! FYI, the full log is here (took me 1min) |
This PR is for #177