940882 - Consolidate waitForCondition implementations and switch to using TestUtils.waitForCondition

Status: RESOLVED DUPLICATE of bug 1710901

(Testing :: Mochitest, defect)

Description

[Matthew N. [:MattN]]

There are multiple, sometimes differing, implementations of waitForCondition in the tree for specific tests and directories. These should be consolidated to reduce duplication and make it easier to be used in other tests.

Using waitForCondition is often used to avoid racy assumptions in tests so this will help reduce intermittent oranges.

Comment 1

[Katie Thomas]

Hey!  I'm wondering with MattN means by consolidated?  Should this function be defined in one place and all the other definitions removed and replaced by calling that one function?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Hi! I'll try to explain what's needed here, but hopefully it's not too long (or short!) of an explanation.

This search, http://mxr.mozilla.org/mozilla-central/search?string=function%20waitForCondition, shows all of the places that the waitForCondition function is currently defined. We will want to attempt to remove all of the duplicate implementations of waitForCondition in the *.js files (leaving the BaseTest.java implementation untouched).

There are roughly two types of waitForCondition implementations in the tree. We can take a multi-step approach to reducing these duplications.

The tests referenced in the above search are what we call mochitests. All mochitests include SimpleTest.js, which is what implements simple testing functions like is(), ok(), etc. We can add an implementation of waitForCondition to SimpleTest.js, preferable after the waitForClipboard function, at http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#724.

The trickiest part of fixing this bug is that not all of the implementations of waitForCondition are the same. All together, there is:

[1] function waitForCondition(condition, nextTest, errorMsg) 
[1] function waitForCondition(condition, nextTest, errorMsg, okMsg) 
[2] function waitForCondition(aConditionFn, aMaxTries=50, aCheckInterval=100)
[2] function waitForCondition(aCondition, aTimeoutMs, aIntervalMs)
[2] function waitForCondition2(aCondition, aTestMsg, aTimeoutMs, aIntervalMs)

I've numbered the functions based on how they operate. The functions with the [1] at the front of them execute the nextTest function argument when the condition passes or if a specified amount of time has expired. The functions with the [2] at the front of them return immediately, but return a Promise object. The caller of this function will use the 'yield' keyword to pause the script execution until the Promise is fulfilled (through either success or rejection).

For the first part of this bug, we can keep the two separate functions but move their implementations to SimpleTest.js. We should use the best implementation of each of these functions.

