567098 - Remove setTimeout's in autofocus tests

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Some autofocus tests need to use setTimeout to be sure an event isn't sent but it's sometimes used when it's not really needed.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This patch is removing a setTimeout (in test-5) and tries to explain in comments why the other setTimeout's are kept. I've also changed the setTimeout value to always wait 2 secs when we expect something will not be focused (it was 1 sec sometimes).

Let me know if you see anything that could be improved.

Comment 2

[(no longer active)]

What makes you need timeouts?  What would you want from the platform/test harness so that you wouldn't have needed these timeouts at all?

And how did you come up with the 2 second value?  Why is 1 second too short?  Why is 3 seconds too long?

Not trying to be picky, but my experience with this type of timeouts is that they almost always result in random oranges when the test machines are under heavy load, and that leads to sorrow for everyone.

Comment 3

[(no longer active)]

In response to comment 2, would you mind discussing each test separately?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Like I said elsewhere, trying to determine that an asynchronously executing action has *not* happened is currently very hard.

What some of the current tests do (at least at some point when I was reviewing them. There were many revisions), is to simply wait a bit and then check that the action wasn't taken.

I don't think there is a risk of random oranges here. There is however some risk that if we break things, such that the action is taken, then we could end up with random greens.

I guess you could do a couple of rounds of executeSoon, but it seems to me that that isn't a whole lot better from a randomness point of view. It is better from the point of view of wanting tests to run fast though.

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> What makes you need timeouts?  What would you want from the platform/test
> harness so that you wouldn't have needed these timeouts at all?

We can't check for an event not sent. At best, the platform could tell us: "if an event has been sent at moment x, it should be received now, otherwise, it has not been sent.". Maybe adding another event to the event queue just after the autofocus will help us because if event y is received, autofocus has proceeded if it has been sent. But I'm wondering if such a mechanism you be reliable.

> And how did you come up with the 2 second value?  Why is 1 second too short? 
> Why is 3 seconds too long?

We need a value long enough to prevent false-positive and not too long for obvious reasons. 2 seconds is completely arbitrary.

> Not trying to be picky, but my experience with this type of timeouts is that
> they almost always result in random oranges when the test machines are under
> heavy load, and that leads to sorrow for everyone.

I understand.

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #3)
> In response to comment 2, would you mind discussing each test separately?

file_bug546995.html:
We want the second element to be autofocused but not the third one so we can't only rely on second element focused because we want third element not focused and we can't check that with an event.

test_bug546995-2.html:
We want to be sure nothing has been focused so we check if the body is still focused after some time... How could we do that ?

test_bug546995-3.html:
We want to be sure no element takes the focus in this test. We still can't rely on the focus event. Even the 'onload' event is not enough because the focus event could be treated after.

test_bug546995-4.html:
Same as test-3: we want to be sure the input is not focused when the focus is currently in the frame.

test_bug546995-5.html:
It wasn't needed, it is removed in this patch.

browser_autofocus_background.js:
Here, the setTimeout is probably the less needed one. It's used to be sure the second tab doesn't get the focus on the input for any reason. That's a way to compare: with autofocus, the input should be focused. Without, it should not. Maybe we could remove this tab then removing this setTimeout.

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 446511 [details] [diff] [review]
Patch v1

Let's have Ehsan reviewing if you see no problem Jonas.

Comment 8

[(no longer active)]

Comment on attachment 446511 [details] [diff] [review]
Patch v1

>diff --git a/content/html/content/test/file_bug546995.html b/content/html/content/test/file_bug546995.html
>--- a/content/html/content/test/file_bug546995.html
>+++ b/content/html/content/test/file_bug546995.html
>@@ -18,15 +18,19 @@
>     e = document.createElement(element);
>     e.autofocus = true;
>     c.appendChild(e)
>   </script>
> </div>
> 
> <script>
> function load() {
>-  // Our parent should have a look at us in 1 sec so focus events can be thrown.
>-  setTimeout("parent.gGen.next()", 1000);
>+  // Our parent should have a look at us two seconds after the frame is loaded.
>+  // We must wait because we expect an element with autofocus will send a focus
>+  // event and another will not. There is no other way than waiting and check
>+  // for the currently focused element when we expect an element will not have
>+  // the focus.
>+  setTimeout("parent.gGen.next()", 2000);

What you can do here is nest N calls to executeSoon (where N is the number of elements injected into the document, and then call parent.gGen.next.  This way you'll make sure that if an element with autofocus == false actually fires a focus event (which means something has regressed), that extra event would've been processed by the next time that the generator is run.

Also, may I suggest moving the DOM manipulations to after DOMContentLoaded?

> }
> </script>
> 
> </body>
> </html>
>diff --git a/content/html/content/test/test_bug546995-2.html b/content/html/content/test/test_bug546995-2.html
>--- a/content/html/content/test/test_bug546995-2.html
>+++ b/content/html/content/test/test_bug546995-2.html
>@@ -51,21 +51,24 @@ function runTests()
> 
>   // Adding elements with autofocus.
>   for each(element in elements) {
>     var e = document.createElement(element);
>     e.autofocus = true;
>     document.getElementById('content').appendChild(e);
>   }
> 
>-  // The element should still be focused. Waiting a second to be sure.
>+  // The element should still be focused. Waiting two seconds to be sure.
>+  // We must wait because we expect the autofocus will not send a focus event
>+  // and there is no other way than waiting and check if the body still have
>+  // the focus.
>   setTimeout(function() {
>-               ok(document.activeElement, document.body,
>-                  "After loading, elements can't be autofocused");
>-               SimpleTest.finish();
>-             }, 1000);
>+    ok(document.activeElement, document.body,
>+       "After loading, elements can't be autofocused");
>+    SimpleTest.finish();
>+    }, 2000)

Again, nest N calls to executeSoon, and make your checks in the innermost, like above.

Nit: trailing semicolon for the setTimeout function (even though it's going away in the next revision of the patch!)

>   yield;
> }
> 
> </script>
> </pre>
> </body>
> </html>
>diff --git a/content/html/content/test/test_bug546995-3.html b/content/html/content/test/test_bug546995-3.html
>--- a/content/html/content/test/test_bug546995-3.html
>+++ b/content/html/content/test/test_bug546995-3.html
>@@ -5,21 +5,20 @@ https://bugzilla.mozilla.org/show_bug.cg
> -->
> <head>
>   <title>Test for Bug 546995</title>
>   <script type="application/javascript" src="/MochiKit/packed.js"></script>
>   <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> </head>
> <!-- When the page is loaded, we wait 2 seconds before launching the tests
>-     because the focus events may not be finished when the page is loaded so
>-     we have to wait or launch the tests when a focus events is thrown.
>-     However, if autofocus is ignored, there will be no focus event so we can't
>-     rely on that. -->
>-<body onload="window.setTimeout(runTests, 2000);">
>+     because we expect the autofocus will not send a focus event and there
>+     is no other way to check that than waiting some time and check if the
>+     elements have been focused. -->
>+<body onload="setTimeout(runTests, 2000);">

Again, nest the correct number of executeSoon calls (based on the number of autofocus elements in the document) when you get the load event, and then call runTests.

> <script type="application/javascript">
>   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>   var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>             .getService(Components.interfaces.nsIPrefBranch);
>   var gAutofocusPref = prefs.getBoolPref("browser.autofocus");
>   prefs.setBoolPref("browser.autofocus", false);
> 
>   var gFocusHandled = false;
>diff --git a/content/html/content/test/test_bug546995-4.html b/content/html/content/test/test_bug546995-4.html
>--- a/content/html/content/test/test_bug546995-4.html
>+++ b/content/html/content/test/test_bug546995-4.html

The timeouts in this test are already removed on mozilla-central.

