Add automated test for "Bookmarks can be backed up to a JSON file and restored back by importing JSON file"

RESOLVED FIXED in Firefox 62

Status

()

P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: gechim, Assigned: gechim)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

a year ago
TestRail link: https://testrail.stage.mozaws.net/index.php?/cases/view/4105
combined with
TestRail link: https://testrail.stage.mozaws.net/index.php?/cases/view/4106

Steps for exporting bookmarks:
1. Launch Firefox.
2. Open the Library from the Bookmarks Menu by selecting Show All Bookmarks.
3. Click Import and Backup and select Backup....
4. Save the backup file to your local drive.

Expected results:
1. Firefox is successfully launched.
2. The Library window is opened, with All Bookmarks selected by default.
3. The user is asked to save the bookmarks backup .json file on the local drive.
4. There are no errors thrown for this action.

Steps for importing bookmarks:
1. Launch Firefox.
2. Open the Library from the Bookmarks Menu by selecting Show All Bookmarks.
3. Click Import and Backup and select Restore → Choose file....
4. Select the *.json backup file and click Open.
5. Click OK.

Expected results:
1. Firefox is successfully launched.
2. The Library window is opened, with All Bookmarks selected by default.
3. The user is asked to open the *.json file containing the bookmarks.
4. The user is prompted with a confirmation dialog, stating that this action will replace all current bookmarks with the ones from the backup file.
5. The bookmarks are successfully restored from the backup file and no errors are thrown for this action.
(Assignee)

Updated

a year ago
Assignee: nobody → gechim
(Assignee)

Updated

a year ago
Blocks: 1419383
(Assignee)

Comment 1

a year ago
Attachment #8952731 - Flags: review?(standard8)
Comment on attachment 8952731 [details] [diff] [review]
mochitest_browser_bookmark_backup_export_import

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

Sorry, but this patch doesn't actually test import/export via the menu options in the library window. It does open the library window, but ends up calling the functions direct.

We already have extensive tests for import/export that call the functions direct - via xpcshell in the the toolkit/components/places/tests/unit directory (e.g. test_bookmarks_html*.js and test_bookmarks_json*.js) so this test wouldn't be adding anything as it stands.

It looks like you could instead use the MockFilePicker: https://searchfox.org/mozilla-central/search?q=symbol:%23MockFilePicker&redirect=false

The test would then become much simpler:

- Open library window
- Setup the MockFilePicker
- Export file
- Check file exists
- Erase everything
- Import file
- Check bookmarks.

We don't need to check the content of the exported file, as the xpcshell-tests do that for us.

There's some comments below as I started commenting before I noticed this, please take those into account as well.

