Closed Bug 1251244 Opened 9 years ago Closed 9 years ago

Clean up tests for bookmarks API

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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: nobody → bob.silverberg
Blocks: 1236121, 1213674
Status: NEW → ASSIGNED
Iteration: --- → 47.3 - Mar 7
Flags: blocking-webextensions+
Note that this also addresses some of the missing coverage, namely coverage for `getSubTree`.
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)
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
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)
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, ...
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/
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+
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/
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: