Consolidate waitForCondition implementations and switch to using TestUtils.waitForCondition

ASSIGNED
Assigned to

Status

defect
ASSIGNED
6 years ago
2 years ago

People

(Reporter: MattN, Assigned: ablayelyfondou, Mentored)

Tracking

unspecified
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good first bug], )

Attachments

(2 attachments, 9 obsolete attachments)

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.
Whiteboard: [good first bug][mentor=jaws][lang=js]
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?
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
I would like to work on this bug. It would be great if someone could assign it to me.
Assignee: nobody → sammygamer
Posted patch Partial bug fix (obsolete) — Splinter Review
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
Attachment #8379174 - Flags: review?(jaws)
Whoops, wrong platform. Try is acting strange, I'll try to push this later.
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
Posted patch Fixed the errors (obsolete) — Splinter Review
Attachment #8380555 - Flags: review?(jaws)
Attachment #8380233 - Attachment is obsolete: true
Attachment #8380555 - Attachment is obsolete: true
Attachment #8380233 - Flags: review?(jaws)
Attachment #8380555 - Flags: review?(jaws)
Posted patch Bug fix (obsolete) — Splinter Review
Some changes were made which earlier caused errors in mochitest-plain
Attachment #8381354 - Flags: review?(jaws)
Attachment #8380638 - Attachment is obsolete: true
Attachment #8380638 - Flags: review?(jaws)
Posted patch Fixed the errors (obsolete) — Splinter Review
The errors were fixed and tested locally.
Attachment #8382469 - Flags: review?(jaws)
Attachment #8382469 - Attachment is obsolete: true
Attachment #8382469 - Flags: review?(jaws)
Attachment #8383076 - Flags: review?(jaws)
Comment on attachment 8381354 [details] [diff] [review]
Bug fix

Please mark out-of-date patches as obsolete when uploading a new patch.
Attachment #8381354 - Attachment is obsolete: true
Attachment #8381354 - Flags: review?(jaws)
Posted patch Fixed metro errors (obsolete) — Splinter Review
Attachment #8383076 - Attachment is obsolete: true
Attachment #8383076 - Flags: review?(jaws)
Attachment #8383334 - Flags: review?(jaws)
(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
Attachment #8383334 - Attachment is obsolete: true
Attachment #8383334 - Flags: review?(jaws)
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
Hello Sambuddha, what's the current status of this? Are you wanting review? How were the try results?
Flags: needinfo?(sammygamer)
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.
Flags: needinfo?(sammygamer)
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.
The patch solves the bug partially.
Attachment #8440585 - Flags: review?(jaws)
Mentor: jaws
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][lang=js]
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.
Attachment #8440585 - Flags: review?(jaws) → feedback+
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!
Flags: needinfo?(sammygamer)
I was away for most of the days. I am back and will be looking at fixing this bug shortly.
Flags: needinfo?(sammygamer)
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!
Whiteboard: [good first bug][lang=js] → [lang=js]
Assignee: sammygamer → nobody
Status: ASSIGNED → NEW
Flags: firefox-backlog+
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.
Blocks: 1263723
(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.
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.
No longer blocks: 1263723
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.
Whiteboard: [lang=js] → [lang=js][good first bug]
(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.
Hi :jaws, can I work on this bug?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
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
Flags: needinfo?(jaws)
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;
> }
Flags: needinfo?(jaws)
Summary: Consolidate waitForCondition implementations in SimpleTest → Consolidate waitForCondition implementations and switch to using BrowserTestUtils.waitForCondition
(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
Okay, we could make moving that as part of this bug. But those should be separate commits to make reviewing easier.
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.
Summary: Consolidate waitForCondition implementations and switch to using BrowserTestUtils.waitForCondition → Consolidate waitForCondition implementations and switch to using TestUtils.waitForCondition
Abdoulaye, could you work on this?
Flags: needinfo?(ablayelyfondou)
Yes, you can assign me this.
Flags: needinfo?(ablayelyfondou) → needinfo?(jaws)
Assignee: nobody → ablayelyfondou
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.