Revision: 6945 Author: [email protected] Date: Mon Nov 16 18:58:40 2009 Log: 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) http://code.google.com/p/google-web-toolkit/source/detail?r=6945 Modified: /trunk/user/src/com/google/gwt/junit/JUnitMessageQueue.java /trunk/user/src/com/google/gwt/junit/JUnitShell.java /trunk/user/src/com/google/gwt/junit/RunStyle.java /trunk/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java /trunk/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java /trunk/user/src/com/google/gwt/junit/RunStyleManual.java /trunk/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java /trunk/user/src/com/google/gwt/junit/RunStyleSelenium.java /trunk/user/test/com/google/gwt/junit/CompileStrategyTest.java /trunk/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java ======================================= --- /trunk/user/src/com/google/gwt/junit/JUnitMessageQueue.java Mon Nov 16 18:58:08 2009 +++ /trunk/user/src/com/google/gwt/junit/JUnitMessageQueue.java Mon Nov 16 18:58:40 2009 @@ -107,7 +107,8 @@ /** * Only instantiable within this package. */ - JUnitMessageQueue() { + JUnitMessageQueue(int numClients) { + this.numClients = numClients; } /** @@ -245,6 +246,10 @@ return results.toArray(new String[results.size()]); } } + + int getNumClients() { + return numClients; + } /** * Returns how many clients have requested the currently-running test. @@ -430,22 +435,6 @@ testResults.remove(testInfo); } } - - /** - * 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) { ======================================= --- /trunk/user/src/com/google/gwt/junit/JUnitShell.java Mon Nov 16 18:58:22 2009 +++ /trunk/user/src/com/google/gwt/junit/JUnitShell.java Mon Nov 16 18:58:40 2009 @@ -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"); } @@ -679,13 +677,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. */ @@ -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(); ======================================= --- /trunk/user/src/com/google/gwt/junit/RunStyle.java Mon Nov 16 18:58:08 2009 +++ /trunk/user/src/com/google/gwt/junit/RunStyle.java Mon Nov 16 18:58:40 2009 @@ -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 ======================================= --- /trunk/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java Mon Nov 16 18:58:08 2009 +++ /trunk/user/src/com/google/gwt/junit/RunStyleExternalBrowser.java Mon Nov 16 18:58:40 2009 @@ -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 ======================================= --- /trunk/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java Mon Nov 16 18:58:08 2009 +++ /trunk/user/src/com/google/gwt/junit/RunStyleHtmlUnit.java Mon Nov 16 18:58:40 2009 @@ -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 ======================================= --- /trunk/user/src/com/google/gwt/junit/RunStyleManual.java Mon Nov 16 18:58:08 2009 +++ /trunk/user/src/com/google/gwt/junit/RunStyleManual.java Mon Nov 16 18:58:40 2009 @@ -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 ======================================= --- /trunk/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java Mon Nov 16 18:58:08 2009 +++ /trunk/user/src/com/google/gwt/junit/RunStyleRemoteWeb.java Mon Nov 16 18:58:40 2009 @@ -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 ======================================= --- /trunk/user/src/com/google/gwt/junit/RunStyleSelenium.java Mon Nov 16 18:58:08 2009 +++ /trunk/user/src/com/google/gwt/junit/RunStyleSelenium.java Mon Nov 16 18:58:40 2009 @@ -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 ======================================= --- /trunk/user/test/com/google/gwt/junit/CompileStrategyTest.java Mon Nov 16 18:58:22 2009 +++ /trunk/user/test/com/google/gwt/junit/CompileStrategyTest.java Mon Nov 16 18:58:40 2009 @@ -120,7 +120,7 @@ private List<TestInfo[]> testBlocks; public MockJUnitMessageQueue() { - super(); + super(1); } @Override ======================================= --- /trunk/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java Fri Nov 6 15:38:19 2009 +++ /trunk/user/test/com/google/gwt/junit/JUnitMessageQueueTest.java Mon Nov 16 18:58:40 2009 @@ -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[]>(); @@ -459,17 +458,6 @@ // check that the updated result appears now. 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. @@ -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; } -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
