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)
Toolkit
Places
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
Assignee | ||
Comment 1•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
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
Comment 6•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•