nacx commented on a change in pull request #82:
URL: https://github.com/apache/jclouds/pull/82#discussion_r490171464
##########
File path: apis/sts/src/test/java/org/jclouds/aws/util/AWSUtilsTest.java
##########
@@ -88,6 +95,20 @@ public void testNoExceptionParsingTextPlain() {
assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(),
response));
}
+ /**
+ * Do not attempt to parse empty payload.
+ */
+ @Test
+ public void testNoExceptionEmptyPayload() {
+ utils.logger = mock(Logger.class);
+ utils.logger.warn(anyObject(Throwable.class), anyString());
+ expectLastCall().andThrow(new IllegalStateException("log spam"));
+ replay(utils.logger);
+ HttpResponse response =
HttpResponse.builder().statusCode(NOT_FOUND.getStatusCode()).payload(new
StringPayload("")).build();
+
response.getPayload().getContentMetadata().setContentType("application/unknown");
+ assertNull(utils.parseAWSErrorFromContent(command.getCurrentRequest(),
response));
+ }
Review comment:
So, I think the right way to approach this is a bit different. You don't
wanna check that the code does not reach the `catch` branch. You wanna verify
that it does not try to even parse the response payload. Having a try/catch
there is anecdotical; the logic you wanna test is that if the payload is
present but empty, the code doesn't attempt to parse it.
For that, looking at the code you could for example mock the factory that is
used to get the apply function, or even provide a mock apply function and fail
if it is called. I think this approach would much better test the behaviour we
want to fix here. WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
