You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ja...@apache.org on 2017/08/23 18:33:36 UTC

geode git commit: GEODE-3184: Cleaned up Cargo tests

Repository: geode
Updated Branches:
  refs/heads/develop fa29ec13f -> a229933ce


GEODE-3184: Cleaned up Cargo tests

This closes #722


Project: http://git-wip-us.apache.org/repos/asf/geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/a229933c
Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/a229933c
Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/a229933c

Branch: refs/heads/develop
Commit: a229933ce4dfa2db462f81fc864a4bb1b78e2d08
Parents: fa29ec1
Author: David Anuta <da...@gmail.com>
Authored: Fri Aug 18 09:41:24 2017 -0700
Committer: Jason Huynh <hu...@gmail.com>
Committed: Wed Aug 23 11:16:05 2017 -0700

----------------------------------------------------------------------
 .../modules/session/catalina/DeltaSession.java  |  46 +++----
 .../geode/session/tests/CargoTestBase.java      | 127 ++++++-------------
 .../org/apache/geode/session/tests/Client.java  |  12 +-
 .../geode/session/tests/ContainerInstall.java   |  37 ++++--
 .../geode/session/tests/ContainerManager.java   |   8 ++
 .../tests/GenericAppServerClientServerTest.java |   8 +-
 .../tests/GenericAppServerContainer.java        |  10 +-
 .../session/tests/GenericAppServerInstall.java  |   4 +-
 .../geode/session/tests/ServerContainer.java    |  13 +-
 .../session/tests/TomcatClientServerTest.java   |  23 ++--
 .../geode/session/tests/TomcatContainer.java    |   4 +-
 .../geode/session/tests/TomcatInstall.java      |   6 +-
 12 files changed, 148 insertions(+), 150 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
----------------------------------------------------------------------
diff --git a/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java b/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
index 27e5bce..4aa894a 100644
--- a/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
+++ b/extensions/geode-modules/src/main/java/org/apache/geode/modules/session/catalina/DeltaSession.java
@@ -14,28 +14,6 @@
  */
 package org.apache.geode.modules.session.catalina;
 
-import org.apache.geode.DataSerializable;
-import org.apache.geode.DataSerializer;
-import org.apache.geode.Delta;
-import org.apache.geode.InvalidDeltaException;
-import org.apache.geode.cache.Region;
-import org.apache.geode.internal.cache.lru.Sizeable;
-import org.apache.geode.internal.util.BlobHelper;
-import org.apache.geode.modules.gatewaydelta.GatewayDelta;
-import org.apache.geode.modules.gatewaydelta.GatewayDeltaEvent;
-import org.apache.geode.modules.session.catalina.internal.DeltaSessionAttributeEvent;
-import org.apache.geode.modules.session.catalina.internal.DeltaSessionAttributeEventBatch;
-import org.apache.geode.modules.session.catalina.internal.DeltaSessionDestroyAttributeEvent;
-import org.apache.geode.modules.session.catalina.internal.DeltaSessionUpdateAttributeEvent;
-import org.apache.catalina.Manager;
-import org.apache.catalina.ha.session.SerializablePrincipal;
-import org.apache.catalina.realm.GenericPrincipal;
-import org.apache.catalina.security.SecurityUtil;
-import org.apache.catalina.session.StandardSession;
-import org.apache.juli.logging.Log;
-import org.apache.juli.logging.LogFactory;
-
-import javax.servlet.http.HttpSession;
 import java.io.DataInput;
 import java.io.DataOutput;
 import java.io.IOException;