>diff --git a/content/html/content/test/test_bug546995-5.html b/content/html/content/test/test_bug546995-5.html
>--- a/content/html/content/test/test_bug546995-5.html
>+++ b/content/html/content/test/test_bug546995-5.html
>@@ -4,43 +4,37 @@
> https://bugzilla.mozilla.org/show_bug.cgi?id=546995
> -->
> <head>
>   <title>Test for Bug 546995</title>
>   <script type="application/javascript" src="/MochiKit/packed.js"></script>
>   <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> </head>
>-<!-- When the page is loaded, we wait 2 seconds before launching the tests
>-     because the focus events may not be finished when the page is loaded so
>-     we have to wait or launch the tests when a focus events is thrown.
>-     However, if autofocus is ignored, there will be no focus event so we can't
>-     rely on that. -->
>-<body onload="setTimeout(runTests, 2000);">
>+<body>
> <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=546995">Mozilla Bug 546995</a>
> <p id="display"></p>
> <script type="application/javascript">
>   document.body.focus();
>   is(document.activeElement, document.body,
>-     "The document body should have te focus");
>+     "The document body should have the focus");
> </script>
> <div id='content'>
>   <input id='i' autofocus>
> </div>
> <pre id="test">
> <script type="application/javascript;version=1.8">
> 
> /** Test for Bug 546995 **/
> 
> SimpleTest.waitForExplicitFinish();
> 
>-function runTests()
>-{
>-  is(document.activeElement, document.getElementById('i'),
>-     "The input element should be focused");
>+document.getElementById('i').addEventListener("focus", function(aEvent) {
>+  aEvent.target.removeEventListener("focus", arguments.callee, false);
> 
>+  ok(true, "The input element has been focused");
>   SimpleTest.finish();
>-}
>+  }, false);

Sweet!

