1377600 - browser_bug581253.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:

 3004 23:05:36 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug581253.js | the bookmark for the test url has been removed -
3009 23:05:36 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug581253.js | star button indicates that the bookmark has been removed - Got 1, expected 0

Comment 1

[Mark Banner (:standard8)]

This test is still failing post bug 1371677, but just needs a simple rewrite so that it can deal with async transactions properly.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1377600 - Rewrite browser_bug581253.js to handle be able to handle the new async PlacesTransactions UI.

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

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8885651 [details]
Bug 1377600 - Rewrite browser_bug581253.js to handle be able to handle the new async PlacesTransactions UI.

I just realised, I should stop this using the old transaction manager. I don't think it is actually necessary for the test from what I can tell - as we just need a bookmark to remove, we're not pre-loading the star UI as such.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8885651 [details]
Bug 1377600 - Rewrite browser_bug581253.js to handle be able to handle the new async PlacesTransactions UI.

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

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8885651 [details]
Bug 1377600 - Rewrite browser_bug581253.js to handle be able to handle the new async PlacesTransactions UI.

https://reviewboard.mozilla.org/r/156504/#review161602

::: browser/base/content/test/general/browser_bug581253.js:10
(Diff revision 2)
>  var testTag = "581253_tag";
> -var timerID = -1;
>  
> -function test() {
> -  registerCleanupFunction(function() {
> +add_task(async function test_remove_bookmark_with_tag_via_edit_bookmark() {
> +  registerCleanupFunction(async function() {
>      PlacesUtils.bookmarks.removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId);

can we use eraseEverything here? my only doubt is whether cleaning up the other roots may cause failures in the next tests...

::: browser/base/content/test/general/browser_bug581253.js:11
(Diff revision 2)
> -var timerID = -1;
>  
> -function test() {
> -  registerCleanupFunction(function() {
> +add_task(async function test_remove_bookmark_with_tag_via_edit_bookmark() {
> +  registerCleanupFunction(async function() {
>      PlacesUtils.bookmarks.removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId);
> -    if (timerID > 0) {
> +    gBrowser.removeCurrentTab();

I'd suggest to use await BrowserTestUtils.removeTab, so in the future it may be easier to fix only one code point.

::: browser/base/content/test/general/browser_bug581253.js:52
(Diff revision 2)
> -}
>  
> -function onPanelHidden(aEvent) {
> -  if (aEvent.target == StarUI.panel) {
> -    StarUI.panel.removeEventListener("popuphidden", arguments.callee);
> -
> +  await popupHiddenPromise;
> +  await BrowserTestUtils.waitForCondition(async () => {
> +    return !(await PlacesUtils.bookmarks.fetch({url: testURL}));
> +  }, "the bookmark for the test url has been removed");

I'd prefer waitForNotification for onItemRemoved

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8885651 [details]
Bug 1377600 - Rewrite browser_bug581253.js to handle be able to handle the new async PlacesTransactions UI.

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

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8885651 [details]
Bug 1377600 - Rewrite browser_bug581253.js to handle be able to handle the new async PlacesTransactions UI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156504/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a8d5e183e80
Rewrite browser_bug581253.js to handle be able to handle the new async PlacesTransactions UI. r=mak

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/7a8d5e183e80