Fix places-related mochitests in browser/ for async-transactions

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mano, Assigned: standard8)

Tracking

unspecified
Firefox 56
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments, 1 obsolete attachment)

There are various tests in browser/ that assume places UI commands are synchronous (e.g. the test for cmd_cut). These tests would fail, as they are, with async. transactions enabled. We need to fix those before we can enable async transactions
Mano, can you please estimate this?
Flags: qe-verify-
Flags: needinfo?(mano)
Flags: firefox-backlog+
Status: NEW → ASSIGNED
Points: --- → 8
Flags: needinfo?(mano)
Iteration: --- → 38.2 - 9 Feb
FTR, please let's review and land the patch in bug 1094864 (to be posted) before continuing here.
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Iteration: 39.1 - 9 Mar → ---
1. Ignore firefox.js and browser.ini changes
2. I'll do browser_bookmarkProperties in a separate patch (in this bug)
3. browser_library_commands won't pass because a query node created there has no bookmarkGuid for some reason. I'll file a separate bug sson.
Attachment #8578527 - Flags: review?(mak77)
Comment on attachment 8578527 [details] [diff] [review]
everything but browser_bookmarkProperties and browser_library_commands

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

It looks ok, so far.

::: browser/components/places/tests/browser/browser_475045.js
@@ +40,5 @@
>      ChromeUtils.synthesizeDrop(placesItems.childNodes[0],
> +                               placesItems, 
> +                               [[{ type: aMimeType, 
> +                                  data: uriSpec}]], 
> +                               aEffect, window);

while here, please remove trailing spaces and fix indentation

@@ +42,5 @@
> +                               [[{ type: aMimeType, 
> +                                  data: uriSpec}]], 
> +                               aEffect, window);
> +
> +		yield window.PlacesControllerDragHelper._onDropPromise;

indentation

@@ +47,3 @@
>  
>      // Verify that the drop produces exactly one bookmark.
> +    let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(uri);

Please use the new API in tests

@@ +52,1 @@
>      PlacesUtils.bookmarks.removeItem(bookmarkIds[0]);

ditto

@@ +52,4 @@
>      PlacesUtils.bookmarks.removeItem(bookmarkIds[0]);
>  
>      // Verify that we removed the bookmark successfully.
>      ok(!PlacesUtils.bookmarks.isBookmarked(uri), "URI should be removed");

ditto

@@ +57,1 @@
>    

while here, please remove the trailing spaces

::: browser/components/places/tests/browser/browser_library_batch_delete.js
@@ +57,5 @@
> +
> +    // doCommand may do async work through PT, so we need to serialize the rest
> +    // of this test.
> +    PlacesTransactions.batch(function* () {
> +      ok(!PlacesUtils.bookmarks.isBookmarked(testURI),

use the new API please

::: browser/components/places/tests/browser/browser_library_commands.js
@@ +130,5 @@
> +
> +  // doCommand may do async work through PT, so we need to serialize the rest
> +  // of this test with PT.
> +  yield PlacesTransactions.batch(() => {});
> +  yield promiseItemRemoved; 

trailing space
Attachment #8578527 - Flags: review?(mak77) → feedback+
Priority: -- → P1
Mano, are you still interest in finish this patch? Or I can help finish that.
Flags: needinfo?(asaf)
Mano is unlikely to continue the work on this, I suspect he's quite busy with his new job.
So feel free to take over. I will unassign his Places bugs so it's clear they are available.
Assignee: asaf → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(asaf)
Priority: P1 → P2
Assignee: nobody → standard8
Priority: P2 → P1
Whiteboard: [fxsearch]
Attachment #8578527 - Attachment is obsolete: true
Note: the last test has been split out to bug 1376925, so that we can get the intermediate work landed.

Comment 10

2 years ago
mozreview-review
Comment on attachment 8881936 [details]
Bug 1119282 - Update browser_library_batch_delete.js to use more modern test constructs.

https://reviewboard.mozilla.org/r/153010/#review158234
Attachment #8881936 - Flags: review?(mak77) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8881937 [details]
Bug 1119282 - Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues.

https://reviewboard.mozilla.org/r/153012/#review158246

::: browser/components/places/tests/browser/browser_475045.js:52
(Diff revision 1)
>  
> +    await promiseItemAddedNotification;
> +
>      // Verify that the drop produces exactly one bookmark.
> -    let bookmarkIds = PlacesUtils.bookmarks
> -                      .getBookmarkIdsForURI(uri);
> +    let bookmark = await PlacesUtils.bookmarks.fetch({url});
> +    Assert.equal(typeof(bookmark), "object", "There should be exactly one bookmark");

why so complex, it's either an object or null, so just Assert.ok(bookmark) should do.
if you want to heck that it's only one, you should use the fetch callback to collect all the bookmarks in an array.

to be clear, fetch always returns just null or the most recent bookmarks, never an array.
Attachment #8881937 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca16ed38806f
Update browser_library_batch_delete.js to use more modern test constructs. r=mak
https://hg.mozilla.org/integration/autoland/rev/5659f6c9a7c5
Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues. r=mak
Backed out for test_engine_changes_during_sync.js, at least on Windows 7 VM:

https://hg.mozilla.org/integration/autoland/rev/b0b36bfe7d8889bfe80217ffcf15e953dca8d8b2
https://hg.mozilla.org/integration/autoland/rev/2ed4887d5010ed74204a4e34003d9e410923371d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5659f6c9a7c5b31929bdda8cd58a2001356190c0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110573003&repo=autoland


18:27:36     INFO -  TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 343] true == true
18:27:36     INFO -  TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 344] "undefined" == "undefined"
18:27:36     INFO -  TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 351] "undefined" == "undefined"
18:27:36     INFO -  TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 352] "undefined" != 1
18:27:36     INFO -  TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 343] true == true
18:27:36     INFO -  TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 344] "undefined" == "undefined"
18:27:36  WARNING -  TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 346] "undefined" == [{"name":"clientCycles","count":1}]
18:27:36     INFO -  c:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/head_helpers.js:assert_success_ping/<:346
18:27:36     INFO -  c:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/head_helpers.js:assert_success_ping:338
18:27:36     INFO -  c:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/test_engine_changes_during_sync.js:test_bookmark_change_during_sync/<:421
18:27:36     INFO -  c:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/test_engine_changes_during_sync.js:test_bookmark_change_during_sync:420
18:27:36     INFO -  exiting test
Flags: needinfo?(standard8)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8881937 [details]
Bug 1119282 - Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues.

https://reviewboard.mozilla.org/r/153012/#review158428

::: toolkit/components/places/PlacesTransactions.jsm:1226
(Diff revision 2)
>        if (newParentId == oldParentId && oldIndex > undoIndex)
>          PlacesUtils.bookmarks.moveItem(itemId, oldParentId, oldIndex + 1);
>        else
>          PlacesUtils.bookmarks.moveItem(itemId, oldParentId, oldIndex);
>      };
> +    return aGuid;

This is unrelated to the failure. I just noticed that this doesn't see to make sense, the guid is already known, so there's no reason Move should return a guid.

Comment 16

2 years ago
mozreview-review
Comment on attachment 8881937 [details]
Bug 1119282 - Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues.

https://reviewboard.mozilla.org/r/153012/#review158484

::: toolkit/components/places/nsNavHistory.cpp:3979
(Diff revision 2)
>      }
>      else {
>        // This is a regular query.
>        resultNode = new nsNavHistoryQueryResultNode(aTitle, aTime, queries, options);
>        resultNode->mItemId = itemId;
> +      resultNode->mBookmarkGuid = aBookmarkGuid;

It looks like this fixes a bug where `bookmarkGuid` would be erroneously empty? I wonder if the sync bookmark validator is expanding queries incorrectly, and this change uncovered that... http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/services/sync/modules/bookmark_validator.js#232-279

Comment 18

2 years ago
mozreview-review
Comment on attachment 8882227 [details]
Bug 1119282 - Fix the sync bookmark validator cycle detection to correctly get folder contents.

https://reviewboard.mozilla.org/r/153330/#review158532
Attachment #8882227 - Flags: review?(kit) → review+
Assignee

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8881937 [details]
Bug 1119282 - Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues.

https://reviewboard.mozilla.org/r/153012/#review158428

> This is unrelated to the failure. I just noticed that this doesn't see to make sense, the guid is already known, so there's no reason Move should return a guid.

Discussed this on irc and decided to leave it in there - the caller needs a transaction returning the guid to work nicely alongside the copy function.

Comment 20

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e7837d88d8b
Update browser_library_batch_delete.js to use more modern test constructs. r=mak
https://hg.mozilla.org/integration/autoland/rev/107b93dffe70
Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues. r=mak
https://hg.mozilla.org/integration/autoland/rev/d05bb3f0b142
Fix the sync bookmark validator cycle detection to correctly get folder contents. r=kitcambridge
Flags: needinfo?(standard8)

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e7837d88d8b
https://hg.mozilla.org/mozilla-central/rev/107b93dffe70
https://hg.mozilla.org/mozilla-central/rev/d05bb3f0b142
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.