Closed Bug 1378132 Opened 7 years ago Closed 7 years ago

browser_bookmarkProperties_addKeywordForThisSearch.js fails with async places transactions turned on

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

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
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 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.
Attachment #8883301 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com: 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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: