1251244 - Clean up tests for bookmarks API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

I started working on bug 1236121 by cleaning up some of the existing tests, in preparation for adding more coverage. I am now going to start working on bug 1213674, which is to complete the implementation of the bookmarks API, and I'd like to build on the few changes that I've done thus far.

For that reason I am submitting this for review at this time.

Comment 1

[Bob Silverberg [:bsilverberg]]

Note that this also addresses some of the missing coverage, namely coverage for `getSubTree`.

Comment 2

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1251244 - Clean up tests for bookmarks API, r?kmag

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

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8723577 [details]
MozReview Request: Bug 1251244 - Clean up tests for bookmarks API, r?kmag

https://reviewboard.mozilla.org/r/36627/#review33271

This looks pretty good, but the error handling needs to be a bit more specific.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:17
(Diff revision 1)
> -  let unsortedId, ourId;
> +  let unsortedId, ourId, createdFolderId;

Does `createdFolderId` really need to be in the outermost scope?

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:82
(Diff revision 1)
> +  }).catch(error => {

This isn't going to work. Errors propagate along the promise chain until the first handler with a catch clause. If you want to test an error from a specific promise, the .catch() handler needs to be attached to that promise, not to the chain, after you return that promise from a handler.

Comment 4

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/36627/#review33271

> Does `createdFolderId` really need to be in the outermost scope?

I do believe it needs to be above the giant `then()` chain as it's used in two different `then` clauses. Where do you suggest it be defined?

> This isn't going to work. Errors propagate along the promise chain until the first handler with a catch clause. If you want to test an error from a specific promise, the .catch() handler needs to be attached to that promise, not to the chain, after you return that promise from a handler.

I _think_ I understand what you mean, but I do not understand how to implement it. Can you provide an example of how I might do the check properly?

Comment 5

[Kris Maglione [:kmag]]

Promise chains don't need to be flat. They can be as hierarchical as you need them to be for any given purpose: https://gist.github.com/1efd6ea13ee06632fde7

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8723577 [details]
MozReview Request: Bug 1251244 - Clean up tests for bookmarks API, r?kmag

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

Comment 7

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/36627/#review33271

> I _think_ I understand what you mean, but I do not understand how to implement it. Can you provide an example of how I might do the check properly?

Thanks for the example Kris. That's pretty cool. I did find, however, that I was only able to use that technique on some methods. It didn't work with `bookmarks.search()` I believe because that was throwing exceptions, rather than rejecting promises. The current code seems to work, but I'm wondering if we need to add some different error handling to `bookmarks.search` to ensure it always rejects rather than bubbling up exceptions. I tried to do that myself, but couldn't quite figure it out. I tried adding a `try` / `catch` around the internal call to `Bookmarks.search` but it never seemed to catch anything. I also tried specifying two methods in the `then()` after `Bookmarks.search` (one for resolve and one for reject) but the reject method never seemed to get called. I also tried adding a `catch()` after the `then()` but again, it never seemed to get called. I think I must be missing something, or maybe the method is fine as is. Please let me know your opinion.

Comment 8

[Kris Maglione [:kmag]]

API methods are expected to throw, rather than rejecting, when given invalid arguments. I think Andrew is working on changing some of them to return rejected promises, but only some of them.

Even so, we need to test for those errors properly, if we're going to test for them. So, you can either use try-catch blocks, or wrap the calls in promise handlers so errors get converted to rejected promises:

  Promise.resolve().then(() => {
    return browser.bookmarks.search();
  }).then(expectedError, ...

Comment 9

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8723577 [details]
MozReview Request: Bug 1251244 - Clean up tests for bookmarks API, r?kmag

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

Comment 10

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/36627/#review33271

> Thanks for the example Kris. That's pretty cool. I did find, however, that I was only able to use that technique on some methods. It didn't work with `bookmarks.search()` I believe because that was throwing exceptions, rather than rejecting promises. The current code seems to work, but I'm wondering if we need to add some different error handling to `bookmarks.search` to ensure it always rejects rather than bubbling up exceptions. I tried to do that myself, but couldn't quite figure it out. I tried adding a `try` / `catch` around the internal call to `Bookmarks.search` but it never seemed to catch anything. I also tried specifying two methods in the `then()` after `Bookmarks.search` (one for resolve and one for reject) but the reject method never seemed to get called. I also tried adding a `catch()` after the `then()` but again, it never seemed to get called. I think I must be missing something, or maybe the method is fine as is. Please let me know your opinion.

Ok, I think I've got it now.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8723577 [details]
MozReview Request: Bug 1251244 - Clean up tests for bookmarks API, r?kmag

https://reviewboard.mozilla.org/r/36627/#review33565

Looks good. Thanks.

Comment 12

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06dfffc819c6

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8723577 [details]
MozReview Request: Bug 1251244 - Clean up tests for bookmarks API, r?kmag

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

Comment 14

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec5234fafff2

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23566cc859df

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/23566cc859df