Closed Bug 1086549 Opened 10 years ago Closed 7 years ago

Convert tests using RemovePage and RemovePages

Categories

(Toolkit :: Places, defect, P2)

defect
Points:
8

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mak, Assigned: standard8)

References

Details

Attachments

(4 files, 8 obsolete files)

59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
markh
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
once History.remove is implemented we can convert tests using the old similar synchronous APIs.
Marco, can you please estimate? Thanks!
Flags: qe-verify-
Flags: needinfo?(mak77)
Flags: firefox-backlog+
let's say 5 cause there's some tests and some of them will require a refactoring.
Points: --- → 8
Flags: needinfo?(mak77)
Assignee: nobody → dteller
Attached patch 1. Places tests, v2 (obsolete) — Splinter Review
Attachment #8510987 - Attachment is obsolete: true
Attachment #8511032 - Flags: review?(mak77)
Attached patch 2. Sync tests (obsolete) — Splinter Review
Attachment #8510990 - Attachment is obsolete: true
Attachment #8511033 - Flags: review?(rnewman)
Attachment #8511033 - Attachment description: sync.diff → 2. Sync tests
Status: NEW → ASSIGNED
Comment on attachment 8510988 [details] [diff] [review]
2. Jetpack tests

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

::: addon-sdk/source/test/addons/places/places-helper.js
@@ +8,5 @@
>                      getService(Ci.nsINavBookmarksService);
>  const hsrv = Cc['@mozilla.org/browser/nav-history-service;1'].
>                getService(Ci.nsINavHistoryService);
>  const brsrv = Cc["@mozilla.org/browser/nav-history-service;1"]
>                       .getService(Ci.nsIBrowserHistory);

could you please remove this, it's unused and we are working to remove this interface...

@@ +24,5 @@
>    Bookmark, Group, Separator,
>    save, search,
>    MENU, TOOLBAR, UNSORTED
>  } = require('sdk/places/bookmarks');
> +const { PlacesUtils: history } = Cu.import("resource://gre/modules/PlacesUtils.jsm", {});

we should import all of PlacesUtils and stop importing single services in this code...
using services directly is far less future proof.

