JUnitShell fails tests that are queued in BrowserManagerServer

[jlab...@google.com]

Reviewers: jat,

Description:
Description:
============
JUnitShell fails tests after 60s, but BrowserManagerServer may have a
queue much longer than that if multiple JUnitShell instances are using
it. For example, our own build runs in parallel, and some tests take
2-5 minutes to complete, so other tests will timeout while waiting in
the queue.

Fix:
====
RunStyleRemoteWeb now takes a similar path as RunStyleManual, giving
clients unlimited time to start the test. RunStyleRemoteWeb already
handles communication errors with JUnitShell, so we just check for
wasInterrupted.

Testing:
========
Verified manually that both manual and remote web tests run as expected.
Also verified that killing BrowserManagerServer or never starting it is
handled by JUnitShell.

Please review this at http://gwt-code-reviews.appspot.com/87805

Affected files:
user/src/com/google/gwt/junit/JUnitShell.java
user/src/com/google/gwt/junit/RunStyle.java
user/src/com/google/gwt/junit/RunStyleManual.java
user/src/com/google/gwt/junit/RunStyleRemoteWeb.java

Index: user/src/com/google/gwt/junit/RunStyleRemoteWeb.java
===================================================================
--- user/src/com/google/gwt/junit/RunStyleRemoteWeb.java (revision 6454)
+++ user/src/com/google/gwt/junit/RunStyleRemoteWeb.java (working copy)
@@ -201,7 +201,15 @@
return true;
}

+ /**
+ * Remote web tests can be queued for extended periods.
+ */
@Override
+ public boolean isStartDelayed() {
+ return true;
+ }
+
+ @Override
public synchronized void launchModule(String moduleName)
throws UnableToCompleteException {
String url = shell.getModuleUrl(moduleName);
Index: user/src/com/google/gwt/junit/JUnitShell.java
===================================================================
--- user/src/com/google/gwt/junit/JUnitShell.java (revision 6454)
+++ user/src/com/google/gwt/junit/JUnitShell.java (working copy)
@@ -688,15 +688,22 @@
*/
protected boolean notDone() {
int activeClients =
messageQueue.getNumClientsRetrievedTest(currentTestInfo);
- if (firstLaunch && runStyle instanceof RunStyleManual) {
- String[] newClients = messageQueue.getNewClients();
- int printIndex = activeClients - newClients.length + 1;
- for (String newClient : newClients) {
- System.out.println(printIndex + " - " + newClient);
- ++printIndex;
+ if (firstLaunch && runStyle.isStartDelayed()) {
+ // Pretty print the list of clients for manual tests.
+ if (runStyle instanceof RunStyleManual) {
+ String[] newClients = messageQueue.getNewClients();
+ int printIndex = activeClients - newClients.length + 1;
+ for (String newClient : newClients) {
+ System.out.println(printIndex + " - " + newClient);
+ ++printIndex;
+ }
}
- if (activeClients != this.numClients) {
- // Wait forever for first contact; user-driven.
+
+ if (runStyle.wasInterrupted()) {
+ // Exit early if the test is interrupted.
+ throw new TimeoutException("A remote browser died a mysterious
death.");
+ } else if (activeClients != this.numClients) {
+ // Wait forever for first contact.
return true;
}
}
Index: user/src/com/google/gwt/junit/RunStyleManual.java
===================================================================
--- user/src/com/google/gwt/junit/RunStyleManual.java (revision 6454)
+++ user/src/com/google/gwt/junit/RunStyleManual.java (working copy)
@@ -46,7 +46,15 @@
return true;
}

+ /**
+ * Manual tests are started by users in user time.
+ */
@Override
+ public boolean isStartDelayed() {
+ return true;
+ }
+
+ @Override
public void launchModule(String moduleName) throws
UnableToCompleteException {
if (numClients == 1) {
System.out.println("Please navigate your browser to this URL:");
Index: user/src/com/google/gwt/junit/RunStyle.java
===================================================================
--- user/src/com/google/gwt/junit/RunStyle.java (revision 6454)
+++ user/src/com/google/gwt/junit/RunStyle.java (working copy)
@@ -51,6 +51,15 @@
}

/**
+ * Check whether or not tests start immediately or if they can be
delayed.
+ *
+ * @return <code>true</code> if the test can be delayed
+ */
+ public boolean isStartDelayed() {
+ return false;
+ }
+
+ /**
* Requests initial launch of the browser. This should only be called
once per
* instance of RunStyle.
*

[John Tamplin]

On Fri, Oct 23, 2009 at 1:38 PM, <jlab...@google.com> wrote:

RunStyleRemoteWeb now takes a similar path as RunStyleManual, giving
clients unlimited time to start the test. RunStyleRemoteWeb already
handles communication errors with JUnitShell, so we just check for
wasInterrupted.

My concern is that it will hang indefinitely if the remote browser is really hosed -- I am ok with waiting longer for the initial connection, but I think it needs to still have a timeout.

Longer term, I think we need t do away with RemoteBrowserManager and replace it with Selenium.  Current versions of Selenium-RC can launch Chrome and IE, so it seems better to use something that is actually supported and avoids the known problems with RemoteBrowserManager.

--
John A. Tamplin
Software Engineer (GWT), Google

[John LaBanca]

RunStyleRemoteWeb#launchModule will detect if the test cannot start, and #doKeepAlives will detect communication errors.  If BrowserManager is stuck running a test, then presumably the JUnitShell that opened it will timeout the test after 20 minutes and kill the test, so I think we're covered all around (can't start, communication fails, hang on server).

Thanks,
John LaBanca
jlab...@google.com

[John Tamplin]

On Fri, Oct 23, 2009 at 2:23 PM, John LaBanca <jlab...@google.com> wrote:

RunStyleRemoteWeb#launchModule will detect if the test cannot start, and #doKeepAlives will detect communication errors.  If BrowserManager is stuck running a test, then presumably the JUnitShell that opened it will timeout the test after 20 minutes and kill the test, so I think we're covered all around (can't start, communication fails, hang on server).

Ok, LGTM.