You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/02/16 18:16:27 UTC
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6568: Various changes to restore integration tests in 'verify' target
rawlinp commented on a change in pull request #6568:
URL: https://github.com/apache/trafficcontrol/pull/6568#discussion_r808311456
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/util/Fetcher.java
##########
@@ -233,4 +239,23 @@ public void createStringBuilderFromResponse (final StringBuilder sb, final HttpU
}
}
}
+
+ private void connectionFailed(final HttpURLConnection http, final String url) {
+ String httpUrl = url;
+ String responseCode = "(none)";
+ try {
+ httpUrl = http.getURL().toString();
+ responseCode = String.valueOf(http.getResponseCode());
+ } catch (Exception e2) {
+ // Don't care
+ LOGGER.info("Exception during call attempt to retrieve url or responseCode from http");
Review comment:
Should this be `error` instead of `info`? Also, should we log the exception?
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/util/Fetcher.java
##########
@@ -233,4 +239,23 @@ public void createStringBuilderFromResponse (final StringBuilder sb, final HttpU
}
}
}
+
+ private void connectionFailed(final HttpURLConnection http, final String url) {
+ String httpUrl = url;
+ String responseCode = "(none)";
+ try {
+ httpUrl = http.getURL().toString();
+ responseCode = String.valueOf(http.getResponseCode());
+ } catch (Exception e2) {
+ // Don't care
+ LOGGER.info("Exception during call attempt to retrieve url or responseCode from http");
+ }
+
+ LOGGER.error("Failed Http Request to " + httpUrl + responseCode);
Review comment:
Can we change this to `LOGGER.error("Failed HTTP request to " + httpUrl + " status code: " + responseCode);`?
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/router/TrafficRouterTest.java
##########
@@ -126,6 +127,9 @@ public void itCreatesHttpResults() throws Exception {
HTTPRequest httpRequest = new HTTPRequest();
httpRequest.setClientIP("192.168.10.11");
httpRequest.setHostname("ccr.example.com");
+ Map<String, String> headers = new HashMap<String, String>();
+ headers.put("x-tc-steering-option", "itCreatesHttpResults");
+ httpRequest.setHeaders(headers);
Review comment:
indentation is off
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/router/TrafficRouterTest.java
##########
@@ -73,6 +73,7 @@ public void before() throws Exception {
when(deliveryService.isCoverageZoneOnly()).thenReturn(false);
when(deliveryService.getDispersion()).thenReturn(mock(Dispersion.class));
when(deliveryService.isAcceptHttp()).thenReturn(true);
+ when(deliveryService.getId()).thenReturn("someDsName");
Review comment:
indentation is off
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/util/FetcherTest.java
##########
@@ -62,4 +69,44 @@ public void itChecksIfDataHasChangedSinceLastFetch() throws Exception {
fetcher.fetchIfModifiedSince("http://www.example.com", 123456L);
verify(httpURLConnection).setIfModifiedSince(123456L);
}
-}
\ No newline at end of file
+
+ @Test
+ public void itChecksIfSocketTimeoutExceptionThrown() throws Exception {
+ // Test that an IOException (ex connection-failure) is caught, re-thrown, and
+ // that the embedded error is correctly encapsulated in the exception
+ //
+ final String mockedException = "Mocked Connection Failure";
+ URL url = mock(URL.class);
+ HttpURLConnection httpURLConnection = mock(HttpURLConnection.class);
+
+ stub(method(URL.class, "openConnection")).toReturn(httpURLConnection);
+ doThrow(new SocketTimeoutException(mockedException)).when(httpURLConnection).connect();
Review comment:
indentation is off
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/util/FetcherTest.java
##########
@@ -62,4 +69,44 @@ public void itChecksIfDataHasChangedSinceLastFetch() throws Exception {
fetcher.fetchIfModifiedSince("http://www.example.com", 123456L);
verify(httpURLConnection).setIfModifiedSince(123456L);
}
-}
\ No newline at end of file
+
+ @Test
+ public void itChecksIfSocketTimeoutExceptionThrown() throws Exception {
+ // Test that an IOException (ex connection-failure) is caught, re-thrown, and
+ // that the embedded error is correctly encapsulated in the exception
+ //
+ final String mockedException = "Mocked Connection Failure";
+ URL url = mock(URL.class);
+ HttpURLConnection httpURLConnection = mock(HttpURLConnection.class);
+
+ stub(method(URL.class, "openConnection")).toReturn(httpURLConnection);
+ doThrow(new SocketTimeoutException(mockedException)).when(httpURLConnection).connect();
+ whenNew(URL.class).withArguments("http://www.example.com").thenReturn(url);
+
+ Fetcher fetcher = new Fetcher();
+ try {
+ fetcher.fetchIfModifiedSince("http://www.example.com", 123456L);
+ assertTrue(false);
+ } catch (SocketTimeoutException e) {
+ assertEquals(e.toString(), "java.net.SocketTimeoutException: " + mockedException);
+ }
+ }
+
+ @Test
+ public void itChecksIfOtherThrown() throws Exception {
+ // Test that other exceptions (ex CastClassException) is caught and
+ // squelched. This matches existing functionality.
+ //
+ // This test relies on the CastClassException which is thrown when "connection"
+ // is cast to type "HttpURLConnection". This will abort execution of
+ // Fetcher::getConnection() during the cast of "connection" to "http". This will
+ // result in the callstack unwinding normally with null return codes.
+ //
+ URLConnection urlConnection = mock(URLConnection.class);
+ stub(method(URL.class, "openConnection")).toReturn(urlConnection);
+
+ URL url = mock(URL.class);
+ Fetcher fetcher = new Fetcher();
+ assertEquals(fetcher.fetchIfModifiedSince("http://www.example.com", 123456L), null);
Review comment:
indentation is off
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/router/TrafficRouterTest.java
##########
@@ -163,6 +167,8 @@ public void itFiltersByIPAvailability() throws Exception {
DeliveryService ds = mock(DeliveryService.class);
+ when(ds.getId()).thenReturn("itFiltersByIpAvailable");
Review comment:
indentation is off
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/util/FetcherTest.java
##########
@@ -62,4 +69,44 @@ public void itChecksIfDataHasChangedSinceLastFetch() throws Exception {
fetcher.fetchIfModifiedSince("http://www.example.com", 123456L);
verify(httpURLConnection).setIfModifiedSince(123456L);
}
-}
\ No newline at end of file
+
+ @Test
+ public void itChecksIfSocketTimeoutExceptionThrown() throws Exception {
+ // Test that an IOException (ex connection-failure) is caught, re-thrown, and
+ // that the embedded error is correctly encapsulated in the exception
+ //
+ final String mockedException = "Mocked Connection Failure";
+ URL url = mock(URL.class);
+ HttpURLConnection httpURLConnection = mock(HttpURLConnection.class);
+
+ stub(method(URL.class, "openConnection")).toReturn(httpURLConnection);
+ doThrow(new SocketTimeoutException(mockedException)).when(httpURLConnection).connect();
+ whenNew(URL.class).withArguments("http://www.example.com").thenReturn(url);
+
+ Fetcher fetcher = new Fetcher();
+ try {
+ fetcher.fetchIfModifiedSince("http://www.example.com", 123456L);
+ assertTrue(false);
+ } catch (SocketTimeoutException e) {
+ assertEquals(e.toString(), "java.net.SocketTimeoutException: " + mockedException);
+ }
Review comment:
indentation is off
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/router/TrafficRouterTest.java
##########
@@ -280,6 +286,9 @@ public void itSetsResultToGeo() throws Exception {
httpRequest.setClientIP("192.168.10.11");
httpRequest.setHostname("ccr.example.com");
httpRequest.setPath("/some/path");
+ Map<String, String> headers = new HashMap<String, String>();
+ headers.put("x-tc-steering-option", "itSetsResultToGeo");
+ httpRequest.setHeaders(headers);
Review comment:
indentation is off
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/util/FetcherTest.java
##########
@@ -62,4 +69,44 @@ public void itChecksIfDataHasChangedSinceLastFetch() throws Exception {
fetcher.fetchIfModifiedSince("http://www.example.com", 123456L);
verify(httpURLConnection).setIfModifiedSince(123456L);
}
-}
\ No newline at end of file
+
+ @Test
+ public void itChecksIfSocketTimeoutExceptionThrown() throws Exception {
+ // Test that an IOException (ex connection-failure) is caught, re-thrown, and
+ // that the embedded error is correctly encapsulated in the exception
+ //
+ final String mockedException = "Mocked Connection Failure";
Review comment:
indentation is off
##########
File path: traffic_router/core/src/test/java/org/apache/traffic_control/traffic_router/core/util/FetcherTest.java
##########
@@ -62,4 +69,44 @@ public void itChecksIfDataHasChangedSinceLastFetch() throws Exception {
fetcher.fetchIfModifiedSince("http://www.example.com", 123456L);
verify(httpURLConnection).setIfModifiedSince(123456L);
}
-}
\ No newline at end of file
+
+ @Test
+ public void itChecksIfSocketTimeoutExceptionThrown() throws Exception {
+ // Test that an IOException (ex connection-failure) is caught, re-thrown, and
+ // that the embedded error is correctly encapsulated in the exception
+ //
+ final String mockedException = "Mocked Connection Failure";
+ URL url = mock(URL.class);
+ HttpURLConnection httpURLConnection = mock(HttpURLConnection.class);
+
+ stub(method(URL.class, "openConnection")).toReturn(httpURLConnection);
+ doThrow(new SocketTimeoutException(mockedException)).when(httpURLConnection).connect();
+ whenNew(URL.class).withArguments("http://www.example.com").thenReturn(url);
+
+ Fetcher fetcher = new Fetcher();
+ try {
+ fetcher.fetchIfModifiedSince("http://www.example.com", 123456L);
+ assertTrue(false);
+ } catch (SocketTimeoutException e) {
+ assertEquals(e.toString(), "java.net.SocketTimeoutException: " + mockedException);
+ }
+ }
+
+ @Test
+ public void itChecksIfOtherThrown() throws Exception {
+ // Test that other exceptions (ex CastClassException) is caught and
+ // squelched. This matches existing functionality.
+ //
+ // This test relies on the CastClassException which is thrown when "connection"
+ // is cast to type "HttpURLConnection". This will abort execution of
+ // Fetcher::getConnection() during the cast of "connection" to "http". This will
+ // result in the callstack unwinding normally with null return codes.
+ //
Review comment:
indentation is off
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org