1094846 - Convert xpcshell-tests in browser/components/places to Bookmarks.jsm API

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

convert tests to the new async API.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Reserving this for myself for the iteration that starts tomorrow.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1094846-Update-head_common.js.patch

Converting head_common.js to use the new Promise() API.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1094846-Convert-xpcshell-tests-in-browser-compon.patch

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8555873 [details] [diff] [review]
0001-Bug-1094846-Update-head_common.js.patch

Review of attachment 8555873 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/head_common.js
@@ +604,5 @@
>   *       serialized, the end of this write operation means that all writes are
>   *       complete.  Note that WAL makes so that writers don't block readers, but
>   *       this is a problem only across different connections.
>   */
>  function promiseAsyncUpdates()

I think this has just been moved to PlacesTestUtils so likely this part of the patch is no more needed

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8555877 [details] [diff] [review]
0002-Bug-1094846-Convert-xpcshell-tests-in-browser-compon.patch

Review of attachment 8555877 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/tests/unit/head_bookmarks.js
@@ +21,5 @@
>  // Put any other stuff relative to this test folder below.
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "bs",
> +                                   "@mozilla.org/browser/nav-bookmarks-service;1",
> +                                   "nsINavBookmarksService");

I'd like if this would disappear completely, and we'd just use PlacesUtils.bookmarks everywhere.
Or in future the shorter Places.bookmarks, when someone implements bug 1099371.

