1011023 - Simplify test_bookmarks_restore_notification.js to use add_task

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

For historical reasons, test http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js?from=test_bookmarks_restore_notification.js#1 uses a complicated of observers to advance between two tests.

The objective of this bug is to replace `beginObserver`, `endObserver`, `currTestIndex` and `doNextTest` by a simple use of `add_task`.

For more details about add_task, see https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests and https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm

Comment 1

[Anup Kumar [:reznord]]

Hi,

I am interested in working on this bug. So, can you assign this bug to me?

Thanks in advance,

Regards,
Anup

Comment 2

[Mike Hoye [:mhoye]]

Hi, Anup - are you making any progress on this?

Comment 3

[Anup Kumar [:reznord]]

(In reply to Mike Hoye [:mhoye] from comment #2)
> Hi, Anup - are you making any progress on this?

Hi Mike,

I am sorry for the delay on the bug. I will complete the patch for this bug with in 3 to 4 days.

Regards,
Anup

Comment 4

[Anup Kumar [:reznord]]

Hi Yoric, 

I have reassigned the bug to mhoye. Sorry for the inconvenience and the delay on the bug.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Why Mike Hoye? I don't think he's interested in this bug.

Comment 6

[Mike Hoye [:mhoye]]

Well, I'm interested in it in principle, but I'm probably not the right person to fix it. Let me see if I can find somebody who is, though!

Comment 7

[[e-mail]]

Attachment: MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

Review commit: https://reviewboard.mozilla.org/r/45715/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45715/

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Currently quite busy, so mak, any chance you could review this?

Comment 9

[[e-mail]]

https://reviewboard.mozilla.org/r/45715/#review42265

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  Cu.import("resource://gre/modules/BookmarkHTMLUtils.jsm");
>  
>  /**
>   * Tests the bookmarks-restore-* nsIObserver notifications after restoring

I saw there might be a requirement to test nsIObserver notification events. If that still needed I'll add related nsIObserver test into test cases, or I'll get rid of these declarations.

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:43
(Diff revision 1)
> -//               test
> -//   file:       the nsILocalFile that the test creates
> -//   folderId:   for HTML restore into a folder, the folder ID to restore into;
> -//               otherwise, set it to null
> -//   run:        a method that actually runs the test
> -var tests = [
> + */
> +function addBookmarks() {
> +  uris.forEach(u => bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder,
> +                                         uri(u),
> +                                         bmsvc.DEFAULT_INDEX,
> +                                         u));

move test cases into each add_task (s)

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js
(Diff revision 1)
> -    })
> -  }
> -];
> -
> -// nsIObserver that observes bookmarks-restore-begin.
> -var beginObserver = {

removed beginObserver, successAndFailedObserver, currTestIndex, doNextTest

Comment 10

[Marco Bonardo [:mak]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #8)
> Currently quite busy, so mak, any chance you could review this?

yep, will look into this.

Comment 11

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/45715/#review43139

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:32
(Diff revision 1)
>    "http://example.com/4",
>    "http://example.com/5",
>  ];
>  
> -// Add tests here.  Each is an object with these properties:
> -//   desc:       description printed before test is run
> +var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
> +            getService(Ci.nsINavBookmarksService);

Please use PlacesUtils.bookmarks instead

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:40
(Diff revision 1)
> -//   finalTopic: the last expected topic that should be observed for the test,
> -//               which then causes the next test to be run
> -//   data:       the data passed to nsIObserver.observe() corresponding to the
> -//               test
> -//   file:       the nsILocalFile that the test creates
> -//   folderId:   for HTML restore into a folder, the folder ID to restore into;
> +
> +/**
> + * Adds some bookmarks for the URIs in |uris|.
> + */
> +function addBookmarks() {
> +  uris.forEach(u => bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder,

Since we use add_task, I'd be very happy if we could convert this test to the new asynchronous bookmarking API.

instead of insertBookmark you can make this a generator and use
for (let url or uris) {
  yield PlacesUtils.bookmarks.insert({
    url, parentGuid: PlacesUtils.bookmarks.menuGuid
  });
}

(remember to yield on addBookmarks() then)

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:54
(Diff revision 1)
> -    folderId:   null,
> -    run: Task.async(function* () {
> + * creating one query per URI and then ORing all the queries.  The number of
> + * results returned should be uris.length.
> + */
> +function checkBookmarksExist() {
> +  var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
> +           getService(Ci.nsINavHistoryService);

Please use PlacesUtils.history

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:78
(Diff revision 1)
> + * @return {Promise}
> + * @resolves to an OS.File path
> + */
> +function promiseFile(aBasename) {
> +  let path = OS.Path.join(OS.Constants.Path.profileDir, aBasename);
> +  dump("\n\nopening " + path + "\n\n");

please change this dump to a do_print

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:86
(Diff revision 1)
> +
> +///////////////////////////////////////////////////////////////////////////////
> +add_task(function* test_json_restore_nonexist() {
> +  yield PlacesUtils.bookmarks.eraseEverything();
> +
> +  do_print("JSON restore: nonexistent file should fail");

shouldn-t this be "JSON restore: normal restore should succeed"?
also the test name looks wrong

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:87
(Diff revision 1)
> +///////////////////////////////////////////////////////////////////////////////
> +add_task(function* test_json_restore_nonexist() {
> +  yield PlacesUtils.bookmarks.eraseEverything();
> +
> +  do_print("JSON restore: nonexistent file should fail");
> -      this.file = yield promiseFile("bookmarks-test_restoreNotification.json");
> +  this.file = yield promiseFile("bookmarks-test_restoreNotification.json");

"this" was referring to the test object, but now we don't have it anymore.
Should be a let file, and before the end of the test function you should try to remove it, using OS.File.

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:98
(Diff revision 1)
> -        yield BookmarkJSONUtils.importFromFile(this.file, true);
> +    yield BookmarkJSONUtils.importFromFile(this.file, true);
> -      }
> +  }
> -      catch (e) {
> +  catch (e) {
> -        do_throw("  Restore should not have failed");
> +    do_throw("  Restore should not have failed" + e);
> -      }
> +  }
> -    })
> +});

yes, we still need to also test notifications, as you suspected.
Each test should add specific observer (begin, end) and check its arguments, then remove it.
You can eventually write an helper function for that.

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:104
(Diff revision 1)
> -    currTopic:  NSIOBSERVER_TOPIC_BEGIN,
> -    finalTopic: NSIOBSERVER_TOPIC_SUCCESS,
> -    data:       NSIOBSERVER_DATA_JSON,
> -    folderId:   null,
> -    run: Task.async(function* () {
> -      this.file = yield promiseFile("bookmarks-test_restoreNotification.json");
> +  this.file = yield promiseFile("bookmarks-test_restoreNotification.json");

ditto... I'm not going to repeat this comment, it's clearly valid for all the tests, as well as the observers comment.

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:210
(Diff revision 1)
> -  do_test_pending();
> +  run_next_test();
> -  obssvc.addObserver(beginObserver, NSIOBSERVER_TOPIC_BEGIN, false);
> -  obssvc.addObserver(successAndFailedObserver, NSIOBSERVER_TOPIC_SUCCESS, false);
> -  obssvc.addObserver(successAndFailedObserver, NSIOBSERVER_TOPIC_FAILED, false);
> -  doNextTest();
>  }

run_test is no more needed when it only invokes run_next_test

Comment 12

[[e-mail]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45715/diff/1-2/

Comment 13

[Marco Bonardo [:mak]]

please update the reviewer on the commit to me

Comment 14

[[e-mail]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

Thanks :mak for feedback! I've fixed most of issues, Will add Observer back then set review again.

Comment 15

[[e-mail]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45715/diff/2-3/

Comment 16

[[e-mail]]

Note to my self: The MDN document seems a bit out of date, at least I did not see PlaceUtil is now the recommend way to access place related apis. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Places_Developer_Guide

Comment 17

[[e-mail]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45715/diff/3-4/

Comment 18

[[e-mail]]

Issues addressed, changed to bookmark & history API, followed MDN to add observers
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserver#Example

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

https://reviewboard.mozilla.org/r/45715/#review44447

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:32
(Diff revision 4)
>    "http://example.com/4",
>    "http://example.com/5",
>  ];
>  
> -// Add tests here.  Each is an object with these properties:
> -//   desc:       description printed before test is run
> +var obssvc = Cc["@mozilla.org/observer-service;1"].
> +        getService(Ci.nsIObserverService);

Can instead use Services.obs (should already be in context thanks to head.js)

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:41
(Diff revision 4)
> -//               which then causes the next test to be run
> -//   data:       the data passed to nsIObserver.observe() corresponding to the
> -//               test
> -//   file:       the nsILocalFile that the test creates
> -//   folderId:   for HTML restore into a folder, the folder ID to restore into;
> -//               otherwise, set it to null
> + * Adds some bookmarks for the URIs in |uris|.
> + */
> +function* addBookmarks() {
> +  for (let u of uris) {
> +    yield PlacesUtils.bookmarks.insert({
> +      url: uri(u), parentGuid: PlacesUtils.bookmarks.menuGuid

url doesn't need to be an nsIURI, the new API accepts text specs, as well as URL and nsIURI objects.
so you can just do
for (let url of uris) {
  yield PlacesUtils.bookmarks.insert({
    url, parentGuid: ...

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:77
(Diff revision 4)
> + * @return {Promise}
> + * @resolves to an OS.File path
> + */
> +function promiseFile(aBasename) {
> +  let path = OS.Path.join(OS.Constants.Path.profileDir, aBasename);
> +  do_print("\n\nopening " + path + "\n\n");

You can remove all the \n from the message

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:78
(Diff revision 4)
> + * @resolves to an OS.File path
> + */
> +function promiseFile(aBasename) {
> +  let path = OS.Path.join(OS.Constants.Path.profileDir, aBasename);
> +  do_print("\n\nopening " + path + "\n\n");
> +  return OS.File.open(path, { truncate: true }).then(aFile => { aFile.close(); return path; });

please reindent as
return OS.File.open(...)
              .then(...);

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:149
(Diff revision 4)
> + * Run before every test cases.
> + */
> +function setup(testData) {
> +  var begin = new beginObserver(NSIOBSERVER_TOPIC_BEGIN, testData);
> +  var success = new successAndFailedObserver(NSIOBSERVER_TOPIC_SUCCESS, testData);
> +  var fail = new successAndFailedObserver(NSIOBSERVER_TOPIC_SUCCESS, testData);

There are a couple problems here:
1. this is not really testing that the notifications are received, it's rather testing that when they are received, they are properly constructed. If inadvertitely we stop sending the notifications, this test won't notice that.
2. the code is probably more complex than it needs to be

I think you could be able to use promiseTopicObserved like:

let promiseBegin = promiseTopicObserved(NSIOBSERVER_TOPIC_BEGIN);
let promiseSuccess = promiseTopicObserved(NSIOBSERVER_TOPIC_SUCCESS);
...the test...
let beginData = (yield promiseBegin)[1];
then compare beginData with what you expect, depending on the test.

In all of the tests you should only hit 2 cases:
begin+fail or begin+success, so you could have a registerObservers(expectSuccess) helper that returns an array like [beginPromise, successFailurePromise] and then a checkObservers(expectSuccess, expecteddata, expectedFolderId) async helper that yields on each promise and checks the result.

It may be a little bit more verbose in each test (the folderId check will move to each test that cares about it), but I think it will look more declarative and readable in the end.

does this make sense?

Comment 20

[[e-mail]]

Thanks for point me to promiseTopicObserved
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#369

I'll revise the patch based on that

Comment 21

[[e-mail]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45715/diff/4-5/

Comment 22

[[e-mail]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45715/diff/5-6/

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

https://reviewboard.mozilla.org/r/45715/#review45137

Yep, this looks quite good! Thank you for your patience following my suggestions :)

Just one thing, your Try run is missing xpcshell tests, and this one is an xpcshell test, so ensure to do a run with X tests before setting checkin needed

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:112
(Diff revision 6)
> -    run: Task.async(function* () {
> -      this.file = yield promiseFile("bookmarks-test_restoreNotification.html");
> -      addBookmarks();
> -      yield BookmarkHTMLUtils.exportToFile(this.file);
> -      yield PlacesUtils.bookmarks.eraseEverything();
> -      try {
> +function* checkObservers(expectPromises, expectedData) {
> +  let [promiseBegin, promiseResult] = expectPromises;
> +
> +  let beginData = (yield promiseBegin)[1];
> +  do_print("  Data for current test should be what is expected");
> +  Assert.equal(beginData, expectedData.data);

just move the message into Assert.equal third param?

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:116
(Diff revision 6)
> -      yield PlacesUtils.bookmarks.eraseEverything();
> -      try {
> -        BookmarkHTMLUtils.importFromFile(this.file, false)
> -                         .then(null, do_report_unexpected_exception);
> +  do_print("  Data for current test should be what is expected");
> +  Assert.equal(beginData, expectedData.data);
> +
> +  let [resultSubject, resultData] = yield promiseResult;
> +  do_print("  Data for current test should be what is expected");
> +  Assert.equal(resultData, expectedData.data);

ditto

Comment 24

[[e-mail]]

Comment on attachment 8740301 [details]
MozReview Request: Bug 1011023 - Simplify test_bookmarks_restore_notification.js to use add_task; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45715/diff/6-7/

Comment 25

[[e-mail]]

Thanks for review!

Addressed last issues, tried with xpcshell test and passed related test.



In this patch we did:

replace getService(Ci.nsINavBookmarksService).insertBookmark to PlacesUtils.bookmarks.insert
replace getService(Ci.nsINavHistoryService) to PlacesUtils.history
replace getService(Ci.nsIObserverService) to promiseTopicObserved


And I found those getService parts are still used in many test cases. Is there a plan / related bugs / or any guide about when or where should replaced them?

Comment 26

[Marco Bonardo [:mak]]

(In reply to Fred Lin [:gasolin] from comment #25)
> replace getService(Ci.nsINavBookmarksService).insertBookmark to
> PlacesUtils.bookmarks.insert
> replace getService(Ci.nsINavHistoryService) to PlacesUtils.history
> replace getService(Ci.nsIObserverService) to promiseTopicObserved
> 
> 
> And I found those getService parts are still used in many test cases. Is
> there a plan / related bugs / or any guide about when or where should
> replaced them?

For bookmarks there is bug 1068032, if you wish to help with that.

history and observers replacement is just something that we are doing in new code, but we are unlikely to explicitly change old code, unless we are already touching it for some other fix, then we update it.

Comment 27

[[e-mail]]

got it, thanks for explanation :)

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9e9e06595a

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce9e9e06595a