Closed
Bug 1094864
Opened 10 years ago
Closed 7 years ago
Convert mochitest-browser tests in browser/components/places to Bookmarks.jsm API
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: mak, Assigned: standard8)
References
Details
(Whiteboard: [fxsearch])
Attachments
(3 files, 1 obsolete file)
convert tests to the new async API. This may be a bit more time consuming than other tests.
Reporter | ||
Updated•10 years ago
|
Points: 5 → 8
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•9 years ago
|
Flags: qe-verify-
Updated•9 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Comment 1•9 years ago
|
||
Comment 2•9 years ago
|
||
Partial patch to land the work I've put into it so far. I'll then wait for bug 1119282 to be done before continuing.
Attachment #8562012 -
Attachment is obsolete: true
Attachment #8562067 -
Flags: review?(mano)
Updated•9 years ago
|
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Comment 4•9 years ago
|
||
Comment on attachment 8562067 [details] [diff] [review] 0001-Bug-1094864-Convert-mochitest-browser-tests-in-brows.patch Review of attachment 8562067 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/tests/browser/browser_425884.js @@ +58,5 @@ > // confirm serialization > ok(rawNode.type, "confirm json node"); > folderANode.containerOpen = false; > > + let testRootId = yield PlacesUtils.promiseItemId(testRoot.guid); An alternative approach for this test would be to use something like ensureEqualBookmarksTrees (see test_async_transactions), rather than comparing result nodes. It would pave the way for removing itemId usage too. But you don't have to do it here, and I could also do that when I convert these tests to use PT. ::: browser/components/places/tests/browser/browser_forgetthissite_single.js @@ +44,5 @@ > + selection.clearSelection(); > + selection.rangedSelect(0, selectionCount - 1, true); > + is(selection.count, selectionCount, "The selected range is as big as expected"); > + > + // Open the context menu nit: . @@ +48,5 @@ > + // Open the context menu > + let contextmenu = doc.getElementById("placesContext"); > + let popupShown = promisePopupShown(contextmenu); > + > + // Get cell coordinates ditto (and elsewhere) @@ +58,5 @@ > + let forgetThisSite = doc.getElementById("placesContext_deleteHost"); > + let hideForgetThisSite = (selectionCount != 1); > + is(forgetThisSite.hidden, hideForgetThisSite, > + "The Forget this site menu item should " + (hideForgetThisSite ? "" : "not ") + > + "be hidden with " + selectionCount + " items selected"); Please use template strings for things like this in new code.
Attachment #8562067 -
Flags: review?(mano) → review+
Comment 5•9 years ago
|
||
(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #4) > > + let testRootId = yield PlacesUtils.promiseItemId(testRoot.guid); > > An alternative approach for this test would be to use something like > ensureEqualBookmarksTrees (see test_async_transactions), rather than > comparing result nodes. It would pave the way for removing itemId usage too. > > But you don't have to do it here, and I could also do that when I convert > these tests to use PT. Ok, left that for you :) https://hg.mozilla.org/integration/fx-team/rev/3215184d1171
Updated•9 years ago
|
Keywords: leave-open
Updated•9 years ago
|
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Comment 7•9 years ago
|
||
That bug is blocked on bug 1119282. Will pick it up again when that's fixed. Shouldn't even have been carried over to the current iteration, sorry :/
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Iteration: 39.1 - 9 Mar → ---
Reporter | ||
Updated•9 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → standard8
Whiteboard: [fxsearch]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8911731 -
Flags: review?(mak77)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8911731 [details] Bug 1094864 - Remove unnecessary checks from browser/components/places. https://reviewboard.mozilla.org/r/183142/#review188420
Attachment #8911731 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8911732 [details] Bug 1094864 - Rewrite most of browser/components/places/tests/browser to use the newer async Places APIs. https://reviewboard.mozilla.org/r/183144/#review188522 ::: browser/components/places/tests/browser/browser_library_search.js:171 (Diff revision 2) > + > + // Cleanup. > + PlacesUtils.tagging.untagURI(PlacesUtils._uri(TEST_URL), ["dummyTag"]); > + > + await PlacesUtils.bookmarks.eraseEverything(); > + await PlacesTestUtils.clearHistory(); PU.history.clear() ::: browser/components/places/tests/browser/browser_sidebarpanels_click.js:26 (Diff revision 2) > ok(false, "Unexpected sidebar found - a previous test failed to cleanup correctly"); > SidebarUI.hide(); > } > > - let sidebar = document.getElementById("sidebar"); > + // Ensure history is clean before starting the test. > + await PlacesTestUtils.clearHistory(); PU.history.clear() ::: browser/components/places/tests/browser/browser_sidebarpanels_click.js:42 (Diff revision 2) > - PlacesUtils.unfiledBookmarksFolderId, PlacesUtils._uri(TEST_URL), > - PlacesUtils.bookmarks.DEFAULT_INDEX, "test" > - ); > - aCallback(); > + parentGuid: PlacesUtils.bookmarks.unfiledGuid, > + title: "test", > + url: TEST_URL, > + }); > + > + this._bookmarkId = await PlacesUtils.promiseItemId(this._bookmark.guid); Looks like you could avoid storing _bookmarkId and just do this in selectNode? ::: browser/components/places/tests/browser/browser_sidebarpanels_click.js:82 (Diff revision 2) > sidebarName: HISTORY_SIDEBAR_ID, > treeName: HISTORY_SIDEBAR_TREE_ID, > desc: "History sidebar test" > }); > > - function testPlacesPanel(preFunc, postFunc) { > + for (let i = 0; i < tests.length; i++) { for...of ::: browser/components/places/tests/browser/browser_views_liveupdate.js:106 (Diff revision 2) > - bs.setItemTitle(id, "bubf1_edited"); > - addedBookmarks.push(id); > - bs.moveItem(id, bs.unfiledBookmarksFolder, 0); > > - // Remove all added bookmarks. > + // Remove all added bookmarks. > - addedBookmarks.forEach(function(aItem) { > + for (let i = 0; i < addedBookmarks.length; i++) { for...of
Attachment #8911732 -
Flags: review?(mak77) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b8d59e74c0bb Remove unnecessary checks from browser/components/places. r=mak https://hg.mozilla.org/integration/autoland/rev/c2131082228a Rewrite most of browser/components/places/tests/browser to use the newer async Places APIs. r=mak
Assignee | ||
Comment 17•7 years ago
|
||
I probably should have mentioned before... the patches here get most of the remaining instances. There's a couple of tests I'm addressing in other bugs. There may also be some rarer old APIs in use that I missed, but we'll get those in the cleanup - I think the important part here is to convert most of the cases.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8d59e74c0bb https://hg.mozilla.org/mozilla-central/rev/c2131082228a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in
before you can comment on or make changes to this bug.
Description
•