@@ -52,6 +30,30 @@ import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
+import javax.servlet.http.HttpSession;
+
+import org.apache.catalina.Manager;
+import org.apache.catalina.ha.session.SerializablePrincipal;
+import org.apache.catalina.realm.GenericPrincipal;
+import org.apache.catalina.security.SecurityUtil;
+import org.apache.catalina.session.StandardSession;
+import org.apache.juli.logging.Log;
+import org.apache.juli.logging.LogFactory;
+
+import org.apache.geode.DataSerializable;
+import org.apache.geode.DataSerializer;
+import org.apache.geode.Delta;
+import org.apache.geode.InvalidDeltaException;
+import org.apache.geode.cache.Region;
+import org.apache.geode.internal.cache.lru.Sizeable;
+import org.apache.geode.internal.util.BlobHelper;
+import org.apache.geode.modules.gatewaydelta.GatewayDelta;
+import org.apache.geode.modules.gatewaydelta.GatewayDeltaEvent;
+import org.apache.geode.modules.session.catalina.internal.DeltaSessionAttributeEvent;
+import org.apache.geode.modules.session.catalina.internal.DeltaSessionAttributeEventBatch;
+import org.apache.geode.modules.session.catalina.internal.DeltaSessionDestroyAttributeEvent;
+import org.apache.geode.modules.session.catalina.internal.DeltaSessionUpdateAttributeEvent;
+
 @SuppressWarnings("serial")
 public class DeltaSession extends StandardSession
     implements DataSerializable, Delta, GatewayDelta, Sizeable, DeltaSessionInterface {

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/CargoTestBase.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/CargoTestBase.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/CargoTestBase.java
index f54141b..92f3490 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/CargoTestBase.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/CargoTestBase.java
@@ -71,6 +71,28 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
   }
 
   /**
+   * Gets the specified key from all the containers within the container manager and check that each
+   * container has the associated expected value
+   */
+  public void getKeyValueDataOnAllClients(String key, String expectedValue, String expectedCookie)
+      throws IOException, URISyntaxException {
+    for (int i = 0; i < manager.numContainers(); i++) {
+      // Set the port for this server
+      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
+      // Get the response to a get on the specified key from this server
+      Client.Response resp = client.get(key);
+
+      // Null would mean we don't expect the same cookie as before
+      if (expectedCookie != null)
+        assertEquals("Sessions are not replicating properly", expectedCookie,
+            resp.getSessionCookie());
+
+      // Check that the response from this server is correct
+      assertEquals("Session data is not replicating properly", expectedValue, resp.getResponse());
+    }
+  }
+
+  /**
    * Test that when multiple containers are using session replication, all of the containers will
    * use the same session cookie for the same client.
    */
@@ -80,14 +102,8 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
     Client.Response resp = client.get(null);
-    String cookie = resp.getSessionCookie();
 
-    for (int i = 1; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(null);
-
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-    }
+    getKeyValueDataOnAllClients(null, "", resp.getSessionCookie());
   }
 
   /**
@@ -103,15 +119,8 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
     Client.Response resp = client.set(key, value);
-    String cookie = resp.getSessionCookie();
-
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
 
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-      assertEquals("Session data is not replicating properly", value, resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, value, resp.getSessionCookie());
   }
 
   /**
@@ -128,17 +137,11 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
     Client.Response resp = client.set(key, value);
-    String cookie = resp.getSessionCookie();
 
     manager.stopContainer(0);
+    manager.removeContainer(0);
 
-    for (int i = 1; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
-
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-      assertEquals("Container failure caused inaccessible data.", value, resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, value, resp.getSessionCookie());
   }
 
   /**
@@ -153,17 +156,11 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
     String value = "Foo";
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
-    Client.Response resp = client.set(key, value);
-    String cookie = resp.getSessionCookie();
+    client.set(key, value);
 
     client.invalidate();
 
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
-
-      assertEquals("Data removal is not being replicated properly.", "", resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, "", null);
   }
 
   /**
@@ -180,27 +177,13 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
     Client.Response resp = client.set(key, value);
-    String cookie = resp.getSessionCookie();
 
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
-
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-      assertEquals(value, resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, value, resp.getSessionCookie());
 
     client.setMaxInactive(1);
-
     Thread.sleep(5000);
 
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
-
-      assertEquals("Session replication is not doing session expiration correctly.", "",
-          resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, "", null);
   }
 
 
@@ -219,7 +202,6 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
     Client.Response resp = client.set(key, value);
-    String cookie = resp.getSessionCookie();
 
     client.setMaxInactive(timeToExp);
 
@@ -232,14 +214,7 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
       curTime = System.currentTimeMillis();
     }
 
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
-
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-      assertEquals("Containers are not replicating session expiration reset", value,
-          resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, value, resp.getSessionCookie());
   }
 
   /**
@@ -254,28 +229,13 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
     Client.Response resp = client.set(key, value);
-    String cookie = resp.getSessionCookie();
 
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
-
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-      assertEquals(value, resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, value, resp.getSessionCookie());
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
     client.remove(key);
 
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
-
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-      assertEquals(
-          "Was expecting an empty response after removal. Double check to make sure that the enableLocalCache cacheProperty is set to false. This test is unreliable on servers which use a local cache.",
-          "", resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, "", resp.getSessionCookie());
   }
 
   /**
@@ -291,29 +251,16 @@ public abstract class CargoTestBase extends JUnit4CacheTestCase {
 
     client.setPort(Integer.parseInt(manager.getContainerPort(0)));
     Client.Response resp = client.set(key, value);
-    String cookie = resp.getSessionCookie();
 
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
+    getKeyValueDataOnAllClients(key, value, resp.getSessionCookie());
 
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-      assertEquals(value, resp.getResponse());
-    }
     int numContainers = manager.numContainers();
-
+    // Add and start new container
     manager.addContainer(getInstall());
     manager.startAllInactiveContainers();
-
+    // Check that a container was added
     assertEquals(numContainers + 1, manager.numContainers());
 
-    for (int i = 0; i < manager.numContainers(); i++) {
-      client.setPort(Integer.parseInt(manager.getContainerPort(i)));
-      resp = client.get(key);
-
-      assertEquals("Sessions are not replicating properly", cookie, resp.getSessionCookie());
-      assertEquals("Containers are not properly sharing data with new arrival", value,
-          resp.getResponse());
-    }
+    getKeyValueDataOnAllClients(key, value, resp.getSessionCookie());
   }
 }

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/Client.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/Client.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/Client.java
index 9b458d0..52968d4 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/Client.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/Client.java
@@ -14,8 +14,9 @@
  */
 package org.apache.geode.session.tests;
 
