rombert commented on code in PR #14:
URL:
https://github.com/apache/sling-org-apache-sling-connection-timeout-agent/pull/14#discussion_r2374862802
##########
src/test/java/org/apache/sling/cta/impl/AgentIT.java:
##########
@@ -16,19 +16,23 @@
*/
package org.apache.sling.cta.impl;
-import static java.time.Duration.ofSeconds;
-import static java.util.Objects.requireNonNull;
-import static org.apache.sling.cta.impl.HttpClientLauncher.ClientType.HC3;
-import static org.apache.sling.cta.impl.HttpClientLauncher.ClientType.HC4;
-import static org.apache.sling.cta.impl.HttpClientLauncher.ClientType.JavaNet;
-import static org.apache.sling.cta.impl.HttpClientLauncher.ClientType.OkHttp;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTimeout;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import org.apache.commons.httpclient.ConnectTimeoutException;
Review Comment:
Please don't reorganise imports, it creates unneeded changes. Applies for
the whole class.
##########
pom.xml:
##########
@@ -188,6 +188,5 @@
</dependencies>
<properties>
<pax-exam.version>4.14.0</pax-exam.version>
- <sling.java.version>8</sling.java.version>
Review Comment:
Please set the default to 11 instead of removing it. This will become
required in the future anyway.
##########
src/test/java/org/apache/sling/cta/impl/HttpClientLauncher.java:
##########
@@ -42,20 +35,31 @@
import org.apache.http.impl.client.HttpClients;
import org.apache.http.util.EntityUtils;
-import okhttp3.OkHttpClient;
-import okhttp3.Request;
-import okhttp3.Response;
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.net.HttpURLConnection;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URL;
+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+import java.time.Duration;
+import java.util.EnumSet;
+import java.util.stream.Collectors;
/**
* CLI interface to run HTTP clients
*/
public class HttpClientLauncher {
public enum ClientType {
- JavaNet(HttpClientLauncher::runUsingJavaNet),
Review Comment:
Please avoid whitespace-only changes, they create extra history entries that
are not needed.
##########
src/test/java/org/apache/sling/cta/impl/HttpClientLauncher.java:
##########
@@ -16,16 +16,9 @@
*/
package org.apache.sling.cta.impl;
-import java.io.BufferedReader;
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.InputStreamReader;
-import java.net.HttpURLConnection;
-import java.net.URL;
-import java.time.Duration;
-import java.util.EnumSet;
-import java.util.stream.Collectors;
-
+import okhttp3.OkHttpClient;
Review Comment:
Please don't reorganise imports, it creates unneeded changes.
##########
src/test/java/org/apache/sling/cta/impl/OsgiIT.java:
##########
@@ -78,7 +78,7 @@ public Option[] config() throws IOException {
mavenBundle("org.apache.httpcomponents", "httpcore-osgi",
"4.4.12"),
mavenBundle("org.apache.httpcomponents", "httpclient-osgi",
"4.5.10"),
mavenBundle("org.apache.felix",
"org.apache.felix.http.servlet-api", "3.0.0"),
-
mavenBundle("org.apache.felix","org.apache.felix.http.jetty","5.1.26"),
+
mavenBundle("org.apache.felix","org.apache.felix.http.jetty","4.2.32"),
Review Comment:
Why is this change needed?
##########
src/test/java/org/apache/sling/cta/impl/AgentIT.java:
##########
@@ -64,7 +61,7 @@
@ExtendWith(MisbehavingServerExtension.class)
public class AgentIT {
- static final int EXECUTION_TIMEOUT_SECONDS = 7;
+ static final int EXECUTION_TIMEOUT_SECONDS = 10;
Review Comment:
Why is this change needed?
##########
src/test/java/org/apache/sling/cta/impl/AgentIT.java:
##########
@@ -151,13 +149,13 @@ public void readTimeout(ClientType clientType,
TestTimeouts timeouts, Misbehavin
RecordedThrowable error =
assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),
() -> runTest("http://127.0.0.1:"; + server.getLocalPort(),
clientType, timeouts, false));
- assertEquals(SocketTimeoutException.class.getName(), error.className);
+ assertEquals(ed.readTimeoutClass.getName(), error.className);
assertTrue(error.message.matches(ed.readTimeoutRegex),
"Actual message " + error.message + " did not match regex " +
ed.readTimeoutRegex);
}
@ParameterizedTest
- @EnumSource(HttpClientLauncher.ClientType.class)
+ @EnumSource(ClientType.class)
Review Comment:
Please avoid making formatting/import changes, it makes changes harder to
track in history.
##########
src/test/java/org/apache/sling/cta/impl/ErrorDescriptor.java:
##########
@@ -26,12 +26,14 @@
class ErrorDescriptor {
Class<? extends IOException> connectTimeoutClass;
String connectTimeoutMessageRegex;
+ Class<? extends IOException> readTimeoutClass;
String readTimeoutRegex;
public ErrorDescriptor(Class<? extends IOException> connectTimeoutClass,
String connectTimeoutMessageRegex,
- String readTimeoutRegex) {
Review Comment:
To minimise changes to existing code, please keep the old constructor and
make it invoke the new constructor with readTimeoutClass set to
SocketTimeoutException.class
##########
src/main/java/org/apache/sling/cta/impl/Agent.java:
##########
@@ -16,17 +16,16 @@
*/
package org.apache.sling.cta.impl;
+import javax.management.InstanceAlreadyExistsException;
Review Comment:
Please don't reorganise imports, it creates unneeded changes.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