This is the best implementation for [1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/head.js#3
This is the best implementation for [2]: http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/head.js#453

After both of those implementations are added to SimpleTest.js, we will need to update the callers of them to make sure that the correct arguments are passed to the functions as some of them are different.

We can also modify the [2] function to use default arguments like in http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/head.js#245 (this is referring to the aMaxTries=50 and aCheckInterval=100). When default arguments are present, the value on the right-side of the equals sign will be used if a value is not specified.

Please let me know if there is anything that is confusing or something that I can expand on.

Comment 3

[Sambuddha Basu [:sambuddhabasu1]]

I would like to work on this bug. It would be great if someone could assign it to me.

Comment 4

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Partial bug fix

Functions replaced in the following files:
/browser/base/content/test/general/head.js
/browser/base/content/test/general/browser_bug906190.js
/browser/base/content/test/general/browser_no_mcb_on_http_site.js
/browser/base/content/test/social/head.js
/browser/modules/test/head.js

Comment 5

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: All functions included in SimpleTest.js

Comment 6

[Manish Goregaokar [:manishearth]]

Pushed to try for the metro tests, as per request in IRC.

https://tbpl.mozilla.org/?tree=Try&rev=f160e9eefcff

Comment 7

[Manish Goregaokar [:manishearth]]

Whoops, wrong platform. Try is acting strange, I'll try to push this later.

Comment 8

[Manish Goregaokar [:manishearth]]

Trypush: https://tbpl.mozilla.org/?tree=Try&rev=766c1fb6847e (the parent is here: https://hg.mozilla.org/try/rev/5465aade5358, I had to push twice)

Comment 9

[Manish Goregaokar [:manishearth]]

Looks like some of the relevant tests failed:

https://tbpl.mozilla.org/?tree=Try&rev=766c1fb6847e

(Be sure to check your diff against the pushed diff: https://hg.mozilla.org/try/rev/5465aade5358)

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochiperf/browser_tabs_01.js | runTests: Task failed - ReferenceError: waitForCondition is not defined at run@chrome://mochitests/content/metro/browser/metro/base/tests/mochiperf/browser_tabs_01.js:37:5 
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_inputs.js | runTests: Task failed - Error: Timed out waiting for 7000 at testCondition@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:808:7 
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_inputs.js | runTests: Task failed - Error: Timed out waiting for 7000 at testCondition@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:808:7 
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_textarea.js | runTests: Task failed - Error: Timed out waiting for 5000 at testCondition@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:808:7 
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_selection_urlbar.js | runTests: Task failed - Error: Timed out waiting for 5000 at testCondition@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:808:7

Comment 10

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Fixed the errors

Comment 11

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Errors were fixed. Partial bug fix.

Comment 12

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Bug fix

Some changes were made which earlier caused errors in mochitest-plain

Comment 13

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Fixed the errors

The errors were fixed and tested locally.

Comment 14

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Fixed the browser-chrome and metro errors

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8381354 [details] [diff] [review]
Bug fix

Please mark out-of-date patches as obsolete when uploading a new patch.

Comment 16

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Fixed metro errors

Comment 17

[Jonathan Griffin (:jgriffin)]

(In reply to Sambuddha Basu from comment #16)
> Created attachment 8383334 [details] [diff] [review]
> Fixed metro errors

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1879eccb8da9

Comment 18

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Some metro functions were changed

Comment 19

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Metro function call changed

Comment 20

[Sunny [:darkowlzz]]

Sambuddha came to #introduction asking for pushing this [1] to try. I have pushed it [2].

[1]: http://www.filedropper.com/bug940882
[2]: https://tbpl.mozilla.org/?tree=Try&rev=93f8843ed788

Comment 21

[Matthew N. [:MattN]]

Hello Sambuddha, what's the current status of this? Are you wanting review? How were the try results?

Comment 22

[Sambuddha Basu [:sambuddhabasu1]]

Hello Matthew, some of the tests were failing on try. However, there was a patch submitted by me, which changed only around half of the functions and ran good on try.

Comment 23

[Matthew N. [:MattN]]

Thanks for the update Sambuddha. If you have a patch which passes try, please request review on it and we may be able to check it in even if it doesn't cover every use. Getting an implementation of waitForCondition in SimpleTest will help new code.

Comment 24

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Consolidates waitForCondition implementations in SimpleTest

The patch solves the bug partially.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8440585 [details] [diff] [review]
Consolidates waitForCondition implementations in SimpleTest

Review of attachment 8440585 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. It looks like you have done an amazing amount of work here. Can you please break up this patch in to multiple parts though? There is too much going on here to be able to tell if everything was changed to use the right function. 

You could either split it up to make separate patches for each waitForCondition implementation or by source control directory.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +749,5 @@
>             wait(inputValidatorFn, aSuccessFn, aFailureFn, requestedFlavor);
>           }, aFailureFn, "text/unicode");
>  }
>  
> +SimpleTest.waitForConditionCallback = function (condition, nextTest, errorMsg, okMsg="") {

please change 'nextTest' to 'callback'

@@ +771,5 @@
> +  }, 100);
> +  var moveOn = function() { clearInterval(interval); nextTest(); };
> +}
> +
> +SimpleTest.waitForCondition1 = function (aConditionFn, aMaxTries=50, aCheckInterval=100) {

please change the name of this function to waitForConditionPromise, and it should be declared as a function*. See http://wiki.ecmascript.org/doku.php?id=harmony:generators#syntax for more information on generator functions and their syntax, if you're curious.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

Sambuddha, do you have enough information to go forward here?  Congrats on your feedback+ from :jaws!  I know he is out a lot over the next few weeks, so if you need any help, just ask!

Comment 27

[Sambuddha Basu [:sambuddhabasu1]]

I was away for most of the days. I am back and will be looking at fixing this bug shortly.

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

Hey Sambuddha, I am checking in with this bug to see if you have all the information you need.  Feel free to ask in the bug if you get stuck or need help. Also if you changed your mind and don't think you want to work on this, let us know and we can put it back in the pool of bugs for others to take!

Comment 29

[Luca Greco [:rpl] [:luca] [:lgreco]]

We are going to add one more mutation of this function (based on the one used in the places tests). 

Linking here the issue which is going to introduce the new mutation, because it has to be consolidated with the other similar test helpers as part of this issue.

Comment 30

[Matthew N. [:MattN]]

(In reply to Luca Greco [:rpl] from comment #29)
> We are going to add one more mutation of this function (based on the one
> used in the places tests). 
> 
> Linking here the issue which is going to introduce the new mutation, because
> it has to be consolidated with the other similar test helpers as part of
> this issue.

Mochitest browser-chrome tests should use `BrowserTestUtils.waitForCondition` nowadays.

Comment 31

[Luca Greco [:rpl] [:luca] [:lgreco]]

Removed Bug 1263723 from the issues blocked by this one, because we changed the test case to use the existing BrowserTestUtils.waitForCondition helper instead.

Thanks Matthew for pointing it out.

Comment 32

[Marco Bonardo [:mak]]

Actually, there are still a bunch of implementations of this:
http://searchfox.org/mozilla-central/search?q=function+waitForCondition&path=
The most problematic ones are the ones closer to the original one: (the link is mostly heuristic, I couldn't make a better search, could have false positives)
http://searchfox.org/mozilla-central/search?q=moveOn%28%29&path=

The problem is here:
    if (tries >= retryTimes) {
      ok(false, errorMsg);
      moveOn();
    }
after moveOn() we should return. Since we don't, we ask the test to continue and then run the conditional check. That ideally fails and should not create any problem, but if it throws (we may be past the test end) we could potentially call ok(false, e + "\n" + e.stack); out of the current subtest scope.
That may cause some of the "undefined assertion name" failures like Bug 1299484 (there are others around)

another problem is that if the condition fn throws, we don't clearInterval, so we will throw again, and again.

The BrowserTestUtils version doesn't have these issues, so it would be great to move consumers to it.

Note I don't think that fixing this will fix any intermittent, they will just report a better error message.

Comment 33

[Matthew N. [:MattN]]

(In reply to Marco Bonardo [::mak] from comment #32)
> The BrowserTestUtils version doesn't have these issues, so it would be great
> to move consumers to it.

I agree except that the method should move out of BrowserTestUtils to TestUtils.jsm since it's not specific to m-bc.

Comment 34

[Bao Quan [:beekill]]

Hi :jaws, can I work on this bug?

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bao Quan [:beekill] from comment #34)
> Hi :jaws, can I work on this bug?

Yes, you can work on the bug. I usually wait until a patch has been uploaded before assigning it.

Comment 36

[Bao Quan [:beekill]]

Hi Jared,

I'm not sure which one should I replace waitForCondition with SimpleTest.waitForCondition. Do all these files in [1] include SimpleTest? If so, can you show me how those files do it?

In the SimpleTest.js, there's a function promiseWaitForCondition [2]. Do I need to code up another function like you said on comment 25? In case of implementing another function, could you provide me with another implementation of the function as the link on comment 1 is dead?
> This is the best implementation for [2]: http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/head.js#453

[1]: https://dxr.mozilla.org/mozilla-central/search?q=function%20waitForCondition
[2]: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1066

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Sorry for the long delay in replying.

Since this bugs filing we now have BrowserTestUtils, which contains its own waitForCondition function which returns a promise. This should be available in all mochitest flavors. I would suggest trying to tackle this bug with separate commits per directory, as it will help with reviewing as well as making sure that tests are still passing.

In case you are wondering, I was able to dig through our source control history and find the implementation I was referring to in comment 2. Here it is:
> /**
>  * Waits a specified number of miliseconds for a supplied callback to
>  * return a truthy value.
>  *
>  * Usage:
>  *     let success = yield waitForCondition(myTestFunction);
>  *     if (success && !(success instanceof Error)) {
>  *       // ...
>  *     }
>  *
>  * @param aCondition the callback that must return a truthy value
>  * @param aTimeoutMs the number of miliseconds to wait before giving up
>  * @param aIntervalMs the number of miliseconds between calls to aCondition
>  * @returns a Promise that resolves to true, or to an Error
>  */
> function waitForCondition(aCondition, aTimeoutMs, aIntervalMs) {
>   let deferred = Promise.defer();
>   let timeoutMs = aTimeoutMs || kDefaultWait;
>   let intervalMs = aIntervalMs || kDefaultInterval;
>   let startTime = Date.now();
>   let stack = new Error().stack;
> 
>   function testCondition() {
>     let now = Date.now();
>     if((now - startTime) > timeoutMs) {
>       deferred.reject( new Error("Timed out waiting for condition to be true at " + stack) );
>       return;
>     }
> 
>     let condition;
>     try {
>       condition = aCondition();
>     } catch (e) {
>       deferred.reject( new Error("Got exception while attempting to test condition: " + e) );
>       return;
>     }
> 
>     if (condition) {
>       deferred.resolve(true);
>     } else {
>       setTimeout(testCondition, intervalMs);
>     }
>   }
> 
>   setTimeout(testCondition, 0);
>   return deferred.promise;
> }

Comment 38

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #37)
> Since this bugs filing we now have BrowserTestUtils, which contains its own
> waitForCondition function which returns a promise. This should be available
> in all mochitest flavors.

It's always bothered me that waitForCondition is in BrowserTestUtils since BrowserTestUtils is only supposed to be for browser-chrome-specific utilities and there's nothing b-c specific about waitForCondition. It should be moved to TestUtils.jsm: https://dxr.mozilla.org/mozilla-central/source/testing/modules/TestUtils.jsm

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

Okay, we could make moving that as part of this bug. But those should be separate commits to make reviewing easier.

Comment 40

[Marco Bonardo [:mak]]

I moved waitForCondition to TestUtils.jsm, leaving just a placeholder in BrowserTestUtils. Ideally this bug could also take care of removing the placeholder and changing the call points.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Abdoulaye, could you work on this?

Comment 42

[Abdoulaye O. LY]

Yes, you can assign me this.

Comment 43

[Mark Banner (:standard8)]

I've now filed two new meta bugs to track the work to remove the duplicated waitForCondition - bug 1710901 to consolidate everything, and bug 1710902 to do the BrowserTestUtils -> TestUtils migration.

From these we should be able to have cleaner mentored bugs and a better place to track it all, as it can't really be done in one chunk.

I'll mark as duplicate of bug 1710901.