Clean up tests for bookmarks API

RESOLVED FIXED in Firefox 47

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

(Blocks 1 bug)

unspecified
mozilla47
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

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.
Assignee

Updated

3 years ago
Assignee: nobody → bob.silverberg
Blocks: 1236121, 1213674
Status: NEW → ASSIGNED
Iteration: --- → 47.3 - Mar 7
Flags: blocking-webextensions+
Assignee

Comment 1

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

Updated

3 years ago
Blocks: 1251269
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.
Attachment #8723577 - Flags: review?(kmaglione+bmo)
Assignee

Comment 4

3 years ago
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?
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
Assignee

Comment 6

3 years ago
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/
Attachment #8723577 - Flags: review?(kmaglione+bmo)
Assignee

Comment 7

3 years ago
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.
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, ...
Assignee

Comment 9

3 years ago
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/
Assignee

Comment 10

3 years ago
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 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.
Attachment #8723577 - Flags: review?(kmaglione+bmo) → review+
Assignee

Comment 13

3 years ago
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/
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23566cc859df
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.