::: browser/components/places/tests/browser/browser_bookmark_backup_export_import.js
@@ +7,5 @@
> +/**
> + * Tests bookmarks backup export as JSON file.
> + */
> +
> +XPCOMUtils.defineLazyModuleGetters(this, {

Browser-chrome Mochitests import their global scope from head*.js files, as well the browser window. I don't think any of these imports should be necessary here.

@@ +44,5 @@
> +  }
> +];
> +
> +const BTYPES_ENUM = {
> +  link: PlacesUtils.TYPE_X_MOZ_PLACE,

Please use these directly, don't duplicate/rename.

@@ +47,5 @@
> +const BTYPES_ENUM = {
> +  link: PlacesUtils.TYPE_X_MOZ_PLACE,
> +  separator: PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
> +  container: PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
> +  TYPE_BOOKMARK: 1,

These are already available as PlacesUtils.bookmarks.TYPE_*

@@ +55,5 @@
> +
> +const ALL_BOOKMARKS_LABEL = "Show All Bookmarks";
> +
> +async function openLibraryPanel() {
> +  const libView = document.getElementById("appMenu-libraryView");

I disagree with this test requiring that the library panel is opened via the toolbar button - that section shouldn't be under test here (that should be a separate test if necessary). I'd rather use our simpler constructs of "promiseLibrary()/promiseLibraryClosed()" that open it directly.

@@ +233,5 @@
> +  }
> +}
> +
> +// Add new bookmarks before running main test.
> +add_task(async function before_test() {

Please change before_test to setup as that's more consistent with what we do elsewhere.

@@ +236,5 @@
> +// Add new bookmarks before running main test.
> +add_task(async function before_test() {
> +
> +  ok(PlacesBackups, "PlacesBackups in scope");
> +  ok(FileUtils, "FileUtils in scope");

These checks aren't necessary, if it isn't in scope, it'll abort with a sane error message. Also, ESLint should catch most of them.
Attachment #8952731 - Flags: review?(standard8) → review-
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [fxsearch]
(Assignee)

Comment 4

a year ago
Hi Mark.

Thanks for reviewing this.
Indeed it makes more sense to use MockFilePicker and I have change the test accordingly.

Since I used PlacesBackups and PlacesSyncUtils:
- Can you let me know how I can import them other than using XPCOMUtils.defineLazyModuleGetters ?
- In case you see a solution for not using them please let me know.

Thank you.
Attachment #8952731 - Attachment is obsolete: true
Attachment #8954803 - Flags: review?(standard8)
Comment on attachment 8954803 [details] [diff] [review]
mochitest_browser_bookmark_backup_export_import_2

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

Please make sure that you run ./mach lint --outgoing on these before exporting, or have a commit hook (https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook). There's various ESLint errors that should be fixed.

Depending on your editor, there's also various ESLint integrations available: https://eslint.org/docs/user-guide/integrations

::: browser/components/places/tests/browser/browser_bookmark_backup_export_import.js
@@ +26,5 @@
> +const PLACES = [
> +  {
> +    guid: PlacesUtils.bookmarks.menuGuid,
> +    prefix: "In Menu",
> +    total: EXPECTED_MENU_TOTAL

There's no need to specify the totals separately, they are only needed in these objects.

Also, do we really need this many?

@@ +42,5 @@
> +];
> +
> +var importExportPicker, saveDir;
> +
> +async function deleteAllBookmarks() {

This really isn't necessary. I checked the output of your test, and the only thing that remains are the root folders (menu/toolbar/unfiled/mobile) - that is intentional, they never get deleted.

eraseEverything gets rid of everything in those folders, so you really don't need the extra functions here.

@@ +67,5 @@
> +  }, "Not able to remove default bookmarks");
> +}
> +
> +async function generateTestBookmarks() {
> +  for (let j = 0; j < PLACES.length; j++) {

Generally it is preferred to use `for (let place of PLACES)` when iterating over an object or an array. You won't need the currentPlace alias then either.

@@ +73,5 @@
> +    let currentPlaceChildren = [];
> +    for (let i = 1; i <= currentPlace.total; i++) {
> +      let currentIndex = (i % 2 == 0) ? 0 : 1;
> +      currentPlaceChildren.push({
> +        url: MOCK_HISTORY[currentIndex],

These urls don't need to be real, so you could just use something like `http://example.com/${currentIndex}` and make it much simpler to calculate.

@@ +89,5 @@
> +  let currentPlace = PLACES.filter((elem) => {
> +    return elem.guid === parentGuid.toString();
> +  })[0];
> +
> +  let childGuids = await PlacesSyncUtils.bookmarks.fetchChildRecordIds(parentGuid);

I think using PlacesUtils.promiseBookmarksTree would be better here. It will avoid the multiple fetches, and you can pass it PlacesUtils.bookmarks.rootGuid to get the whole bookmarks tree.

Also, prior to exporting, you could save the bookmarks tree as it is then, and then use it in this validation i.e. compare the two trees together. That would probably be simpler.

@@ +150,5 @@
> +  });
> +}
> +
> +function initMockFilePicker() {
> +  Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/toolkit/content/tests/browser/common/mockTransfer.js", this);

You don't need the mockTransfer.js / mockTransferRegisterer, only the MockFilePicker stuff.

@@ +159,5 @@
> +
> +add_task(async function setup() {
> +  await deleteAllBookmarks();
> +  await generateTestBookmarks();
> +  initMockFilePicker();

initMockFilePicker doesn't really need to be a separate function, especially with the reduction. Generally for tests, there's a balance between splitting everything up into multiple functions, but allowing the test to flow without having to go to multiple places to find the actual code. At the moment, it feels you're on the splitting up too much side.

@@ +162,5 @@
> +  await generateTestBookmarks();
> +  initMockFilePicker();
> +
> +  let libraryButton = CustomizableUI.getPlacementOfWidget("library-button");
> +  if (!libraryButton) {

You don't need to modify the position of the library button in this test, as we're using promiseLibrary (and likewise, no need to reset CustomizableUI)

@@ +165,5 @@
> +  let libraryButton = CustomizableUI.getPlacementOfWidget("library-button");
> +  if (!libraryButton) {
> +    CustomizableUI.addWidgetToArea("library-button", CustomizableUI.AREA_NAVBAR);
> +  }
> +  registerCleanupFunction(async () => {

Please include a call for eraseEverything() here, to clean up in preparation for other tests.

@@ +172,5 @@
> +    importExportPicker.cleanup();
> +  });
> +});
> +
> +add_task(async () => {

The individual add_task functions should be given titles, e.g. `add_task(async test_export_json() {`, this aids finding which section of the test you're in when you're looking at debug output.

@@ +188,5 @@
> +  let libraryWindow = await promiseLibrary();
> +  libraryWindow.document.querySelector("#maintenanceButtonPopup #restoreFromFile").click();
> +
> +  await promiseImportExport();
> +  await promiseAlertDialogOpen("accept");

This can be replaced by `await BrowserTestUtils.promiseAlertDialogOpen("accept");`, although it would probably be more reliable, if you assign the result to a variable before the promiseImportExport, and then await it afterwards.

@@ +191,5 @@
> +  await promiseImportExport();
> +  await promiseAlertDialogOpen("accept");
> +
> +  await waitForCondition(async () => {
> +    let aBookmark = await PlacesUtils.bookmarks.fetch({

This would be better waiting on PlacesTestUtils.waitForNotification and waiting for onItemChanged. You'll probably have to count them or something to be reliable as we can't fully guarantee the order.

@@ +201,5 @@
> +
> +  await validateImportedBookmarks(PLACES);
> +
> +  registerCleanupFunction(async () => {
> +    Services.wm.getMostRecentWindow("Places:Organizer").close();

should be `await promiseLibraryClosed(libraryWindow);`

@@ +202,5 @@
> +  await validateImportedBookmarks(PLACES);
> +
> +  registerCleanupFunction(async () => {
> +    Services.wm.getMostRecentWindow("Places:Organizer").close();
> +    await PlacesUtils.bookmarks.eraseEverything();

Lets just have the eraseEverything in the setup's registerCleanupFunction
Attachment #8954803 - Flags: review?(standard8)
(Assignee)

Comment 6

a year ago
Hi Mark,

Thanks for reviewing this.

I have installed an ESLint extension and updated the patch.

I was unable to:
1) Only use eraseEverything - I have added deleteBookmarksByParentGuid because eraseEverything resolves to soon / not deleting as expected so when bookmarks are restored there are more bookmarks than expected. Please let me know if I am not using it correctly. 
2) Use BrowserTestUtils.promiseAlertDialogOpen because I received TypeError: BrowserTestUtils.promiseAlertDialogOpen is not a function.

Please let me know if I need to change anything else.
Attachment #8954803 - Attachment is obsolete: true
Attachment #8955150 - Flags: review?(standard8)
(In reply to George Echim from comment #6)
> I was unable to:
> 1) Only use eraseEverything - I have added deleteBookmarksByParentGuid
> because eraseEverything resolves to soon / not deleting as expected so when
> bookmarks are restored there are more bookmarks than expected. Please let me
> know if I am not using it correctly. 

I tried it locally, and replaced both `await deleteAllBookmarks();` with `await PlacesUtils.bookmarks.eraseEverything()` and it has worked fine. If you're still seeing an error, please copy & paste it here.

> 2) Use BrowserTestUtils.promiseAlertDialogOpen because I received TypeError:
> BrowserTestUtils.promiseAlertDialogOpen is not a function.
> 
> Please let me know if I need to change anything else.

I checked via https://searchfox.org/mozilla-central/search?q=promiseAlertDialogOpen&case=false&regexp=false&path= and it is definitely there, looking at the change history, it was added on approx 28th Feb, so maybe your tree isn't up-to-date?
Attachment #8955150 - Flags: review?(standard8)
(Assignee)

Comment 8

a year ago
Hi Mark,

Thank you for reviewing the patch.

Indeed I can now use BrowserTestUtils.promiseAlertDialogOpen after updating the tree.

eraseEverything still resolves to soon making bookmark restore to overlap. I have removed the manual delete part but added a filter later on.

I have uploaded the new patch.
Attachment #8955150 - Attachment is obsolete: true
Attachment #8956098 - Flags: review?(standard8)
George, please can you confirm details of your test setup, e.g. are you using a build from mozilla-central, if so what type (artifact, opt etc), and also how you are running the tests.

eraseEverything really should be erasing everything and I've not seen behaviour like it before, so I'm really suspicious that there's something bad going on somewhere.
Flags: needinfo?(gechim)
(Assignee)

Comment 10

a year ago
Posted file bookmarks-2018-03-05.json (obsolete) —
(In reply to Mark Banner (:standard8) from comment #9)

Hi Mark,

I am updating my tree every week from the default branch with update.
Sometimes I use named branches but lately I am trying to use bookmarks for each test.
I use "./mach build" for build and "./mach mochitest path/to/file.js " to run each test.

Last time I run this test was on: 
- Version: 60.0a1
- User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
- OS : Linux 4.4.0-116-generic

I noticed that when only using eraseEverything the exported file from temp folder (attached)
contained bookmarks like : Most Visited, Get Involved, Recent Tags.
I thought that eraseEverything resolves to soon or it ignores default bookmarks like Most Visited, Get Involved, Recent Tags

For this reason I used a manually delete by parent guid to make sure everything is deleted.

Hope this info helps.
Please let me know if I need to change the last patch.
Flags: needinfo?(gechim) → needinfo?(standard8)
Can you try adding a `Cu.reportError` around here: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/components/nsBrowserGlue.js#2446 and seeing if that shows up on test output (if it doesn't try the console.log variant as well).

I'm wondering if that is somehow kicking in at the wrong time, as that's the only way they'd get recreated.

BTW, you also don't need to import PlacesSyncUtils.jsm now. I think you can also avoid the PlacesBackups import with just hard-coding the filename - it doesn't need to use getFilenameForDate(). It would be interesting to know if avoiding those imports also help with things (I don't think they should but...).
Flags: needinfo?(standard8)
(Assignee)

Comment 12

a year ago
Hi Mark,

I have removed the unnecessary import and uploaded a new patch.

My debug output is:

TEST-START | browser/components/places/tests/browser/browser_bookmark_backup_export_import.js
Entering test bound setup
GECKO(18767) | console.log: > before eraseEverything from setup
Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
GECKO(18767) | console.log: > after eraseEverything from setup
GECKO(18767) | console.log: > before insertTree In Menu
GECKO(18767) | console.log: "> ensurePlacesDefaultQueriesInitialized triggered"
GECKO(18767) | console.log: > after insertTree In Menu
GECKO(18767) | console.log: > before insertTree In Toolbar
GECKO(18767) | console.log: > after insertTree In Toolbar
GECKO(18767) | console.log: > before insertTree In Other
GECKO(18767) | console.log: > after insertTree In Other
Leaving test bound setup
Entering test bound test_export_json
GECKO(18767) | console.log: > test_export_json start
must wait for load
must wait for focus
GECKO(18767) | console.log: > export bookmarks
Leaving test bound test_export_json

Looks like ensurePlacesDefaultQueriesInitialized is triggered once by PlacesUtils.bookmarks.insertTree
Hope this helps.

Do you thing we can land the test since it ignores Recent Tags, Most Visited and Get Involved asserting only the generated bookmarks ?
Or should I get the smart bookmarks dynamically and then ignore them ?
Attachment #8956098 - Attachment is obsolete: true
Attachment #8956362 - Attachment is obsolete: true
Attachment #8956098 - Flags: review?(standard8)
Attachment #8956769 - Flags: review?(standard8)
Comment on attachment 8956769 [details] [diff] [review]
mochitest_browser_bookmark_backup_export_import_5

ensurePlacesDefaultQueriesInitialized isn't in the call path from insertTree, or at least shouldn't be, especially as eraseEverything should have already initialised places.

The call sites are here:

https://searchfox.org/mozilla-central/search?q=ensurePlacesDefaultQueriesInitialized&case=false&regexp=false&path=

The observe one is a test-only call, and the last two are within _initPlaces. I think what is happening, is that when we startup for the test, places may not have initialised before the test runs, the first call being eraseEverything() which triggers the initialisation (which happens async to the erase), and so then the default queries get inserted.

I think the best way around this is at the very start of the setup function, to `await promisePlacesInitComplete();` - that should ensure the initialisation is completed before the erase.
Attachment #8956769 - Flags: review?(standard8)
(Assignee)

Comment 14

a year ago
Hi Mark,

Thank you for your help.
Adding the promisePlacesInitComplete fixed the issue.

I have uploaded a new patch.
Attachment #8956769 - Attachment is obsolete: true
Attachment #8957140 - Flags: review?(standard8)
Comment on attachment 8957140 [details] [diff] [review]
mochitest_browser_bookmark_backup_export_import_6

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

Great, thanks for the update. I'm glad we found out what the issue was.
Attachment #8957140 - Flags: review?(standard8) → review+
Comment hidden (mozreview-request)
Comment on attachment 8957140 [details] [diff] [review]
mochitest_browser_bookmark_backup_export_import_6

Bitrot fixing patch attached via mozreview.
Attachment #8957140 - Attachment is obsolete: true

Comment 18

11 months ago
mozreview-review
Comment on attachment 8974645 [details]
Bug 1439935 - Add a test for testing backup JSON file export/import.

https://reviewboard.mozilla.org/r/243038/#review248834
Attachment #8974645 - Flags: review?(standard8) → review+

Comment 19

11 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4092dcafedd0
Add a test for testing backup JSON file export/import. r=standard8

Comment 20

11 months ago
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/191f35d165fa
Backed out changeset 4092dcafedd0 for failing on /tests/browser/browser_toolbar_overflow.js on a CLOSED TREE

Comment 21

11 months ago
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64588dd6efa3
Add a test for testing backup JSON file export/import. r=standard8 a=reland on a CLOSED TREE

Comment 22

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64588dd6efa3
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.