RunStyleHtmlUnit was not setting the number of clients.
- Changed API to force all RunStyles to return the number of clients, preventing future bugs like this.
- JUnitMessageQueue now holds numClients as a final variable, preventing state mismatch.
Review by: jlabanca (desk)
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@6945 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 6e3fca4..b8f671a 100644
--- a/user/src/com/google/gwt/junit/JUnitMessageQueue.java
+++ b/user/src/com/google/gwt/junit/JUnitMessageQueue.java
@@ -107,7 +107,8 @@
/**
* Only instantiable within this package.
*/
- JUnitMessageQueue() {
+ JUnitMessageQueue(int numClients) {
+ this.numClients = numClients;
}
/**
@@ -246,6 +247,10 @@
}
}
+ int getNumClients() {
+ return numClients;
+ }
+
/**
* Returns how many clients have requested the currently-running test.
*
@@ -431,22 +436,6 @@
}
}
- /**
- * 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 72e5c32..07ba2e4 100644
--- a/user/src/com/google/gwt/junit/JUnitShell.java
+++ b/user/src/com/google/gwt/junit/JUnitShell.java
@@ -603,8 +603,6 @@
if (!argProcessor.processArgs(args)) {
throw new JUnitFatalLaunchException("Error processing shell arguments");
}
- unitTestShell.messageQueue = new JUnitMessageQueue();
-
if (!unitTestShell.startUp()) {
throw new JUnitFatalLaunchException("Shell failed to start");
}
@@ -680,13 +678,6 @@
private JUnitMessageQueue messageQueue;
/**
- * The number of test clients executing in parallel. With -remoteweb, users
- * can specify a number of parallel test clients, but by default we only have
- * 1.
- */
- private int numClients = 1;
-
- /**
* An exception that should by fired the next time runTestImpl runs.
*/
private UnableToCompleteException pendingException;
@@ -775,10 +766,12 @@
if (!super.doStartup()) {
return false;
}
- if (!createRunStyle(runStyleName)) {
+ int numClients = createRunStyle(runStyleName);
+ if (numClients < 0) {
// RunStyle already logged reasons for its failure
return false;
}
+ messageQueue = new JUnitMessageQueue(numClients);
if (tries >= 1) {
runStyle.setTries(tries);
@@ -807,6 +800,7 @@
*/
protected boolean notDone() {
int activeClients = messageQueue.getNumClientsRetrievedTest(currentTestInfo);
+ int expectedClients = messageQueue.getNumClients();
if (firstLaunch && runStyle instanceof RunStyleManual) {
String[] newClients = messageQueue.getNewClients();
int printIndex = activeClients - newClients.length + 1;
@@ -814,7 +808,7 @@
System.out.println(printIndex + " - " + newClient);
++printIndex;
}
- if (activeClients != this.numClients) {
+ if (activeClients != expectedClients) {
// Wait forever for first contact; user-driven.
return true;
}
@@ -822,7 +816,7 @@
// Limit permutations after all clients have connected.
if (remoteUserAgents == null
- && messageQueue.getNumConnectedClients() == numClients) {
+ && messageQueue.getNumConnectedClients() == expectedClients) {
remoteUserAgents = messageQueue.getUserAgents();
String userAgentList = "";
for (int i = 0; i < remoteUserAgents.length; i++) {
@@ -838,11 +832,11 @@
}
long currentTimeMillis = System.currentTimeMillis();
- if (activeClients >= numClients) {
- if (activeClients > numClients) {
+ if (activeClients >= expectedClients) {
+ if (activeClients > expectedClients) {
getTopLogger().log(
TreeLogger.WARN,
- "Too many clients: expected " + numClients + ", found "
+ "Too many clients: expected " + expectedClients + ", found "
+ activeClients);
}
firstLaunch = false;
@@ -941,16 +935,12 @@
}
/**
- * Set the expected number of clients.
- *
- * <p>
- * Should only be called by RunStyle subtypes.
- *
- * @param numClients
+ * @deprecated TODO(fabbott) delete me
*/
+ @Deprecated
+ @SuppressWarnings("unused")
void setNumClients(int numClients) {
- this.numClients = numClients;
- messageQueue.setNumClients(numClients);
+ throw new UnsupportedOperationException("This method should be deleted.");
}
void setStandardsMode(boolean standardsMode) {
@@ -969,9 +959,9 @@
* Create the specified (or default) runStyle.
*
* @param runStyleName the argument passed to -runStyle
- * @return true if the runStyle was successfully created/initialized
+ * @return the number of clients, or -1 if initialization was unsuccessful
*/
- private boolean createRunStyle(String runStyleName) {
+ private int createRunStyle(String runStyleName) {
String args = null;
String name = runStyleName;
int colon = name.indexOf(':');
@@ -1034,7 +1024,8 @@
Map<String, JUnitResult> results = messageQueue.getResults(currentTestInfo);
assert results != null;
- assert results.size() == numClients : results.size() + " != " + numClients;
+ assert results.size() == messageQueue.getNumClients() : results.size()
+ + " != " + messageQueue.getNumClients();
for (Entry<String, JUnitResult> entry : results.entrySet()) {
String clientId = entry.getKey();
diff --git a/user/src/com/google/gwt/junit/RunStyle.java b/user/src/com/google/gwt/junit/RunStyle.java
index d669294..bed70d2 100644
--- a/user/src/com/google/gwt/junit/RunStyle.java
+++ b/user/src/com/google/gwt/junit/RunStyle.java
@@ -31,7 +31,7 @@
*/
protected final JUnitShell shell;
- protected int tries = 1;
+ private int tries = 1;
/**
* Constructor for RunStyle. Any subclass must provide a constructor with the
@@ -77,15 +77,13 @@
}
/**
- * Initialize the runstyle with any supplied arguments.
+ * Initialize the runstyle with any supplied arguments, and return the number
+ * of clients this runstyle controls.
*
* @param args arguments passed in -runStyle option, null if none supplied
- * @return true if this runstyle is initialized successfully, false if it was
- * unsuccessful
+ * @return the number of clients, or -1 if initialization was unsuccessful
*/
- public boolean initialize(String args) {
- return true;
- }
+ public abstract int initialize(String args);
/**
* Requests initial launch of the browser. This should only be called once per
diff --git a/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java b/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java
index 64246bc..adf4af8 100644
--- a/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java
+++ b/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java
@@ -97,17 +97,16 @@
}
@Override
- public boolean initialize(String args) {
+ public int initialize(String args) {
if (args == null || args.length() == 0) {
getLogger().log(
TreeLogger.ERROR,
"ExternalBrowser runstyle requires an "
+ "argument listing one or more executables of external browsers to "
+ "launch");
- return false;
+ return -1;
}
String browsers[] = args.split(",");
- shell.setNumClients(browsers.length);
synchronized (this) {
this.externalBrowsers = new ExternalBrowser[browsers.length];
for (int i = 0; i < browsers.length; ++i) {
@@ -115,7 +114,7 @@
}
}
Runtime.getRuntime().addShutdownHook(new ShutdownCb());
- return true;
+ return browsers.length;
}
@Override
diff --git a/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java b/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java
index 4f94186..f5519e3 100644
--- a/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java
+++ b/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java
@@ -192,12 +192,7 @@
}
@Override
- public int getTries() {
- return tries;
- }
-
- @Override
- public boolean initialize(String args) {
+ public int initialize(String args) {
if (args == null || args.length() == 0) {
// If no browsers specified, default to Firefox 3.
args = "FF3";
@@ -210,14 +205,14 @@
TreeLogger.ERROR,
"RunStyleHtmlUnit: Unknown browser " + "name " + browserName
+ ", expected browser name: one of " + BROWSER_MAP.keySet());
- return false;
+ return -1;
}
browserSet.add(browser);
}
browsers = Collections.unmodifiableSet(browserSet);
setTries(DEFAULT_TRIES); // set to the default value for this RunStyle
- return true;
+ return browsers.size();
}
@Override
diff --git a/user/src/com/google/gwt/junit/RunStyleManual.java b/user/src/com/google/gwt/junit/RunStyleManual.java
index c4b4de6..668e1a9 100644
--- a/user/src/com/google/gwt/junit/RunStyleManual.java
+++ b/user/src/com/google/gwt/junit/RunStyleManual.java
@@ -31,7 +31,7 @@
}
@Override
- public boolean initialize(String args) {
+ public int initialize(String args) {
numClients = 1;
if (args != null) {
try {
@@ -39,11 +39,10 @@
} catch (NumberFormatException e) {
getLogger().log(TreeLogger.ERROR,
"Error parsing argument \"" + args + "\"", e);
- return false;
+ return -1;
}
}
- shell.setNumClients(numClients);
- return true;
+ return numClients;
}
@Override
diff --git a/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java b/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java
index 90c7741..e1a1968 100644
--- a/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java
+++ b/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java
@@ -168,11 +168,11 @@
}
@Override
- public boolean initialize(String args) {
+ public int initialize(String args) {
if (args == null || args.length() == 0) {
getLogger().log(TreeLogger.ERROR,
"RemoteWeb runstyle requires comma-separated RMI URLs");
- return false;
+ return -1;
}
String[] urls = args.split(",");
try {
@@ -180,10 +180,9 @@
} catch (IOException e) {
getLogger().log(TreeLogger.ERROR,
"RemoteWeb: Error initializing RMISocketFactory", e);
- return false;
+ return -1;
}
int numClients = urls.length;
- shell.setNumClients(numClients);
BrowserManager[] browserManagers = new BrowserManager[numClients];
for (int i = 0; i < numClients; ++i) {
long callStart = System.currentTimeMillis();
@@ -200,7 +199,7 @@
cause = e.getCause();
}
getLogger().log(TreeLogger.ERROR, message, cause);
- return false;
+ return -1;
}
}
synchronized (this) {
@@ -210,7 +209,7 @@
}
}
Runtime.getRuntime().addShutdownHook(new ShutdownCb());
- return true;
+ return numClients;
}
@Override
diff --git a/user/src/com/google/gwt/junit/RunStyleSelenium.java b/user/src/com/google/gwt/junit/RunStyleSelenium.java
index 7e3bfd2..d3259b7 100644
--- a/user/src/com/google/gwt/junit/RunStyleSelenium.java
+++ b/user/src/com/google/gwt/junit/RunStyleSelenium.java
@@ -119,11 +119,11 @@
}
@Override
- public boolean initialize(String args) {
+ public int initialize(String args) {
if (args == null || args.length() == 0) {
getLogger().log(TreeLogger.ERROR,
"Selenium runstyle requires comma-separated Selenium-RC targets");
- return false;
+ return -1;
}
String[] targetsIn = args.split(",");
SeleniumWrapper targets[] = new SeleniumWrapper[targetsIn.length];
@@ -133,12 +133,11 @@
targets[i] = createSeleniumWrapper(targetsIn[i]);
} catch (IllegalArgumentException e) {
getLogger().log(TreeLogger.ERROR, e.getMessage());
- return false;
+ return -1;
}
}
this.remotes = targets;
- shell.setNumClients(targets.length);
// Install a shutdown hook that will close all of our outstanding Selenium
// sessions.
@@ -158,7 +157,7 @@
}
});
start();
- return true;
+ return targets.length;
}
@Override
diff --git a/user/test/com/google/gwt/junit/CompileStrategyTest.java b/user/test/com/google/gwt/junit/CompileStrategyTest.java
index 308c104..8792940 100644
--- a/user/test/com/google/gwt/junit/CompileStrategyTest.java
+++ b/user/test/com/google/gwt/junit/CompileStrategyTest.java
@@ -120,7 +120,7 @@
private List<TestInfo[]> testBlocks;
public MockJUnitMessageQueue() {
- super();
+ super(1);
}
@Override
diff --git a/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java b/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java
index 96dd50b..0f5a525 100644
--- a/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java
+++ b/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java
@@ -37,8 +37,7 @@
private final static int ONE_TEST_PER_BLOCK = 1;
public void testAddTestBlocks() {
- JUnitMessageQueue queue = new JUnitMessageQueue();
- queue.setNumClients(10);
+ JUnitMessageQueue queue = new JUnitMessageQueue(10);
assertEquals(0, queue.getTestBlocks().size());
List<TestInfo[]> expectedBlocks = new ArrayList<TestInfo[]>();
@@ -460,17 +459,6 @@
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.
@@ -502,8 +490,7 @@
*/
private JUnitMessageQueue createQueue(int numClients, int numBlocks,
int testsPerBlock) {
- JUnitMessageQueue queue = new JUnitMessageQueue();
- queue.setNumClients(numClients);
+ JUnitMessageQueue queue = new JUnitMessageQueue(numClients);
queue.addTestBlocks(createTestBlocks(numBlocks, testsPerBlock), true);
return queue;
}
