You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by ro...@apache.org on 2019/06/18 14:50:06 UTC

[sling-whiteboard] branch feature/test-improvements created (now 6d639af)

This is an automated email from the ASF dual-hosted git repository.

rombert pushed a change to branch feature/test-improvements
in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git.


      at 6d639af  Add a set of positive tests

This branch includes the following new commits:

     new bf02cb0  Minor test cleanups
     new 50cf6f2  Add support for setting timeouts using the client API in HttpClientLauncher
     new ad73ce2  Add tests for client-api enforced timeouts
     new d601361  Improve logging in HttpClientLauncher
     new 8736107  Move classes and methods out of AgentIT to make it simpler to understand
     new 6d639af  Add a set of positive tests

The 6 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.



[sling-whiteboard] 01/06: Minor test cleanups

Posted by ro...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

rombert pushed a commit to branch feature/test-improvements
in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git

commit bf02cb0866f4f86c2680f6d2c263e287bc3c20c9
Author: Robert Munteanu <ro...@apache.org>
AuthorDate: Tue Jun 18 14:13:28 2019 +0200

    Minor test cleanups
---
 .../src/test/java/org/apache/sling/uca/impl/AgentIT.java | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
index f8ea7c7..21be70a 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
@@ -65,6 +65,10 @@ import org.slf4j.LoggerFactory;
 @ExtendWith(MisbehavingServerExtension.class)
 public class AgentIT {
     
+    private static final int EXECUTION_TIMEOUT_SECONDS = 5;
+    private static final int CONNECT_TIMEOUT_SECONDS = 3;
+    private static final int READ_TIMEOUT_SECONDS = 3;
+    
     private static final String EXCEPTION_MARKER = "Exception in thread \"main\" ";
     private static final Path STDERR = Paths.get("target", "stderr.txt");
     private static final Path STDOUT = Paths.get("target", "stdout.txt");
@@ -95,7 +99,7 @@ public class AgentIT {
     public void connectTimeout(ClientType clientType) throws IOException {
 
         ErrorDescriptor ed =  requireNonNull(errorDescriptors.get(clientType), "Unhandled clientType " + clientType);
-        RecordedThrowable error = assertTimeout(ofSeconds(5),  () -> runTest("http://repo1.maven.org:81", clientType));
+        RecordedThrowable error = assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),  () -> runTest("http://repo1.maven.org:81", clientType));
         
         assertEquals(ed.connectTimeoutClass.getName(), error.className);
         assertTrue(error.message.matches(ed.connectTimeoutMessageRegex), 
@@ -112,7 +116,7 @@ public class AgentIT {
     public void readTimeout(ClientType clientType, MisbehavingServerControl server) throws IOException {
         
         ErrorDescriptor ed =  requireNonNull(errorDescriptors.get(clientType), "Unhandled clientType " + clientType);
-        RecordedThrowable error = assertTimeout(ofSeconds(5),  () -> runTest("http://localhost:" + server.getLocalPort(), clientType));
+        RecordedThrowable error = assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),  () -> runTest("http://localhost:" + server.getLocalPort(), clientType));
         
         assertEquals(SocketTimeoutException.class.getName(), error.className);
         assertEquals(ed.readTimeoutMessage, error.message);
@@ -120,8 +124,8 @@ public class AgentIT {
 
     private RecordedThrowable runTest(String urlSpec, ClientType clientType) throws IOException, InterruptedException {
 
-        Process process = runForkedCommandWithAgent(new URL(urlSpec), 3, 3, clientType);
-        boolean done = process.waitFor(30, TimeUnit.SECONDS);
+        Process process = runForkedCommandWithAgent(new URL(urlSpec), CONNECT_TIMEOUT_SECONDS, READ_TIMEOUT_SECONDS, clientType);
+        boolean done = process.waitFor(EXECUTION_TIMEOUT_SECONDS, TimeUnit.SECONDS);
         
         LOG.info("Dump of stdout: ");
         Files
@@ -135,7 +139,7 @@ public class AgentIT {
 
         if ( !done ) {
             process.destroy();
-            throw new IllegalStateException("Terminated process since it did not exit in a reasonable amount of time.");
+            throw new IllegalStateException("Terminated process since it did not complete within " + EXECUTION_TIMEOUT_SECONDS + " seconds");
         }
         int exitCode = process.exitValue();
         LOG.info("Exited with code {}", exitCode);
@@ -147,7 +151,7 @@ public class AgentIT {
                 .filter( l -> l.startsWith(EXCEPTION_MARKER))
                 .map( l -> newRecordedThrowable(l) )
                 .findFirst()
-                .orElseThrow(() -> new RuntimeException("Exit code was zero but did not find any exception information in stderr.txt"));
+                .orElseThrow(() -> new RuntimeException("Exit code was not zero ( " + exitCode + " ) but did not find any exception information in stderr.txt"));
         }
     }
     


[sling-whiteboard] 06/06: Add a set of positive tests

Posted by ro...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

rombert pushed a commit to branch feature/test-improvements
in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git

commit 6d639af5e0c98a66c84b2eb95f3a3a2911ec0528
Author: Robert Munteanu <ro...@apache.org>
AuthorDate: Tue Jun 18 16:48:00 2019 +0200

    Add a set of positive tests
    
    Validate that requests go through as expected if the server-side delay is small.
---
 .../java/org/apache/sling/uca/impl/AgentIT.java    | 30 ++++++++++++++---
 .../sling/uca/impl/MisbehavingServerControl.java   | 14 ++++++++
 .../sling/uca/impl/MisbehavingServerExtension.java | 39 ++++++++++++----------
 3 files changed, 61 insertions(+), 22 deletions(-)

diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
index 17638d7..d67a67d 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
@@ -44,6 +44,7 @@ import org.apache.sling.uca.impl.HttpClientLauncher.ClientType;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.EnumSource;
 import org.junit.jupiter.params.provider.MethodSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -127,7 +128,7 @@ public class AgentIT {
 
         ErrorDescriptor ed =  requireNonNull(errorDescriptors.get(clientType), "Unhandled clientType " + clientType);
         RecordedThrowable error = assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),  
-            () -> runTest("http://repo1.maven.org:81", clientType, timeouts));
+            () -> runTest("http://repo1.maven.org:81", clientType, timeouts, false));
         
         assertEquals(ed.connectTimeoutClass.getName(), error.className);
         assertTrue(error.message.matches(ed.connectTimeoutMessageRegex), 
@@ -138,20 +139,32 @@ public class AgentIT {
      * Validates that connecting to a host that delays the response fails with a read timeout
      * 
      * @throws IOException various I/O problems
+     * @throws InterruptedException 
      */
     @ParameterizedTest
     @MethodSource("argumentsMatrix")
-    public void readTimeout(ClientType clientType, TestTimeouts timeouts, MisbehavingServerControl server) throws IOException {
+    public void readTimeout(ClientType clientType, TestTimeouts timeouts, MisbehavingServerControl server) throws IOException, InterruptedException {
         
         ErrorDescriptor ed =  requireNonNull(errorDescriptors.get(clientType), "Unhandled clientType " + clientType);
         RecordedThrowable error = assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),
-           () -> runTest("http://localhost:" + server.getLocalPort(), clientType, timeouts));
-        
+           () -> runTest("http://localhost:" + server.getLocalPort(), clientType, timeouts, false));
+
         assertEquals(SocketTimeoutException.class.getName(), error.className);
         assertEquals(ed.readTimeoutMessage, error.message);
     }
+    
+    @ParameterizedTest
+    @EnumSource(HttpClientLauncher.ClientType.class)
+    public void connectAndReadSuccess(ClientType clientType, MisbehavingServerControl server) throws IOException, InterruptedException {
+        
+        // set a small accept delay for the server so the requests have time to complete
+        server.setHandleDelay(Duration.ofMillis(100));
+        
+        assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),
+                () ->runTest("http://localhost:" + server.getLocalPort(), clientType, TestTimeouts.DEFAULT, true));
+    }
 
