Closed Bug 1095415 Opened 10 years ago Closed 9 years ago

Convert mochitest-chrome in toolkit/components/places/tests/chrome to Bookmarks.jsm API

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed

People

(Reporter: mak, Assigned: ttaubert)

References

Details

Attachments

(1 file)

tests should be converted
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Comment on attachment 8601425 [details] [diff] [review]
0001-Bug-1095415-Convert-mochitest-chrome-in-toolkit-comp.patch

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

You know what, most of these tests have no reason to be mochitest-chrome VS xpcshell... We might open a mentored bug to convert the ones not actually doing something with UI pieces... xpcshell is much lighter on tinderboxes and parallel.

r=me with the following addressed

::: toolkit/components/places/tests/chrome/test_303567.xul
@@ +76,5 @@
>              let node = nodes[aIndex];
>              is(node.uri, aUrl, aLivemarkData.message);
>            });
>  
> +          PlacesUtils.bookmarks.remove(aLivemark.guid).then(() => {

please use PlacesUtils.livemarks.removeLivemark instead, while using the bookmarks API is possible, the process of removing the livemark from the livemarks service cache is async once it gets the notification from the bookmarks service, and we don't want this test to influence the next ones. the Livemarks API waits for cache removal before resolving.

::: toolkit/components/places/tests/chrome/test_341972a.xul
@@ +44,5 @@
>        waitForLivemarkLoad(aLivemark, function (aLivemark) {
>          is(aLivemark.siteURI.spec, FEEDSITESPEC,
>             "livemark site URI set to value in feed");
>  
> +        PlacesUtils.bookmarks.remove(aLivemark.guid).then(() => {

ditto

::: toolkit/components/places/tests/chrome/test_341972b.xul
@@ +41,5 @@
>        waitForLivemarkLoad(aLivemark, function (aLivemark) {
>          is(aLivemark.siteURI.spec, FEEDSITESPEC,
>             "livemark site URI set to value in feed");
>  
> +        PlacesUtils.bookmarks.remove(aLivemark.guid).then(() => {

ditto

::: toolkit/components/places/tests/chrome/test_342484.xul
@@ +45,5 @@
>            let node = nodes[i];
>            ok(GOOD_URLS.indexOf(node.uri) != -1, "livemark item created with bad uri " + node.uri);
>          }
>  
> +        PlacesUtils.bookmarks.remove(aLivemark.guid).then(() => {

ditto

::: toolkit/components/places/tests/chrome/test_371798.xul
@@ +22,4 @@
>  
> +Cu.import("resource://gre/modules/PlacesUtils.jsm");
> +const histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
> +                  getService(Ci.nsINavHistoryService);

Please use PlacesUtils.history instead

@@ +26,2 @@
>  
> +const TEST_URI = Services.io.newURI("http://foo.com", null, null);

Better importing NetUtil instead of Services and using its newURI here?

@@ +47,3 @@
>  }
>  
> +Task.spawn(function* () {

I think runTest is still mandatory in mochitest chrome? I'd not want to confuse the test harness, until bug 1078657 is implemented.
And I'm surprised the harness doesn't fail a test not having a runTest. But maybe things changed and I don't know...

@@ +63,5 @@
> +  let query = histsvc.getNewQuery();
> +  query.setFolders([PlacesUtils.toolbarFolderId], 1);
> +  let result = histsvc.executeQuery(query, options);
> +  let rootNode = result.root;
> +  rootNode.containerOpen = true;

All of this can be replaced with

let rootNode = PlacesUtils.getFolderContents(PlacesUtils.toolbarFolderId).root;

@@ +82,5 @@
> +    which is triggered by updating the item's title.
> +    After receiving the notification, our original query should also
> +    have been live-updated, so we can iterate through its children,
> +    to check that only the modified bookmark has changed.
> +  */

nit: please replace multi-line comment with multi-1-line comments, I find easier to comment out parts of code if there aren't /**/ in the middle.

@@ +103,2 @@
>    SimpleTest.finish();
> +});

in case it throws we don't finish, what does the m-c harness do in such a case? will it detect the exception and fail correctly?
I honestly don't know since it doesn't support tasks natively and I have no idea if anyone tested that.

::: toolkit/components/places/tests/chrome/test_381357.xul
@@ +41,5 @@
>  
>          is(nodes[0].title, "The First Title",
>             "livemark site URI set to value in feed");
>  
> +        PlacesUtils.bookmarks.remove(aLivemark.guid).then(() => {

removeLivemark
Attachment #8601425 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #2)
> You know what, most of these tests have no reason to be mochitest-chrome VS
> xpcshell... We might open a mentored bug to convert the ones not actually
> doing something with UI pieces... xpcshell is much lighter on tinderboxes
> and parallel.

Indeed.

> > +Task.spawn(function* () {
> 
> I think runTest is still mandatory in mochitest chrome? I'd not want to
> confuse the test harness, until bug 1078657 is implemented.
> And I'm surprised the harness doesn't fail a test not having a runTest. But
> maybe things changed and I don't know...

All the other tests have <window onload="runTest()"> so I'm sure it's fine to leave it out. I tested, runTest() isn't called by the harness itself.

> nit: please replace multi-line comment with multi-1-line comments, I find
> easier to comment out parts of code if there aren't /**/ in the middle.

Yeah, I do that a lot too :)

> @@ +103,2 @@
> >    SimpleTest.finish();
> > +});
> 
> in case it throws we don't finish, what does the m-c harness do in such a
> case? will it detect the exception and fail correctly?
> I honestly don't know since it doesn't support tasks natively and I have no
> idea if anyone tested that.

Replaced it with:

}).catch(err => {
  ok(false, `uncaught error: ${err}`);
}).then(() => {
  SimpleTest.finish();
});

And tested that it fails but finishes in time if an error is thrown.
https://hg.mozilla.org/mozilla-central/rev/d4e4b4b55b5c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.