@@ +74,5 @@
>  const DEFAULT_BOOKMARKS_ON_MENU = 1;
> +
> +const SMART_BOOKMARKS_ANNO = "Places/SmartBookmark";
> +
> +function checkItemAnnotation(guid, name) {

sounds a bit generic, maybe checkItemHasAnnotation

@@ +90,5 @@
> +}
> +
> +function promiseEndUpdateBatch() {
> +  return new Promise(resolve => {
> +    bs.addObserver({

PlacesUtils.bookmarks.addObserver

@@ +100,5 @@
> +      onItemVisited: function() {},
> +      onItemMoved: function() {},
> +
> +      QueryInterface: XPCOMUtils.generateQI([Ci.nsINavBookmarkObserver])
> +    }, false);

{
  __proto__: NavBookmarkObserver.prototype,
  onEndUpdateBatch: resolve
}

@@ +104,5 @@
> +    }, false);
> +  });
> +}
> +
> +function createCorruptDB() {

nit: would be nice to use OS.File while touching this.
I think we have a way to get gTestDir, I don't recall if it's OS.File.getCurrentDirectory()

btw, if this creates complication to the conversion, let it alone.

::: browser/components/places/tests/unit/test_421483.js
@@ +94,5 @@
>      PlacesUtils.annotations.getItemsWithAnnotation(SMART_BOOKMARKS_ANNO);
> +  Assert.equal(smartBookmarkItemIds.length, smartBookmarksCount);
> +
> +  guid = yield PlacesUtils.promiseItemGuid(smartBookmarkItemIds[0]);
> +  bm = yield PlacesUtils.bookmarks.fetch({guid});

you don't need to do {guid}, you can just pass guid. note fetch accept either a guid or a bookmark object

::: browser/components/places/tests/unit/test_browserGlue_corrupt.js
@@ +35,5 @@
>    Cc["@mozilla.org/browser/browserglue;1"].getService(Ci.nsIBrowserGlue);
>  
>    // Initialize Places through the History Service.
>    let hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>             getService(Ci.nsINavHistoryService);

would be nice to remove this and use PlacesUtils.history.

::: browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js
@@ +29,5 @@
>    Cc["@mozilla.org/browser/browserglue;1"].getService(Ci.nsIBrowserGlue);
>  
>    // Initialize Places through the History Service.
>    let hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>             getService(Ci.nsINavHistoryService);

ditto

::: browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js
@@ +27,5 @@
>    Cc["@mozilla.org/browser/browserglue;1"].getService(Ci.nsIBrowserGlue);
>  
>    // Initialize Places through the History Service.
>    let hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>             getService(Ci.nsINavHistoryService);

ditto

::: browser/components/places/tests/unit/test_browserGlue_distribution.js
@@ +58,4 @@
>  
>    // Force distribution.
> +  let obs = Cc["@mozilla.org/browser/browserglue;1"].getService(Ci.nsIObserver)
> +  obs.observe(null, TOPIC_BROWSERGLUE_TEST, TOPICDATA_DISTRIBUTION_CUSTOMIZATION);

s/obs/glue/

::: browser/components/places/tests/unit/test_browserGlue_migrate.js
@@ +53,2 @@
>    // Check the created bookmarks still exist.
> +  bm = yield PlacesUtils.bookmarks.fetch({

s/bookmarks/bookmark/ in the comment

::: browser/components/places/tests/unit/test_browserGlue_prefs.js
@@ +15,5 @@
>  const TOPICDATA_FORCE_PLACES_INIT = "force-places-init";
>  
>  let bg = Cc["@mozilla.org/browser/browserglue;1"].
> +         getService(Ci.nsIBrowserGlue).
> +         QueryInterface(Ci.nsIObserver);

can directly .getService(Ci.nsIObserver)

::: browser/components/places/tests/unit/test_leftpane_corruption_handling.js
@@ +9,5 @@
>   */
>  
> +function run_test() {
> +  // We want empty roots.
> +  remove_all_bookmarks();

we need a bug filed to replace remove_all_bookmarks internals for some of the cases.
In most cases it's used just to cleanup roots, but that's done by eraseEverything.

In other cases though, like this one, it's really needed to remove also other roots (like the left pane folder), we can do that with the new API, by fetching children of the root that are not the main roots and removing them.

@@ +16,5 @@
> +  Assert.ok(!!PlacesUIUtils);
> +
> +  // Check getters.
> +  gLeftPaneFolderIdGetter = PlacesUIUtils.__lookupGetter__("leftPaneFolderId");
> +  Assert.equal(typeof(gLeftPaneFolderIdGetter), "function");

should probably replace this with

Object.getOwnPropertyDescriptor(PlacesUIUtils, "leftPaneFolderId").get;

@@ +18,5 @@
> +  // Check getters.
> +  gLeftPaneFolderIdGetter = PlacesUIUtils.__lookupGetter__("leftPaneFolderId");
> +  Assert.equal(typeof(gLeftPaneFolderIdGetter), "function");
> +  gAllBookmarksFolderIdGetter = PlacesUIUtils.__lookupGetter__("allBookmarksFolderId");
> +  Assert.equal(typeof(gAllBookmarksFolderIdGetter), "function");

ditto

@@ +42,5 @@
> +    type: PlacesUtils.bookmarks.TYPE_FOLDER
> +  });
> +
> +  let folderID = yield PlacesUtils.promiseItemId(folder.guid);
> +  PlacesUtils.annotations.setItemAnnotation(folderID, ORGANIZER_QUERY_ANNO,

nit: we usually use camelCase "Id" or Guid, we are trying to get rid of ID or GUID to have a more coherent notation (excluded comments)

@@ +56,5 @@
> +    // Run current test.
> +    yield Task.spawn(gTests.shift());
> +
> +    // Regenerate getters.
> +    PlacesUIUtils.__defineGetter__("leftPaneFolderId", gLeftPaneFolderIdGetter);

Object.defineProperty(PlacesUIUtils, "leftPaneFolderId", ...

@@ +58,5 @@
> +
> +    // Regenerate getters.
> +    PlacesUIUtils.__defineGetter__("leftPaneFolderId", gLeftPaneFolderIdGetter);
> +    gLeftPaneFolderId = PlacesUIUtils.leftPaneFolderId;
> +    PlacesUIUtils.__defineGetter__("allBookmarksFolderId", gAllBookmarksFolderIdGetter);

ditto

@@ +64,5 @@
> +    // Check the new left pane folder.
> +    let leftPaneHierarchy = folderIdToHierarchy(gLeftPaneFolderId)
> +    if (gReferenceHierarchy != leftPaneHierarchy) {
> +      do_throw("hierarchies differ!\n" + gReferenceHierarchy +
> +                                  "\n" + leftPaneHierarchy);

Assert.notEqual?

@@ +100,3 @@
>    },
>  
>    function test5() {

function*

@@ +113,5 @@
>                                                "PlacesRoot", 0,
>                                                PlacesUtils.annotations.EXPIRE_NEVER);
>    },
>  
>    function test6() {

all next tests are missing *

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> ::: browser/components/places/tests/unit/head_bookmarks.js
> > +function createCorruptDB() {
> 
> nit: would be nice to use OS.File while touching this.
> I think we have a way to get gTestDir, I don't recall if it's
> OS.File.getCurrentDirectory()

Done. (All other comments addressed too.)

> ::: browser/components/places/tests/unit/test_leftpane_corruption_handling.js
> > +function run_test() {
> > +  // We want empty roots.
> > +  remove_all_bookmarks();
> 
> we need a bug filed to replace remove_all_bookmarks internals for some of
> the cases.

Filed bug 1129498.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/e5483dcb0a70
https://hg.mozilla.org/integration/fx-team/rev/6d082a4775d0

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/e5483dcb0a70
https://hg.mozilla.org/mozilla-central/rev/6d082a4775d0