Last Comment Bug 852411 - Use the new "Promise.jsm" implementation in Firefox Health Report
: Use the new "Promise.jsm" implementation in Firefox Health Report
Status: RESOLVED FIXED
:
Product: Firefox Health Report
Classification: Client Software
Component: Client: Desktop (show other bugs)
: unspecified
: All All
: P4 normal
: Firefox 24
Assigned To: Stefan Mirea [:smirea]
:
:
Mentors:
Depends on: 810490 845842
Blocks: 996671
  Show dependency treegraph
 
Reported: 2013-03-18 20:41 PDT by Richard Newman [:rnewman]
Modified: 2014-04-15 09:18 PDT (History)
10 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Migrating from promise.js to Promise.jsm; (38.72 KB, patch)
2013-06-12 17:24 PDT, Stefan Mirea [:smirea]
gps: feedback+
Details | Diff | Splinter Review
Migrating from promise.js to Promise.jsm; (40.66 KB, patch)
2013-06-13 18:39 PDT, Stefan Mirea [:smirea]
gps: review+
Details | Diff | Splinter Review

Description Richard Newman [:rnewman] 2013-03-18 20:41:29 PDT
Follow-up from hack in Bug 845842.
Comment 1 :Paolo Amadini 2013-06-09 03:36:48 PDT
Now that bug 810490 landed, depending on whether the FHR client code has
dependencies on the helper functions in the Add-on SDK module "promise.js",
it can be switched to use the "Promise.jsm" module directly, instead of blocking
the improvements to "promise.js" in bug 881047.

In either case, the work here consists in figuring out which tests start to fail
because of the new delayed behavior compliant with "Promises/A+". The following
is an example already reported in bug 810490.

Various events occur that cause data to be reported, and tests may wait for the
same events (for instance, a DOM mutation or other DOM event). But FHR writes
data in the background, and we can't know when the operation completes - so,
tests use executeSoon after receiving the event that triggers FHR.

Fortunately, FHR has a write queue internally, thus if we change tests to trigger
a fake write after receiving the event, and wait for it, we know for sure that the
previous asynchronous write has completed and we can get a reliable result.
Comment 2 Stefan Mirea [:smirea] 2013-06-12 17:24:18 PDT
Created attachment 761807 [details] [diff] [review]
Migrating from promise.js to Promise.jsm;

Migrated the services/ from using the old "resource://gre/modules/commonjs/sdk/core/promise.js" to "resource://gre/modules/Promise.jsm". As the API interfaces were mostly the same, for the most part, no
 other changes had to be made other than simply replacing the include path from one to the other.

However, in "services/datareporting/tests/xpcshell/test_policy.js" several tests started to fail due to the new delayed behaviour compliant with "Promises/A+" which caused consequent functions to be evaluated before the Promise was resolved thus resulting in several undefined attribute errors.

To fix this, all methods in the "/services/datareporting/policy.js" file that used promises were modified to also return the promise. As a result, all the tests that failed due to this asynchronicity issue were changed into tasks and all their containing promise function calls were yielded, forcing a synchronus test
Comment 3 Gregory Szorc [:gps] 2013-06-12 17:49:50 PDT
Comment on attachment 761807 [details] [diff] [review]
Migrating from promise.js to Promise.jsm;

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

I want to r+ this. However, there are some mochitest browser chrome failures from this change:

browser/base/content/test/browser_urlbar_search_healthreport.js
browser/components/search/test/browser_healthreport.js
browser/base/content/test/browser_datareporting_notification.js

And possibly more. IIRC these tests rely on executeSoon() (wait for next tick in event loop). Since promises are now delayed by another tick, well, yeah.

You can run these tests locally via e.g.:

mach mochitest-browser browser/base/content/test/browser_urlbar_search_healthreport.js

I pushed a try build to https://tbpl.mozilla.org/?tree=Try&rev=ab7bd4a344ae. It should have a comprehensive list of test failures in a few hours.

::: services/datareporting/policy.jsm
@@ +94,5 @@
>     * @param error
>     *        (Error) Explains what went wrong.
>     */
>    onUserNotifyFailed: function onUserNotifyFailed(error) {
> +    this.deferred.reject(error);

I'm kinda surprised we didn't need to return (and yield) the promise here! Perhaps it points to a lack of test coverage? I don't care too much since this code is slated for removal in bug 862563.
Comment 4 Stefan Mirea [:smirea] 2013-06-13 18:39:58 PDT
Created attachment 762443 [details] [diff] [review]
Migrating from promise.js to Promise.jsm;

Fixed the test failures by updating "services/datareporting/policy.jsm" to use Promises.jsm and by removing one redundant test in "browser/base/content/test/browser_datareporting_notification.js"
Comment 5 Gregory Szorc [:gps] 2013-06-14 09:22:34 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=5dad7bad9013
Comment 6 Gregory Szorc [:gps] 2013-06-14 11:48:27 PDT
Looks like that try still has 1 bc failure :(

I'll look into this for you since you can't reproduce it locally.
Comment 7 Gregory Szorc [:gps] 2013-06-14 11:57:49 PDT
Comment on attachment 762443 [details] [diff] [review]
Migrating from promise.js to Promise.jsm;

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

Looks good. I was able to reproduce the remaining test failure locally and was able to code up a trivial fix (new executeSoon, sadly) to cause it to pass.

I'll check this in for you (after lunch).

::: browser/base/content/test/browser_datareporting_notification.js
@@ -22,5 @@
>    is(policy.notifyState, policy.STATE_NOTIFY_UNNOTIFIED, "Policy is in unnotified state.");
>  
>    service.healthReporter.onInit().then(function onInit() {
>      is(policy.ensureNotifyResponse(new Date()), false, "User has not responded to policy.");
> -    is(policy.notifyState, policy.STATE_NOTIFY_WAIT, "Policy is waiting for notification response.");

Just so it's documented somewhere, I suggested this verbally to Stefan.

This check is excessive and redundant with existing xpcshell tests. It was put in here for good measure, but it isn't strictly required

Making this work would require exposing promises from the bowels of FHR into mochitests. It's not worth the effort, IMO.

::: browser/base/content/test/browser_urlbar_search_healthreport.js
@@ +59,5 @@
>            is(newCount, oldCount + 1, "Exactly one search has been recorded.");
>            finish();
>          });
> +      })
> +      )

The formatting is off here. We want indentation after every { and }. I'll fix this for you and show you what I did.

::: services/datareporting/policy.jsm
@@ +158,5 @@
>     */
>    onNoDataAvailable: function onNoDataAvailable() {
>      this.state = this.NO_DATA_AVAILABLE;
>      this.promise.resolve(this);
> +    return this.promise.promise;

We should consider renaming this.promise to this.deferred as well, but meh.
Comment 9 Gregory Szorc [:gps] 2013-06-14 14:40:24 PDT
And backed out for causing some xpcshell tests to not run test functions. I suspect the cause has to do with add_task() test functions not being registered until a later tick. Will file a new bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/86155a5809da
Comment 10 Gregory Szorc [:gps] 2013-06-14 14:58:03 PDT
Fixed. Turns out run_test() is expected to run synchronously. We had a promise in there before and it worked because it was being resolved on the same tick. Chalk up something else that would be detected if we "fuzzed" when promises are resolved.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b384740198c
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-06-14 18:53:12 PDT
https://hg.mozilla.org/mozilla-central/rev/2b384740198c

Note You need to log in before you can comment on or make changes to this bug.