-import org.apache.geode.modules.session.CommandServlet;
-import org.apache.geode.modules.session.QueryCommand;
+import java.io.IOException;
+import java.net.URISyntaxException;
+
 import org.apache.http.Header;
 import org.apache.http.StatusLine;
 import org.apache.http.client.methods.CloseableHttpResponse;
@@ -30,8 +31,8 @@ import org.apache.http.protocol.BasicHttpContext;
 import org.apache.http.protocol.HttpContext;
 import org.apache.http.util.EntityUtils;
 
-import java.io.IOException;
-import java.net.URISyntaxException;
+import org.apache.geode.modules.session.CommandServlet;
+import org.apache.geode.modules.session.QueryCommand;
 
 /**
  * A simple http client that talks to a server running the session-testing-war.
@@ -207,7 +208,8 @@ public class Client {
 
     StatusLine status = resp.getStatusLine();
     if (status.getStatusCode() != 200) {
-      throw new IOException("Http request failed. " + status);
+      throw new IOException("Http request to " + req.getURI().getHost() + "["
+          + req.getURI().getPort() + "] failed. " + status);
     }
 
     Response response = new Response(reqCookie, EntityUtils.toString(resp.getEntity()), isNew);

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerInstall.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerInstall.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerInstall.java
index f9bce0a..ede100e 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerInstall.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerInstall.java
@@ -385,17 +385,25 @@ public abstract class ContainerInstall {
 
   protected static void editXMLFile(String XMLPath, String tagId, String tagName,
       String parentTagName, HashMap<String, String> attributes) {
-    editXMLFile(XMLPath, tagId, tagName, parentTagName, attributes, false);
+    editXMLFile(XMLPath, tagId, tagName, tagName, parentTagName, attributes, false);
   }
 
   protected static void editXMLFile(String XMLPath, String tagName, String parentTagName,
       HashMap<String, String> attributes) {
-    editXMLFile(XMLPath, null, tagName, parentTagName, attributes, false);
+    editXMLFile(XMLPath, tagName, parentTagName, attributes, false);
   }
 
   protected static void editXMLFile(String XMLPath, String tagName, String parentTagName,
       HashMap<String, String> attributes, boolean writeOnSimilarAttributeNames) {
-    editXMLFile(XMLPath, null, tagName, parentTagName, attributes, writeOnSimilarAttributeNames);
+    editXMLFile(XMLPath, null, tagName, tagName, parentTagName, attributes,
+        writeOnSimilarAttributeNames);
+  }
+
+  protected static void editXMLFile(String XMLPath, String tagName, String replacementTagName,
+      String parentTagName, HashMap<String, String> attributes,
+      boolean writeOnSimilarAttributeNames) {
+    editXMLFile(XMLPath, null, tagName, replacementTagName, parentTagName, attributes,
+        writeOnSimilarAttributeNames);
   }
 
   /**
@@ -410,6 +418,7 @@ public abstract class ContainerInstall {
    * @param tagId The id of tag to edit. If null, then this method will add a new xml element,
    *        unless writeOnSimilarAttributeNames is set to true.
    * @param tagName The name of the xml element to edit
+   * @param replacementTagName The new name of the XML attribute that is being edited
    * @param parentTagName The parent element of the element we should edit
    * @param attributes the xml attributes for the element to edit
    * @param writeOnSimilarAttributeNames If true, find an existing element with the same set of
@@ -418,7 +427,7 @@ public abstract class ContainerInstall {
    *        not null).
    */
   protected static void editXMLFile(String XMLPath, String tagId, String tagName,
-      String parentTagName, HashMap<String, String> attributes,
+      String replacementTagName, String parentTagName, HashMap<String, String> attributes,
       boolean writeOnSimilarAttributeNames) {
 
     try {
@@ -431,23 +440,33 @@ public abstract class ContainerInstall {
       // Get node with specified tagId
       if (tagId != null) {
         node = findNodeWithAttribute(doc, tagName, "id", tagId);
-      } else if (writeOnSimilarAttributeNames) {
+      }
+      // If writing on similar attributes then search by tag name
+      else if (writeOnSimilarAttributeNames) {
+        // Get all the nodes with the given tag name
         NodeList nodes = doc.getElementsByTagName(tagName);
         for (int i = 0; i < nodes.getLength(); i++) {
           Node n = nodes.item(i);
+          // If the node being iterated across has the exact attributes then it is the one that
+          // should be edited
           if (nodeHasExactAttributes(n, attributes, false)) {
             node = n;
             break;
           }
         }
       }
-      // If no node is found
+      // If a node if found
       if (node != null) {
+        doc.renameNode(node, null, replacementTagName);
+        // Rewrite the node attributes
         rewriteNodeAttributes(node, attributes);
+        // Write the tagId so that it can be found easier next time
         if (tagId != null)
           ((Element) node).setAttribute("id", tagId);
-      } else {
-        Element e = doc.createElement(tagName);
+      }
+      // No node found creates new element under the parent tag passed in
+      else {
+        Element e = doc.createElement(replacementTagName);
         // Set id attribute
         if (tagId != null) {
           e.setAttribute("id", tagId);
@@ -485,11 +504,13 @@ public abstract class ContainerInstall {
    */
   private static Node findNodeWithAttribute(Document doc, String nodeName, String name,
       String value) {
+    // Get all nodes with given name
     NodeList nodes = doc.getElementsByTagName(nodeName);
     if (nodes == null) {
       return null;
     }
 
+    // Find and return the first node that has the given attribute
     for (int i = 0; i < nodes.getLength(); i++) {
       Node node = nodes.item(i);
       Node nodeAttr = node.getAttributes().getNamedItem(name);

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerManager.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerManager.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerManager.java
index 500cfa9..036c6a5 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerManager.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/ContainerManager.java
@@ -140,6 +140,7 @@ public class ContainerManager {
   public ArrayList<Integer> getContainerIndexesWithState(String state) {
     ArrayList<Integer> indexes = new ArrayList<>();
     for (int i = 0; i < numContainers(); i++) {
+      // Checks that the state passed in is one of the 5 supported by Cargo
       if (state.equals(State.STARTED.toString()) || state.equals(State.STOPPED.toString())
           || state.equals(State.STARTED.toString()) || state.equals(State.STOPPING.toString())
           || state.equals(State.UNKNOWN.toString())) {
@@ -198,6 +199,13 @@ public class ContainerManager {
   }
 
   /**
+   * Remove the container in the given index from the list
+   */
+  public ServerContainer removeContainer(int index) {
+    return containers.remove(index);
+  }
+
+  /**
    * Get the indexes of all active containers
    */
   private ArrayList<Integer> getActiveContainerIndexes() {

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerClientServerTest.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerClientServerTest.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerClientServerTest.java
index 08f9978..ea4022b 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerClientServerTest.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerClientServerTest.java
@@ -14,11 +14,12 @@
  */
 package org.apache.geode.session.tests;
 
+import org.junit.Before;
+
 import org.apache.geode.cache.Cache;
 import org.apache.geode.cache.server.CacheServer;
 import org.apache.geode.test.dunit.Host;
 import org.apache.geode.test.dunit.VM;
-import org.junit.Before;
 
 /**
  * Extends the {@link CargoTestBase} class to support client server tests of generic app servers
@@ -31,14 +32,17 @@ public abstract class GenericAppServerClientServerTest extends CargoTestBase {
    */
   @Before
   public void startServers() throws InterruptedException {
+    // Setup host
     Host host = Host.getHost(0);
     VM vm0 = host.getVM(0);
+    // Start server in VM
     vm0.invoke(() -> {
       Cache cache = getCache();
+      // Add cache server
       CacheServer server = cache.addCacheServer();
       server.setPort(0);
+      // Start the server in this VM
       server.start();
     });
-    Thread.sleep(5000);
   }
 }

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerContainer.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerContainer.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerContainer.java
index 7a2cfaf..11c76fc 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerContainer.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerContainer.java
@@ -95,18 +95,25 @@ public class GenericAppServerContainer extends ServerContainer {
   private List<String> buildCommand() throws IOException {
     ContainerInstall install = getInstall();
 
+    // Start command list
     List<String> command = new ArrayList<>();
+    // Path to the modify war script to run
     command.add(modifyWarScript.getAbsolutePath());
+    // Path to the WAR file to modify
     command.add("-w");
     command.add(install.getWarFilePath());
+    // Get connection type for the WAR (peer-to-peer or client-server)
     command.add("-t");
     command.add(install.getConnectionType().getName());
+    // Path to the modified version of the origin WAR file
     command.add("-o");
     command.add(getWarFile().getAbsolutePath());
+    // Add all the cache properties setup to the WAR file
     for (String property : cacheProperties.keySet()) {
       command.add("-p");
       command.add("gemfire.cache." + property + "=" + getCacheProperty(property));
     }
+    // Add all the system properties to the WAR file
     for (String property : systemProperties.keySet()) {
       command.add("-p");
       command.add("gemfire.property." + property + "=" + getSystemProperty(property));
@@ -148,8 +155,7 @@ public class GenericAppServerContainer extends ServerContainer {
   }
 
   /**
-   * Update the container's settings by calling by modifying the war file through the
-   * {@link #modifyWarFile()} function
+   * Update the container's settings by calling {@link #modifyWarFile()} method
    */
   @Override
   public void writeSettings() throws Exception {

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerInstall.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerInstall.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerInstall.java
index aead718..4e56887 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerInstall.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/GenericAppServerInstall.java
@@ -14,7 +14,6 @@
  */
 package org.apache.geode.session.tests;
 
-import java.awt.Container;
 import java.io.File;
 import java.io.IOException;
 
@@ -35,7 +34,8 @@ import java.io.IOException;
 public class GenericAppServerInstall extends ContainerInstall {
 
   /**
-   * Get the download URL and container name of a generic app server using hardcoded keywords
+   * Get the version number, download URL, and container name of a generic app server using
+   * hardcoded keywords
    *
    * Currently the only supported keyword instance is JETTY9.
    */

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/ServerContainer.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/ServerContainer.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/ServerContainer.java
index dbd438a..39ec42c 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/ServerContainer.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/ServerContainer.java
@@ -90,8 +90,10 @@ public abstract class ServerContainer {
     this.install = install;
     // Get a container description for logging and output
     description = generateUniqueContainerDescription(containerDescriptors);
+    // Setup logging
     loggingLevel = DEFAULT_LOGGING_LEVEL;
     logDir = new File(DEFAULT_LOG_DIR + description);
+    logDir.mkdirs();
 
     logger.info("Creating new container " + description);
 
@@ -105,8 +107,6 @@ public abstract class ServerContainer {
     systemProperties = new HashMap<>();
     // Set WAR file to session testing war
     warFile = new File(install.getWarFilePath());
-    // Setup logging folders
-    logDir.mkdirs();
 
     // Create the Cargo Container instance wrapping our physical container
     LocalConfiguration configuration = (LocalConfiguration) new DefaultConfigurationFactory()
@@ -122,18 +122,20 @@ public abstract class ServerContainer {
     gemfireLogFile = new File(logDir.getAbsolutePath() + "/gemfire.log");
     gemfireLogFile.getParentFile().mkdirs();
     setSystemProperty("log-file", gemfireLogFile.getAbsolutePath());
-    logger.info("Gemfire logs in " + gemfireLogFile.getAbsolutePath());
+
+    logger.info("Gemfire logs can be found in " + gemfireLogFile.getAbsolutePath());
 
     // Create the container
     container = (InstalledLocalContainer) (new DefaultContainerFactory())
         .createContainer(install.getInstallId(), ContainerType.INSTALLED, configuration);
     // Set container's home dir to where it was installed
     container.setHome(install.getHome());
-    // Set container output log
+    // Set container output log to directory setup for it
     container.setOutput(logDir.getAbsolutePath() + "/container.log");
 
     // Set cacheXML file
     File installXMLFile = install.getCacheXMLFile();
+    // Sets the cacheXMLFile variable and adds the cache XML file server system property map
     setCacheXMLFile(new File(logDir.getAbsolutePath() + "/" + installXMLFile.getName()));
     // Copy the cacheXML file to a new, unique location for this container
     FileUtils.copyFile(installXMLFile, cacheXMLFile);
@@ -182,7 +184,10 @@ public abstract class ServerContainer {
 
     try {
       logger.info("Starting container " + description);
+
+      // Writes settings to the expected form (either XML or WAR file)
       writeSettings();
+      // Start the container through cargo
       container.start();
     } catch (Exception e) {
       throw new RuntimeException(

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatClientServerTest.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatClientServerTest.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatClientServerTest.java
index 817428b..a5c6fa4 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatClientServerTest.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatClientServerTest.java
@@ -33,6 +33,7 @@ import org.apache.geode.test.dunit.rules.LocatorServerStartupRule;
  * Sets up the server needed for the client container to connect to
  */
 public abstract class TomcatClientServerTest extends CargoTestBase {
+  private String serverName;
 
   @Rule
   public transient TemporaryFolder temporaryFolder = new TemporaryFolder();
@@ -43,10 +44,6 @@ public abstract class TomcatClientServerTest extends CargoTestBase {
   @Rule
   public transient LocatorServerStartupRule locatorStartup = new LocatorServerStartupRule();
 
-
-  private String serverName;
-  private File workingDirectory;
-
   /**
    * Starts a server for the client Tomcat container to connect to using the GFSH command line
    * before each test
@@ -54,31 +51,33 @@ public abstract class TomcatClientServerTest extends CargoTestBase {
   @Before
   public void startServer() throws Exception {
     TomcatInstall install = (TomcatInstall) getInstall();
+    // List of all the jars for tomcat to put on the server classpath
     String libDirJars = install.getHome() + "/lib/*";
     String binDirJars = install.getHome() + "/bin/*";
 
-    CommandStringBuilder command = new CommandStringBuilder(CliStrings.START_SERVER);
+    // Set server name based on the test about to be run
     serverName = getClass().getSimpleName().concat("_").concat(getTestMethodName());
-    workingDirectory = temporaryFolder.newFolder(serverName);
-
 
+    // Create command string for starting server
+    CommandStringBuilder command = new CommandStringBuilder(CliStrings.START_SERVER);
     command.addOption(CliStrings.START_SERVER__NAME, serverName);
     command.addOption(CliStrings.START_SERVER__SERVER_PORT, "0");
+    // Add Tomcat jars to server classpath
     command.addOption(CliStrings.START_SERVER__CLASSPATH,
         binDirJars + File.pathSeparator + libDirJars);
     command.addOption(CliStrings.START_SERVER__LOCATORS, DUnitEnv.get().getLocatorString());
-    command.addOption(CliStrings.START_SERVER__DIR, workingDirectory.getCanonicalPath());
 
+    // Start server
     gfsh.executeAndVerifyCommand(command.toString());
   }
 
+  /**
+   * Stops the server for the client Tomcat container is has been connecting to
+   */
   @After
   public void stopServer() throws Exception {
     CommandStringBuilder command = new CommandStringBuilder(CliStrings.STOP_SERVER);
-
-    // command.addOption(CliStrings.START_SERVER__NAME, serverName);
-    command.addOption(CliStrings.STOP_SERVER__DIR, workingDirectory.getCanonicalPath());
-
+    command.addOption(CliStrings.STOP_SERVER__DIR, serverName);
     gfsh.executeAndVerifyCommand(command.toString());
   }
 }

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatContainer.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatContainer.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatContainer.java
index d1bf714..a75ba90 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatContainer.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatContainer.java
@@ -53,13 +53,13 @@ public class TomcatContainer extends ServerContainer {
       String containerDescriptors) throws IOException {
     super(install, containerConfigHome, containerDescriptors);
 
-    // Modify the cargo configuration to load off of the installation's context.xml
-    setConfigFile(DEFAULT_CONF_DIR + "context.xml", "conf", "context.xml");
     // Setup container specific XML files
     contextXMLFile = new File(logDir.getAbsolutePath() + "/context.xml");
     serverXMLFile = new File(DEFAULT_CONF_DIR + "server.xml");
 
+    // Copy the default container context XML file from the install to the specified path
     FileUtils.copyFile(new File(DEFAULT_CONF_DIR + "context.xml"), contextXMLFile);
+    // Set the container context XML file to the new location copied to above
     setConfigFile(contextXMLFile.getAbsolutePath(), DEFAULT_TOMCAT_XML_REPLACEMENT_DIR,
         DEFAULT_TOMCAT_CONTEXT_XML_REPLACEMENT_NAME);
 

http://git-wip-us.apache.org/repos/asf/geode/blob/a229933c/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatInstall.java
----------------------------------------------------------------------
diff --git a/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatInstall.java b/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatInstall.java
index 57dc519..28a4e8c 100644
--- a/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatInstall.java
+++ b/geode-assembly/src/test/java/org/apache/geode/session/tests/TomcatInstall.java
@@ -163,10 +163,12 @@ public class TomcatInstall extends ContainerInstall {
   public void setupDefaultSettings() {
     HashMap<String, String> attributes = new HashMap<>();
 
+    // Set the session manager class within the context XML file
     attributes.put("className", getContextSessionManagerClass());
     editXMLFile(getDefaultContextXMLFile().getAbsolutePath(), "Tomcat", "Manager", "Context",
         attributes);
 
+    // Set the server lifecycle listener within the server XML file
     attributes.put("className", getServerLifeCycleListenerClass());
     editXMLFile(getDefaultServerXMLFile().getAbsolutePath(), "Tomcat", "Listener", "Server",
         attributes);
@@ -276,7 +278,7 @@ public class TomcatInstall extends ContainerInstall {
     // Don't need to copy any jars already in the tomcat install
     File tomcatLib = new File(tomcatLibPath);
 
-    // Find all the required jars in the tomcatModulePath
+    // Find all jars in the tomcatModulePath and add them as required jars
     try {
       for (File file : (new File(moduleJarDir)).listFiles()) {
         if (file.isFile() && file.getName().endsWith(".jar")) {
@@ -318,9 +320,11 @@ public class TomcatInstall extends ContainerInstall {
    */
   private void updateProperties() throws Exception {
     String jarsToSkip = "";
+    // Adds all the required jars as jars to skip when starting Tomcat
     for (String jarName : tomcatRequiredJars)
       jarsToSkip += "," + jarName + "*.jar";
 
+    // Add the jars to skip to the catalina property file
     editPropertyFile(getHome() + "/conf/catalina.properties", version.jarSkipPropertyName(),
         jarsToSkip, true);
   }