> 
> </script>
> </pre>
> </body>
> </html>
>diff --git a/dom/tests/browser/browser_autofocus_background.js b/dom/tests/browser/browser_autofocus_background.js
>--- a/dom/tests/browser/browser_autofocus_background.js
>+++ b/dom/tests/browser/browser_autofocus_background.js
>@@ -30,18 +30,22 @@ function test() {
>     // Now load the second tab on background.
>     tabs[2].linkedBrowser.addEventListener("load", onLoadBackgroundSecondTab, true);
>     tabs[2].linkedBrowser.loadURI(testingList[1].uri);
>   }
> 
>   function onLoadBackgroundSecondTab() {
>     tabs[2].linkedBrowser.removeEventListener("load", onLoadBackgroundSecondTab, true);
> 
>-    // Wait a second to be sure all focus events are done before launching tests.
>-    setTimeout(doTest, 1000);
>+    // Wait two second to be sure all focus events are done before testing.
>+    // For the first tab, we could check for the focus event because it has to
>+    // be sent but for the second tab, there should not bo a focus event on the
>+    // input element so we can't do anything else than waiting and check which
>+    // element in focused in each tab.
>+    setTimeout(doTest, 2000);

Again, nest the correct number of executeSoon calls here (which seems to be one, because you're only waiting for 1 possible autofocus event, right?).

>   }
> 
>   function doTest() {
>     for (var i=0; i<testingList.length; ++i) {
>       // Get active element in the tab.
>       var e = tabs[i+1].linkedBrowser.contentDocument.activeElement;
> 
>       is(e.tagName, testingList[i].tagName,

Also, what is the purpose of the last addTab call in <http://mxr.mozilla.org/mozilla-central/source/dom/tests/browser/browser_autofocus_background.js#56>?  It should be removed, unless you can present a compelling reason why not!  ;-)

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

I hope I'm using correctly |executeSoon| in this patch.

Comment 10

[(no longer active)]

Comment on attachment 447133 [details] [diff] [review]
Patch v2

>diff --git a/content/html/content/test/file_bug546995.html b/content/html/content/test/file_bug546995.html
>--- a/content/html/content/test/file_bug546995.html
>+++ b/content/html/content/test/file_bug546995.html
>@@ -1,10 +1,13 @@
> <!DOCTYPE HTML>
> <html>
>+<head>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+</head>

I'm not sure what the implications of pulling SimpleTest.js in here are.  Can you remove this and just use setTimeout(..., 0) here (which basically does the same thing)?

> <body onload="load();">
> <div id="content">
>   <script>
>     var c = document.getElementById('content');
>     var element = parent.gElement;
> 
>     var e = document.createElement(element);
>     e.autofocus = false;
>@@ -18,15 +21,16 @@
>     e = document.createElement(element);
>     e.autofocus = true;
>     c.appendChild(e)
>   </script>
> </div>
> 
> <script>
> function load() {
>-  // Our parent should have a look at us in 1 sec so focus events can be thrown.
>-  setTimeout("parent.gGen.next()", 1000);
>+  // When the function will be executed, the focus events from autofocus should
>+  // have been processed.
>+  SimpleTest.executeSoon(parent.gGen.next());
> }
> </script>
> 
> </body>
> </html>
>diff --git a/content/html/content/test/test_bug546995-2.html b/content/html/content/test/test_bug546995-2.html
>--- a/content/html/content/test/test_bug546995-2.html
>+++ b/content/html/content/test/test_bug546995-2.html
>@@ -51,21 +51,22 @@ function runTests()
> 
>   // Adding elements with autofocus.
>   for each(element in elements) {
>     var e = document.createElement(element);
>     e.autofocus = true;
>     document.getElementById('content').appendChild(e);
>   }
> 
>-  // The element should still be focused. Waiting a second to be sure.
>-  setTimeout(function() {
>-               ok(document.activeElement, document.body,
>-                  "After loading, elements can't be autofocused");
>-               SimpleTest.finish();
>-             }, 1000);
>+  // When the function will be executed, the focus event from autofocus should
>+  // have been processed.
>+  SimpleTest.executeSoon(function() {
>+    ok(document.activeElement, document.body,
>+       "After loading, elements can't be autofocused");
>+    SimpleTest.finish();
>+    });

Why is one executeSoon enough here?  If I'm reading the code, you're injecting N elements (N == elements.length) with autofocus, and then you're calling executeSoon, which causes the function to be executed only after the first autofocus event handler being processed.

>   yield;
> }
> 
> </script>
> </pre>
> </body>
> </html>
>diff --git a/content/html/content/test/test_bug546995-3.html b/content/html/content/test/test_bug546995-3.html
>--- a/content/html/content/test/test_bug546995-3.html
>+++ b/content/html/content/test/test_bug546995-3.html
>@@ -4,22 +4,17 @@
> https://bugzilla.mozilla.org/show_bug.cgi?id=546995
> -->
> <head>
>   <title>Test for Bug 546995</title>
>   <script type="application/javascript" src="/MochiKit/packed.js"></script>
>   <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> </head>
>-<!-- When the page is loaded, we wait 2 seconds before launching the tests
>-     because the focus events may not be finished when the page is loaded so
>-     we have to wait or launch the tests when a focus events is thrown.
>-     However, if autofocus is ignored, there will be no focus event so we can't
>-     rely on that. -->
>-<body onload="window.setTimeout(runTests, 2000);">
>+<body onload="SimpleTest.executeSoon(runTests);">

This looks good.

> <script type="application/javascript">
>   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>   var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>             .getService(Components.interfaces.nsIPrefBranch);
>   var gAutofocusPref = prefs.getBoolPref("browser.autofocus");
>   prefs.setBoolPref("browser.autofocus", false);
> 
>   var gFocusHandled = false;
>diff --git a/content/html/content/test/test_bug546995-5.html b/content/html/content/test/test_bug546995-5.html
>--- a/content/html/content/test/test_bug546995-5.html
>+++ b/content/html/content/test/test_bug546995-5.html
>@@ -4,43 +4,37 @@
> https://bugzilla.mozilla.org/show_bug.cgi?id=546995
> -->
> <head>
>   <title>Test for Bug 546995</title>
>   <script type="application/javascript" src="/MochiKit/packed.js"></script>
>   <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> </head>
>-<!-- When the page is loaded, we wait 2 seconds before launching the tests
>-     because the focus events may not be finished when the page is loaded so
>-     we have to wait or launch the tests when a focus events is thrown.
>-     However, if autofocus is ignored, there will be no focus event so we can't
>-     rely on that. -->
>-<body onload="setTimeout(runTests, 2000);">
>+<body>
> <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=546995">Mozilla Bug 546995</a>
> <p id="display"></p>
> <script type="application/javascript">
>   document.body.focus();
>   is(document.activeElement, document.body,
>-     "The document body should have te focus");
>+     "The document body should have the focus");
> </script>
> <div id='content'>
>   <input id='i' autofocus>
> </div>
> <pre id="test">
> <script type="application/javascript;version=1.8">
> 
> /** Test for Bug 546995 **/
> 
> SimpleTest.waitForExplicitFinish();
> 
>-function runTests()
>-{
>-  is(document.activeElement, document.getElementById('i'),
>-     "The input element should be focused");
>+document.getElementById('i').addEventListener("focus", function(aEvent) {
>+  aEvent.target.removeEventListener("focus", arguments.callee, false);
> 
>+  ok(true, "The input element has been focused");
>   SimpleTest.finish();
>-}
>+  }, false);

This also looks good.

> </script>
> </pre>
> </body>
> </html>
>diff --git a/dom/tests/browser/browser_autofocus_background.js b/dom/tests/browser/browser_autofocus_background.js
>--- a/dom/tests/browser/browser_autofocus_background.js
>+++ b/dom/tests/browser/browser_autofocus_background.js
>@@ -1,47 +1,39 @@
> function test() {
>   waitForExplicitFinish();
> 
>   let fm = Components.classes["@mozilla.org/focus-manager;1"]
>                      .getService(Components.interfaces.nsIFocusManager);
> 
>-  let tabs = [ gBrowser.selectedTab, gBrowser.addTab(), gBrowser.addTab() ];
>+  let tabs = [ gBrowser.selectedTab, gBrowser.addTab() ];
> 
>   // The first tab has an autofocused element.
>   // The second tab is exactly like the first one without the autofocus.
>   let testingList = [
>     { uri: "data:text/html,<!DOCTYPE html><html><body><input autofocus id='target'></body></html>",
>       tagName: "INPUT"},
>-    { uri: "data:text/html,<!DOCTYPE html><html><body><input id='target'></body></html>",
>-      tagName: "BODY"}
>   ];
> 
>   function runTest() {
>     // Set the focus to the first tab.
>     tabs[0].linkedBrowser.focus();
> 
>     // Load the first tab on background.
>-    tabs[1].linkedBrowser.addEventListener("load", onLoadBackgroundFirstTab, true);
>+    tabs[1].linkedBrowser.addEventListener("load", onLoadBackgroundTab, true);
>     tabs[1].linkedBrowser.loadURI(testingList[0].uri);
>   }
> 
>-  function onLoadBackgroundFirstTab() {
>-    tabs[1].linkedBrowser.removeEventListener("load", onLoadBackgroundFirstTab, true);
>+  function onLoadBackgroundTab() {
>+    tabs[1].linkedBrowser.removeEventListener("load", onLoadBackgroundTab, true);
> 
>-    // Now load the second tab on background.
>-    tabs[2].linkedBrowser.addEventListener("load", onLoadBackgroundSecondTab, true);
>-    tabs[2].linkedBrowser.loadURI(testingList[1].uri);
>-  }
>-
>-  function onLoadBackgroundSecondTab() {
>-    tabs[2].linkedBrowser.removeEventListener("load", onLoadBackgroundSecondTab, true);
>-
>-    // Wait a second to be sure all focus events are done before launching tests.
>-    setTimeout(doTest, 1000);
>+    // The focus event (from autofocus) has been sent,
>+    // let's test with executeSoon so we are sure the test is done
>+    // after the focus event is processed.
>+    executeSoon(doTest);

This also looks good.

BTW, my previous comment on this file still applies! :-)

> Also, what is the purpose of the last addTab call in
> <http://mxr.mozilla.org/mozilla-central/source/dom/tests/browser/browser_autofocus_background.js#56>?
>  It should be removed, unless you can present a compelling reason why not!  ;-)

r- based on the above for now.

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

Comment 12

[(no longer active)]

(In reply to comment #10)
> >-  // The element should still be focused. Waiting a second to be sure.
> >-  setTimeout(function() {
> >-               ok(document.activeElement, document.body,
> >-                  "After loading, elements can't be autofocused");
> >-               SimpleTest.finish();
> >-             }, 1000);
> >+  // When the function will be executed, the focus event from autofocus should
> >+  // have been processed.
> >+  SimpleTest.executeSoon(function() {
> >+    ok(document.activeElement, document.body,
> >+       "After loading, elements can't be autofocused");
> >+    SimpleTest.finish();
> >+    });
> 
> Why is one executeSoon enough here?  If I'm reading the code, you're injecting
> N elements (N == elements.length) with autofocus, and then you're calling
> executeSoon, which causes the function to be executed only after the first
> autofocus event handler being processed.

Clarification: this was my mistake.  In fact, one executeSoon call here is just fine, because the dispatched event will be processed after all the dispatched autofocus events sent when binding the elements to tree.

Comment 13

[(no longer active)]

Comment on attachment 447557 [details] [diff] [review]
Patch v2.1

r=me.  Thanks for your patch!

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/09eee26d0124