Adding the host path to the error message when a remote browser dies during a test. Also fixing an NPE that occurs when a RunStyle cannot be initialized.
Patch by: jlabanca
Review by: amitmanjhi
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@6707 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/junit/JUnitMessageQueue.java b/user/src/com/google/gwt/junit/JUnitMessageQueue.java
index 0137956..4c06882 100644
--- a/user/src/com/google/gwt/junit/JUnitMessageQueue.java
+++ b/user/src/com/google/gwt/junit/JUnitMessageQueue.java
@@ -75,7 +75,7 @@
/**
* The number of TestCase clients executing in parallel.
*/
- private final int numClients;
+ private int numClients = 1;
/**
* Maps the TestInfo to the results from each clientId. If JUnitResult is
@@ -97,14 +97,8 @@
/**
* Only instantiable within this package.
- *
- * @param numClients The number of parallel clients being served by this
- * queue.
*/
- JUnitMessageQueue(int numClients) {
- synchronized (clientStatusesLock) {
- this.numClients = numClients;
- }
+ JUnitMessageQueue() {
}
/**
@@ -428,6 +422,22 @@
}
}
+ /**
+ * Set the number of clients. This must be called before adding any tests.
+ *
+ * @param numClients The number of parallel clients being served by this
+ * queue.
+ */
+ void setNumClients(int numClients) {
+ synchronized (clientStatusesLock) {
+ if (testBlocks.size() > 0) {
+ throw new IllegalStateException(
+ "Cannot change number of clients after adding tests");
+ }
+ this.numClients = numClients;
+ }
+ }
+
void waitForResults(int millis) {
synchronized (clientStatusesLock) {
try {
diff --git a/user/src/com/google/gwt/junit/JUnitShell.java b/user/src/com/google/gwt/junit/JUnitShell.java
index eeb8e9a..5af0e79 100644
--- a/user/src/com/google/gwt/junit/JUnitShell.java
+++ b/user/src/com/google/gwt/junit/JUnitShell.java
@@ -240,7 +240,8 @@
@Override
public boolean setString(String runStyleArg) {
- return createRunStyle(runStyleArg);
+ runStyleName = runStyleArg;
+ return true;
}
});
@@ -568,11 +569,7 @@
if (!argProcessor.processArgs(args)) {
throw new JUnitFatalLaunchException("Error processing shell arguments");
}
- if (unitTestShell.runStyle == null) {
- unitTestShell.createRunStyle("HtmlUnit");
- }
- unitTestShell.messageQueue = new JUnitMessageQueue(
- unitTestShell.numClients);
+ unitTestShell.messageQueue = new JUnitMessageQueue();
if (!unitTestShell.startUp()) {
throw new JUnitFatalLaunchException("Shell failed to start");
@@ -670,6 +667,12 @@
*/
private RunStyle runStyle = null;
+ /**
+ * The argument passed to -runStyle. This is parsed later so we can pass in
+ * a logger.
+ */
+ private String runStyleName = "HtmlUnit";
+
private boolean shouldAutoGenerateResources = true;
private boolean standardsMode = false;
@@ -732,6 +735,11 @@
if (!super.doStartup()) {
return false;
}
+ if (!createRunStyle(runStyleName)) {
+ // RunStyle already logged reasons for its failure
+ return false;
+ }
+
if (!runStyle.setupMode(getTopLogger(), developmentMode)) {
getTopLogger().log(TreeLogger.ERROR, "Run style does not support "
+ (developmentMode ? "development" : "production") + " mode");
@@ -816,8 +824,16 @@
+ "\n Actual time elapsed: " + elapsed + " seconds.\n");
}
- if (runStyle.wasInterrupted()) {
- throw new TimeoutException("A remote browser died a mysterious death.");
+ // Check that we haven't lost communication with a remote host.
+ String[] interruptedHosts = runStyle.getInterruptedHosts();
+ if (interruptedHosts != null) {
+ StringBuilder msg = new StringBuilder();
+ msg.append("A remote browser died a mysterious death.\n");
+ msg.append(" We lost communication with:");
+ for (String host : interruptedHosts) {
+ msg.append("\n ").append(host);
+ }
+ throw new TimeoutException(msg.toString());
}
if (messageQueue.hasResults(currentTestInfo)) {
@@ -884,6 +900,7 @@
*/
void setNumClients(int numClients) {
this.numClients = numClients;
+ messageQueue.setNumClients(numClients);
}
void setStandardsMode(boolean standardsMode) {
diff --git a/user/src/com/google/gwt/junit/RunStyle.java b/user/src/com/google/gwt/junit/RunStyle.java
index e6d7b94..2208d36 100644
--- a/user/src/com/google/gwt/junit/RunStyle.java
+++ b/user/src/com/google/gwt/junit/RunStyle.java
@@ -40,6 +40,15 @@
}
/**
+ * Tests whether the test was interrupted.
+ *
+ * @return the interrupted hosts, or null if not interrupted
+ */
+ public String[] getInterruptedHosts() {
+ return null;
+ }
+
+ /**
* Returns the number of times this test should be tried to run. A test
* succeeds if it succeeds even once.
*/
@@ -91,15 +100,6 @@
}
/**
- * Tests whether the test was interrupted.
- *
- * @return <code>true</code> if the test has been interrupted.
- */
- public boolean wasInterrupted() {
- return false;
- }
-
- /**
* Gets the shell logger.
*/
protected TreeLogger getLogger() {
diff --git a/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java b/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java
index 73442d8..12239ca 100644
--- a/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java
+++ b/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java
@@ -18,6 +18,9 @@
import com.google.gwt.core.ext.TreeLogger;
import com.google.gwt.core.ext.UnableToCompleteException;
+import java.util.HashSet;
+import java.util.Set;
+
/**
* Runs in web mode via browsers managed as an external process. This feature is
* experimental and is not officially supported.
@@ -74,6 +77,26 @@
}
@Override
+ public String[] getInterruptedHosts() {
+ // Make sure all browsers are still running
+ Set<String> toRet = null;
+ for (ExternalBrowser browser : externalBrowsers) {
+ try {
+ browser.getProcess().exitValue();
+
+ // The host exited, so return its path.
+ if (toRet == null) {
+ toRet = new HashSet<String>();
+ }
+ toRet.add(browser.getPath());
+ } catch (IllegalThreadStateException e) {
+ // The process is still active, keep looking.
+ }
+ }
+ return toRet == null ? null : toRet.toArray(new String[toRet.size()]);
+ }
+
+ @Override
public boolean initialize(String args) {
if (args == null || args.length() == 0) {
getLogger().log(TreeLogger.ERROR, "ExternalBrowser runstyle requires an "
@@ -82,6 +105,7 @@
return false;
}
String browsers[] = args.split(",");
+ shell.setNumClients(browsers.length);
synchronized (this) {
this.externalBrowsers = new ExternalBrowser[browsers.length];
for (int i = 0; i < browsers.length; ++i) {
@@ -118,20 +142,4 @@
browser.setProcess(child);
}
}
-
- @Override
- public boolean wasInterrupted() {
-
- // Make sure all browsers are still running
- for (ExternalBrowser browser : externalBrowsers) {
- try {
- browser.getProcess().exitValue();
- } catch (IllegalThreadStateException e) {
- // The process is still active, keep looking.
- continue;
- }
- return true;
- }
- return false;
- }
}
diff --git a/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java b/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java
index 795762d..e870a3a 100644
--- a/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java
+++ b/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java
@@ -27,6 +27,8 @@
import java.net.SocketTimeoutException;
import java.rmi.Naming;
import java.rmi.server.RMISocketFactory;
+import java.util.HashSet;
+import java.util.Set;
/**
* Runs in web mode via browsers managed over RMI. This feature is experimental
@@ -134,13 +136,13 @@
private RemoteBrowser[] remoteBrowsers;
/**
- * Whether one of the remote browsers was interrupted.
+ * The list of hosts that were interrupted.
*/
- private boolean wasInterrupted;
+ private Set<String> interruptedHosts;
/**
- * A separate lock to control access to {@link #wasInterrupted}. This keeps
- * the main thread calls into {@link #wasInterrupted()} from having to
+ * A separate lock to control access to {@link #interruptedHosts}. This keeps
+ * the main thread calls into {@link #getInterruptedHosts()} from having to
* synchronized on the containing instance and potentially block on RPC calls.
* It is okay to take the {@link #wasInterruptedLock} while locking the
* containing instance; it is NOT okay to do the opposite or deadlock could
@@ -154,7 +156,17 @@
public RunStyleRemoteWeb(JUnitShell shell) {
super(shell);
}
-
+
+ @Override
+ public String[] getInterruptedHosts() {
+ synchronized (wasInterruptedLock) {
+ if (interruptedHosts == null) {
+ return null;
+ }
+ return interruptedHosts.toArray(new String[interruptedHosts.size()]);
+ }
+ }
+
@Override
public boolean initialize(String args) {
if (args == null || args.length() == 0) {
@@ -247,13 +259,6 @@
keepAliveThread.start();
}
- @Override
- public boolean wasInterrupted() {
- synchronized (wasInterruptedLock) {
- return wasInterrupted;
- }
- }
-
private synchronized boolean doKeepAlives() {
for (RemoteBrowser remoteBrowser : remoteBrowsers) {
if (remoteBrowser.getToken() > 0) {
@@ -278,7 +283,10 @@
}
remoteBrowser.setToken(0);
synchronized (wasInterruptedLock) {
- wasInterrupted = true;
+ if (interruptedHosts == null) {
+ interruptedHosts = new HashSet<String>();
+ }
+ interruptedHosts.add(remoteBrowser.getRmiUrl());
}
break;
}
@@ -286,7 +294,7 @@
}
synchronized (wasInterruptedLock) {
- return !wasInterrupted;
+ return interruptedHosts == null;
}
}
}
diff --git a/user/src/com/google/gwt/junit/RunStyleSelenium.java b/user/src/com/google/gwt/junit/RunStyleSelenium.java
index 1833c30..b1f883d 100644
--- a/user/src/com/google/gwt/junit/RunStyleSelenium.java
+++ b/user/src/com/google/gwt/junit/RunStyleSelenium.java
@@ -24,6 +24,8 @@
import java.net.InetAddress;
import java.net.UnknownHostException;
+import java.util.HashSet;
+import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
@@ -68,13 +70,13 @@
private RCSelenium remotes[];
/**
- * Whether one of the remote browsers was interrupted.
+ * The list of hosts that were interrupted.
*/
- private boolean wasInterrupted;
+ private Set<String> interruptedHosts;
/**
- * A separate lock to control access to {@link #wasInterrupted}. This keeps
- * the main thread calls into {@link #wasInterrupted()} from having to be
+ * A separate lock to control access to {@link #interruptedHosts}. This keeps
+ * the main thread calls into {@link #getInterruptedHosts()} from having to be
* synchronized on the containing instance and potentially block on RPC calls.
* It is okay to take the {@link #wasInterruptedLock} while locking the
* containing instance; it is NOT okay to do the opposite or deadlock could
@@ -85,7 +87,17 @@
public RunStyleSelenium(final JUnitShell shell) {
super(shell);
}
-
+
+ @Override
+ public String[] getInterruptedHosts() {
+ synchronized (wasInterruptedLock) {
+ if (interruptedHosts == null) {
+ return null;
+ }
+ return interruptedHosts.toArray(new String[interruptedHosts.size()]);
+ }
+ }
+
@Override
public boolean initialize(String args) {
if (args == null || args.length() == 0) {
@@ -160,13 +172,6 @@
}
}
- @Override
- public boolean wasInterrupted() {
- synchronized (wasInterruptedLock) {
- return wasInterrupted;
- }
- }
-
/**
* Create the keep-alive thread.
*/
@@ -199,17 +204,17 @@
remote.getSelenium().getTitle();
}
} catch (Throwable e) {
- setWasInterrupted(true);
+ synchronized (wasInterruptedLock) {
+ if (interruptedHosts == null) {
+ interruptedHosts = new HashSet<String>();
+ }
+ interruptedHosts.add(remote.getHost() + ":" + remote.getPort()
+ + "/" + remote.getBrowser());
+ }
}
}
}
- return !wasInterrupted();
- }
-
- private void setWasInterrupted(boolean wasInterrupted) {
- synchronized (wasInterruptedLock) {
- this.wasInterrupted = wasInterrupted;
- }
+ return interruptedHosts == null;
}
}
diff --git a/user/test/com/google/gwt/junit/CompileStrategyTest.java b/user/test/com/google/gwt/junit/CompileStrategyTest.java
index d04689c..df25b4a 100644
--- a/user/test/com/google/gwt/junit/CompileStrategyTest.java
+++ b/user/test/com/google/gwt/junit/CompileStrategyTest.java
@@ -63,7 +63,7 @@
private boolean isLastBlock;
public MockJUnitMessageQueue() {
- super(1);
+ super();
}
@Override
diff --git a/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java b/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java
index f8cd726..d7f7058 100644
--- a/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java
+++ b/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java
@@ -36,7 +36,8 @@
private final static int ONE_TEST_PER_BLOCK = 1;
public void testAddTestBlocks() {
- JUnitMessageQueue queue = new JUnitMessageQueue(10);
+ JUnitMessageQueue queue = new JUnitMessageQueue();
+ queue.setNumClients(10);
assertEquals(0, queue.getTestBlocks().size());
List<TestInfo[]> expectedBlocks = new ArrayList<TestInfo[]>();
@@ -458,6 +459,17 @@
assertTrue(queue.getResults(testInfo).get("client0").getException() == null);
}
+ public void testSetNumClients() {
+ JUnitMessageQueue queue = new JUnitMessageQueue();
+ queue.addTestBlocks(createTestBlocks(ONE_BLOCK, ONE_TEST_PER_BLOCK), true);
+ try {
+ queue.setNumClients(2);
+ fail("Expected IllegalStateException");
+ } catch (IllegalStateException e) {
+ // expected.
+ }
+ }
+
/**
* Assert that two arrays are the same size and contain the same elements.
* Ordering does not matter.
@@ -489,7 +501,8 @@
*/
private JUnitMessageQueue createQueue(int numClients, int numBlocks,
int testsPerBlock) {
- JUnitMessageQueue queue = new JUnitMessageQueue(numClients);
+ JUnitMessageQueue queue = new JUnitMessageQueue();
+ queue.setNumClients(numClients);
queue.addTestBlocks(createTestBlocks(numBlocks, testsPerBlock), true);
return queue;
}
