Call webClient.closeAllWindows() in htmlunit code

[amitm...@google.com]

Reviewers: kathrin,

Description:
Call webClient.closeAllWindows() in htmlunit code when there are no
remaining JS tasks. We do not have to worry about infinite loop because
the test timeout will kill the process eventually.

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

Affected files:
user/src/com/google/gwt/junit/RunStyleHtmlUnit.java

Index: user/src/com/google/gwt/junit/RunStyleHtmlUnit.java
--- user/src/com/google/gwt/junit/RunStyleHtmlUnit.java (revision 5956)
+++ user/src/com/google/gwt/junit/RunStyleHtmlUnit.java (working copy)
@@ -48,6 +48,10 @@ public class RunStyleHtmlUnit extends RunStyleRemote {
protected static class HtmlUnitThread extends Thread implements
AlertHandler,
IncorrectnessListener, OnbeforeunloadHandler {

+ private static final int JAVASCRIPT_WAIT_TIME = 2000;
+
+ private static final int ACTIVE_JOBS_WAIT_TIME = 60000;
+
private final BrowserVersion browser;
private final String url;
private Object waitForUnload = new Object();
@@ -82,29 +86,36 @@ public class RunStyleHtmlUnit extends RunStyleRemote {
@Override
public void run() {
WebClient webClient = new WebClient(browser);
- webClient.setAlertHandler(this);
- webClient.setIncorrectnessListener(this);
- webClient.setThrowExceptionOnFailingStatusCode(false);
- webClient.setThrowExceptionOnScriptError(true);
- webClient.setOnbeforeunloadHandler(this);
- setupWebClient(webClient);
try {
+ webClient.setAlertHandler(this);
+ webClient.setIncorrectnessListener(this);
+ webClient.setThrowExceptionOnFailingStatusCode(false);
+ webClient.setThrowExceptionOnScriptError(true);
+ webClient.setOnbeforeunloadHandler(this);
+ setupWebClient(webClient);
Page page = webClient.getPage(url);
- // TODO(jat): is this necessary?
- webClient.waitForBackgroundJavaScriptStartingBefore(2000);
- page.getEnclosingWindow().getJobManager().waitForJobs(60000);
+ int count = 0;
+ while
((webClient.waitForBackgroundJavaScriptStartingBefore(JAVASCRIPT_WAIT_TIME))
> 0) {
+ treeLogger.log(TreeLogger.WARN, (++count)
+ + " waiting for background javascript for additional "
+ + JAVASCRIPT_WAIT_TIME + " ms");
+ }
+ count = 0;
+ while
((page.getEnclosingWindow().getJobManager().waitForJobs(ACTIVE_JOBS_WAIT_TIME))
> 0) {
+ treeLogger.log(TreeLogger.WARN, (++count)
+ + " waiting for javascript jobs for additional "
+ + ACTIVE_JOBS_WAIT_TIME + " ms");
+ }
treeLogger.log(TreeLogger.DEBUG, "getPage returned "
+ ((HtmlPage) page).asXml());
- // TODO(amitmanjhi): call webClient.closeAllWindows()
} catch (FailingHttpStatusCodeException e) {
treeLogger.log(TreeLogger.ERROR, "HTTP request failed", e);
- return;
} catch (MalformedURLException e) {
treeLogger.log(TreeLogger.ERROR, "Bad URL", e);
- return;
} catch (IOException e) {
treeLogger.log(TreeLogger.ERROR, "I/O error on HTTP request", e);
- return;
+ } finally {
+ webClient.closeAllWindows();
}
}

[kpr...@google.com]

Hi Amit,

I think this is the right direction! But I do think there's still a
problem here, if I read the HtmlUnit documentation correctly. It says:

-----
waitForBackgroundJavaScriptStartingBefore

This method blocks until all background JavaScript tasks scheduled
to start executing before (now + delayMillis) have finished executing.
Background JavaScript tasks are JavaScript tasks scheduled for execution
via window.setTimeout, window.setInterval or asynchronous
XMLHttpRequest.
...

Returns:
the number of background JavaScript jobs still executing or
waiting to be executed when this method returns; will be 0 if there are
no jobs left to execute
-----

Now what's happening here is that you wait x ms for the jobs to finish,
but then there will at least be 1 still running (because GWT apps have a
setTimeout(200?) that keeps setting a setTimeout(200) to check whether
the history has changed. So unless I'm missing something, every time
this method returns, it'll return a value > 0, which means you'll just
loop around until the test times out. Am I missing something?

http://gwt-code-reviews.appspot.com/62802/diff/1/2
File user/src/com/google/gwt/junit/RunStyleHtmlUnit.java (right):

http://gwt-code-reviews.appspot.com/62802/diff/1/2#newcode101
Line 101: + JAVASCRIPT_WAIT_TIME + " ms");
Rephrase a bit? Maybe say "Waiting for (++count) javascript jobs for
xxx ms"

http://gwt-code-reviews.appspot.com/62802

[kpr...@google.com]

In a discussion with Amit and Joel, we've determined that the tests that
this will apply to do not use History tokens, so the repeated timeout
won't apply. We couldn't think of any other cases where there would be
lingering JavaScript jobs. TODO - we should revisit this issue at some
point and come up with something more stable.

[Scott Blum]

Does JUnit's GWTRunner keep pinging the server on a timer to see if more tests are wanted?

[Amit Manjhi]

Not while a test is running. Only when a test or a block of test ends does the GWTRunner ping again.