1378132 - browser_bookmarkProperties_addKeywordForThisSearch.js fails with async places transactions turned on

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mark Banner (:standard8)]

This test fails when browser.places.useAsyncTransactions is set to true:

1579 INFO TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_bookmarkProperties_addKeywordForThisSearch.js | undefined assertion name - Got , expected kw
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:967
chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_bookmarkProperties_addKeywordForThisSearch.js:reopen_same_field/</<:72
chrome://mochitests/content/browser/browser/components/places/tests/browser/head.js:withBookmarksDialog:321

Comment 1

[Mark Banner (:standard8)]

The issue here is that now we're async, we aren't waiting for the keyword to be added. We also need to make sure that the close & update is complete before we move onto the next test.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1378132 - Update browser_bookmarkProperties_addKeywordForThisSearch.js to wait for keywords in an async situation, also ensure the remove notifications happen before finishing the test.

Review commit: https://reviewboard.mozilla.org/r/154210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154210/

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8883301 [details]
Bug 1378132 - Update browser_bookmarkProperties_addKeywordForThisSearch.js to wait for keywords in an async situation, also ensure the remove notifications happen before finishing the test.

https://reviewboard.mozilla.org/r/154210/#review159342

::: browser/components/places/tests/browser/browser_bookmarkProperties_addKeywordForThisSearch.js:8
(Diff revision 1)
>  const TEST_URL = "http://mochi.test:8888/browser/browser/components/places/tests/browser/keyword_form.html";
>  
> +function closeHandler(dialogWin) {
> +  let savedItemId = dialogWin.gEditItemOverlay.itemId;
> +  return promiseBookmarksNotification("onItemRemoved",
> +                                      itemId => itemId === savedItemId);

Would be great if this would use the new PlacesTestUtils.waitForNotification, that I'm adding in bug 1376929.

We should probably also file a mentored bug to replace existing exotic systems with that.

::: browser/components/places/tests/browser/head.js:277
(Diff revision 1)
>   * @param openFn
>   *        generator function causing the dialog to open
> - * @param task
> + * @param taskFn
>   *        the task to execute once the dialog is open
> + * @param closeFn
> + *        the task to execute when the dialog is closing

I think the comment should better explain that this is used in cases one wants to wait for something to happen before proceeding.
Something like "a function returning a promise, executed when the dialog is closing, to wait for pending work." but maybe written in proper English :)

Should also specify that it will take the dialog window handle as input.

::: browser/components/places/tests/browser/head.js:340
(Diff revision 1)
>        info("withBookmarksDialog: canceling the dialog");
> +
>        doc.documentElement.cancelDialog();
> +
> +      if (closePromise) {
> +        await closePromise;

Alternatively you could set closePromise defautl value to "() => Promise.resolve()" and always await it. Would remove some boilerplate code.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8883301 [details]
Bug 1378132 - Update browser_bookmarkProperties_addKeywordForThisSearch.js to wait for keywords in an async situation, also ensure the remove notifications happen before finishing the test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154210/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b25d1f1ad48a
Update browser_bookmarkProperties_addKeywordForThisSearch.js to wait for keywords in an async situation, also ensure the remove notifications happen before finishing the test. r=mak

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b25d1f1ad48a