1132772 - afterCache and afterCommit should return Promise

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: wait_for_afterCache.patch

test_nocache.js can be failed because the test does not wait for afterCache() on search.init().

The failure can be easily reproduced with removing "firefox-appdir = browser" from toolkit/components/search/tests/xpcshell/xpcshell.ini.

The result:

./mach xpcshell-test toolkit/components/search/tests/xpcshell/test_nocache.js

<snip>
 0:01.11 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) NOTIFY: Engine: "Test search engine"; Verb: "engine-loaded""
 0:01.11 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) nsSearchService::observe: Done installation of Test search engine."
 0:01.11 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) _addEngineToStore: Adding engine: "Test search engine""
 0:01.11 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) NOTIFY: Engine: "Test search engine"; Verb: "engine-added""
 0:01.14 LOG: Thread-1 INFO "afterCache: write-cache-to-disk-complete"
 0:01.14 LOG: Thread-1 INFO "afterCache: write-cache-to-disk-complete"
 0:01.14 TEST_STATUS: Thread-1 test_nocache PASS [test_nocache : 31] true == true
 0:01.14 LOG: Thread-1 INFO "Searching test engine in cache"
 0:01.14 TEST_STATUS: Thread-1 test_nocache FAIL [test_nocache : 62] false == true
    /home/ikezoe/mozilla-central/obj-firefox/_tests/xpcshell/toolkit/components/search/tests/xpcshell/test_nocache.js:test_nocache:62
    self-hosted:next:610
    _run_next_test@/home/ikezoe/mozilla-central/testing/xpcshell/head.js:1375:9
    do_execute_soon/<.run@/home/ikezoe/mozilla-central/testing/xpcshell/head.js:644:9
    _do_main@/home/ikezoe/mozilla-central/testing/xpcshell/head.js:207:5
    _execute_test@/home/ikezoe/mozilla-central/testing/xpcshell/head.js:504:5
    @-e:1:1
 0:01.14 LOG: Thread-1 INFO exiting test
 0:01.14 LOG: Thread-1 ERROR Unexpected exception 2147500036

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Pushed a try now.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4a04691d886

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8563839 [details] [diff] [review]
wait_for_afterCache.patch

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

::: toolkit/components/search/tests/xpcshell/test_nocache.js
@@ +21,5 @@
>  
>  add_task(function* test_nocache() {
>    let search = Services.search;
>  
> +  let cachePromise = new Promise(resolve => afterCache(resolve));

let's rather make afterCache return a Promise, and the call points using the callback can use .then()

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: return_Promise.patch

Changed bug title.

afterCommit() is also changed to return a Promise.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7984f66dde3

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8564933 [details] [diff] [review]
return_Promise.patch

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

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +142,5 @@
>    return UTCOffset >= 150 && UTCOffset <= 600;
>  }
>  
>  /**
> + * Waits for being metadata committed.

s/being metadata/metadata being/

@@ +150,1 @@
>  {

while here please move brace in line with the function definition, per coding style

@@ +152,4 @@
>  }
>  
>  /**
> + * Waits for saving cache file.

Waits for the cache file to be saved.

::: toolkit/components/search/tests/xpcshell/test_nocache.js
@@ +34,1 @@
>      // Check that search.json has been created.

I'd prefer a more readable:

yield afterCachePromise;

// Check that search.json has...
...

@@ +43,5 @@
>      { name: "Test search engine", xmlFileName: "engine.xml" },
>    ]);
>  
>    do_print("Engine has been added, let's wait for the cache to be built");
> +  yield afterCachePromise;

Do we really need to store the promise in a temp var in this case?

The notification we wait for is notified on a promise resolution, so basically it should run one tick after us... If that would not be true, the previous code would have failed already.
I think this will just do:

yield promiseAfterCache();

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Marco Bonardo [::mak] -- currently sick :( (delayed answers) from comment #4)

> @@ +43,5 @@
> >      { name: "Test search engine", xmlFileName: "engine.xml" },
> >    ]);
> >  
> >    do_print("Engine has been added, let's wait for the cache to be built");
> > +  yield afterCachePromise;
> 
> Do we really need to store the promise in a temp var in this case?
> 
> The notification we wait for is notified on a promise resolution, so
> basically it should run one tick after us... If that would not be true, the
> previous code would have failed already.

Ah, right.
My intention was to avoid waiting for wrong notification as the previous code did. Actually the notification received after addTestEngines() in the previous code came from search.init(). But unfortunately the temp var does not avoid the notification mismatch at all!. The mismatch can be fixed by the yield after search.init() as attachment 8564933 [details] [diff] [review] does, not the temp var!

I will attach a revised patch.

Thanks Marco! Take care!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: return Promise v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4940a4a12f8a
There are a few build bustages unrelated this patch.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8570267 [details] [diff] [review]
return Promise v2

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

Taking over for Marco. LGTM with the one comment corrected. Thanks!

::: toolkit/components/search/tests/xpcshell/test_nocache.js
@@ +28,5 @@
>    yield new Promise((resolve, reject) => search.init(rv => {
>      Components.isSuccessCode(rv) ? resolve() : reject();
>    }));
>  
> +  // Check that cache is created at startup

Nit: // Check that the cache is created at startup.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: return Promise v3

Thanks Tim!
Carrying over review+.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/97f74a12b427

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97f74a12b427