browser_bug581253.js fails with async places transactions turned on

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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
Depends on: 1371677
This test is still failing post bug 1371677, but just needs a simple rewrite so that it can deal with async transactions properly.
Comment hidden (mozreview-request)
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.
Attachment #8885651 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Assignee: nobody → standard8
Status: NEW → ASSIGNED

Comment 5

a year ago
mozreview-review
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
Attachment #8885651 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by mbanner@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7a8d5e183e80
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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.