-    private RecordedThrowable runTest(String urlSpec, ClientType clientType, TestTimeouts timeouts) throws IOException, InterruptedException {
+    private RecordedThrowable runTest(String urlSpec, ClientType clientType, TestTimeouts timeouts, boolean expectSuccess) throws IOException, InterruptedException {
 
         Process process = new AgentLauncher(new URL(urlSpec), timeouts, clientType, STDOUT, STDERR).launch();
         boolean done = process.waitFor(timeouts.executionTimeout.toMillis(), TimeUnit.MILLISECONDS);
@@ -173,6 +186,13 @@ public class AgentIT {
         int exitCode = process.exitValue();
         LOG.info("Exited with code {}", exitCode);
         
+        if ( expectSuccess ) {
+            if ( exitCode != 0 )
+                throw new RuntimeException("Expected success, but command exited with code " + exitCode);
+            
+            return null;
+        }
+        
         if ( exitCode == 0 ) {
             throw new RuntimeException("Command terminated successfully. That is unexpected.");
         } else {
diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/MisbehavingServerControl.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/MisbehavingServerControl.java
index 9b0bf93..0161006 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/MisbehavingServerControl.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/MisbehavingServerControl.java
@@ -16,6 +16,8 @@
  */
 package org.apache.sling.uca.impl;
 
+import java.time.Duration;
+
 /**
  * Allows control of a local server
  *
@@ -28,4 +30,16 @@ public interface MisbehavingServerControl {
      * @return the port
      */
     int getLocalPort();
+
+    /**
+     * Sets a new value for the handleDelay parameter
+     * 
+     * <p>This value reflects how long the HTTP handler will wait before handling the client request.</p>
+     * 
+     * <p>The value only takes effect for the current test method invocation and will be reset
+     * for the next one.</p>
+     * 
+     * @param handleDelay the new duration
+     */
+    void setHandleDelay(Duration handleDelay);
 }
diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/MisbehavingServerExtension.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/MisbehavingServerExtension.java
index b659451..f5bfe1f 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/MisbehavingServerExtension.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/MisbehavingServerExtension.java
@@ -17,13 +17,12 @@
 package org.apache.sling.uca.impl;
 
 import java.io.IOException;
-import java.util.concurrent.TimeUnit;
+import java.time.Duration;
 
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-import org.eclipse.jetty.server.Connector;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
@@ -46,6 +45,8 @@ import org.slf4j.LoggerFactory;
  */
 class MisbehavingServerExtension implements BeforeEachCallback, AfterEachCallback, ParameterResolver, MisbehavingServerControl {
     
+    private static final Duration DEFAULT_HANDLE_DELAY = Duration.ofSeconds(10);
+    
     private final Logger logger = LoggerFactory.getLogger(getClass());
     
     public int getLocalPort() {
@@ -54,6 +55,8 @@ class MisbehavingServerExtension implements BeforeEachCallback, AfterEachCallbac
     
     private Server server;
     
+    private Duration handleDelay = DEFAULT_HANDLE_DELAY;
+    
     @Override
     public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext)
             throws ParameterResolutionException {
@@ -72,31 +75,28 @@ class MisbehavingServerExtension implements BeforeEachCallback, AfterEachCallbac
     @Override
     public void beforeEach(ExtensionContext context) throws Exception {
         
-        server = new Server();
-        ServerConnector connector = new ServerConnector(server) {
+        // reset the delay before each execution to make test logic simpler 
+        handleDelay = DEFAULT_HANDLE_DELAY;
+        
+        server = new Server(0);
+        server.setHandler(new AbstractHandler() {
+            
             @Override
-            public void accept(int acceptorID) throws IOException {
-                LOG.info("Waiting before accepting");
+            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response)
+                    throws IOException, ServletException {
+                logger.info("Waiting for " + handleDelay + " before handling");
                 try {
-                    Thread.sleep(TimeUnit.SECONDS.toMillis(10));
+                    Thread.sleep(handleDelay.toMillis());
                 } catch (InterruptedException e) {
                     Thread.currentThread().interrupt();
                     return;
                 }
-                super.accept(acceptorID);
-                LOG.info("Accepted");
-            }
-        };
-        server.setConnectors(new Connector[] { connector });
-        server.setHandler(new AbstractHandler() {
-            
-            @Override
-            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response)
-                    throws IOException, ServletException {
+
                 response.setStatus(HttpServletResponse.SC_NO_CONTENT);
                 if ( baseRequest.getHeader("User-Agent") != null )
                     response.addHeader("Original-User-Agent", baseRequest.getHeader("User-Agent"));
                 baseRequest.setHandled(true);
+                logger.info("Handled");
             }
         });
         
@@ -113,4 +113,9 @@ class MisbehavingServerExtension implements BeforeEachCallback, AfterEachCallbac
             logger.info("Failed shutting down server", e);
         }
     }
+    
+    @Override
+    public void setHandleDelay(Duration handleDelay) {
+        this.handleDelay = handleDelay;
+    }
 }
\ No newline at end of file


[sling-whiteboard] 05/06: Move classes and methods out of AgentIT to make it simpler to understand

Posted by ro...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

rombert pushed a commit to branch feature/test-improvements
in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git

commit 8736107e9f9b4bd03d6757955154adc244d1ecd4
Author: Robert Munteanu <ro...@apache.org>
AuthorDate: Tue Jun 18 16:06:57 2019 +0200

    Move classes and methods out of AgentIT to make it simpler to understand
---
 .../java/org/apache/sling/uca/impl/AgentIT.java    | 156 +--------------------
 .../org/apache/sling/uca/impl/AgentLauncher.java   | 107 ++++++++++++++
 .../org/apache/sling/uca/impl/ErrorDescriptor.java |  37 +++++
 .../apache/sling/uca/impl/HttpClientLauncher.java  |   2 +-
 .../apache/sling/uca/impl/RecordedThrowable.java   |  40 ++++++
 .../org/apache/sling/uca/impl/TestTimeouts.java    |  64 +++++++++
 6 files changed, 255 insertions(+), 151 deletions(-)

diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
index f42dfb4..17638d7 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
@@ -26,9 +26,7 @@ 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 java.io.File;
 import java.io.IOException;
-import java.lang.ProcessBuilder.Redirect;
 import java.net.SocketTimeoutException;
 import java.net.URL;
 import java.nio.file.Files;
@@ -36,13 +34,9 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.time.Duration;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.EnumMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import org.apache.commons.httpclient.ConnectTimeoutException;
@@ -67,11 +61,11 @@ import org.slf4j.LoggerFactory;
 @ExtendWith(MisbehavingServerExtension.class)
 public class AgentIT {
     
-    private static final int EXECUTION_TIMEOUT_SECONDS = 5;
-    private static final int CONNECT_TIMEOUT_SECONDS = 3;
-    private static final int READ_TIMEOUT_SECONDS = 3;
+    static final int EXECUTION_TIMEOUT_SECONDS = 5;
+    static final int CONNECT_TIMEOUT_SECONDS = 3;
+    static final int READ_TIMEOUT_SECONDS = 3;
     
-    private static final String EXCEPTION_MARKER = "Exception in thread \"main\" ";
+    static final String EXCEPTION_MARKER = "Exception in thread \"main\" ";
     private static final Path STDERR = Paths.get("target", "stderr.txt");
     private static final Path STDOUT = Paths.get("target", "stdout.txt");
     private static final Logger LOG = LoggerFactory.getLogger(AgentIT.class);
@@ -159,7 +153,7 @@ public class AgentIT {
 
     private RecordedThrowable runTest(String urlSpec, ClientType clientType, TestTimeouts timeouts) throws IOException, InterruptedException {
 
-        Process process = runForkedCommandWithAgent(new URL(urlSpec), timeouts, clientType);
+        Process process = new AgentLauncher(new URL(urlSpec), timeouts, clientType, STDOUT, STDERR).launch();
         boolean done = process.waitFor(timeouts.executionTimeout.toMillis(), TimeUnit.MILLISECONDS);
         
         LOG.info("Dump of stdout: ");
@@ -184,147 +178,9 @@ public class AgentIT {
         } else {
             return Files.lines(STDERR)
                 .filter( l -> l.startsWith(EXCEPTION_MARKER))
-                .map( l -> newRecordedThrowable(l) )
+                .map( RecordedThrowable::fromLine )
                 .findFirst()
                 .orElseThrow(() -> new RuntimeException("Exit code was not zero ( " + exitCode + " ) but did not find any exception information in stderr.txt"));
         }
     }
-    
-    private Process runForkedCommandWithAgent(URL url, TestTimeouts timeouts, ClientType clientType) throws IOException {
-        
-        Path jar = Files.list(Paths.get("target"))
-            .filter( p -> p.getFileName().toString().endsWith("-jar-with-dependencies.jar"))
-            .findFirst()
-            .orElseThrow( () -> new IllegalStateException("Did not find the agent jar. Did you run mvn package first?"));
-        
-        String classPath = buildClassPath();
-
-        String javaHome = System.getProperty("java.home");
-        Path javaExe = Paths.get(javaHome, "bin", "java");
-        ProcessBuilder pb = new ProcessBuilder(
-            javaExe.toString(),
-            "-showversion",
-            "-javaagent:" + jar +"=" + timeouts.agentConnectTimeout.toMillis() +"," + timeouts.agentReadTimeout.toMillis()+",v",
-            "-cp",
-            classPath,
-            HttpClientLauncher.class.getName(),
-            url.toString(),
-            clientType.toString(),
-            String.valueOf(timeouts.clientConnectTimeout.toMillis()),
-            String.valueOf(timeouts.clientReadTimeout.toMillis())
-        );
-        
-        pb.redirectInput(Redirect.INHERIT);
-        pb.redirectOutput(STDOUT.toFile());
-        pb.redirectError(STDERR.toFile());
-        
-        return pb.start();
-    }
-    
-    private String buildClassPath() throws IOException {
-        
-        List<String> elements = new ArrayList<>();
-        elements.add(Paths.get("target", "test-classes").toString());
-        
-        Set<String> dependencies = new HashSet<>(Arrays.asList(new String[] {
-            "commons-httpclient.jar",
-            "commons-codec.jar",
-            "slf4j-simple.jar",
-            "slf4j-api.jar",
-            "jcl-over-slf4j.jar",
-            "httpclient.jar",
-            "httpcore.jar",
-            "okhttp.jar",
-            "okio.jar"
-        }));
-        
-        Files.list(Paths.get("target", "it-dependencies"))
-            .filter( p -> dependencies.contains(p.getFileName().toString()) )
-            .forEach( p -> elements.add(p.toString()));
-        
-        return String.join(File.pathSeparator, elements);
-    }
-
-    private RecordedThrowable newRecordedThrowable(String line) {
-        
-        line = line.replace(EXCEPTION_MARKER, "");
-
-        String className = line.substring(0, line.indexOf(':'));
-        String message = line.substring(line.indexOf(':') + 2); // ignore ':' and leading ' '
-
-        return new RecordedThrowable(className, message);
-    }
-    
-    /**
-     * Data class for defining specific error messages related to individual {@link ClientType client types}. 
-     */
-    static class ErrorDescriptor {
-        private Class<? extends IOException> connectTimeoutClass;
-        private String connectTimeoutMessageRegex;
-        private String readTimeoutMessage;
-
-        public ErrorDescriptor(Class<? extends IOException> connectTimeoutClass, String connectTimeoutMessageRegex,
-                String readTimeoutMessage) {
-            this.connectTimeoutClass = connectTimeoutClass;
-            this.connectTimeoutMessageRegex = connectTimeoutMessageRegex;
-            this.readTimeoutMessage = readTimeoutMessage;
-        }
-    }
-    
-    /**
-     * Basic information about a {@link Throwable} that was recorded in a file
-     */
-    static class RecordedThrowable {
-        String className;
-        String message;
-
-        public RecordedThrowable(String className, String message) {
-            this.className = className;
-            this.message = message;
-        }
-    }
-    
-    /**
-     * Data class for holding information about various timeouts set in the tests
-     */
-    static class TestTimeouts {
-        
-        private Duration executionTimeout = Duration.ofSeconds(EXECUTION_TIMEOUT_SECONDS);
-        private Duration agentConnectTimeout = Duration.ofSeconds(CONNECT_TIMEOUT_SECONDS);
-        private Duration agentReadTimeout = Duration.ofSeconds(READ_TIMEOUT_SECONDS);
-        private Duration clientConnectTimeout = Duration.ZERO;
-        private Duration clientReadTimeout = Duration.ZERO;
-        
-        public static TestTimeouts DEFAULT = new TestTimeouts();
-        
-        static class Builder {
-            private TestTimeouts timeouts = new TestTimeouts();
-            
-            public Builder executionTimeout(Duration duration) {
-                timeouts.executionTimeout = Objects.requireNonNull(duration);
-                return this;
-            }
-
-            public Builder agentTimeouts(Duration connectTimeout, Duration readTimeout) {
-                timeouts.agentConnectTimeout = Objects.requireNonNull(connectTimeout);
-                timeouts.agentReadTimeout = Objects.requireNonNull(readTimeout);
-                return this;
-            }
-            
-            public Builder clientTimeouts(Duration connectTimeout, Duration readTimeout) {
-                timeouts.clientConnectTimeout = Objects.requireNonNull(connectTimeout);
-                timeouts.clientReadTimeout = Objects.requireNonNull(readTimeout);
-                return this;
-            }
-            
-            public TestTimeouts build() {
-                return timeouts;
-            }
-        }
-        
-        @Override
-        public String toString() {
-            return getClass().getSimpleName() + ": execution " + executionTimeout + ", agent: " + agentConnectTimeout + "/" + agentReadTimeout + ", client : " + clientConnectTimeout + "/" + clientReadTimeout;
-        }
-    }
 }
diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentLauncher.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentLauncher.java
new file mode 100644
index 0000000..93a006b
--- /dev/null
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentLauncher.java
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.sling.uca.impl;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.ProcessBuilder.Redirect;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.sling.uca.impl.HttpClientLauncher.ClientType;
+
+/**
+ * Launches the {@link HttpClientLauncher} as a separate process with the timeout agent enabled
+ *
+ */
+class AgentLauncher {
+    private final URL url;
+    private final TestTimeouts timeouts;
+    private final ClientType clientType;
+    private Path stdout;
+    private Path stderr;
+
+    public AgentLauncher(URL url, TestTimeouts timeouts, ClientType clientType, Path stdout, Path stderr) {
+        this.url = url;
+        this.timeouts = timeouts;
+        this.clientType = clientType;
+        this.stdout = stdout;
+        this.stderr = stderr;
+    }
+    
+    public Process launch() throws IOException {
+        
+        Path jar = Files.list(Paths.get("target"))
+            .filter( p -> p.getFileName().toString().endsWith("-jar-with-dependencies.jar"))
+            .findFirst()
+            .orElseThrow( () -> new IllegalStateException("Did not find the agent jar. Did you run mvn package first?"));
+        
+        String classPath = buildClassPath();
+
+        String javaHome = System.getProperty("java.home");
+        Path javaExe = Paths.get(javaHome, "bin", "java");
+        ProcessBuilder pb = new ProcessBuilder(
+            javaExe.toString(),
+            "-showversion",
+            "-javaagent:" + jar +"=" + timeouts.agentConnectTimeout.toMillis() +"," + timeouts.agentReadTimeout.toMillis()+",v",
+            "-cp",
+            classPath,
+            HttpClientLauncher.class.getName(),
+            url.toString(),
+            clientType.toString(),
+            String.valueOf(timeouts.clientConnectTimeout.toMillis()),
+            String.valueOf(timeouts.clientReadTimeout.toMillis())
+        );
+        
+        pb.redirectInput(Redirect.INHERIT);
+        pb.redirectOutput(stdout.toFile());
+        pb.redirectError(stderr.toFile());
+        
+        return pb.start();
+    }
+    
+    private String buildClassPath() throws IOException {
+        
+        List<String> elements = new ArrayList<>();
+        elements.add(Paths.get("target", "test-classes").toString());
+        
+        Set<String> dependencies = new HashSet<>(Arrays.asList(new String[] {
+            "commons-httpclient.jar",
+            "commons-codec.jar",
+            "slf4j-simple.jar",
+            "slf4j-api.jar",
+            "jcl-over-slf4j.jar",
+            "httpclient.jar",
+            "httpcore.jar",
+            "okhttp.jar",
+            "okio.jar"
+        }));
+        
+        Files.list(Paths.get("target", "it-dependencies"))
+            .filter( p -> dependencies.contains(p.getFileName().toString()) )
+            .forEach( p -> elements.add(p.toString()));
+        
+        return String.join(File.pathSeparator, elements);
+    }
+}
\ No newline at end of file
diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/ErrorDescriptor.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/ErrorDescriptor.java
new file mode 100644
index 0000000..840ea05
--- /dev/null
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/ErrorDescriptor.java
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.sling.uca.impl;
+
+import java.io.IOException;
+
+import org.apache.sling.uca.impl.HttpClientLauncher.ClientType;
+
+/**
+ * Data class for defining specific error messages related to individual {@link ClientType client types}. 
+ */
+class ErrorDescriptor {
+    Class<? extends IOException> connectTimeoutClass;
+    String connectTimeoutMessageRegex;
+    String readTimeoutMessage;
+
+    public ErrorDescriptor(Class<? extends IOException> connectTimeoutClass, String connectTimeoutMessageRegex,
+            String readTimeoutMessage) {
+        this.connectTimeoutClass = connectTimeoutClass;
+        this.connectTimeoutMessageRegex = connectTimeoutMessageRegex;
+        this.readTimeoutMessage = readTimeoutMessage;
+    }
+}
\ No newline at end of file
diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
index f24b5b5..d2408de 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
@@ -82,7 +82,7 @@ public class HttpClientLauncher {
     }
     
     /**
-     * A <tt>Consumer</tt> that allows throwing checked exceptions.</p>
+     * Thin wrapper for various http client abstractions
      *
      */
     @FunctionalInterface
diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/RecordedThrowable.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/RecordedThrowable.java
new file mode 100644
index 0000000..63ba806
--- /dev/null
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/RecordedThrowable.java
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.sling.uca.impl;
+
+/**
+ * Basic information about a {@link Throwable} that was recorded in a file
+ */
+class RecordedThrowable {
+    
+    static RecordedThrowable fromLine(String line) {
+        line = line.replace(AgentIT.EXCEPTION_MARKER, "");
+
+        String className = line.substring(0, line.indexOf(':'));
+        String message = line.substring(line.indexOf(':') + 2); // ignore ':' and leading ' '
+
+        return new RecordedThrowable(className, message);
+    }
+    
+    String className;
+    String message;
+
+    public RecordedThrowable(String className, String message) {
+        this.className = className;
+        this.message = message;
+    }
+}
\ No newline at end of file
diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/TestTimeouts.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/TestTimeouts.java
new file mode 100644
index 0000000..0d5cb36
--- /dev/null
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/TestTimeouts.java
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.sling.uca.impl;
+
+import java.time.Duration;
+import java.util.Objects;
+
+/**
+ * Data class for holding information about various timeouts set in the tests
+ */
+class TestTimeouts {
+
+    Duration executionTimeout = Duration.ofSeconds(AgentIT.EXECUTION_TIMEOUT_SECONDS);
+    Duration agentConnectTimeout = Duration.ofSeconds(AgentIT.CONNECT_TIMEOUT_SECONDS);
+    Duration agentReadTimeout = Duration.ofSeconds(AgentIT.READ_TIMEOUT_SECONDS);
+    Duration clientConnectTimeout = Duration.ZERO;
+    Duration clientReadTimeout = Duration.ZERO;
+    
+    public static TestTimeouts DEFAULT = new TestTimeouts();
+    
+    static class Builder {
+        private TestTimeouts timeouts = new TestTimeouts();
+        
+        public TestTimeouts.Builder executionTimeout(Duration duration) {
+            timeouts.executionTimeout = Objects.requireNonNull(duration);
+            return this;
+        }
+
+        public TestTimeouts.Builder agentTimeouts(Duration connectTimeout, Duration readTimeout) {
+            timeouts.agentConnectTimeout = Objects.requireNonNull(connectTimeout);
+            timeouts.agentReadTimeout = Objects.requireNonNull(readTimeout);
+            return this;
+        }
+        
+        public TestTimeouts.Builder clientTimeouts(Duration connectTimeout, Duration readTimeout) {
+            timeouts.clientConnectTimeout = Objects.requireNonNull(connectTimeout);
+            timeouts.clientReadTimeout = Objects.requireNonNull(readTimeout);
+            return this;
+        }
+        
+        public TestTimeouts build() {
+            return timeouts;
+        }
+    }
+    
+    @Override
+    public String toString() {
+        return getClass().getSimpleName() + ": execution " + executionTimeout + ", agent: " + agentConnectTimeout + "/" + agentReadTimeout + ", client : " + clientConnectTimeout + "/" + clientReadTimeout;
+    }
+}
\ No newline at end of file


[sling-whiteboard] 03/06: Add tests for client-api enforced timeouts

Posted by ro...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

rombert pushed a commit to branch feature/test-improvements
in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git

commit ad73ce22eea7f67ea478fad09d0c5bc6b5a44cef
Author: Robert Munteanu <ro...@apache.org>
AuthorDate: Tue Jun 18 15:46:41 2019 +0200

    Add tests for client-api enforced timeouts
    
    This validates that the agent-set timeouts only kick in when no explicit timeouts are set from the client api.
---
 .../java/org/apache/sling/uca/impl/AgentIT.java    | 113 ++++++++++++++++++---
 1 file changed, 97 insertions(+), 16 deletions(-)

diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
index 21be70a..f42dfb4 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/AgentIT.java
@@ -34,12 +34,14 @@ import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.EnumMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
@@ -47,7 +49,8 @@ import org.apache.commons.httpclient.ConnectTimeoutException;
 import org.apache.sling.uca.impl.HttpClientLauncher.ClientType;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -60,7 +63,6 @@ import org.slf4j.LoggerFactory;
  * 
  * <p>It does so by reusing the same JVM as the one running the test. Validation is done by looking for a
  * Throwable information in the stderr and recording the exception class name and the message.</p>
- *
  */
 @ExtendWith(MisbehavingServerExtension.class)
 public class AgentIT {
@@ -82,6 +84,37 @@ public class AgentIT {
                 "Connect to repo1.maven.org:81 \\[.*\\] failed: connect timed out", "Read timed out"));
         errorDescriptors.put(OkHttp, new ErrorDescriptor(SocketTimeoutException.class, "connect timed out", "timeout"));
     }
+
+    /**
+     * Creates a matrix of all arguments to use for the read and connect timeout tests
+     * 
+     * <p>This matrix uses all client types and pairs them with each of the timeouts, for now:
+     * 
+     * <ol>
+     *   <li>default timeouts, which mean the agent-set default timeout will kick in</li>
+     *   <li>lower client API timeouts, which mean that the client-enforced timeouts will be applied</li>
+     * </ol>
+     * 
+     * </p>
+     * 
+     * @return a list of arguments to use for the tests
+     */
+    static List<Arguments> argumentsMatrix() {
+        
+        List<Arguments> args = new ArrayList<Arguments>();
+        
+        TestTimeouts clientLower = new TestTimeouts.Builder()
+            .agentTimeouts(Duration.ofMinutes(1), Duration.ofMinutes(1))
+            .clientTimeouts(Duration.ofSeconds(CONNECT_TIMEOUT_SECONDS), Duration.ofSeconds(READ_TIMEOUT_SECONDS))
+            .build();
+    
+        for ( ClientType client : ClientType.values() )
+            for ( TestTimeouts timeout : new TestTimeouts[] { TestTimeouts.DEFAULT, clientLower } )
+                args.add(Arguments.of(client, timeout));
+        
+        return args;
+    }
+
     
     /**
      * Validates that connecting to a unaccessible port on an existing port fails with a connect 
@@ -95,11 +128,12 @@ public class AgentIT {
      * @throws IOException various I/O problems 
      */
     @ParameterizedTest
-    @EnumSource(HttpClientLauncher.ClientType.class)
-    public void connectTimeout(ClientType clientType) throws IOException {
+    @MethodSource("argumentsMatrix")
+    public void connectTimeout(ClientType clientType, TestTimeouts timeouts) throws IOException {
 
         ErrorDescriptor ed =  requireNonNull(errorDescriptors.get(clientType), "Unhandled clientType " + clientType);
-        RecordedThrowable error = assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),  () -> runTest("http://repo1.maven.org:81", clientType));
+        RecordedThrowable error = assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),  
+            () -> runTest("http://repo1.maven.org:81", clientType, timeouts));
         
         assertEquals(ed.connectTimeoutClass.getName(), error.className);
         assertTrue(error.message.matches(ed.connectTimeoutMessageRegex), 
@@ -112,20 +146,21 @@ public class AgentIT {
      * @throws IOException various I/O problems
      */
     @ParameterizedTest
-    @EnumSource(HttpClientLauncher.ClientType.class)
-    public void readTimeout(ClientType clientType, MisbehavingServerControl server) throws IOException {
+    @MethodSource("argumentsMatrix")
+    public void readTimeout(ClientType clientType, TestTimeouts timeouts, MisbehavingServerControl server) throws IOException {
         
         ErrorDescriptor ed =  requireNonNull(errorDescriptors.get(clientType), "Unhandled clientType " + clientType);
-        RecordedThrowable error = assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),  () -> runTest("http://localhost:" + server.getLocalPort(), clientType));
+        RecordedThrowable error = assertTimeout(ofSeconds(EXECUTION_TIMEOUT_SECONDS),
+           () -> runTest("http://localhost:" + server.getLocalPort(), clientType, timeouts));
         
         assertEquals(SocketTimeoutException.class.getName(), error.className);
         assertEquals(ed.readTimeoutMessage, error.message);
     }
 
-    private RecordedThrowable runTest(String urlSpec, ClientType clientType) throws IOException, InterruptedException {
+    private RecordedThrowable runTest(String urlSpec, ClientType clientType, TestTimeouts timeouts) throws IOException, InterruptedException {
 
-        Process process = runForkedCommandWithAgent(new URL(urlSpec), CONNECT_TIMEOUT_SECONDS, READ_TIMEOUT_SECONDS, clientType);
-        boolean done = process.waitFor(EXECUTION_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+        Process process = runForkedCommandWithAgent(new URL(urlSpec), timeouts, clientType);
+        boolean done = process.waitFor(timeouts.executionTimeout.toMillis(), TimeUnit.MILLISECONDS);
         
         LOG.info("Dump of stdout: ");
         Files
@@ -139,7 +174,7 @@ public class AgentIT {
 
         if ( !done ) {
             process.destroy();
-            throw new IllegalStateException("Terminated process since it did not complete within " + EXECUTION_TIMEOUT_SECONDS + " seconds");
+            throw new IllegalStateException("Terminated process since it did not complete within " + timeouts.executionTimeout.toMillis() + " milliseconds");
         }
         int exitCode = process.exitValue();
         LOG.info("Exited with code {}", exitCode);
@@ -155,7 +190,7 @@ public class AgentIT {
         }
     }
     
-    private Process runForkedCommandWithAgent(URL url, int connectTimeoutSeconds, int readTimeoutSeconds, ClientType clientType) throws IOException {
+    private Process runForkedCommandWithAgent(URL url, TestTimeouts timeouts, ClientType clientType) throws IOException {
         
         Path jar = Files.list(Paths.get("target"))
             .filter( p -> p.getFileName().toString().endsWith("-jar-with-dependencies.jar"))
@@ -169,12 +204,14 @@ public class AgentIT {
         ProcessBuilder pb = new ProcessBuilder(
             javaExe.toString(),
             "-showversion",
-            "-javaagent:" + jar +"=" + TimeUnit.SECONDS.toMillis(connectTimeoutSeconds) +"," + TimeUnit.SECONDS.toMillis(readTimeoutSeconds)+",v",
+            "-javaagent:" + jar +"=" + timeouts.agentConnectTimeout.toMillis() +"," + timeouts.agentReadTimeout.toMillis()+",v",
             "-cp",
             classPath,
             HttpClientLauncher.class.getName(),
             url.toString(),
-            clientType.toString()
+            clientType.toString(),
+            String.valueOf(timeouts.clientConnectTimeout.toMillis()),
+            String.valueOf(timeouts.clientReadTimeout.toMillis())
         );
         
         pb.redirectInput(Redirect.INHERIT);
@@ -217,7 +254,7 @@ public class AgentIT {
 
         return new RecordedThrowable(className, message);
     }
-
+    
     /**
      * Data class for defining specific error messages related to individual {@link ClientType client types}. 
      */
@@ -246,4 +283,48 @@ public class AgentIT {
             this.message = message;
         }
     }
+    
+    /**
+     * Data class for holding information about various timeouts set in the tests
+     */
+    static class TestTimeouts {
+        
+        private Duration executionTimeout = Duration.ofSeconds(EXECUTION_TIMEOUT_SECONDS);
+        private Duration agentConnectTimeout = Duration.ofSeconds(CONNECT_TIMEOUT_SECONDS);
+        private Duration agentReadTimeout = Duration.ofSeconds(READ_TIMEOUT_SECONDS);
+        private Duration clientConnectTimeout = Duration.ZERO;
+        private Duration clientReadTimeout = Duration.ZERO;
+        
+        public static TestTimeouts DEFAULT = new TestTimeouts();
+        
+        static class Builder {
+            private TestTimeouts timeouts = new TestTimeouts();
+            
+            public Builder executionTimeout(Duration duration) {
+                timeouts.executionTimeout = Objects.requireNonNull(duration);
+                return this;
+            }
+
+            public Builder agentTimeouts(Duration connectTimeout, Duration readTimeout) {
+                timeouts.agentConnectTimeout = Objects.requireNonNull(connectTimeout);
+                timeouts.agentReadTimeout = Objects.requireNonNull(readTimeout);
+                return this;
+            }
+            
+            public Builder clientTimeouts(Duration connectTimeout, Duration readTimeout) {
+                timeouts.clientConnectTimeout = Objects.requireNonNull(connectTimeout);
+                timeouts.clientReadTimeout = Objects.requireNonNull(readTimeout);
+                return this;
+            }
+            
+            public TestTimeouts build() {
+                return timeouts;
+            }
+        }
+        
+        @Override
+        public String toString() {
+            return getClass().getSimpleName() + ": execution " + executionTimeout + ", agent: " + agentConnectTimeout + "/" + agentReadTimeout + ", client : " + clientConnectTimeout + "/" + clientReadTimeout;
+        }
+    }
 }


[sling-whiteboard] 02/06: Add support for setting timeouts using the client API in HttpClientLauncher

Posted by ro...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

rombert pushed a commit to branch feature/test-improvements
in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git

commit 50cf6f2399d3eddb981763c9626b13e30c51de19
Author: Robert Munteanu <ro...@apache.org>
AuthorDate: Tue Jun 18 14:45:43 2019 +0200

    Add support for setting timeouts using the client API in HttpClientLauncher
---
 url-connection-agent/README.md                     | 12 ++++--
 .../apache/sling/uca/impl/HttpClientLauncher.java  | 50 ++++++++++++++++++----
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/url-connection-agent/README.md b/url-connection-agent/README.md
index 96995aa..d743d88 100644
--- a/url-connection-agent/README.md
+++ b/url-connection-agent/README.md
@@ -15,16 +15,20 @@ It currently supports setting timeouts for HTTP connections done using:
 
 ## Validation
 
-Build the project with `mvn clean package` and then run a simple connection test with 
+In addition to running the integration tests, you can also build the project with `mvn clean package` and then run a simple connection test with 
 
-    java -javaagent:target/org.apache.sling.connection-timeout-agent-0.0.1-SNAPSHOT-jar-with-dependencies.jar=<connect-timeout>,<read-timeout> -cp target/test-classes:target/it-dependencies/* org.apache.sling.uca.impl.HttpClientLauncher <url> <client-type>
+    java -javaagent:target/org.apache.sling.connection-timeout-agent-0.0.1-SNAPSHOT-jar-with-dependencies.jar=<agent-connect-timeout>,<agent-read-timeout> -cp target/test-classes:target/it-dependencies/* org.apache.sling.uca.impl.HttpClientLauncher <url> <client-type> [<client-connect-timeout> <client-read-timeout>]
     
  The parameters are as follows:
  
- - `<connect-timeout>` - connection timeout in milliseconds
- - `<read-timeout>`- read timeout in milliseconds
+ - `<agent-connect-timeout>` - connection timeout in milliseconds to apply via the agent
+ - `<agent-read-timeout>`- read timeout in milliseconds to apply via the agent
  - `<url>` - the URL to access
  - `<client-type>` - the client type, either `JavaNet` for java.net.URL-based connections ,`HC3` for Apache Commons HttpClient 3.x, `HC4` for Apache Commons HttpClient 4.x or `OkHttp` for OK HTTP.
+ - `<client-connect-timeout>` (optional) - the connection timeout in milliseconds to apply via client APIs
+ - `<client-read-timeout>` (optional) - the read timeout in milliseconds to apply via client APIs
+ 
+The read and connect timeouts may be specified for both the agent and client APIs. The reason is that the agent should not change the timeout defaults if they are already set. Therefore, setting the agent timeouts to a very high value and the client API timeouts to a very low value ( e.g. 1 millisecond ) should still result in a timeout. 
  
  
  For a test that always fails, set one of the timeouts to 1. Both executions listed below will typically fail:
diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
index df1de9e..06953e3 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
@@ -22,6 +22,7 @@ import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.net.HttpURLConnection;
 import java.net.URL;
+import java.time.Duration;
 import java.util.Date;
 import java.util.EnumSet;
 import java.util.stream.Collectors;
@@ -32,7 +33,10 @@ import org.apache.commons.httpclient.HttpClient;
 import org.apache.commons.httpclient.HttpMethod;
 import org.apache.commons.httpclient.methods.GetMethod;
 import org.apache.commons.httpclient.params.HttpClientParams;
+import org.apache.commons.httpclient.params.HttpConnectionParams;
 import org.apache.commons.httpclient.params.HttpMethodParams;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.config.RequestConfig.Builder;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.impl.client.CloseableHttpClient;
@@ -84,12 +88,12 @@ public class HttpClientLauncher {
      */
     @FunctionalInterface
     interface HttpConsumer {
-        void accept(String http) throws Exception;
+        void accept(String http, int connectTimeoutSeconds, int readTimeoutSeconds) throws Exception;
     }
 
     public static void main(String[] args) throws Exception {
         
-        if ( args.length != 2 )
+        if ( args.length < 2 )
             throw new IllegalArgumentException(usage());
         
         ClientType type = ClientType.fromString(args[1]);
@@ -98,17 +102,23 @@ public class HttpClientLauncher {
         
         System.out.println("[WEB] Executing request via " + type);
         
-        type.consumer.accept(args[0]);
+        int connectTimeout = args.length > 2 ? Integer.parseInt(args[2]) : 0;
+        int readTimeout = args.length > 3 ? Integer.parseInt(args[3]) : 0;
+        
+        type.consumer.accept(args[0], connectTimeout, readTimeout);
     }
 
     private static String usage() {
         return "Usage: java -cp ... " + HttpClientLauncher.class.getName() + " <URL> " + ClientType.pipeSeparatedString();
     }
 
-    private static void runUsingJavaNet(String targetUrl) throws IOException  {
+    private static void runUsingJavaNet(String targetUrl, int connectTimeoutMillis, int readTimeoutMillis) throws IOException  {
         HttpURLConnection con = (HttpURLConnection) new URL(targetUrl).openConnection();
         System.out.println("Connection type is " + con);
         
+        con.setConnectTimeout(connectTimeoutMillis);
+        con.setReadTimeout(readTimeoutMillis);
+        
         try (InputStream in = con.getInputStream();
                 InputStreamReader isr = new InputStreamReader(in);
                 BufferedReader br = new BufferedReader(isr)) {
@@ -122,10 +132,16 @@ public class HttpClientLauncher {
     }
 
 
-    private static void runUsingHttpClient3(String targetUrl) throws IOException {
+    private static void runUsingHttpClient3(String targetUrl, int connectTimeoutMillis, int readTimeoutMillis) throws IOException {
         HttpClient client = new HttpClient();
         // disable retries, to make sure that we get equivalent behaviour with other implementations
         client.getParams().setParameter(HttpMethodParams.RETRY_HANDLER, new DefaultHttpMethodRetryHandler(0, false));
+        
+        if ( connectTimeoutMillis != 0 )
+            client.getParams().setParameter(HttpConnectionParams.CONNECTION_TIMEOUT, Integer.valueOf(connectTimeoutMillis));
+        if ( readTimeoutMillis != 0 )
+            client.getParams().setParameter(HttpMethodParams.SO_TIMEOUT, Integer.valueOf(readTimeoutMillis));
+        
         HttpMethod get = new GetMethod(targetUrl);
         System.out.format("Connection timeouts: connect: %d, so: %s%n", 
                 client.getHttpConnectionManager().getParams().getConnectionTimeout(),
@@ -140,9 +156,19 @@ public class HttpClientLauncher {
             System.out.print(new Date() + " [WEB] " + header.toExternalForm());
     }
     
-    private static void runUsingHttpClient4(String targetUrl) throws IOException {
+    private static void runUsingHttpClient4(String targetUrl, int connectTimeoutMillis, int readTimeoutMillis) throws IOException {
         // disable retries, to make sure that we get equivalent behaviour with other implementations
-        try ( CloseableHttpClient client = HttpClients.custom().disableAutomaticRetries().build() ) {
+        
+        Builder config = RequestConfig.custom();
+        if ( connectTimeoutMillis != 0 )
+            config.setConnectTimeout(connectTimeoutMillis);
+        if ( readTimeoutMillis != 0 )
+            config.setSocketTimeout(readTimeoutMillis);
+        
+        try ( CloseableHttpClient client = HttpClients.custom()
+                .setDefaultRequestConfig(config.build())
+                .disableAutomaticRetries().build() ) {
+            
             HttpGet get = new HttpGet(targetUrl);
             try ( CloseableHttpResponse response = client.execute(get)) {
                 System.out.println("[WEB] " + response.getStatusLine());
@@ -154,8 +180,14 @@ public class HttpClientLauncher {
         }
     }
 
-    private static void runUsingOkHttp(String targetUrl) throws IOException {
-        OkHttpClient client = new OkHttpClient();
+    private static void runUsingOkHttp(String targetUrl, int connectTimeoutSeconds, int readTimeoutSeconds) throws IOException {
+        OkHttpClient.Builder clientBuilder = new OkHttpClient().newBuilder();
+        if ( connectTimeoutSeconds != 0 )
+            clientBuilder.connectTimeout(Duration.ofMillis(connectTimeoutSeconds));
+        if ( readTimeoutSeconds != 0 )
+            clientBuilder.readTimeout(Duration.ofMillis(readTimeoutSeconds));
+        
+        OkHttpClient client = clientBuilder.build();
         
         Request request = new Request.Builder()
             .url(targetUrl)


[sling-whiteboard] 04/06: Improve logging in HttpClientLauncher

Posted by ro...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

rombert pushed a commit to branch feature/test-improvements
in repository https://gitbox.apache.org/repos/asf/sling-whiteboard.git

commit d601361137fc9a202ecbba543170e5df0d597fe2
Author: Robert Munteanu <ro...@apache.org>
AuthorDate: Tue Jun 18 15:55:00 2019 +0200

    Improve logging in HttpClientLauncher
---
 .../apache/sling/uca/impl/HttpClientLauncher.java  | 31 +++++++++++++---------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
index 06953e3..f24b5b5 100644
--- a/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
+++ b/url-connection-agent/src/test/java/org/apache/sling/uca/impl/HttpClientLauncher.java
@@ -23,7 +23,6 @@ import java.io.InputStreamReader;
 import java.net.HttpURLConnection;
 import java.net.URL;
 import java.time.Duration;
-import java.util.Date;
 import java.util.EnumSet;
 import java.util.stream.Collectors;
 
@@ -100,21 +99,27 @@ public class HttpClientLauncher {
         if ( type == null )
             throw new IllegalArgumentException(usage());
         
-        System.out.println("[WEB] Executing request via " + type);
+        log("Executing request via " + type);
         
         int connectTimeout = args.length > 2 ? Integer.parseInt(args[2]) : 0;
         int readTimeout = args.length > 3 ? Integer.parseInt(args[3]) : 0;
         
+        log("Client API configured timeouts: " + connectTimeout + "/" + readTimeout);
+        
         type.consumer.accept(args[0], connectTimeout, readTimeout);
     }
 
     private static String usage() {
         return "Usage: java -cp ... " + HttpClientLauncher.class.getName() + " <URL> " + ClientType.pipeSeparatedString();
     }
+    
+    private static void log(String msg, Object... args) {
+        System.out.format("[LAUNCHER] " + msg + "%n", args);
+    }
 
     private static void runUsingJavaNet(String targetUrl, int connectTimeoutMillis, int readTimeoutMillis) throws IOException  {
         HttpURLConnection con = (HttpURLConnection) new URL(targetUrl).openConnection();
-        System.out.println("Connection type is " + con);
+        log("Connection type is %s", con);
         
         con.setConnectTimeout(connectTimeoutMillis);
         con.setReadTimeout(readTimeoutMillis);
@@ -123,10 +128,10 @@ public class HttpClientLauncher {
                 InputStreamReader isr = new InputStreamReader(in);
                 BufferedReader br = new BufferedReader(isr)) {
             
-            System.out.println("[WEB] "  + con.getResponseCode() + " " + con.getResponseMessage());
+            log(con.getResponseCode() + " " + con.getResponseMessage());
 
             con.getHeaderFields().forEach( (k, v) -> {
-                System.out.println("[WEB] " + k + " : " + v);
+                log(k + " : " + v);
             });
         }
     }
@@ -143,17 +148,17 @@ public class HttpClientLauncher {
             client.getParams().setParameter(HttpMethodParams.SO_TIMEOUT, Integer.valueOf(readTimeoutMillis));
         
         HttpMethod get = new GetMethod(targetUrl);
-        System.out.format("Connection timeouts: connect: %d, so: %s%n", 
+        log("Connection timeouts: connect: %d, so: %s", 
                 client.getHttpConnectionManager().getParams().getConnectionTimeout(),
                 client.getHttpConnectionManager().getParams().getSoTimeout());
-        System.out.format("Client so timeout: %d (raw: %s) %n", client.getParams().getSoTimeout(), 
+        log("Client so timeout: %d (raw: %s)", client.getParams().getSoTimeout(), 
                 client.getParams().getParameter(HttpClientParams.SO_TIMEOUT));
         client.executeMethod(get);
         
-        System.out.println(new Date() + " [WEB] " + get.getStatusLine());
+        log(get.getStatusLine().toString());
         
         for ( Header header : get.getResponseHeaders() )
-            System.out.print(new Date() + " [WEB] " + header.toExternalForm());
+            log(header.toExternalForm());
     }
     
     private static void runUsingHttpClient4(String targetUrl, int connectTimeoutMillis, int readTimeoutMillis) throws IOException {
@@ -171,9 +176,9 @@ public class HttpClientLauncher {
             
             HttpGet get = new HttpGet(targetUrl);
             try ( CloseableHttpResponse response = client.execute(get)) {
-                System.out.println("[WEB] " + response.getStatusLine());
+                log(response.getStatusLine().toString());
                 for ( org.apache.http.Header header : response.getAllHeaders() )
-                    System.out.println("[WEB] " + header);
+                    log(header.toString());
                 
                 EntityUtils.consume(response.getEntity());
             }
@@ -194,9 +199,9 @@ public class HttpClientLauncher {
             .build();
 
         try (Response response = client.newCall(request).execute()) {
-            System.out.println("[WEB] " + response.code() + " " + response.message());
+            log("%s %s", response.code(), response.message());
             response.headers().toMultimap().forEach( (n, v) -> {
-                System.out.println("[WEB] " + n + ": " + v);
+                log("%s : %s", n, v);
             });
         }
     }