@@ +108,5 @@
>  }
>  exports.addVisits = addVisits;
>  
>  function removeVisits (urls) {
> +  return history.remove(urls);

note the old API was doing the job synchronously, I'm not sure what consumers expect (I think this is fine fwiw).
Comment on attachment 8511032 [details] [diff] [review]
1. Places tests, v2

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

::: toolkit/components/places/tests/unit/test_536081.js
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;

the QI should not be needed here... at that point I'd just hardcode PlacesUtils.history.DBConnection in the only row that used this.

singe it's a SELECT, we could even just use 

let db = yield PlacesUtils.promiseDBConnection();
let rows = db.execute(`SELECT 1 FROM ${table_name}`);
Assert.equal(rows.length, 0);

::: toolkit/components/places/tests/unit/test_browserhistory.js
@@ +46,2 @@
>    
> +  yield PlacesUtils.history.remove(pages);

while here could you please remove trailing spaces from this file?

::: toolkit/components/places/tests/unit/test_hosts_triggers.js
@@ +86,5 @@
>  });
>  
>  add_task(function test_remove_places()
>  {
> +  yield PlacesUtils.history.remove([url.uri for (url of urls)]);

urls.map(u => u.uri)?
Attachment #8511032 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #8)
> note the old API was doing the job synchronously, I'm not sure what
> consumers expect (I think this is fine fwiw).

Yeah, but the only caller (afaict) was async, so this should be fine.
Attached patch 2. Jetpack tests, v2 (obsolete) — Splinter Review
Applied mak's feedback.
Attachment #8510988 - Attachment is obsolete: true
Attachment #8510988 - Flags: review?(rFobic)
Attachment #8512554 - Flags: review?(rFobic)
Attached patch 1. Places tests, v3 (obsolete) — Splinter Review
Applied mak's feedback.
Attachment #8511032 - Attachment is obsolete: true
Attachment #8512556 - Flags: review+
Attached patch 2. Jetpack tests, v3 (obsolete) — Splinter Review
Ah, found out that Jetpack tests just don't show up on TreeHerder by default. Fixed a typo.
Attachment #8512554 - Attachment is obsolete: true
Attachment #8512554 - Flags: review?(rFobic)
Attachment #8512570 - Flags: review?(rFobic)
Irakli, Richard, review ping?
Flags: needinfo?(rnewman)
Flags: needinfo?(rFobic)
Comment on attachment 8511033 [details] [diff] [review]
2. Sync tests

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

::: services/sync/tests/unit/test_history_tracker.js
@@ +84,3 @@
>    _("Add hook for save completion.");
>    tracker.persistChangedIDs = true;
> +  

Nit: trailing ws.
Attachment #8511033 - Flags: review?(rnewman) → review+
Flags: needinfo?(rnewman)
Comment on attachment 8512570 [details] [diff] [review]
2. Jetpack tests, v3

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

As far as I can tell (and it could be I'm missing something) used name does not matches imported name so this code will throw exceptions.

::: addon-sdk/source/test/addons/places/places-helper.js
@@ +18,5 @@
>    Bookmark, Group, Separator,
>    save, search,
>    MENU, TOOLBAR, UNSORTED
>  } = require('sdk/places/bookmarks');
> +const { PlacesUtils: history } = Cu.import("resource://gre/modules/PlacesUtils.jsm", {});

Would it make sense to just import history ?

const { PlacesUtils: {history, asyncHistory} } = Cu.import("resource://gre/modules/PlacesUtils.jsm", {});

@@ +47,5 @@
>    [MENU, TOOLBAR, UNSORTED].forEach(clearBookmarks);
>  }
>  
>  function clearHistory (done) {
> +  PlacesUtils.history.removeAllPages();

You imported PlacesUtils as `history` so this does not seem right.

@@ +91,5 @@
>  exports.compareWithHost = compareWithHost;
>  
>  function addVisits (urls) {
>    var deferred = defer();
> +  PlacesUtils.asyncHistory.updatePlaces([].concat(urls).map(createVisit), {

I think at this point it could be ..updatePlaces([...urls]).map... as it's more clear IMO.

@@ +102,5 @@
>  }
>  exports.addVisits = addVisits;
>  
>  function removeVisits (urls) {
> +  return PlacesUtils.history.remove(urls);

No need for return as it did not used to return anything before.

You imported PlacesUtils as `history` so this does not seem right.

@@ +117,1 @@
>      visitDate: +(new Date()) * 1000,

You imported PlacesUtils as `history` so this does not seem right.
Attachment #8512570 - Flags: review?(rFobic) → review-
Flags: needinfo?(rFobic)
No time to work on this atm. Feel free to steal.
Assignee: dteller → nobody
Priority: -- → P2
Blocks: 739219
No longer blocks: 854886
Status: ASSIGNED → NEW
Flags: needinfo?(mak77)
Blocks: 1306276
I've taken David's patches, and unbitrotted them and updated according to the current code. A lot of the changes had already landed or been superseded by other patches.
Assignee: nobody → standard8
Flags: needinfo?(mak77)
Attachment #8511033 - Attachment is obsolete: true
Attachment #8512556 - Attachment is obsolete: true
Attachment #8512570 - Attachment is obsolete: true
Comment on attachment 8845098 [details]
Bug 1086549 - Converting Sync tests from removePage to History.remove. Patch by Yoric, updated by Standard8.

https://reviewboard.mozilla.org/r/118310/#review120200
Attachment #8845098 - Flags: review?(markh) → review+
Comment on attachment 8845096 [details]
Bug 1086549 - Converting Places tests from removePage to History.remove. Patch by Yoric, updated by Standard8.

https://reviewboard.mozilla.org/r/118306/#review121062

::: toolkit/components/places/tests/queries/test_searchTerms_includeHidden.js:79
(Diff revision 3)
>      root.containerOpen = false;
>  
>      do_check_eq(cc, data.expectedResults);
>      do_check_eq(cc_update, data.expectedResults + (data.includeHidden ? 1 : 0));
>  
> -    PlacesUtils.bhistory.removePage(uri("http://hidden.example.com/"));
> +    yield PlacesUtils.bhistory.remove("http://hidden.example.com/");

should be .history, not .bhistory.
It may be working regardless, since they are basically the same object, but bhistory will be removed one day (bug 739219).

::: toolkit/components/places/tests/unit/test_536081.js:20
(Diff revision 3)
>  
>  add_task(function* test_execute() {
>    for (let url of URLS) {
>      yield task_test_url(url);
>    }
>  });

could you please remove all the boilerplate code for run_test and this first task, since there's only a single url to test?

I wrote this test 7 years ago, and I regret that :D

::: toolkit/components/places/tests/unit/test_536081.js:22
(Diff revision 3)
>    for (let url of URLS) {
>      yield task_test_url(url);
>    }
>  });
>  
>  function* task_test_url(aURL) {

thus, just add_task here

::: toolkit/components/places/tests/unit/test_536081.js:36
(Diff revision 3)
>    do_check_eq(cc, 1);
>    print("Checking url is in the query.");
>    let node = root.getChild(0);
>    print("Found " + node.uri);
>    root.containerOpen = false;
> -  bh.removePage(uri(node.uri));
> +  yield PlacesUtils.history.remove(node.uri);

it would be nice if the test would actually be testing something.

It is invoking remove on node.uri, but it's not checking that the page is gone.

We should check isPageInDB just after addvisits and here after remove.

::: toolkit/components/places/tests/unit/test_hosts_triggers.js:88
(Diff revision 3)
>  add_task(function* test_remove_places() {
> -  for (let idx in urls) {
> +  yield PlacesUtils.history.remove(urls.map(x => x.uri));
> -    PlacesUtils.history.removePage(urls[idx].uri);
> -  }
>  
>    yield PlacesTestUtils.clearHistory();

doesn't seem to make much sense to remove SOME pages and then remove all of them... I wonder if this clearHistory is here by mistake?

::: toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js:117
(Diff revision 3)
>        do_check_eq(resultObserver.nodeChangedByTitle.title, "baz");
>  
>        // nsINavHistoryResultObserver.nodeRemoved
>        var removedURI = uri("http://google.com");
>        PlacesTestUtils.addVisits(removedURI).then(function() {
> -        bhist.removePage(removedURI);
> +        return histsvc.remove(removedURI);

let's use PlacesUtils.history here as well.
Attachment #8845096 - Flags: review?(mak77)
Comment on attachment 8845097 [details]
Bug 1086549 - Converting add-on sdk tests from removePage to History.remove. Patch by Yoric, updated by Standard8.

https://reviewboard.mozilla.org/r/118308/#review121066

::: addon-sdk/source/test/addons/places/lib/places-helper.js:60
(Diff revision 4)
>  
>  function compareWithHost (assert, item) {
>    let id = item.id;
> -  let type = item.type === 'group' ? bmsrv.TYPE_FOLDER : bmsrv['TYPE_' + item.type.toUpperCase()];
> +  let type = item.type === 'group' ?
> +             PlacesUtils.bookmarks.TYPE_FOLDER :
> +             PlacesUtils.bookmarks['TYPE_' + item.type.toUpperCase()];

imo, either indent just 2 spaces, or indent 2 spaces more than the condition

::: addon-sdk/source/test/addons/places/lib/places-helper.js:106
(Diff revision 4)
>      handleResult: function () {},
>      handleError: deferred.reject,
>      handleCompletion: deferred.resolve
>    });
>  
>    return deferred.promise;

please change this to use PlacesUtils.history.insertMany, it already returns a promise.
The only needed change may be to createVisit, since the input is slightly different, an array of:
{
  url: any of nsIURI, href or URL, 
  title: ...,
  visits: [
    {
      date: a Date object (optional, defaults to new Date()),
      transition: (optional, defaults to LINK)
    },
    ...
  ]
Attachment #8845097 - Flags: review?(mak77)
Comment on attachment 8845098 [details]
Bug 1086549 - Converting Sync tests from removePage to History.remove. Patch by Yoric, updated by Standard8.

https://reviewboard.mozilla.org/r/118310/#review121072

::: services/sync/tests/unit/test_history_tracker.js:225
(Diff revision 4)
>  
>    // Observe expiration.
>    Services.obs.addObserver(function onExpiration(aSubject, aTopic, aData) {
>      Services.obs.removeObserver(onExpiration, aTopic);
>      // Remove the remaining page to update its score.
> -    PlacesUtils.history.removePage(uriToRemove);
> +    PlacesUtils.history.remove(uriToRemove);

this may be wrong, nothing is waiting on it. The old API was synchronous, but this one is not.
Comment on attachment 8845099 [details]
Bug 1086549 - Converting browser tests from removePage to History.remove. Patch by Yoric, updated by Standard8.

https://reviewboard.mozilla.org/r/118312/#review121074
Attachment #8845099 - Flags: review?(mak77) → review+
Comment on attachment 8845098 [details]
Bug 1086549 - Converting Sync tests from removePage to History.remove. Patch by Yoric, updated by Standard8.

https://reviewboard.mozilla.org/r/118310/#review121072

> this may be wrong, nothing is waiting on it. The old API was synchronous, but this one is not.

Earlier on in the test, a promise is set up to listen for uriToRemove being removed, and subsequently, it is waited upon. Therefore I believe this change is fine.
Comment on attachment 8845096 [details]
Bug 1086549 - Converting Places tests from removePage to History.remove. Patch by Yoric, updated by Standard8.

https://reviewboard.mozilla.org/r/118306/#review122004

::: toolkit/components/places/tests/unit/test_536081.js:7
(Diff revision 4)
>  /* vim:set ts=2 sw=2 sts=2 et: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
> +const URL =

This may be confusing since also the URL object is in the scope, this is likely shadowing it.

I'd suggest to rename to TEST_URL
Attachment #8845096 - Flags: review?(mak77) → review+
Comment on attachment 8845097 [details]
Bug 1086549 - Converting add-on sdk tests from removePage to History.remove. Patch by Yoric, updated by Standard8.

https://reviewboard.mozilla.org/r/118308/#review122010

::: addon-sdk/source/test/addons/places/lib/places-helper.js:110
(Diff revision 5)
>  }
>  exports.removeVisits = removeVisits;
>  
>  // Creates a mozIVisitInfo object
>  function createVisit (url) {
> -  let place = {}
> +  let uri = newURI(url);

this is not needed, the new API supports: nsIURI, URL or plain href. so just put "url," in the returned object.

::: addon-sdk/source/test/addons/places/lib/places-helper.js:113
(Diff revision 5)
>  // Creates a mozIVisitInfo object
>  function createVisit (url) {
> -  let place = {}
> -  place.uri = newURI(url);
> -  place.title = "Test visit for " + place.uri.spec;
> -  place.visits = [{
> +  let uri = newURI(url);
> +  return {
> +    url: uri,
> +    title: "Test visit for " + uri.spec,

and url here, as well.
Attachment #8845097 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4b3b7377c7f
Converting Places tests from removePage to History.remove. Patch by Yoric, updated by Standard8. r=mak
https://hg.mozilla.org/integration/autoland/rev/fa3d459b70a5
Converting add-on sdk tests from removePage to History.remove. Patch by Yoric, updated by Standard8. r=mak
https://hg.mozilla.org/integration/autoland/rev/7e2fc0b3f192
Converting Sync tests from removePage to History.remove. Patch by Yoric, updated by Standard8. r=markh
https://hg.mozilla.org/integration/autoland/rev/e998d849b34e
Converting browser tests from removePage to History.remove. Patch by Yoric, updated by Standard8. r=mak
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: